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

[Daniel Vetter <daniel@ffwll.ch>]

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@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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch