Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1597106777-30913-1-git-send-email-venkateshwar.rao.gannavarapu@xilinx.com>
In-Reply-To: <1597106777-30913-1-git-send-email-venkateshwar.rao.gannavarapu@xilinx.com>
From:   Daniel Vetter <daniel@ffwll.ch>
Date:   Tue, 11 Aug 2020 22:37:04 +0200
Message-ID: <CAKMK7uGcbaABtUuzWpOi8TA=ERSHBP6hwiX-YnQSXVHKWQo+mA@mail.gmail.com>
Subject: Re: [RFC PATCH V2 0/2] Add Xilinx DSI TX driver
To:     Venkateshwar Rao Gannavarapu 
        <venkateshwar.rao.gannavarapu@xilinx.com>
Cc:     Hyun Kwon <hyun.kwon@xilinx.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        Dave Airlie <airlied@linux.ie>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        sandipk@xilinx.com, vgannava@xilinx.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Aug 11, 2020 at 2:46 AM Venkateshwar Rao Gannavarapu
<venkateshwar.rao.gannavarapu@xilinx.com> wrote:
>
> Xilinx DSI-TX subsytem consists of DSI controller core, AXI crossbar
> and D-PHY as sub blocks. DSI TX subsystem driver supports multiple lanes
> upto 4, RGB color formats, video mode and command modes.
>
> DSI-TX driver is implemented as an encoder driver, as it going to be
> the final node in display pipeline. Xilinx doesn't support any converter
> logic to make this as bridge driver. Xilinx doesn't have such
> use cases where end node can't be an encoder like DSI-TX. And Xilinx
> encoder drivers represents a subsystem where individual blocks can't be
> used with external components / encoders.

So there's indeed a bit of an argument about when it should just be
stitched together as components, and when it is better a bridge. But
bridges also come with plenty of convenience glue, and we're talking
about fpga so the point pretty much is to rewire this all eventually.
Maybe not by you folks, but people are r/e-ing these things so who
knows.

The other thing is you do have a bridge attached already, or should
have: The drm_panel. And you do get the entire drm-connector
instantiation rather wrong, at least the ->dpms hook is breaking
atomic real bad. The ->detect implementation is also quite the
adventure. So panel bridge is definitely required here.

Finally this also would avoid component.c usage in xlnx - not that
there's something wrong with that per se, but it always means driver
specific logic and interactions, so harder to maintain for people not
involved in the code base on a daily basis. Also I'm not sure how this
works, the component integration glue in the main driver seems to be
missing. And the upstream drm/xlnx doesn't seem to have it either.

So all together I do think drm_bridge sounds like the right thing to do her=
e.
-Daniel

>
> Venkateshwar Rao Gannavarapu (2):
>   dt-bindings: display: xlnx: dsi: This add a DT binding for Xilinx DSI
>     TX     subsystem.
>   drm: xlnx: dsi: driver for Xilinx DSI TX subsystem
>
>  .../devicetree/bindings/display/xlnx/xlnx,dsi.yaml | 147 +++++
>  drivers/gpu/drm/xlnx/Kconfig                       |  11 +
>  drivers/gpu/drm/xlnx/Makefile                      |   2 +
>  drivers/gpu/drm/xlnx/xlnx_dsi.c                    | 701 +++++++++++++++=
++++++
>  4 files changed, 861 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/xlnx/xlnx,d=
si.yaml
>  create mode 100644 drivers/gpu/drm/xlnx/xlnx_dsi.c
>
> --
> 1.8.3.1
>
> This email and any attachments are intended for the sole use of the named=
 recipient(s) and contain(s) confidential information that may be proprieta=
ry, privileged or copyrighted under applicable law. If you are not the inte=
nded recipient, do not read, copy, or forward this email message or any att=
achments. Delete this email message and any attachments immediately.


--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch