On Thu, Apr 24, 2014 at 11:08 PM, Ajay kumar ajaynumb@gmail.com wrote:
On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark robdclark@gmail.com wrote:
On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar ajaynumb@gmail.com wrote:
Rob,
On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark robdclark@gmail.com wrote:
On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar ajaynumb@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@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