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

[Archit Taneja <architt@codeaurora.org>]

On 08/04/2015 05:54 PM, Rob Clark wrote:
> 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.

Could we get to a consensus for this?

Is it okay for bridge and i2c_encoder to co-exist for now?

Thanks,
Archit

>
> 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
>>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project