[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls

[Rob Clark]

On Mon, Apr 28, 2014 at 9:08 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
> Daniel,
>
> On Sun, Apr 27, 2014 at 6:25 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
>> On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
>>> We can call panel_enable/disable at the right point. Even the bridge chips
>>> expect the same. So, I am not ok with combining the bridge and panel and
>>> calling the fxn pointers from crtc_helpers.
>>> So, either we leave it the way it is in this patch (call drm_panel
>>> functions at right points) or
>>> we don't use drm_panel to control the panel sequence and instead use few DT
>>> properties and regulators, inside the bridge chip driver.
>>
>> That wasn't really suggested, but instead the idea was to provide a
>> default drm_bridge which wraps the drm_panel so that you could more
>> easily chain them up.
> What do you mean by "default" drm_bridge?
> I just want to clear few things. Let me explain what I have understood:
> What I understand is that Rob wants me to create a common
> structure which wraps up drm_panel and drm_bridge into a single structure:
>   struct drm_panel_bridge {
>     struct drm_bridge base;
>     struct drm_panel *panel;
>     struct drm_bridge *bridge; /* optional */  ==> Really not sure why
> this is needed!
>   };
>
>   static void drm_panel_bridge_enable(struct drm_bridge *bridge)
>   {
>     struct drm_panel_bridge *pb = to_panel_bridge(bridge);
>     if (pb->bridge)
>       pb->bridge->funcs->enable(pb->bridge);
>     pb->panel->funcs->enable(pb->panel);
>   }
>
> And now, the above function becomes a common function
> (also with an ordering issue btw panel and bridge).
> Where do we keep it? And, where do we call it from?

my proposal involves no change to crtc helpers.  The later variation
(with composite_bridge so you could choose the order) would have a
sequence along the lines of:

crtc helpers:
+ bridge_funcs->enable(): -> composite_bridge_enable()
   + cbridge->b1->funcs->enable(): -> ptn3460_bridge_enable()
   + cbridge->b2->funcs->enable(): -> panel_bridge_enable();
       + pbridge->panel->funcs->enable(): -> foo_panel_enable()

or if the order needs to be swapped you can use ptn3460_bridge as b2
and panel bridge as b1.

BR,
-R

>> Also I'm not really happy that the bridge callbacks have been added to
>> the drm helpers (I'd prefer if driver callbacks would bear that
>> responsibility). But you can always create your own drm_bridge
>> integration.
> Do you mean, the bridge calls should be moved out of common drm_helper
> functions and called from platform specific drivers instead?
>
>> In any case your concern that drivers need to control
>> when certain callbacks are called is shared by everyone here afaics.
>> And I also don't see any issues with Rob's proposal in this regard.
>> -Daniel
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
> Please let me know if my understanding is correct and correct me if
> there is a misconception.
>
> Regards,
> Ajay Kumar