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

[Ajay kumar]

On Thu, Apr 24, 2014 at 11:08 PM, Ajay kumar <ajaynumb at gmail.com> wrote:
> On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark <robdclark at gmail.com> wrote:
>> On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar <ajaynumb at gmail.com> wrote:
>>> Rob,
>>>
>>> On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar <ajaynumb at gmail.com> wrote:
>>>>> Sorry for the previous reply,
>>>>>
>>>>> Here goes the full explaination:
>>>>>
>>>>>> Rob,
>>>>>>
>>>>>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>>>>> So what about, rather than adding drm_panel support to each bridge
>>>>>>> individually, we introduce a drm_panel_bridge (with a form of
>>>>>>> chaining).. ie:
>>>>>>>
>>>>>>>   struct drm_panel_bridge {
>>>>>>>     struct drm_bridge base;
>>>>>>>     struct drm_panel *panel;
>>>>>>>     struct drm_bridge *bridge; /* optional */
>>>>>>>   };
>>>>>>>
>>>>>>>   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);
>>>>>>>   }
>>>>>>>
>>>>> We cannot call them like this from crtc helpers in the order you mentioned,
>>>>> since each individual bridge chip expects the panel controls at
>>>>> different places.
>>>>> I mean,
>>>>> -- sometimes panel controls needs to be done before bridge
>>>>> controls(ptn3460: before RST_N and PD_N)
>>>>
>>>> well, this is why bridge has pre-enable/enable/disable/post-disable
>>>> hooks, so you can choose before or after..
>>> These calls are for a bridge to sync with the encoder calls.
>>> We might end up defining pre-enable/enable/disable/post-disable for a
>>> panel to sync
>>> with the bridge calls! I have explained below.
>>>
>>>>> -- sometimes in between the bridge controls (ps8622: one panel control
>>>>> before SLP_N and one after SLP_N)
>>>>> -- sometimes panel controls needs to be done after bridge controls.
>>>>
>>>> I am not convinced that a generic panel/bridge adapter is not
>>>> possible.  Maybe we need more fine grained fxn ptr callbacks, although
>>>> seems like pre+post should give you enough.  It would certainly be
>>>> easier than having to add panel support in each individual bridge
>>>> driver (which seems like it will certainly result that only certain
>>>> combinations of panel+bridge actually work as intended)
>>> Ok. Consider this case:
>>> Currently, we have the sequence as "bridge->pre_enable,
>>> encoder_enable, bridge->enable"
>>> And, the bridge should obviously be enabled in bridge->pre_enable.
>>> Now, where do you choose to call panel->pre_enable?
>>> -- as the first step of bridge->pre_enable, before we pull up/down bridge GPIOs.
>>> -- at the last step of bridge->pre_enable, after we pull up/down the
>>> bridge GPIOs.
>>>
>>> Ideally, PTN3460 expects it to be the second case, and PS8625 expects
>>> it to be the first case.
>>> So, we will end up adding pre_pre_enable, post_pre_enable to
>>> accomodate such scenarios.
>>
>> ok, well I think we can deal with this with a slight modification of
>> my original proposal.  Split drm_panel_bridge into
>> drm_composite_bridge and drm_panel_bridge:
>>
>>   struct drm_composite_bridge {
>>     struct drm_bridge base;
>>     struct drm_bridge *b1, *b2;
>>   }
>>
>>   static void drm_composite_bridge_enable(struct drm_bridge *bridge)
>>   {
>>     struct drm_composite_bridge *cbridge = to_composite_bridge(bridge);
>>     cbridge->b1->funcs->enable(cbridge->b1);
>>     cbridge->b2->funcs->enable(cbridge->b2);
>>   }
>>
>>   .. and so on ..
>>
>>   struct drm_panel_bridge {
>>     struct drm_bridge base;
>>     struct drm_panel *panel;
>>   }
>>
>>   static void drm_panel_bridge_enable(struct drm_bridge *bridge)
>>   {
>>     struct drm_panel_bridge *pbridge = to_panel_bridge(bridge);
>>     pbridge->panel->funcs->enable(pbridge->panel);
>>   }
>>
>>   .. and so on..
>>
>>
>> then in initialization, order can be managed like:
>>
>>   if (panel_first)
>>     bridge = drm_composite_bridge_init(panel_bridge, actual_bridge)
>>   else
>>     bridge = drm_composite_bridge_init(actual_bridge, panel_bridge);
>>
>>   possibly parametrize drm_panel_bridge if we need to map
>> panel->enable() to bridge->pre_enable() vs bridge->enable(), etc..
>
> Well, this really does seems complex to me.
> Don't you think just having a drm_panel inside drm_bridge structure is
> sufficient?!
> And, make a call for pre_panel_enable and post_panel_enable around
> bridge->pre_enable..and so on.?
Adding more comments:
The beauty of drm_panel is in the flexibility which it provides.
We can call panel_enable/disable at the right point. Even the