Re: [PATCH RFC v3 09/11] platform/x86: ideapad-laptop: Decouple hardware & classdev brightness for keyboard backlight

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Sun, 19 Jul 2026, Rong Zhang wrote:

> Some recent models come with an ambient light sensor (ALS). On these
> models, their EC will automatically set the keyboard backlight to an
> appropriate brightness when the effective "hardware brightness" is 3.
> "Hardware brightness" can't be perfectly mapped to an LED classdev
> brightness, but the EC does use this predefined brightness value to
> represent auto mode.
> 
> Currently, the code processing keyboard backlight is coupled with LED
> classdev, making it hard to expose the auto brightness (ALS) mode to the
> userspace.
> 
> As the first step toward the goal, decouple hardware brightness from LED
> classdev brightness, and update comments about corresponding backlight
> modes.
> 
> Since upcoming changes will heavily rely on kbd_bl.last_hw_brightness,
> also convert it into an atomic_t to prevent potential race conditions.
> 
> To minimalize the diff set in upcoming changes, a trivial refactor
> also converts the initialization path into another equivalent form.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  drivers/platform/x86/lenovo/Kconfig          |   1 +
>  drivers/platform/x86/lenovo/ideapad-laptop.c | 144 ++++++++++++++++++---------
>  2 files changed, 100 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/Kconfig b/drivers/platform/x86/lenovo/Kconfig
> index 4443f40ef8aa..e92b1e900795 100644
> --- a/drivers/platform/x86/lenovo/Kconfig
> +++ b/drivers/platform/x86/lenovo/Kconfig
> @@ -16,6 +16,7 @@ config IDEAPAD_LAPTOP
>  	select INPUT_SPARSEKMAP
>  	select NEW_LEDS
>  	select LEDS_CLASS
> +	select LEDS_TRIGGERS
>  	help
>  	  This is a driver for Lenovo IdeaPad netbooks contains drivers for
>  	  rfkill switch, hotkey, fan control and backlight control.
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 4fbc904f1fc3..5aa2fedb8472 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -9,6 +9,7 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/acpi.h>
> +#include <linux/atomic.h>
>  #include <linux/backlight.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
> @@ -134,10 +135,31 @@ enum {
>  };
>  
>  /*
> - * These correspond to the number of supported states - 1
> - * Future keyboard types may need a new system, if there's a collision
> - * KBD_BL_TRISTATE_AUTO has no way to report or set the auto state
> - * so it effectively has 3 states, but needs to handle 4
> + * The enumeration has two purposes:
> + *   - as an internal identifier for all known types of keyboard backlight
> + *   - as a mandatory parameter of the KBLC command
> + *
> + * For each type, the hardware brightness values are defined as follows:
> + * +--------------------------+----------+-----+------+------+
> + * |      Hardware brightness |        0 |   1 |    2 |    3 |
> + * | Type                     |          |     |      |      |
> + * +--------------------------+----------+-----+------+------+
> + * | KBD_BL_STANDARD          |      off |  on |  N/A |  N/A |
> + * +--------------------------+----------+-----+------+------+
> + * | KBD_BL_TRISTATE          |      off | low | high |  N/A |
> + * +--------------------------+----------+-----+------+------+
> + * | KBD_BL_TRISTATE_AUTO     |      off | low | high | auto |
> + * +--------------------------+----------+-----+------+------+
> + *
> + * We map LED classdev brightness for KBD_BL_TRISTATE_AUTO as follows:
> + * +--------------------------+----------+-----+------+
> + * |  LED classdev brightness |        0 |   1 |    2 |
> + * | Operation                |          |     |      |
> + * +--------------------------+----------+-----+------+
> + * | Read                     | off/auto | low | high |
> + * +--------------------------+----------+-----+------+
> + * | Write                    |      off | low | high |
> + * +--------------------------+----------+-----+------+
>   */
>  enum {
>  	KBD_BL_STANDARD      = 1,
> @@ -145,6 +167,8 @@ enum {
>  	KBD_BL_TRISTATE_AUTO = 3,
>  };
>  
> +#define KBD_BL_AUTO_MODE_HW_BRIGHTNESS	3
> +
>  #define KBD_BL_QUERY_TYPE		0x1
>  #define KBD_BL_TRISTATE_TYPE		0x5
>  #define KBD_BL_TRISTATE_AUTO_TYPE	0x7
> @@ -203,7 +227,7 @@ struct ideapad_private {
>  		bool initialized;
>  		int type;
>  		struct led_classdev led;
> -		unsigned int last_brightness;
> +		atomic_t last_hw_brightness;
>  	} kbd_bl;
>  	struct {
>  		bool initialized;
> @@ -1592,7 +1616,24 @@ static int ideapad_kbd_bl_check_tristate(int type)
>  	return (type == KBD_BL_TRISTATE) || (type == KBD_BL_TRISTATE_AUTO);
>  }
>  
> -static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> +static int ideapad_kbd_bl_brightness_parse(struct ideapad_private *priv, int hw_brightness)
> +{
> +	/* Off, low or high */
> +	if (hw_brightness <= priv->kbd_bl.led.max_brightness)
> +		return hw_brightness;
> +
> +	/* Auto (controlled by EC according to ALS), report as off */
> +	if (priv->kbd_bl.type == KBD_BL_TRISTATE_AUTO &&
> +	    hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS)
> +		return 0;
> +
> +	/* Unknown value */
> +	dev_warn(&priv->platform_device->dev,

Please (finally) add the include.

> +		 "Unknown keyboard backlight value: %d", hw_brightness);
> +	return -EINVAL;
> +}
> +
> +static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
>  {
>  	unsigned long value;
>  	int err;
> @@ -1606,21 +1647,7 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
>  		if (err)
>  			return err;
>  
> -		/* Convert returned value to brightness level */
> -		value = FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
> -
> -		/* Off, low or high */
> -		if (value <= priv->kbd_bl.led.max_brightness)
> -			return value;
> -
> -		/* Auto, report as off */
> -		if (value == priv->kbd_bl.led.max_brightness + 1)
> -			return 0;
> -
> -		/* Unknown value */
> -		dev_warn(&priv->platform_device->dev,
> -			 "Unknown keyboard backlight value: %lu", value);
> -		return -EINVAL;
> +		return FIELD_GET(KBD_BL_GET_BRIGHTNESS, value);
>  	}
>  
>  	err = eval_hals(priv->adev->handle, &value);
> @@ -1630,6 +1657,16 @@ static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
>  	return !!test_bit(HALS_KBD_BL_STATE_BIT, &value);
>  }
>  
> +static int ideapad_kbd_bl_brightness_get(struct ideapad_private *priv)
> +{
> +	int hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +
> +	if (hw_brightness < 0)
> +		return hw_brightness;
> +
> +	return ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
> +}
> +
>  static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_classdev *led_cdev)
>  {
>  	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> @@ -1637,32 +1674,37 @@ static enum led_brightness ideapad_kbd_bl_led_cdev_brightness_get(struct led_cla
>  	return ideapad_kbd_bl_brightness_get(priv);
>  }
>  
> -static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> +static int ideapad_kbd_bl_hw_brightness_set(struct ideapad_private *priv, int hw_brightness)
>  {
> -	int err;
>  	unsigned long value;
>  	int type = priv->kbd_bl.type;
> +	int err;
>  
>  	if (ideapad_kbd_bl_check_tristate(type)) {
> -		if (brightness > priv->kbd_bl.led.max_brightness)
> -			return -EINVAL;
> -
> -		value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, brightness) |
> +		value = FIELD_PREP(KBD_BL_SET_BRIGHTNESS, hw_brightness) |
>  			FIELD_PREP(KBD_BL_COMMAND_TYPE, type) |
>  			KBD_BL_COMMAND_SET;
>  		err = exec_kblc(priv->adev->handle, value);
>  	} else {
> -		err = exec_sals(priv->adev->handle, brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> +		value = hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF;
> +		err = exec_sals(priv->adev->handle, value);
>  	}
> -
>  	if (err)
>  		return err;
>  
> -	priv->kbd_bl.last_brightness = brightness;
> +	atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);
>  
>  	return 0;
>  }
>  
> +static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, int brightness)
> +{
> +	if (brightness > priv->kbd_bl.led.max_brightness)
> +		return -EINVAL;
> +
> +	return ideapad_kbd_bl_hw_brightness_set(priv, brightness);
> +}
> +
>  static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
>  						  enum led_brightness brightness)
>  {
> @@ -1673,26 +1715,29 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
>  
>  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  {
> -	int brightness;
> +	int hw_brightness, brightness, last_hw_brightness;
>  
>  	if (!priv->kbd_bl.initialized)
>  		return;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
>  		return;
>  
> -	if (brightness == priv->kbd_bl.last_brightness)
> -		return;
> +	brightness = ideapad_kbd_bl_brightness_parse(priv, hw_brightness);
> +	if (brightness < 0)
> +		return; /* Reject insane values early. */
>  
> -	priv->kbd_bl.last_brightness = brightness;
> +	last_hw_brightness = atomic_xchg(&priv->kbd_bl.last_hw_brightness, hw_brightness);
> +	if (hw_brightness == last_hw_brightness)
> +		return;
>  
>  	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
>  }
>  
>  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  {
> -	int brightness, err;
> +	int hw_brightness, err;
>  
>  	if (!priv->features.kbd_bl)
>  		return -ENODEV;
> @@ -1700,21 +1745,30 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  	if (WARN_ON(priv->kbd_bl.initialized))
>  		return -EEXIST;
>  
> -	if (ideapad_kbd_bl_check_tristate(priv->kbd_bl.type))
> -		priv->kbd_bl.led.max_brightness = 2;
> -	else
> -		priv->kbd_bl.led.max_brightness = 1;
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
> +		return hw_brightness;
>  
> -	brightness = ideapad_kbd_bl_brightness_get(priv);
> -	if (brightness < 0)
> -		return brightness;
> +	atomic_set(&priv->kbd_bl.last_hw_brightness, hw_brightness);
>  
> -	priv->kbd_bl.last_brightness = brightness;
>  	priv->kbd_bl.led.name                    = "platform::" LED_FUNCTION_KBD_BACKLIGHT;
>  	priv->kbd_bl.led.brightness_get          = ideapad_kbd_bl_led_cdev_brightness_get;
>  	priv->kbd_bl.led.brightness_set_blocking = ideapad_kbd_bl_led_cdev_brightness_set;
>  	priv->kbd_bl.led.flags                   = LED_BRIGHT_HW_CHANGED | LED_RETAIN_AT_SHUTDOWN;
>  
> +	switch (priv->kbd_bl.type) {
> +	case KBD_BL_TRISTATE_AUTO:
> +	case KBD_BL_TRISTATE:
> +		priv->kbd_bl.led.max_brightness = 2;
> +		break;
> +	case KBD_BL_STANDARD:
> +		priv->kbd_bl.led.max_brightness = 1;
> +		break;
> +	default:
> +		/* This has already been validated by ideapad_check_features(). */
> +		unreachable();

Please add include.

> +	}
> +
>  	err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
>  	if (err)
>  		return err;
> 
> 

-- 
 i.