Hi, Lars-Peter Clausen. > > Why do we need a new notifier chain? Can't we introduce a new event for > > the > > existing chain? Yes, it's good point. I will consider a new event as you said. thank you for your advice. :) > -----Original Message----- > From: Inki Dae [mailto:inki.dae@xxxxxxxxxxx] > Sent: Monday, September 26, 2011 7:37 PM > To: 'Lars-Peter Clausen' > Cc: 'FlorianSchandinat@xxxxxx'; 'linux-fbdev@xxxxxxxxxxxxxxx'; > 'akpm@xxxxxxxxxxxxxxxxxxxx'; 'linux-kernel@xxxxxxxxxxxxxxx'; > 'kyungmin.park@xxxxxxxxxxx' > Subject: RE: [PATCH] FB: add early fb blank feature. > > Hi, Lars-Peter Clausen. > > Sorry for being late. > > > -----Original Message----- > > From: Lars-Peter Clausen [mailto:lars@xxxxxxxxxx] > > Sent: Thursday, September 15, 2011 8:37 PM > > To: Inki Dae > > Cc: FlorianSchandinat@xxxxxx; linux-fbdev@xxxxxxxxxxxxxxx; akpm@linux- > > foundation.org; linux-kernel@xxxxxxxxxxxxxxx; kyungmin.park@xxxxxxxxxxx > > Subject: Re: [PATCH] FB: add early fb blank feature. > > > > Hi > > > > I have a LCD panel with an similar issue, and I think the idea to > > introduce a > > early fb blank event is the right solution. I have some comments and > > questions > > on this particular implementation though. > > > > On 09/09/2011 07:03 AM, Inki Dae wrote: > > > this patch adds early fb blank feature that this is a callback of > > > lcd panel driver would be called prior to fb driver's one. > > > in case of MIPI-DSI based video mode LCD Panel, for lcd power off, > > > the power off commands should be transferred to lcd panel with display > > > and mipi-dsi controller enabled because the commands is set to lcd > panel > > > at vsync porch period. on the other hand, in opposite case, the > callback > > > of fb driver should be called prior to lcd panel driver's one because > of > > > same issue. now we could handle call order to fb blank properly. > > > > > > the order is as the following: > > > > > > at fb_blank function of fbmem.c > > > -> fb_early_notifier_call_chain() > > > -> lcd panel driver's early_set_power() > > > -> info->fbops->fb_blank() > > > -> fb driver's fb_blank() > > > -> fb_notifier_call_chain() > > > -> lcd panel driver's set_power() > > > > > > > I wonder if we really need the lcd_ops early_set_power callback. I can't > > really > > imagine a situation where you need to power the LCD down only after the > > LCD > > controller has been shutdown. > > > > if fb_blank mode is changed to FB_BLANK_POWERDOWN then display controller > would be off(clock disable). On the other hand, lcd panel would be still > on. at this time, you could see some issue like sparkling on lcd panel > because video clock to be delivered to ldi module of lcd panel was > disabled. > > > So I wonder if we couldn't just have the set_power callback, but listen > to > > both > > events and call set_power for early blank events with code != > > FB_BLANK_UNBLANK > > and for normal blank events with code == FB_BLANK_UNBLANK? > > > > with this feaure, if a event is FB_BLANK_POWERDOWN then early_set_power() > callback would be called for lcd panel to be off and if FB_BLANK_UNBLANK > or FB_BLANK_NORMAL then set_power() callback would be called for lcd panel > to be on. > > > > note that early fb blank mode is valid only if lcd_ops- > >early_blank_mode > > is 1. > > > if the value is 0 then early fb blank callback would be ignored. > > > > > > this patch is based on git repository below: > > > git://github.com/schandinat/linux-2.6.git > > > branch: fbdev-next > > > commit-id: a67472ad1ae040f073e45048cbc5a01195f2e3f5 > > > > > > Signed-off-by: Inki Dae <inki.dae@xxxxxxxxxxx> > > > Signed-off-by: KyungMin Park <kyungmin.park@xxxxxxxxxxx> > > > --- > > > drivers/video/backlight/lcd.c | 77 > > ++++++++++++++++++++++++++++++++++++++-- > > > drivers/video/fb_notify.c | 31 ++++++++++++++++ > > > drivers/video/fbmem.c | 25 +++++++------ > > > include/linux/fb.h | 4 ++ > > > include/linux/lcd.h | 61 ++++++++++++++++++++++++-------- > > > > In my opinion this should be split into two patches, one adding the > early > > blank > > event, one adding support for it to the LCD framework. > > > > You are right. this patch should be split into two patches. Thank you. > > > > 5 files changed, 167 insertions(+), 31 deletions(-) > > > > > > [...] > > > diff --git a/drivers/video/fb_notify.c b/drivers/video/fb_notify.c > > > index 8c02038..3930c7c 100644 > > > --- a/drivers/video/fb_notify.c > > > +++ b/drivers/video/fb_notify.c > > > @@ -13,9 +13,20 @@ > > > #include <linux/fb.h> > > > #include <linux/notifier.h> > > > > > > +static BLOCKING_NOTIFIER_HEAD(fb_early_notifier_list); > > > static BLOCKING_NOTIFIER_HEAD(fb_notifier_list); > > > > > > /** > > > + * fb_register_early_client - register a client early notifier > > > + * @nb: notifier block to callback on events > > > + */ > > > +int fb_register_early_client(struct notifier_block *nb) > > > +{ > > > + return blocking_notifier_chain_register(&fb_early_notifier_list, > > nb); > > > +} > > > +EXPORT_SYMBOL(fb_register_early_client); > > > + > > > > Why do we need a new notifier chain? Can't we introduce a new event for > > the > > existing chain? > > > > Suppose that we have only fb_notifier_list. Once lcd_device_register() is > called by lcd panel driver, existing two notifiers would be added > fb_notifier_list and when fb_blank is called by user process, some > callback registered to the fb_notifier_list would be called two times, one > is set_power() and another one is early_set_power(). as you know, this is > not the thing we want. If there is any missing point, please give me your > comment. > > > > > +/** > > > * fb_register_client - register a client notifier > > > * @nb: notifier block to callback on events > > > */ > > > @@ -26,6 +37,16 @@ int fb_register_client(struct notifier_block *nb) > > > EXPORT_SYMBOL(fb_register_client); > > > > > > /** > > > + * fb_unregister_early_client - unregister a client early > notifier > > > + * @nb: notifier block to callback on events > > > + */ > > > +int fb_unregister_early_client(struct notifier_block *nb) > > > +{ > > > + return blocking_notifier_chain_unregister(&fb_early_notifier_list, > > nb); > > > +} > > > +EXPORT_SYMBOL(fb_unregister_early_client); > > > + > > > +/** > > > * fb_unregister_client - unregister a client notifier > > > * @nb: notifier block to callback on events > > > */ > > > @@ -36,6 +57,16 @@ int fb_unregister_client(struct notifier_block *nb) > > > EXPORT_SYMBOL(fb_unregister_client); > > > > > > /** > > > + * fb_early_notifier_call_chain - early notify clients of fb_events > > > + * > > > + */ > > > +int fb_early_notifier_call_chain(unsigned long val, void *v) > > > +{ > > > + return blocking_notifier_call_chain(&fb_early_notifier_list, val, > > v); > > > +} > > > +EXPORT_SYMBOL_GPL(fb_early_notifier_call_chain); > > > + > > > +/** > > > * fb_notifier_call_chain - notify clients of fb_events > > > * > > > */ > > > diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c > > > index ad93629..cf22516 100644 > > > --- a/drivers/video/fbmem.c > > > +++ b/drivers/video/fbmem.c > > > @@ -1031,24 +1031,25 @@ fb_set_var(struct fb_info *info, struct > > fb_var_screeninfo *var) > > > > > > int > > > fb_blank(struct fb_info *info, int blank) > > > -{ > > > - int ret = -EINVAL; > > > +{ > > > + struct fb_event event; > > > + int ret = -EINVAL; > > > > > > - if (blank > FB_BLANK_POWERDOWN) > > > - blank = FB_BLANK_POWERDOWN; > > > + if (blank > FB_BLANK_POWERDOWN) > > > + blank = FB_BLANK_POWERDOWN; > > > > > > - if (info->fbops->fb_blank) > > > - ret = info->fbops->fb_blank(blank, info); > > > + event.info = info; > > > + event.data = ␣ > > > > > > - if (!ret) { > > > - struct fb_event event; > > > + fb_early_notifier_call_chain(FB_EVENT_BLANK, &event); > > > > > > - event.info = info; > > > - event.data = ␣ > > > + if (info->fbops->fb_blank) > > > + ret = info->fbops->fb_blank(blank, info); > > > > I think we have to handle the case where the fb_blank callback fails and > > should > > somehow revert the effects of the early blank event. > > > > You are right. it should be reverted with fail. thank you. > > > > > > + > > > + if (!ret) > > > fb_notifier_call_chain(FB_EVENT_BLANK, &event); > > > - } > > > > > > > > > > > > - return ret; > > > + return ret; > > > } > > > > > > static long do_fb_ioctl(struct fb_info *info, unsigned int cmd, > > > diff --git a/include/linux/fb.h b/include/linux/fb.h > > > index 1d6836c..1d7d995 100644 > > > --- a/include/linux/fb.h > > > +++ b/include/linux/fb.h > > > @@ -562,6 +562,10 @@ struct fb_blit_caps { > > > u32 flags; > > > }; > > > > > > +extern int fb_register_early_client(struct notifier_block *nb); > > > +extern int fb_unregister_early_client(struct notifier_block *nb); > > > +extern int fb_early_notifier_call_chain(unsigned long val, void *v); > > > + > > > extern int fb_register_client(struct notifier_block *nb); > > > extern int fb_unregister_client(struct notifier_block *nb); > > > extern int fb_notifier_call_chain(unsigned long val, void *v); > > > diff --git a/include/linux/lcd.h b/include/linux/lcd.h > > > index 8877123..930d1cc 100644 > > > --- a/include/linux/lcd.h > > > +++ b/include/linux/lcd.h > > > @@ -37,10 +37,21 @@ struct lcd_properties { > > > }; > > > > > > struct lcd_ops { > > > - /* Get the LCD panel power status (0: full on, 1..3: controller > > > - power on, flat panel power off, 4: full off), see FB_BLANK_XXX */ > > > + /* > > > + * Get the LCD panel power status (0: full on, 1..3: controller > > > + * power on, flat panel power off, 4: full off), see FB_BLANK_XXX > > > + */ > > > int (*get_power)(struct lcd_device *); > > > - /* Enable or disable power to the LCD (0: on; 4: off, see > > FB_BLANK_XXX) */ > > > + /* > > > + * Get the current contrast setting (0-max_contrast) and > > > > ??? > > > > Ah, I missed it. this is a comment wrong so I will remove it. thank you. > > > > + * Enable or disable power to the LCD (0: on; 4: off, see > > FB_BLANK_XXX) > > > + * this callback would be called proir to fb driver's fb_blank > > callback. > > > + */ > > > + int (*early_set_power)(struct lcd_device *, int power); > > > + /* > > > + * Get the current contrast setting (0-max_contrast) > > > + * Enable or disable power to the LCD (0: on; 4: off, see > > FB_BLANK_XXX) > > > + */ > > > int (*set_power)(struct lcd_device *, int power); > > > /* Get the current contrast setting (0-max_contrast) */ > > > int (*get_contrast)(struct lcd_device *); > > > @@ -48,21 +59,35 @@ struct lcd_ops { > > > int (*set_contrast)(struct lcd_device *, int contrast); > > > /* Set LCD panel mode (resolutions ...) */ > > > int (*set_mode)(struct lcd_device *, struct fb_videomode *); > > > - /* Check if given framebuffer device is the one LCD is bound to; > > > - return 0 if not, !=0 if it is. If NULL, lcd always matches the fb. > > */ > > > + /* > > > + * Check if given framebuffer device is the one LCD is bound to; > > > + * return 0 if not, !=0 if it is. If NULL, lcd always matches the > > fb. > > > + */ > > > int (*check_fb)(struct lcd_device *, struct fb_info *); > > > + > > > + /* > > > + * indicate whether enabling early blank mode or not. > > > + * (0: disable; 1: enable); > > > + * if enabled, lcd blank callback would be called prior > > > + * to fb blank callback. > > > + */ > > > + unsigned int early_blank_mode; > > > > I think it should be sufficient to check early_set_power for NULL > instead > > of > > adding this additional flag. > > > > You are right. I will modify it. thank you. > > > > }; > > > > > > struct lcd_device { > > > struct lcd_properties props; > > > - /* This protects the 'ops' field. If 'ops' is NULL, the driver that > > > - registered this device has been unloaded, and if > > class_get_devdata() > > > - points to something in the body of that driver, it is also > > invalid. */ > > > + /* > > > + * This protects the 'ops' field. If 'ops' is NULL, the driver that > > > + * registered this device has been unloaded, and if > > class_get_devdata() > > > + * points to something in the body of that driver, it is also > > invalid. > > > + */ > > > struct mutex ops_lock; > > > /* If this is NULL, the backing module is unloaded */ > > > struct lcd_ops *ops; > > > /* Serialise access to set_power method */ > > > struct mutex update_lock; > > > + /* The framebuffer early notifier block */ > > > + struct notifier_block fb_early_notif; > > > /* The framebuffer notifier block */ > > > struct notifier_block fb_notif; > > > > > > @@ -72,16 +97,22 @@ struct lcd_device { > > > struct lcd_platform_data { > > > /* reset lcd panel device. */ > > > int (*reset)(struct lcd_device *ld); > > > - /* on or off to lcd panel. if 'enable' is 0 then > > > - lcd power off and 1, lcd power on. */ > > > + /* > > > + * on or off to lcd panel. if 'enable' is 0 then > > > + * lcd power off and 1, lcd power on. > > > + */ > > > int (*power_on)(struct lcd_device *ld, int enable); > > > > > > - /* it indicates whether lcd panel was enabled > > > - from bootloader or not. */ > > > + /* > > > + * it indicates whether lcd panel was enabled > > > + * from bootloader or not. > > > + */ > > > int lcd_enabled; > > > - /* it means delay for stable time when it becomes low to high > > > - or high to low that is dependent on whether reset gpio is > > > - low active or high active. */ > > > + /* > > > + * it means delay for stable time when it becomes low to high > > > + * or high to low that is dependent on whether reset gpio is > > > + * low active or high active. > > > + */ > > > > The formatting cleanup patches should go into a separate patch. > > Ok, get it. and I will re-send this patch. thank you again. :) -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
