Re: [PATCH 3/6] backlight/omap1-bl: Replace FB_BLANK_ states with simple on/off

[Daniel Thompson <daniel.thompson@linaro.org>]

On Wed, Mar 13, 2024 at 04:45:02PM +0100, Thomas Zimmermann wrote:
> The backlight is on for fb_blank eq FB_BLANK_UNBLANK, or off for
> any other value in fb_blank. But the field fb_blank in struct
> backlight_properties is deprecated and should not be used any
> longer.
>
> Replace the test for fb_blank in omap's backlight code with a
> simple boolean parameter and push the test into the update_status
> helper. Instead of reading fb_blank directly, decode the backlight
> device's status with backlight_is_blank().
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/video/backlight/omap1_bl.c | 46 ++++++++++++++----------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/video/backlight/omap1_bl.c b/drivers/video/backlight/omap1_bl.c
> index 84d148f385951..3fd8bbb7f5877 100644
> --- a/drivers/video/backlight/omap1_bl.c
> +++ b/drivers/video/backlight/omap1_bl.c
> @@ -9,7 +9,6 @@
>  #include <linux/kernel.h>
>  #include <linux/init.h>
>  #include <linux/platform_device.h>
> -#include <linux/fb.h>
>  #include <linux/backlight.h>
>  #include <linux/slab.h>
>  #include <linux/platform_data/omap1_bl.h>
> @@ -20,7 +19,7 @@
>  #define OMAPBL_MAX_INTENSITY		0xff
>
>  struct omap_backlight {
> -	int powermode;
> +	bool power;

The new name is hard to read in several of the conditionals below (which
were previously "documented" by the comparisons to constants.

This boolean effectively controls what we send to omapbl_send_enable().
On that basis I'd rather it was called something like "enabled".


>  	int current_intensity;
>
>  	struct device *dev;
> @@ -37,21 +36,14 @@ static inline void omapbl_send_enable(int enable)
>  	omap_writeb(enable, OMAP_PWL_CLK_ENABLE);
>  }
>
> -static void omapbl_blank(struct omap_backlight *bl, int mode)
> +static void omapbl_power(struct omap_backlight *bl, bool on)

As above omapbl_enable would be better.


>  {
> -	switch (mode) {
> -	case FB_BLANK_NORMAL:
> -	case FB_BLANK_VSYNC_SUSPEND:
> -	case FB_BLANK_HSYNC_SUSPEND:
> -	case FB_BLANK_POWERDOWN:
> -		omapbl_send_intensity(0);
> -		omapbl_send_enable(0);
> -		break;
> -
> -	case FB_BLANK_UNBLANK:
> +	if (on) {
>  		omapbl_send_intensity(bl->current_intensity);
>  		omapbl_send_enable(1);
> -		break;
> +	} else {
> +		omapbl_send_intensity(0);
> +		omapbl_send_enable(0);
>  	}
>  }
>
> @@ -61,7 +53,7 @@ static int omapbl_suspend(struct device *dev)
>  	struct backlight_device *bl_dev = dev_get_drvdata(dev);
>  	struct omap_backlight *bl = bl_get_data(bl_dev);
>
> -	omapbl_blank(bl, FB_BLANK_POWERDOWN);
> +	omapbl_power(bl, false);
>  	return 0;
>  }
>
> @@ -70,33 +62,37 @@ static int omapbl_resume(struct device *dev)
>  	struct backlight_device *bl_dev = dev_get_drvdata(dev);
>  	struct omap_backlight *bl = bl_get_data(bl_dev);
>
> -	omapbl_blank(bl, bl->powermode);
> +	omapbl_power(bl, bl->power);
>  	return 0;
>  }
>  #endif
>
> -static int omapbl_set_power(struct backlight_device *dev, int state)
> +static void omapbl_set_power(struct backlight_device *dev, bool on)

May also like a new name...


>  {
>  	struct omap_backlight *bl = bl_get_data(dev);
>
> -	omapbl_blank(bl, state);
> -	bl->powermode = state;
> -
> -	return 0;
> +	omapbl_power(bl, on);
> +	bl->power = on;
>  }
>
>  static int omapbl_update_status(struct backlight_device *dev)
>  {
>  	struct omap_backlight *bl = bl_get_data(dev);
> +	bool power;
>
>  	if (bl->current_intensity != dev->props.brightness) {
> -		if (bl->powermode == FB_BLANK_UNBLANK)
> +		if (bl->power)
>  			omapbl_send_intensity(dev->props.brightness);
>  		bl->current_intensity = dev->props.brightness;
>  	}
>
> -	if (dev->props.fb_blank != bl->powermode)
> -		omapbl_set_power(dev, dev->props.fb_blank);
> +	if (backlight_is_blank(dev))
> +		power = false;
> +	else
> +		power = true;

I'd be happy with:

    enable = !backlight_is_blank()


Daniel.