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..
BR,
-R
> So, we leave the choice for the individual bridge chip drivers to
> implement panel
> power up/down controls in the place where they wish to.
>
>
> Thanks and regards,
> Ajay Kumar
