[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Mon, Apr 28, 2014 at 9:08 AM, Ajay kumar wrote:
> Daniel,
>
> On Sun, Apr 27, 2014 at 6:25 PM, Daniel Vetter wrote:
>> On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar 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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
Daniel,
On Sun, Apr 27, 2014 at 6:25 PM, Daniel Vetter wrote:
> On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar 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?
> 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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Fri, Apr 25, 2014 at 8:17 AM, Ajay kumar 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. 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. 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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
Sorry for the previous reply. Here goes the full explaination.
On Fri, Apr 25, 2014 at 11:40 AM, Ajay kumar wrote:
> On Thu, Apr 24, 2014 at 11:08 PM, Ajay kumar wrote:
>> On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark wrote:
>>> On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar wrote:
Rob,
On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark wrote:
> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
>> Sorry for the previous reply,
>>
>> Here goes the full explaination:
>>
>>> Rob,
>>>
>>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark
>>> 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 p
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Thu, Apr 24, 2014 at 11:08 PM, Ajay kumar wrote:
> On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark wrote:
>> On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar wrote:
>>> Rob,
>>>
>>> On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark wrote:
On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
> Sorry for the previous reply,
>
> Here goes the full explaination:
>
>> Rob,
>>
>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark
>> 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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Thu, Apr 24, 2014 at 10:55 PM, Rob Clark wrote:
> On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar wrote:
>> Rob,
>>
>> On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark wrote:
>>> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
Sorry for the previous reply,
Here goes the full explaination:
> Rob,
>
> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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.?
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
Rob,
On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark wrote:
> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
>> Sorry for the previous reply,
>>
>> Here goes the full explaination:
>>
>>> Rob,
>>>
>>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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.
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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Thu, Apr 24, 2014 at 12:55 PM, Ajay kumar wrote:
> Rob,
>
> On Thu, Apr 24, 2014 at 9:41 PM, Rob Clark wrote:
>> On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
>>> Sorry for the previous reply,
>>>
>>> Here goes the full explaination:
>>>
Rob,
On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Wed, Apr 23, 2014 at 3:02 PM, Ajay kumar wrote:
> Sorry for the previous reply,
>
> Here goes the full explaination:
>
>> Rob,
>>
>> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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..
> -- 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)
> So, putting these drm_panel controls inside individual bridge drivers will
> work,
> but keeping them in crtc_helpers might break stuff.
I'm not talking about crtc changing helpers.
BR,
-R
> Thanks and regards,
> Ajay Kumar
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
Sorry for the previous reply,
Here goes the full explaination:
> Rob,
>
> On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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)
-- 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.
So, putting these drm_panel controls inside individual bridge drivers will work,
but keeping them in crtc_helpers might break stuff.
Thanks and regards,
Ajay Kumar
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
Rob,
On Tue, Apr 22, 2014 at 5:04 PM, Rob Clark 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,
> ... and so on.
>
> If you don't need a real bridge, just create one of these w/
> pb->bridge==NULL, otherwise create it as a wrapper for your real
> bridge.
>
> BR,
> -R
>
> On Mon, Apr 21, 2014 at 6:39 PM, Ajay Kumar
> wrote:
>> attach ptn3460 connector to drm_panel and support drm_panel routines,
>> if a valid drm_panel object is passed to ptn3460_init.
>>
>> Signed-off-by: Ajay Kumar
>> ---
>> Changes since V1:
>> Address few coding style comments from Jingoo Han
>>
>> drivers/gpu/drm/bridge/Kconfig |1 +
>> drivers/gpu/drm/bridge/ptn3460.c| 20 +++-
>> drivers/gpu/drm/exynos/exynos_dp_core.c | 16
>> include/drm/bridge/ptn3460.h|6 --
>> 4 files changed, 36 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>> index 884923f..3bc6845 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -2,4 +2,5 @@ config DRM_PTN3460
>> tristate "PTN3460 DP/LVDS bridge"
>> depends on DRM
>> select DRM_KMS_HELPER
>> + select DRM_PANEL
>> ---help---
>> diff --git a/drivers/gpu/drm/bridge/ptn3460.c
>> b/drivers/gpu/drm/bridge/ptn3460.c
>> index f1d2afc..3920202 100644
>> --- a/drivers/gpu/drm/bridge/ptn3460.c
>> +++ b/drivers/gpu/drm/bridge/ptn3460.c
>> @@ -19,6 +19,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "drmP.h"
>> #include "drm_edid.h"
>> @@ -38,6 +39,7 @@ struct ptn3460_bridge {
>> struct i2c_client *client;
>> struct drm_encoder *encoder;
>> struct drm_bridge *bridge;
>> + struct drm_panel *panel;
>> struct edid *edid;
>> int gpio_pd_n;
>> int gpio_rst_n;
>> @@ -126,6 +128,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>> gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>> }
>>
>> + drm_panel_pre_enable(ptn_bridge->panel);
>> +
>> /*
>> * There's a bug in the PTN chip where it falsely asserts hotplug
>> before
>> * it is fully functional. We're forced to wait for the maximum
>> start up
>> @@ -142,6 +146,10 @@ static void ptn3460_pre_enable(struct drm_bridge
>> *bridge)
>>
>> static void ptn3460_enable(struct drm_bridge *bridge)
>> {
>> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
>> +
>> + if (ptn_bridge->enabled)
>> + drm_panel_enable(ptn_bridge->panel);
>> }
>>
>> static void ptn3460_disable(struct drm_bridge *bridge)
>> @@ -153,6 +161,9 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>>
>> ptn_bridge->enabled = false;
>>
>> + drm_panel_disable(ptn_bridge->panel);
>> + drm_panel_post_disable(ptn_bridge->panel);
>> +
>> if (gpio_is_valid(ptn_bridge->gpio_rst_n))
>> gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>>
>> @@ -198,6 +209,7 @@ int ptn3460_get_modes(struct drm_connector *connector)
>>
>> power_off = !ptn_bridge->enabled;
>> ptn3460_pre_enable(ptn_bridge->bridge);
>> + ptn3460_enable(ptn_bridge->bridge);
>>
>> edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
>> if (!edid) {
>> @@ -265,7 +277,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
>> };
>>
>> int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
>> - struct i2c_client *client, struct device_node *node)
>> + struct i2c_client *client, struct device_node *node,
>> + struct drm_panel *panel)
>> {
>> int ret;
>> struct drm_bridge *bridge;
>> @@ -324,6 +337,11 @@ int ptn3460_init(struct drm_device *dev, struct
>> drm_encoder *encoder,
>> goto err;
>> }
>>
>> + if (panel) {
>> + ptn_bridge->panel = panel;
>> + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
>> + }
>> +
>> bridge->driver_private = ptn_bridge;
>> encoder->bridge = bridge;
>> ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
>> diff --git a/drivers/gpu/drm/exyno
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Tue, Apr 22, 2014 at 07:34:03AM -0400, Rob Clark 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);
> }
>
> ... and so on.
>
> If you don't need a real bridge, just create one of these w/
> pb->bridge==NULL, otherwise create it as a wrapper for your real
> bridge.
Yeah I think that's how I'd have implemented some generic abstraction for
drivers using the crtc helpers. This allows you to keep bridge drivers,
panel drivers and anything else you might have in your driver to feed the
pixel stream to those 2 guys separate. And it also allows you to set it
all up in different ways, e.g. using device tree metadata, or acpi or some
other table hardcoded in a video rom somewhere.
Imo we also should have something similar to chain up drm_bridge devices.
tbh I'm not terribly happy about how the current integration with the crtc
modeset helpers is done ...
-Daniel
>
> BR,
> -R
>
> On Mon, Apr 21, 2014 at 6:39 PM, Ajay Kumar
> wrote:
> > attach ptn3460 connector to drm_panel and support drm_panel routines,
> > if a valid drm_panel object is passed to ptn3460_init.
> >
> > Signed-off-by: Ajay Kumar
> > ---
> > Changes since V1:
> > Address few coding style comments from Jingoo Han
> >
> > drivers/gpu/drm/bridge/Kconfig |1 +
> > drivers/gpu/drm/bridge/ptn3460.c| 20 +++-
> > drivers/gpu/drm/exynos/exynos_dp_core.c | 16
> > include/drm/bridge/ptn3460.h|6 --
> > 4 files changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> > index 884923f..3bc6845 100644
> > --- a/drivers/gpu/drm/bridge/Kconfig
> > +++ b/drivers/gpu/drm/bridge/Kconfig
> > @@ -2,4 +2,5 @@ config DRM_PTN3460
> > tristate "PTN3460 DP/LVDS bridge"
> > depends on DRM
> > select DRM_KMS_HELPER
> > + select DRM_PANEL
> > ---help---
> > diff --git a/drivers/gpu/drm/bridge/ptn3460.c
> > b/drivers/gpu/drm/bridge/ptn3460.c
> > index f1d2afc..3920202 100644
> > --- a/drivers/gpu/drm/bridge/ptn3460.c
> > +++ b/drivers/gpu/drm/bridge/ptn3460.c
> > @@ -19,6 +19,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "drmP.h"
> > #include "drm_edid.h"
> > @@ -38,6 +39,7 @@ struct ptn3460_bridge {
> > struct i2c_client *client;
> > struct drm_encoder *encoder;
> > struct drm_bridge *bridge;
> > + struct drm_panel *panel;
> > struct edid *edid;
> > int gpio_pd_n;
> > int gpio_rst_n;
> > @@ -126,6 +128,8 @@ static void ptn3460_pre_enable(struct drm_bridge
> > *bridge)
> > gpio_set_value(ptn_bridge->gpio_rst_n, 1);
> > }
> >
> > + drm_panel_pre_enable(ptn_bridge->panel);
> > +
> > /*
> > * There's a bug in the PTN chip where it falsely asserts hotplug
> > before
> > * it is fully functional. We're forced to wait for the maximum
> > start up
> > @@ -142,6 +146,10 @@ static void ptn3460_pre_enable(struct drm_bridge
> > *bridge)
> >
> > static void ptn3460_enable(struct drm_bridge *bridge)
> > {
> > + struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
> > +
> > + if (ptn_bridge->enabled)
> > + drm_panel_enable(ptn_bridge->panel);
> > }
> >
> > static void ptn3460_disable(struct drm_bridge *bridge)
> > @@ -153,6 +161,9 @@ static void ptn3460_disable(struct drm_bridge *bridge)
> >
> > ptn_bridge->enabled = false;
> >
> > + drm_panel_disable(ptn_bridge->panel);
> > + drm_panel_post_disable(ptn_bridge->panel);
> > +
> > if (gpio_is_valid(ptn_bridge->gpio_rst_n))
> > gpio_set_value(ptn_bridge->gpio_rst_n, 1);
> >
> > @@ -198,6 +209,7 @@ int ptn3460_get_modes(struct drm_connector *connector)
> >
> > power_off = !ptn_bridge->enabled;
> > ptn3460_pre_enable(ptn_bridge->bridge);
> > + ptn3460_enable(ptn_bridge->bridge);
> >
> > edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> > if (!edid) {
> > @@ -265,7 +277,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
> > };
> >
> > int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
> > - struct i2c_client *client, struct device_node *node)
> > + struct i2c_client *client, struct device_node *node,
> > + struct drm_panel *panel)
> > {
> > int ret;
> > struct drm_bridge *bridge;
> > @@ -324,6 +337,
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
On Tue, Apr 22, 2014 at 04:09:16AM +0530, Ajay Kumar wrote:
[...]
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> b/drivers/gpu/drm/exynos/exynos_dp_core.c
[...]
> @@ -1012,9 +1013,16 @@ static int exynos_dp_create_connector(struct
> exynos_drm_display *display,
> dp->encoder = encoder;
>
> /* Pre-empt DP connector creation if there's a bridge */
> - ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder);
> - if (ret)
> + ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder, dp->drm_panel);
> + if (ret) {
> + /*
> + * Also set "dp->drm_panel = NULL" so that we don't end up
> + * controlling panel power both in exynos_dp and bridge
> + * DPMS routines.
> + */
> + dp->drm_panel = NULL;
> return 0;
> + }
This kind of hack should set off an alarm that you're doing something
wrong. I'm not sure integration of bridges and panels has been thought
about or discussed a lot, but this doesn't look right to me at all.
Thierry
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
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);
}
... and so on.
If you don't need a real bridge, just create one of these w/
pb->bridge==NULL, otherwise create it as a wrapper for your real
bridge.
BR,
-R
On Mon, Apr 21, 2014 at 6:39 PM, Ajay Kumar wrote:
> attach ptn3460 connector to drm_panel and support drm_panel routines,
> if a valid drm_panel object is passed to ptn3460_init.
>
> Signed-off-by: Ajay Kumar
> ---
> Changes since V1:
> Address few coding style comments from Jingoo Han
>
> drivers/gpu/drm/bridge/Kconfig |1 +
> drivers/gpu/drm/bridge/ptn3460.c| 20 +++-
> drivers/gpu/drm/exynos/exynos_dp_core.c | 16
> include/drm/bridge/ptn3460.h|6 --
> 4 files changed, 36 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 884923f..3bc6845 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -2,4 +2,5 @@ config DRM_PTN3460
> tristate "PTN3460 DP/LVDS bridge"
> depends on DRM
> select DRM_KMS_HELPER
> + select DRM_PANEL
> ---help---
> diff --git a/drivers/gpu/drm/bridge/ptn3460.c
> b/drivers/gpu/drm/bridge/ptn3460.c
> index f1d2afc..3920202 100644
> --- a/drivers/gpu/drm/bridge/ptn3460.c
> +++ b/drivers/gpu/drm/bridge/ptn3460.c
> @@ -19,6 +19,7 @@
> #include
> #include
> #include
> +#include
>
> #include "drmP.h"
> #include "drm_edid.h"
> @@ -38,6 +39,7 @@ struct ptn3460_bridge {
> struct i2c_client *client;
> struct drm_encoder *encoder;
> struct drm_bridge *bridge;
> + struct drm_panel *panel;
> struct edid *edid;
> int gpio_pd_n;
> int gpio_rst_n;
> @@ -126,6 +128,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
> gpio_set_value(ptn_bridge->gpio_rst_n, 1);
> }
>
> + drm_panel_pre_enable(ptn_bridge->panel);
> +
> /*
> * There's a bug in the PTN chip where it falsely asserts hotplug
> before
> * it is fully functional. We're forced to wait for the maximum start
> up
> @@ -142,6 +146,10 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
>
> static void ptn3460_enable(struct drm_bridge *bridge)
> {
> + struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
> +
> + if (ptn_bridge->enabled)
> + drm_panel_enable(ptn_bridge->panel);
> }
>
> static void ptn3460_disable(struct drm_bridge *bridge)
> @@ -153,6 +161,9 @@ static void ptn3460_disable(struct drm_bridge *bridge)
>
> ptn_bridge->enabled = false;
>
> + drm_panel_disable(ptn_bridge->panel);
> + drm_panel_post_disable(ptn_bridge->panel);
> +
> if (gpio_is_valid(ptn_bridge->gpio_rst_n))
> gpio_set_value(ptn_bridge->gpio_rst_n, 1);
>
> @@ -198,6 +209,7 @@ int ptn3460_get_modes(struct drm_connector *connector)
>
> power_off = !ptn_bridge->enabled;
> ptn3460_pre_enable(ptn_bridge->bridge);
> + ptn3460_enable(ptn_bridge->bridge);
>
> edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
> if (!edid) {
> @@ -265,7 +277,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
> };
>
> int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
> - struct i2c_client *client, struct device_node *node)
> + struct i2c_client *client, struct device_node *node,
> + struct drm_panel *panel)
> {
> int ret;
> struct drm_bridge *bridge;
> @@ -324,6 +337,11 @@ int ptn3460_init(struct drm_device *dev, struct
> drm_encoder *encoder,
> goto err;
> }
>
> + if (panel) {
> + ptn_bridge->panel = panel;
> + drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
> + }
> +
> bridge->driver_private = ptn_bridge;
> encoder->bridge = bridge;
> ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
> b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index dbc5ccc..4853f31 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -989,13 +989,14 @@ static bool find_bridge(const char *compat, struct
> bridge_init *bridge)
>
> /* returns the number of bridges attached */
> static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
> - struct drm_encoder *encoder)
[PATCH V2 7/9] drm/bridge: ptn3460: add drm_panel controls
attach ptn3460 connector to drm_panel and support drm_panel routines,
if a valid drm_panel object is passed to ptn3460_init.
Signed-off-by: Ajay Kumar
---
Changes since V1:
Address few coding style comments from Jingoo Han
drivers/gpu/drm/bridge/Kconfig |1 +
drivers/gpu/drm/bridge/ptn3460.c| 20 +++-
drivers/gpu/drm/exynos/exynos_dp_core.c | 16
include/drm/bridge/ptn3460.h|6 --
4 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 884923f..3bc6845 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -2,4 +2,5 @@ config DRM_PTN3460
tristate "PTN3460 DP/LVDS bridge"
depends on DRM
select DRM_KMS_HELPER
+ select DRM_PANEL
---help---
diff --git a/drivers/gpu/drm/bridge/ptn3460.c b/drivers/gpu/drm/bridge/ptn3460.c
index f1d2afc..3920202 100644
--- a/drivers/gpu/drm/bridge/ptn3460.c
+++ b/drivers/gpu/drm/bridge/ptn3460.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include "drmP.h"
#include "drm_edid.h"
@@ -38,6 +39,7 @@ struct ptn3460_bridge {
struct i2c_client *client;
struct drm_encoder *encoder;
struct drm_bridge *bridge;
+ struct drm_panel *panel;
struct edid *edid;
int gpio_pd_n;
int gpio_rst_n;
@@ -126,6 +128,8 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
gpio_set_value(ptn_bridge->gpio_rst_n, 1);
}
+ drm_panel_pre_enable(ptn_bridge->panel);
+
/*
* There's a bug in the PTN chip where it falsely asserts hotplug before
* it is fully functional. We're forced to wait for the maximum start up
@@ -142,6 +146,10 @@ static void ptn3460_pre_enable(struct drm_bridge *bridge)
static void ptn3460_enable(struct drm_bridge *bridge)
{
+ struct ptn3460_bridge *ptn_bridge = bridge->driver_private;
+
+ if (ptn_bridge->enabled)
+ drm_panel_enable(ptn_bridge->panel);
}
static void ptn3460_disable(struct drm_bridge *bridge)
@@ -153,6 +161,9 @@ static void ptn3460_disable(struct drm_bridge *bridge)
ptn_bridge->enabled = false;
+ drm_panel_disable(ptn_bridge->panel);
+ drm_panel_post_disable(ptn_bridge->panel);
+
if (gpio_is_valid(ptn_bridge->gpio_rst_n))
gpio_set_value(ptn_bridge->gpio_rst_n, 1);
@@ -198,6 +209,7 @@ int ptn3460_get_modes(struct drm_connector *connector)
power_off = !ptn_bridge->enabled;
ptn3460_pre_enable(ptn_bridge->bridge);
+ ptn3460_enable(ptn_bridge->bridge);
edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
if (!edid) {
@@ -265,7 +277,8 @@ struct drm_connector_funcs ptn3460_connector_funcs = {
};
int ptn3460_init(struct drm_device *dev, struct drm_encoder *encoder,
- struct i2c_client *client, struct device_node *node)
+ struct i2c_client *client, struct device_node *node,
+ struct drm_panel *panel)
{
int ret;
struct drm_bridge *bridge;
@@ -324,6 +337,11 @@ int ptn3460_init(struct drm_device *dev, struct
drm_encoder *encoder,
goto err;
}
+ if (panel) {
+ ptn_bridge->panel = panel;
+ drm_panel_attach(ptn_bridge->panel, &ptn_bridge->connector);
+ }
+
bridge->driver_private = ptn_bridge;
encoder->bridge = bridge;
ptn_bridge->connector.polled = DRM_CONNECTOR_POLL_HPD;
diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c
b/drivers/gpu/drm/exynos/exynos_dp_core.c
index dbc5ccc..4853f31 100644
--- a/drivers/gpu/drm/exynos/exynos_dp_core.c
+++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
@@ -989,13 +989,14 @@ static bool find_bridge(const char *compat, struct
bridge_init *bridge)
/* returns the number of bridges attached */
static int exynos_drm_attach_lcd_bridge(struct drm_device *dev,
- struct drm_encoder *encoder)
+ struct drm_encoder *encoder, struct drm_panel *panel)
{
struct bridge_init bridge;
int ret;
if (find_bridge("nxp,ptn3460", &bridge)) {
- ret = ptn3460_init(dev, encoder, bridge.client, bridge.node);
+ ret = ptn3460_init(dev, encoder, bridge.client, bridge.node,
+ panel);
if (!ret)
return 1;
}
@@ -1012,9 +1013,16 @@ static int exynos_dp_create_connector(struct
exynos_drm_display *display,
dp->encoder = encoder;
/* Pre-empt DP connector creation if there's a bridge */
- ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder);
- if (ret)
+ ret = exynos_drm_attach_lcd_bridge(dp->drm_dev, encoder, dp->drm_panel);
+ if (ret) {
+ /*
+* Also set "dp->drm_panel = NULL" so that we don't end up
+* controlling panel power bo
