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

[Ajay kumar]

ping!

On Fri, Apr 25, 2014 at 11:46 PM, Ajay kumar <ajaynumb at gmail.com> wrote:
> 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