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

[Thomas Zimmermann]

Hi Sam,

long time no see.

Am 11.06.24 um 19:55 schrieb Sam Ravnborg:
> 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 at 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 at 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

Makes perfect sens to me.

>
> 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.

OK. I only used BL_CORE because it was there already.


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

Should I attempt to fix that? Many drivers appear to do something like

   props.brightness = ...
   props.power = UNBLANK
   backlight_update_status()

That's the same pattern as in backlight_enable().

Best regards
Thomas
>
> 	Sam
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)