Re: [PATCH 01/17] backlight: Add BL_CORE_ constants for power states

[Sam Ravnborg <sam@ravnborg.org>]

Hi Thomas.

On Tue, Jun 11, 2024 at 02:41:56PM +0200, Thomas Zimmermann wrote:
> Duplicate FB_BLANK_ constants as BL_CORE_ constants in the backlight
> header file. Allows backlight drivers to avoid including the fbdev
> header file and removes a compile-time dependency between the two
> subsystems.
Good to remove this dependency.
> 
> The new BL_CORE constants have the same values as their FB_BLANK_
> counterparts. Hence UAPI and internal semantics do not change. The
> backlight drivers can be converted one by one.
This seems like the right approach.

> 
> Backlight code or drivers do not use FB_BLANK_VSYNC_SUSPEND and
> FB_BLANK_HSYNC_SUSPEND, so no new constants for these are being
> added.
> 
> The semantics of FB_BLANK_NORMAL appear inconsistent. In fbdev,
> NORMAL means display off with sync enabled. In backlight code,
> this translates to turn the backlight off, but some drivers
> interpret it as backlight on. So we keep the current code as is,
> but mark BL_CORE_NORMAL as deprecated. Drivers should be fixed
> and the constant removed. This affects ams369fg06 and a few DRM
> panel drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  Documentation/ABI/stable/sysfs-class-backlight |  7 ++++---
>  include/linux/backlight.h                      | 16 ++++++++++------
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/ABI/stable/sysfs-class-backlight b/Documentation/ABI/stable/sysfs-class-backlight
> index 023fb52645f8b..6102d6bebdf9a 100644
> --- a/Documentation/ABI/stable/sysfs-class-backlight
> +++ b/Documentation/ABI/stable/sysfs-class-backlight
> @@ -3,10 +3,11 @@ Date:		April 2005
>  KernelVersion:	2.6.12
>  Contact:	Richard Purdie <rpurdie@rpsys.net>
>  Description:
> -		Control BACKLIGHT power, values are FB_BLANK_* from fb.h
> +		Control BACKLIGHT power, values are compatible with
> +		FB_BLANK_* from fb.h
>  
> -		 - FB_BLANK_UNBLANK (0)   : power on.
> -		 - FB_BLANK_POWERDOWN (4) : power off
> +		 - 0 (FB_BLANK_UNBLANK)   : power on.
> +		 - 4 (FB_BLANK_POWERDOWN) : power off
>  Users:		HAL
>  
>  What:		/sys/class/backlight/<backlight>/brightness
> diff --git a/include/linux/backlight.h b/include/linux/backlight.h
> index 19a1c0e22629d..e0cfd89ffadd2 100644
> --- a/include/linux/backlight.h
> +++ b/include/linux/backlight.h
> @@ -210,14 +210,18 @@ struct backlight_properties {
>  	 * When the power property is updated update_status() is called.
>  	 *
>  	 * The possible values are: (0: full on, 1 to 3: power saving
> -	 * modes; 4: full off), see FB_BLANK_XXX.
> +	 * modes; 4: full off), see BL_CORE_XXX constants.
>  	 *
>  	 * When the backlight device is enabled @power is set
> -	 * to FB_BLANK_UNBLANK. When the backlight device is disabled
> -	 * @power is set to FB_BLANK_POWERDOWN.
> +	 * to BL_CORE_UNBLANK. When the backlight device is disabled
> +	 * @power is set to BL_CORE_POWERDOWN.
>  	 */
>  	int power;
>  
> +#define BL_CORE_UNBLANK		(0)
> +#define BL_CORE_NORMAL		(1) // deprecated; don't use in new code
> +#define BL_CORE_POWERDOWN	(4)

When introducing backlight specific constants then it would be good to
get away from the ackward fbdev naming and use something that is more
obvious.

Something like:

#define BACKLIGHT_POWER_ON	0
#define BACKLIGHT_POWER_OFF	4
#define BACKLIGHT_POWER_NORMAL	1	// deprecated; don't use in new code

This will make the code more obvious to read / understand - at least
IMO.

The proposal did not use the BL_CORE_ naming, lets keep this for
suspend/resume stuff.

On top of this - many users of the power states could benefit using the
backlight_enable()/backlight_disable() helpers, but that's another story.

	Sam