Message-ID: <915d4251-8162-b97f-5449-b4ea42c2202e@samsung.com>
Date:   Mon, 12 Mar 2018 14:47:20 +0100
From:   Andrzej Hajda <a.hajda@...sung.com>
To:     jacopo mondi <jacopo@...ndi.org>
Cc:     Jacopo Mondi <jacopo+renesas@...ndi.org>, architt@...eaurora.org,
        Laurent.pinchart@...asonboard.com, airlied@...ux.ie,
        horms@...ge.net.au, magnus.damm@...il.com, geert@...ux-m68k.org,
        niklas.soderlund@...natech.se, sergei.shtylyov@...entembedded.com,
        robh+dt@...nel.org, mark.rutland@....com,
        dri-devel@...ts.freedesktop.org, linux-renesas-soc@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 0/3] drm: Add LVDS decoder bridge

On 12.03.2018 13:30, jacopo mondi wrote:
> Hi Andrzej,
>
> On Mon, Mar 12, 2018 at 10:07:27AM +0100, Andrzej Hajda wrote:
>> On 09.03.2018 14:51, Jacopo Mondi wrote:
>>> Hello,
>>>    after some discussion on the proposed bindings for generic lvds decoder and
>>> Thine THC63LVD1024, I decided to drop the THC63 specific part and just live with
>>> a transparent decoder that does not support any configuration from DT.
>>>
>>> Dropping THC63 support to avoid discussion on how to better implement support
>>> for a DRM bridge with 2 input ports and focus on LVDS mode propagation through
>>> bridges as explained in v1 cover letter (for DRM people: please see [1] as why
>>> I find difficult to implement support for bridges with multiple input endpoints)
>>>
>>> Same base branch as v1, with same patches for V3M Eagle applied on top.
>>> git://jmondi.org/linux v3m/v4.16-rc3/base
>>>
>>> Thanks
>>>    j
>>>
>>> v1 -> v2:
>>> - Drop support for THC63LVD1024
>>>
>>> [1] I had a quick at how to model a DRM bridge with multiple input
>>> ports, and I see a blocker in how DRM identifies and matches bridges using
>>> the devices node in place of the endpoint nodes.
>>>
>>> As THC63LVD1024 supports up to 2 LVDS inputs and 2 LVDS outputs, I see only
>>> a few ways to support that:
>>>  1) register 2 drm bridges from the same driver (one for each input/output pair)
>>>     but they would both be matches on the same device node when the preceding
>>>     bridge calls "of_drm_find_bridge()".
>>>  2) register a single bridge with multiple "next bridges", but when the bridge
>>>     gets attached I don't see a way on how to identify on which next bridge
>>>     "drm_bridge_attach()" on, as it depends on the endpoint the current bridge
>>>     has been attached on first, and we don't have that information.
>>>  3) Register more instances of the same chip in DTS, one for each input/output
>>>     pair. They gonna share supplies and gpios, and I don't like that.
>>>
>>> I had a quick look at the currently in mainline bridges and none of them has
>>> multiple input endpoints, except for HDMI audio endpoint, which I haven't found
>>> in use in any DTS. I guess the problem has been already debated and maybe solved
>>> in the past, so feel free to point me to other sources.
>> I think this is is a step in wrong direction, IMHO. Your previous
>> patchset was quite OK, at least bindings, IMHO. Few things needed only
>> polishing.
>> Here we have unmanaged/transparent bridge, which is totally different,
>> what happened to regulators and gpios from previous iteration.
>> I do not have schematics of the board, but I guess respective pins of
>> the bridge must be connected somehow.
>> I think the problem you want to avoid (double bridge) should not be a
>> problem at all.
>> I suppose the most important is to have correct bindings - as they need
>> to be stable.
>> If you really must to do hacks better is to put them into driver.
>>
> I understand. The "transparent bridge" is of no actual use, but I don't see
> how the "double bridge" thing is not an issue if I were to develop
> support for the actual Thine chip.

What is exactly your configuration: single/dual input, single/dual output?
Current bindings suggests single/single, am I right? In such case you do
not need to implement dual link functionality in the driver - since you
even do not have possibility to test it.
But the bindings should support all modes of operation, or at least
should be easy to extend them in the future with backward compatibility.

>
> Please see my reply from yesterday to Archit. I still think having two
> bridges is somehow an issue...

Yes, I agree. But do we have such case? If no - no problem ATM :), if
yes - lets try to implement it and see where is the problem, as Archit
already suggested it would be just a matter of assigning bridge to port
node, instead of device node.

>
> While we clarify that, would it be fine an initial driver version for
> the actualt Thine chip with a single input support[1]? I would ditch this
> transparent bridge and resume working on a THC63LVD1024 driver from
> comments received on v1.

I think, yes: driver with only single input, and full or extend-able
bindings.

>
> Thanks
>   j
>
> [1] Single input support implies a single input port in DT bindings
> even if the chip supports two, and my understanding was that you
> didn't like this.

I am sorry if I was not clear enough, only thing I wanted was complete
or at least consistent/extend-able binding.
So when I saw word "multiple LVDS streams" in bindings I was looking
further to see how these multiple LVDS bindings are defined, and found
nothing :)
Btw I think it may be better to use "Dual Link" instead of "Multiple
streams", it is more precise and quite well established in docs.

Regards
Andrzej

>
>
>> Regards
>> Andrzej
>>
>>> Jacopo Mondi (3):
>>>   dt-bindings: display: bridge: Document LVDS to parallel decoder
>>>   drm: bridge: Add LVDS decoder driver
>>>   arm64: dts: renesas: Add LVDS decoder to R-Car V3M Eagle
>>>
>>>  .../bindings/display/bridge/lvds-decoder.txt       |  42 ++++++
>>>  arch/arm64/boot/dts/renesas/r8a77970-eagle.dts     |  31 +++-
>>>  drivers/gpu/drm/bridge/Kconfig                     |   8 ++
>>>  drivers/gpu/drm/bridge/Makefile                    |   1 +
>>>  drivers/gpu/drm/bridge/lvds-decoder.c              | 157 +++++++++++++++++++++
>>>  5 files changed, 237 insertions(+), 2 deletions(-)
>>>  create mode 100644 Documentation/devicetree/bindings/display/bridge/lvds-decoder.txt
>>>  create mode 100644 drivers/gpu/drm/bridge/lvds-decoder.c
>>>
>>> --
>>> 2.7.4
>>>
>>>
>>>
>>>

Powered by blists - more mailing lists