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

[Ajay kumar]

On Fri, Apr 25, 2014 at 11:34 AM, Ajay kumar <ajaynumb at gmail.com> wrote:
> On Friday, April 25, 2014, Thierry Reding <thierry.reding at gmail.com> wrote:
>>
>> On Thu, Apr 24, 2014 at 12:56:02AM +0530, Ajay kumar wrote:
>> > Thierry,
>> >
>> > On Wed, Apr 23, 2014 at 1:12 PM, Thierry Reding
>> > <thierry.reding at gmail.com> wrote:
>> > > On Wed, Apr 23, 2014 at 09:29:15AM +0200, Daniel Vetter wrote:
>> [...]
>> > >> 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.
>> > >
>> > > I'm not at all comfortable with these. The names are totally confusing.
>> > > Next somebody will need to do something after the panel has been enabled
>> > > and we'll have to introduce .post_enable() and .pre_disable() functions.
>> > > And worse, what if somebody needs something to be done between
>> > > .pre_enable() and .enable()? .post_pre_enable()? Where does it end?
>> > >
>> > > According to the above description the only reason why we need this is
>> > > to avoid visible glitches when the panel is already on, but the video
>> > > stream hasn't started yet. If that's the only reason why we need this,
>> > > then perhaps adding a way for a panel to expose the associated backlight
>> > > would be better?
>> > Actually, we need not expose the entire backlight device.
>> > AFAIK, the glitch is caused when you enable BL_EN before
>> > there is valid video data. So, one way to mask off the glitch is to
>> > follow this sequence:
>> > -- power up the panel.
>> > -- start video data, (start PWM here or)
>> > -- (start PWM here), enable backlight
>>
>> That's very difficult to get right, isn't it? Even if you have fine-
>> grained control over what to enable you still need a way to determine
>> _when_ it's safe to enable the backlight. Typically I guess that would
>> be the duration of one frame (or perhaps 2, depending on when the panel
>> syncs to the video signal).
> We need not "determine", its already present in LVDS datasheet.
> The LVDS datasheet says at least 200ms delay is needed from "Valid
> data" to "BL on".
>
>> Perhaps it could even by sync'ed to the VBLANK?
> No. vblanks are related to crtc. And the bridge/panel driver should be
> independent of vblank.
>
>> > The problem is that the above scenario cannot be mapped to panel-simple driver.
>> > IMO, panel_simple should provide enable/disable controls both for LCD
>> > and backlight.
>> > something like panel_simple_lcd_enable/panel_simple_led_enable, and
>> > panel_simple_lcd_disable/panel_simple_led_disable.
>>
>> That's not what the simple panel driver can do. If we want this it needs
>> to be solved in a generic way for all panels since they all need to use
>> the drm_panel_*() functions to abstract away the details from drivers
>> that use the panels.
> Right. So only I have added pre_enable and post_disable callbacks.
> Using that name won't harm existing panel drivers and still addresses
> our requirement.
>
>
> Regards,
> Ajay


Thierry :
Are you really ok with the new callback names? like pre_enable and post_disable?
Adding those new callbacks really won't harm the existing panels anyhow.

Daniel, Rob :
I think I have given sufficient amount of information as to why the concept of
drm_panel_bridge fails and why we cannot have callbacks for drm_panel
inside crtc helpers
or any other generic place.

Please let me know your final opinion so that I can start reworking on
this series.

Thanks and regards,
Ajay Kumar