[PATCH V2 2/9] drm/panel: add pre_enable and post_disable routines

[Ajay kumar]

Daniel,

On Wed, Apr 23, 2014 at 12:59 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Apr 22, 2014 at 08:06:19PM +0530, Ajay kumar wrote:
>> Hi Thierry,
>>
>>
>> On Tue, Apr 22, 2014 at 1:49 PM, Thierry Reding <thierry.reding at gmail.com>
>> wrote:
>> > On Tue, Apr 22, 2014 at 04:09:11AM +0530, Ajay Kumar wrote:
>> >> Most of the panels need an init sequence as mentioned below:
>> >>       -- poweron LCD unit/LCD_EN
>> >>       -- start video data
>> >>       -- poweron LED unit/BL_EN
>> >> And, a de-init sequence as mentioned below:
>> >>       -- poweroff LED unit/BL_EN
>> >>       -- stop video data
>> >>       -- poweroff LCD unit/LCD_EN
>> >> With existing callbacks for drm panel, we cannot accomodate such panels,
>> >> since only two callbacks, i.e "panel_enable" and panel_disable are
>> supported.
>> >>
>> >> This patch adds:
>> >>       -- "pre_enable" callback which can be called before
>> >>       the actual video data is on, and then call the "enable"
>> >>       callback after the video data is available.
>> >>
>> >>       -- "post_disable" callback which can be called after
>> >>       the video data is off, and use "disable" callback
>> >>       to do something before switching off the video data.
>> >>
>> >> Now, we can easily map the above scenario as shown below:
>> >>       poweron LCD unit/LCD_EN = "pre_enable" callback
>> >>       poweron LED unit/BL_EN = "enable" callback
>> >>       poweroff LED unit/BL_EN = "disable" callback
>> >>       poweroff LCD unit/LCD_EN = "post_disable" callback
>> >
>> > I don't like this. What happens when the next panel comes around that
>> > has a yet slightly different requirement? Will we introduce a new
>> > pre_pre_enable() and post_post_disable() function then?
>> >
>> As I have already explained, these 2 callbacks are sufficient enough to
>> take care
>> the power up/down sequence for LVDS and eDP panels. And, definitely having
>> just 2 callbacks "enable" and "disable" is not at all sufficient.
>>
>> I initially thought of using panel_simple_enable from panel-simple driver.
>> But it doesn't go well with case wherein there are 2 regulators(one for LCD
>> and one for LED)
>> a BL_EN signal etc. And, often(Believe me, I have referred to both eDP
>> panel datasheets
>> and LVDS panel datasheets) proper powerup sequence for such panels is as
>> mentioned below:
>>
>> powerup:
>> -- power up the supply (LCD_VCC).
>> -- start video data.
>> -- enable backlight.
>>
>> powerdown
>> -- disable backlight.
>> -- stop video data.
>> -- power off the supply (LCD VCC)
>>
>> For the above cases, if I have to somehow fit in all the required settings,
>> it breaks the sequence and I end up getting glitches during bootup/DPMS.
>>
>> Also, when the drm_bridge can support pre_enable and post_disable, why not
>> drm_panel provide that both should work in tandem?
>
> Imo this makes sense, especially if we go through with the idea talked
> about yesterday of creating a drm_bridge to wrap-up a drm_panel with
> sufficient isolation between all components.
> -Daniel

Actually, I tried implementing this for ptn3460. But, it breaks the working.
As explained in the other patch(reply for Rob), we cannot truly ISOLATE
the panel controls from bridge controls and keep them as seperate.
They should be kept together, in the individual bridge chip drivers,
so that the bridge chip driver can decide which panel control to call
at what point.

So, I think combining bridge chip and panel controls was really a bad idea.
I will implement the basic panel controls required by the bridge
as optional bridge properties via DT. By that way, at least the driver
remains robust.


Thanks and regards,
Ajay Kumar