| Message ID | 20240611125321.6927-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | backlight: Introduce power-state constants | expand |
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
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@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 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 >
Hi Thomas, On Wed, Jun 12, 2024 at 09:26:11AM +0200, Thomas Zimmermann wrote: > Hi Sam, > > long time no see. Had some spare time between jobs, started on my new job last week. Time will tell if there will be energy and time for hobby 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. > > 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(). I would keep the changes at a minimum, hoping someone else jumps in and do the cleanup. Then you can keep the patches that remove the fbdev dependency simple and easy to review (and thus get applied). Maybe the obvious places, and do the simple replacement for the rest.. The drivers initialize and use the backlight properties in interesting ways so that would require a bit more effort to implement and review. I did it once for most of drm - but it was buggy so I ended up scrapping the patches :-( Sam
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) + /** * @type: The type of backlight supported. * @@ -346,7 +350,7 @@ static inline int backlight_enable(struct backlight_device *bd) if (!bd) return 0; - bd->props.power = FB_BLANK_UNBLANK; + bd->props.power = BL_CORE_UNBLANK; bd->props.state &= ~BL_CORE_FBBLANK; return backlight_update_status(bd); @@ -361,7 +365,7 @@ static inline int backlight_disable(struct backlight_device *bd) if (!bd) return 0; - bd->props.power = FB_BLANK_POWERDOWN; + bd->props.power = BL_CORE_POWERDOWN; bd->props.state |= BL_CORE_FBBLANK; return backlight_update_status(bd); @@ -380,7 +384,7 @@ static inline int backlight_disable(struct backlight_device *bd) */ static inline bool backlight_is_blank(const struct backlight_device *bd) { - return bd->props.power != FB_BLANK_UNBLANK || + return bd->props.power != BL_CORE_UNBLANK || bd->props.state & (BL_CORE_SUSPENDED | BL_CORE_FBBLANK); }
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. 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. 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(-)