Sorry for the previous reply. Here goes the full explaination.
On Fri, Apr 25, 2014 at 11:40 AM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
> On Thu, Apr 24, 2014 at 11:08 PM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
>> On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>>> On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
>>>> Rob,
>>>>
>>>> On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark <robdclark@xxxxxxxxx> wrote:
>>>>> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote:
>>>>>> Sorry for the previous reply,
>>>>>>
>>>>>> Here goes the full explaination:
>>>>>>
>>>>>>> Rob,
>>>>>>>
>>>>>>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark <robdclark@xxxxxxxxx> 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 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
