Re: [PATCH 3/5] drm/i2c: adv7511: Refactor encoder slave functions

[Rob Clark <robdclark@gmail.com>]

On Tue, Aug 4, 2015 at 1:16 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
> On 08/03/2015 04:04 PM, Rob Clark wrote:
>> On Mon, Aug 3, 2015 at 8:03 AM, Andrzej Hajda <a.hajda@samsung.com> wrote:
>>> Hi,
>>>
>>> On 07/31/2015 04:48 PM, Rob Clark wrote:
>>>> On Fri, Jul 31, 2015 at 8:58 AM, Boris Brezillon
>>>> <boris.brezillon@free-electrons.com> wrote:
>>>>> Hi Rob,
>>>>>
>>>>> On Fri, 31 Jul 2015 08:13:59 -0400
>>>>> Rob Clark <robdclark@gmail.com> wrote:
>>>>>
>>> (...)
>>>
>>>>> Another problem I've seen with some drm bridge drivers is that they
>>>>> directly create their own connector, which, as Archit stated, is wrong
>>>>> if you decide to chain this bridge with another bridge. The other
>>>> I agree with Archit on this.  He took this approach w/ msm support for
>>>> external bridges, and it seems sensible that the last bridge
>>>> constructs the connector.  (Plus presumably that is where hpd, ddc
>>>> probing, etc, is happing)
>>> With this approach many bridges should construct connector conditionally,
>>> depending if there is something behind them in pipeline (the same is true for
>>> encoders and even crtcs). It works, but for me there is lot of unnecessary code
>>> and all those conditional paths make things less friendly for development.
>>> Wouldn't be better to move creation of the connector to the main drm driver,
>>> it would require probably adding some opses/fields to drm_bridges but the
>>> drivers would be simpler, I guess.
>> six of one, half dozen of the other..   you'd still need to implement
>> the hpd and ddc probe bits, and that sort of thing *somewhere*.
>> Whether it is directly by implementing drm_connector in the bridge, or
>> indirectly via some extra drm_bridge op's called by a shim connector
>> in the main drm driver, doesn't really seem to change things..
>
> The difference is that instead of duplicating connector related code in every
> driver you will call one helper from the main drm driver/core.

Which isn't more than a few lines of code.. I mean, looking at a
couple connectors, the bulk of the code is hpd and ddc related.  That
doesn't magically go away.  There isn't that many lines of
boilerplate, so meh.

BR,
-R

> Regards
> Andrzej
>
>>
>> BR,
>> -R
>>
>>> Regards
>>> Andrzej
>>>
>>>>> reason why the bridge should not create the connector by its own is
>>>>> that in some case the encoder supports different types of connectors (a
>>>>> TDMS encoder can be used to output HDMI or DVI), and thus, selecting
>>>>> the connector type should be left to the part responsible for creating
>>>>> the display pipelines.
>>>> did you mean "should" instead of "should not"?  Otherwise I don't
>>>> think I understand..
>>>>
>>>> BR,
>>>> -R
>>>>
>>>>> This being said, I'm definitely not an expert in this area, so don't
>>>>> hesitate to show me where I'm wrong.
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> Boris
>>>>>
>>>>> --
>>>>> Boris Brezillon, Free Electrons
>>>>> Embedded Linux and Kernel engineering
>>>>> http://free-electrons.com
>