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@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
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)
