On Fri, Apr 25, 2014 at 11:34 AM, Ajay kumar <ajaynumb@xxxxxxxxx> wrote: > On Friday, April 25, 2014, Thierry Reding <thierry.reding@xxxxxxxxx> 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@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |