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.
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?
> 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.
> 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?
> +/**
> * 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.
> +
> + 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
???
> + * 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.
> };
>
> 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.
--
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
