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@xxxxxxx>
> ---
> 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@xxxxxxxxx>
> 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
