[PATCH 0/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[PATCH 0/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Hi all,

Some recent models supported by ideapad-laptop 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 "HW
brightness" is 3. The term "HW brightness" means that it cannot be
perfectly mapped to an LED classdev brightness, but the EC does use this
predefined brightness value to represent such a mode.

Currently, the auto brightness mode of keyboard backlight maps to
brightness=0 in LED classdev. The only method to switch to this mode is
by pressing the manufacturer-defined shortcut (Fn+Space). However, 0 is
a multiplexed brightness value; writing 0 simply results in the
backlight being turned off.

Fully support this mode by adding a sysfs node:
/sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
to align with dell-laptop, which provides a similar feature.

Thanks,
Rong

Rong Zhang (2):
  platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd
    backlight
  platform/x86: ideapad-laptop: Fully support auto kbd backlight

 .../ABI/testing/sysfs-platform-ideapad-laptop |  12 ++
 drivers/platform/x86/lenovo/ideapad-laptop.c  | 186 ++++++++++++++----
 2 files changed, 163 insertions(+), 35 deletions(-)


base-commit: 7e161a991ea71e6ec526abc8f40c6852ebe3d946
-- 
2.50.1

[PATCH 1/2] platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd backlight

[Rong Zhang]

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 "HW brightness" is 3. The term
"HW brightness" means that it cannot be perfectly mapped to an LED
classdev brightness, but the EC does use this predefined brightness
value to represent such a 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 HW brightness from LED
classdev brightness, and update comments about corresponding backlight
modes.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 drivers/platform/x86/lenovo/ideapad-laptop.c | 125 +++++++++++++------
 1 file changed, 90 insertions(+), 35 deletions(-)

diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index fcebfbaf0460..5014c1d0b633 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -119,17 +119,40 @@ 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 HW brightness values are defined as follows:
+ * +--------------------------+----------+-----+------+------+
+ * |            HW 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,
-	KBD_BL_TRISTATE      = 2,
-	KBD_BL_TRISTATE_AUTO = 3,
+	KBD_BL_STANDARD		=	1,
+	KBD_BL_TRISTATE		=	2,
+	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
@@ -1559,7 +1582,25 @@ 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,
+					   unsigned 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,
+		 "Unknown keyboard backlight value: %u", hw_brightness);
+	return -EINVAL;
+}
+
+static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
 {
 	unsigned long value;
 	int err;
@@ -1573,21 +1614,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);
@@ -1597,6 +1624,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);
@@ -1604,24 +1641,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,
+					    unsigned int hw_brightness)
 {
 	int err;
 	unsigned long value;
 	int type = priv->kbd_bl.type;
 
 	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);
+		err = exec_sals(priv->adev->handle,
+				hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
 	}
 
+	if (err)
+		return err;
+
+	return 0;
+}
+
+static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
+{
+	int err;
+
+	if (brightness > priv->kbd_bl.led.max_brightness)
+		return -EINVAL;
+
+	err = ideapad_kbd_bl_hw_brightness_set(priv, brightness);
 	if (err)
 		return err;
 
@@ -1638,6 +1688,16 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
 	return ideapad_kbd_bl_brightness_set(priv, brightness);
 }
 
+static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
+{
+	if (brightness == priv->kbd_bl.last_brightness)
+		return;
+
+	priv->kbd_bl.last_brightness = brightness;
+
+	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
+}
+
 static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 {
 	int brightness;
@@ -1649,12 +1709,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 	if (brightness < 0)
 		return;
 
-	if (brightness == priv->kbd_bl.last_brightness)
-		return;
-
-	priv->kbd_bl.last_brightness = brightness;
-
-	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
+	ideapad_kbd_bl_notify_known(priv, brightness);
 }
 
 static int ideapad_kbd_bl_init(struct ideapad_private *priv)
-- 
2.50.1

Re: [PATCH 1/2] platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd backlight

[Ilpo Järvinen]

On Tue, 5 Aug 2025, 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 "HW brightness" is 3. The term
> "HW brightness" means that it cannot be perfectly mapped to an LED
> classdev brightness, but the EC does use this predefined brightness
> value to represent such a 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 HW brightness from LED
> classdev brightness, and update comments about corresponding backlight
> modes.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  drivers/platform/x86/lenovo/ideapad-laptop.c | 125 +++++++++++++------
>  1 file changed, 90 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index fcebfbaf0460..5014c1d0b633 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -119,17 +119,40 @@ 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 HW brightness values are defined as follows:
> + * +--------------------------+----------+-----+------+------+
> + * |            HW 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,
> -	KBD_BL_TRISTATE      = 2,
> -	KBD_BL_TRISTATE_AUTO = 3,
> +	KBD_BL_STANDARD		=	1,
> +	KBD_BL_TRISTATE		=	2,
> +	KBD_BL_TRISTATE_AUTO	=	3,

Pure space change for no reason. Please leave as is.

>  };
>  
> +#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
> @@ -1559,7 +1582,25 @@ 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,
> +					   unsigned 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;

There seems to be code move/function refactoring (split to get+parse) 
and changes mixed up in here as this doesn't match to what was there 
beforehand AFAICT.

Could you please try to separate the pure function refactoring from the 
changes to make the real changes easier to understand/see.

> +	/* Unknown value */
> +	dev_warn(&priv->platform_device->dev,
> +		 "Unknown keyboard backlight value: %u", hw_brightness);
> +	return -EINVAL;
> +}
> +
> +static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
>  {
>  	unsigned long value;
>  	int err;
> @@ -1573,21 +1614,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);
> @@ -1597,6 +1624,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);
> @@ -1604,24 +1641,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,
> +					    unsigned int hw_brightness)
>  {
>  	int err;
>  	unsigned long value;
>  	int type = priv->kbd_bl.type;
>  
>  	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);
> +		err = exec_sals(priv->adev->handle,
> +				hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
>  	}
>  
> +	if (err)
> +		return err;
> +
> +	return 0;
> +}
> +
> +static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> +{
> +	int err;
> +
> +	if (brightness > priv->kbd_bl.led.max_brightness)
> +		return -EINVAL;
> +
> +	err = ideapad_kbd_bl_hw_brightness_set(priv, brightness);
>  	if (err)
>  		return err;
>  
> @@ -1638,6 +1688,16 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
>  	return ideapad_kbd_bl_brightness_set(priv, brightness);
>  }
>  
> +static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
> +{
> +	if (brightness == priv->kbd_bl.last_brightness)
> +		return;
> +
> +	priv->kbd_bl.last_brightness = brightness;
> +
> +	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> +}
> +
>  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  {
>  	int brightness;
> @@ -1649,12 +1709,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  	if (brightness < 0)
>  		return;
>  
> -	if (brightness == priv->kbd_bl.last_brightness)
> -		return;
> -
> -	priv->kbd_bl.last_brightness = brightness;
> -
> -	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> +	ideapad_kbd_bl_notify_known(priv, brightness);
>  }
>  
>  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> 

-- 
 i.

Re: [PATCH 1/2] platform/x86: ideapad-laptop: Decouple HW & cdev brightness for kbd backlight

[Rong Zhang]

Hi Ilpo,

On Wed, 2025-08-06 at 14:19 +0300, Ilpo Järvinen wrote:
> On Tue, 5 Aug 2025, 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 "HW brightness" is 3. The term
> > "HW brightness" means that it cannot be perfectly mapped to an LED
> > classdev brightness, but the EC does use this predefined brightness
> > value to represent such a 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 HW brightness from LED
> > classdev brightness, and update comments about corresponding backlight
> > modes.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >  drivers/platform/x86/lenovo/ideapad-laptop.c | 125 +++++++++++++------
> >  1 file changed, 90 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > index fcebfbaf0460..5014c1d0b633 100644
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > @@ -119,17 +119,40 @@ 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 HW brightness values are defined as follows:
> > + * +--------------------------+----------+-----+------+------+
> > + * |            HW 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,
> > -	KBD_BL_TRISTATE      = 2,
> > -	KBD_BL_TRISTATE_AUTO = 3,
> > +	KBD_BL_STANDARD		=	1,
> > +	KBD_BL_TRISTATE		=	2,
> > +	KBD_BL_TRISTATE_AUTO	=	3,
> 
> Pure space change for no reason. Please leave as is.

Sure. Will remove the change in v2.

> >  };
> >  
> > +#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
> > @@ -1559,7 +1582,25 @@ 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,
> > +					   unsigned 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;
> 
> There seems to be code move/function refactoring (split to get+parse) 
> and changes mixed up in here as this doesn't match to what was there 
> beforehand AFAICT.
> 
> Could you please try to separate the pure function refactoring from the 
> changes to make the real changes easier to understand/see.

Sure. Will move real changes into [PATCH v2 2/2]. Besides, the macro
KBD_BL_AUTO_MODE_HW_BRIGHTNESS will then be unused in [PATCH v2 1/2],
so I will also move it into [PATCH v2 2/2].

> > +	/* Unknown value */
> > +	dev_warn(&priv->platform_device->dev,
> > +		 "Unknown keyboard backlight value: %u", hw_brightness);
> > +	return -EINVAL;
> > +}
> > +
> > +static int ideapad_kbd_bl_hw_brightness_get(struct ideapad_private *priv)
> >  {
> >  	unsigned long value;
> >  	int err;
> > @@ -1573,21 +1614,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);
> > @@ -1597,6 +1624,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);
> > @@ -1604,24 +1641,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,
> > +					    unsigned int hw_brightness)
> >  {
> >  	int err;
> >  	unsigned long value;
> >  	int type = priv->kbd_bl.type;
> >  
> >  	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);
> > +		err = exec_sals(priv->adev->handle,
> > +				hw_brightness ? SALS_KBD_BL_ON : SALS_KBD_BL_OFF);
> >  	}
> >  
> > +	if (err)
> > +		return err;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ideapad_kbd_bl_brightness_set(struct ideapad_private *priv, unsigned int brightness)
> > +{
> > +	int err;
> > +
> > +	if (brightness > priv->kbd_bl.led.max_brightness)
> > +		return -EINVAL;
> > +
> > +	err = ideapad_kbd_bl_hw_brightness_set(priv, brightness);
> >  	if (err)
> >  		return err;
> >  
> > @@ -1638,6 +1688,16 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
> >  	return ideapad_kbd_bl_brightness_set(priv, brightness);
> >  }
> >  
> > +static void ideapad_kbd_bl_notify_known(struct ideapad_private *priv, unsigned int brightness)
> > +{
> > +	if (brightness == priv->kbd_bl.last_brightness)
> > +		return;
> > +
> > +	priv->kbd_bl.last_brightness = brightness;
> > +
> > +	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> > +}
> > +
> >  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> >  {
> >  	int brightness;
> > @@ -1649,12 +1709,7 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> >  	if (brightness < 0)
> >  		return;
> >  
> > -	if (brightness == priv->kbd_bl.last_brightness)
> > -		return;
> > -
> > -	priv->kbd_bl.last_brightness = brightness;
> > -
> > -	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
> > +	ideapad_kbd_bl_notify_known(priv, brightness);
> >  }
> >  
> >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> > 

Thanks for your review,
Rong

[PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Currently, the auto brightness mode of keyboard backlight maps to
brightness=0 in LED classdev. The only method to switch to such a mode
is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
is a multiplexed brightness value; writing 0 simply results in the
backlight being turned off.

With brightness processing code decoupled from LED classdev, we can now
fully support the auto brightness mode. In this mode, the keyboard
backlight is controlled by the EC according to the ambient light sensor
(ALS).

To utilize this, a sysfs node is exposed to the userspace:
/sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
to align with dell-laptop, which provides a similar feature.

Signed-off-by: Rong Zhang <i@rong.moe>
---
 .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
 drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
 2 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
index 5ec0dee9e707..a2b78aa60aaa 100644
--- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
+++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
@@ -50,3 +50,15 @@ Description:
 		Controls whether the "always on USB charging" feature is
 		enabled or not. This feature enables charging USB devices
 		even if the computer is not turned on.
+
+What:		/sys/class/leds/platform::kbd_backlight/als_enabled
+Date:		July 2025
+KernelVersion:	6.17
+Contact:	platform-driver-x86@vger.kernel.org
+Description:
+		This file allows to control the automatic keyboard
+		illumination mode on some systems that have an ambient
+		light sensor. Write 1 to this file to enable the auto
+		mode, 0 to disable it. In this mode, the actual
+		brightness level is not available and reading the
+		"brightness" file always returns 0.
diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
index 5014c1d0b633..49f2fc68add4 100644
--- a/drivers/platform/x86/lenovo/ideapad-laptop.c
+++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
@@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
 	ideapad_kbd_bl_notify_known(priv, brightness);
 }
 
+static ssize_t als_enabled_show(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
+	int hw_brightness;
+
+	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
+	if (hw_brightness < 0)
+		return hw_brightness;
+
+	return sysfs_emit(buf, "%d\n", hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
+}
+
+static ssize_t als_enabled_store(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	struct led_classdev *led_cdev = dev_get_drvdata(dev);
+	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
+	bool state;
+	int err;
+
+	err = kstrtobool(buf, &state);
+	if (err)
+		return err;
+
+	/*
+	 * Auto (ALS) mode uses a predefined HW brightness value. It is
+	 * impossible to disable it without setting another brightness value.
+	 * Set the brightness to 0 when disabling is requested.
+	 */
+	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
+	if (err)
+		return err;
+
+	/* Both HW brightness values map to 0 in the LED classdev. */
+	ideapad_kbd_bl_notify_known(priv, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(als_enabled);
+
+static struct attribute *ideapad_kbd_bl_als_attrs[] = {
+	&dev_attr_als_enabled.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
+
 static int ideapad_kbd_bl_init(struct ideapad_private *priv)
 {
 	int brightness, err;
@@ -1722,10 +1773,20 @@ 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)) {
+	switch (priv->kbd_bl.type) {
+	case KBD_BL_TRISTATE_AUTO:
+		/* The sysfs node will be /sys/class/leds/platform::kbd_backlight/als_enabled */
+		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
+		fallthrough;
+	case KBD_BL_TRISTATE:
 		priv->kbd_bl.led.max_brightness = 2;
-	} else {
+		break;
+	case KBD_BL_STANDARD:
 		priv->kbd_bl.led.max_brightness = 1;
+		break;
+	default:
+		/* This has already been validated by ideapad_check_features(). */
+		unreachable();
 	}
 
 	brightness = ideapad_kbd_bl_brightness_get(priv);
-- 
2.50.1

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Ilpo Järvinen]

On Tue, 5 Aug 2025, Rong Zhang wrote:

> Currently, the auto brightness mode of keyboard backlight maps to
> brightness=0 in LED classdev. The only method to switch to such a mode
> is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
> is a multiplexed brightness value; writing 0 simply results in the
> backlight being turned off.
> 
> With brightness processing code decoupled from LED classdev, we can now
> fully support the auto brightness mode. In this mode, the keyboard
> backlight is controlled by the EC according to the ambient light sensor
> (ALS).
> 
> To utilize this, a sysfs node is exposed to the userspace:
> /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
> to align with dell-laptop, which provides a similar feature.
> 
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
>  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
>  2 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> index 5ec0dee9e707..a2b78aa60aaa 100644
> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> @@ -50,3 +50,15 @@ Description:
>  		Controls whether the "always on USB charging" feature is
>  		enabled or not. This feature enables charging USB devices
>  		even if the computer is not turned on.
> +
> +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
> +Date:		July 2025
> +KernelVersion:	6.17

This ship has sailed.

> +Contact:	platform-driver-x86@vger.kernel.org
> +Description:
> +		This file allows to control the automatic keyboard

Please avoid using "This file" entirely in the description.

Start with "Controls ..."

> +		illumination mode on some systems that have an ambient
> +		light sensor. Write 1 to this file to enable the auto
> +		mode, 0 to disable it. In this mode, the actual

What is "this mode" ? Did you mean, e.g., "When the auto mode is enabled,"
?

> +		brightness level is not available and reading the
> +		"brightness" file always returns 0.
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 5014c1d0b633..49f2fc68add4 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  	ideapad_kbd_bl_notify_known(priv, brightness);
>  }
>  
> +static ssize_t als_enabled_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> +	int hw_brightness;
> +
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
> +		return hw_brightness;
> +
> +	return sysfs_emit(buf, "%d\n", hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> +}
> +
> +static ssize_t als_enabled_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> +	bool state;
> +	int err;
> +
> +	err = kstrtobool(buf, &state);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
> +	 * impossible to disable it without setting another brightness value.
> +	 * Set the brightness to 0 when disabling is requested.
> +	 */
> +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
> +	if (err)
> +		return err;
> +
> +	/* Both HW brightness values map to 0 in the LED classdev. */
> +	ideapad_kbd_bl_notify_known(priv, 0);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(als_enabled);
> +
> +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
> +	&dev_attr_als_enabled.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
> +
>  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  {
>  	int brightness, err;
> @@ -1722,10 +1773,20 @@ 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)) {
> +	switch (priv->kbd_bl.type) {
> +	case KBD_BL_TRISTATE_AUTO:
> +		/* The sysfs node will be /sys/class/leds/platform::kbd_backlight/als_enabled */
> +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
> +		fallthrough;
> +	case KBD_BL_TRISTATE:
>  		priv->kbd_bl.led.max_brightness = 2;
> -	} else {
> +		break;
> +	case KBD_BL_STANDARD:
>  		priv->kbd_bl.led.max_brightness = 1;
> +		break;
> +	default:
> +		/* This has already been validated by ideapad_check_features(). */
> +		unreachable();
>  	}
>  
>  	brightness = ideapad_kbd_bl_brightness_get(priv);
> 

-- 
 i.

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Hi Ilpo,

On Wed, 2025-08-06 at 14:26 +0300, Ilpo Järvinen wrote:
> On Tue, 5 Aug 2025, Rong Zhang wrote:
> 
> > Currently, the auto brightness mode of keyboard backlight maps to
> > brightness=0 in LED classdev. The only method to switch to such a mode
> > is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
> > is a multiplexed brightness value; writing 0 simply results in the
> > backlight being turned off.
> > 
> > With brightness processing code decoupled from LED classdev, we can now
> > fully support the auto brightness mode. In this mode, the keyboard
> > backlight is controlled by the EC according to the ambient light sensor
> > (ALS).
> > 
> > To utilize this, a sysfs node is exposed to the userspace:
> > /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
> > to align with dell-laptop, which provides a similar feature.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
> >  2 files changed, 75 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > index 5ec0dee9e707..a2b78aa60aaa 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > @@ -50,3 +50,15 @@ Description:
> >  		Controls whether the "always on USB charging" feature is
> >  		enabled or not. This feature enables charging USB devices
> >  		even if the computer is not turned on.
> > +
> > +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
> > +Date:		July 2025
> > +KernelVersion:	6.17
> 
> This ship has sailed.

How time flies! It is embarrassing that I wrote this several weeks ago
but forgot to update it before finishing the patch. Thanks for your
reminder and I will update this in v2.

> 
> > +Contact:	platform-driver-x86@vger.kernel.org
> > +Description:
> > +		This file allows to control the automatic keyboard
> 
> Please avoid using "This file" entirely in the description.
> 
> Start with "Controls ..."

Sure.

> > +		illumination mode on some systems that have an ambient
> > +		light sensor. Write 1 to this file to enable the auto
> > +		mode, 0 to disable it. In this mode, the actual
> 
> What is "this mode" ? Did you mean, e.g., "When the auto mode is enabled,"
> ?

It does sound more understandable to describe it that way. Thanks for
your suggestion and I will improve the wording in v2.

> > +		brightness level is not available and reading the
> > +		"brightness" file always returns 0.
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > index 5014c1d0b633..49f2fc68add4 100644
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
> >  	ideapad_kbd_bl_notify_known(priv, brightness);
> >  }
> >  
> > +static ssize_t als_enabled_show(struct device *dev,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> > +	int hw_brightness;
> > +
> > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> > +	if (hw_brightness < 0)
> > +		return hw_brightness;
> > +
> > +	return sysfs_emit(buf, "%d\n", hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> > +}
> > +
> > +static ssize_t als_enabled_store(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 const char *buf, size_t count)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> > +	bool state;
> > +	int err;
> > +
> > +	err = kstrtobool(buf, &state);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
> > +	 * impossible to disable it without setting another brightness value.
> > +	 * Set the brightness to 0 when disabling is requested.
> > +	 */
> > +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Both HW brightness values map to 0 in the LED classdev. */
> > +	ideapad_kbd_bl_notify_known(priv, 0);
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(als_enabled);
> > +
> > +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
> > +	&dev_attr_als_enabled.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
> > +
> >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> >  {
> >  	int brightness, err;
> > @@ -1722,10 +1773,20 @@ 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)) {
> > +	switch (priv->kbd_bl.type) {
> > +	case KBD_BL_TRISTATE_AUTO:
> > +		/* The sysfs node will be /sys/class/leds/platform::kbd_backlight/als_enabled */
> > +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
> > +		fallthrough;
> > +	case KBD_BL_TRISTATE:
> >  		priv->kbd_bl.led.max_brightness = 2;
> > -	} else {
> > +		break;
> > +	case KBD_BL_STANDARD:
> >  		priv->kbd_bl.led.max_brightness = 1;
> > +		break;
> > +	default:
> > +		/* This has already been validated by ideapad_check_features(). */
> > +		unreachable();
> >  	}
> >  
> >  	brightness = ideapad_kbd_bl_brightness_get(priv);
> > 

Thanks for your review,
Rong

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Mark Pearson]

Hi Rong,

On Tue, Aug 5, 2025, at 10:01 AM, Rong Zhang wrote:
> Currently, the auto brightness mode of keyboard backlight maps to
> brightness=0 in LED classdev. The only method to switch to such a mode
> is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
> is a multiplexed brightness value; writing 0 simply results in the
> backlight being turned off.
>
> With brightness processing code decoupled from LED classdev, we can now
> fully support the auto brightness mode. In this mode, the keyboard
> backlight is controlled by the EC according to the ambient light sensor
> (ALS).
>
> To utilize this, a sysfs node is exposed to the userspace:
> /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
> to align with dell-laptop, which provides a similar feature.
>
> Signed-off-by: Rong Zhang <i@rong.moe>
> ---
>  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
>  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
>  2 files changed, 75 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
> b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> index 5ec0dee9e707..a2b78aa60aaa 100644
> --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> @@ -50,3 +50,15 @@ Description:
>  		Controls whether the "always on USB charging" feature is
>  		enabled or not. This feature enables charging USB devices
>  		even if the computer is not turned on.
> +
> +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
> +Date:		July 2025
> +KernelVersion:	6.17
> +Contact:	platform-driver-x86@vger.kernel.org
> +Description:
> +		This file allows to control the automatic keyboard
> +		illumination mode on some systems that have an ambient
> +		light sensor. Write 1 to this file to enable the auto
> +		mode, 0 to disable it. In this mode, the actual
> +		brightness level is not available and reading the
> +		"brightness" file always returns 0.
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c 
> b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 5014c1d0b633..49f2fc68add4 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct 
> ideapad_private *priv)
>  	ideapad_kbd_bl_notify_known(priv, brightness);
>  }
> 
> +static ssize_t als_enabled_show(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct ideapad_private *priv = container_of(led_cdev, struct 
> ideapad_private, kbd_bl.led);
> +	int hw_brightness;
> +
> +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> +	if (hw_brightness < 0)
> +		return hw_brightness;
> +
> +	return sysfs_emit(buf, "%d\n", hw_brightness == 
> KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> +}
> +
> +static ssize_t als_enabled_store(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> +	struct ideapad_private *priv = container_of(led_cdev, struct 
> ideapad_private, kbd_bl.led);
> +	bool state;
> +	int err;
> +
> +	err = kstrtobool(buf, &state);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
> +	 * impossible to disable it without setting another brightness value.
> +	 * Set the brightness to 0 when disabling is requested.
> +	 */
> +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? 
> KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
> +	if (err)
> +		return err;
> +
> +	/* Both HW brightness values map to 0 in the LED classdev. */
> +	ideapad_kbd_bl_notify_known(priv, 0);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR_RW(als_enabled);
> +
> +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
> +	&dev_attr_als_enabled.attr,
> +	NULL,
> +};
> +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
> +
>  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  {
>  	int brightness, err;
> @@ -1722,10 +1773,20 @@ 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)) {
> +	switch (priv->kbd_bl.type) {
> +	case KBD_BL_TRISTATE_AUTO:
> +		/* The sysfs node will be 
> /sys/class/leds/platform::kbd_backlight/als_enabled */
> +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
> +		fallthrough;
> +	case KBD_BL_TRISTATE:
>  		priv->kbd_bl.led.max_brightness = 2;
> -	} else {
> +		break;
> +	case KBD_BL_STANDARD:
>  		priv->kbd_bl.led.max_brightness = 1;
> +		break;
> +	default:
> +		/* This has already been validated by ideapad_check_features(). */
> +		unreachable();
>  	}
> 
>  	brightness = ideapad_kbd_bl_brightness_get(priv);
> -- 
> 2.50.1

We're looking to implement this feature on the Thinkpads, so this change is timely :)

I did wonder if we should be making changes at the LED class level? Something similar to LED_BRIGHT_HW_CHANGED maybe as a way to advertise that auto mode is supported and some hooks to support that in sysfs?
I know it would be more work, but I'm guessing this is going to be a common feature across multiple vendors it might need doing at a common layer.

As a note - on the Thinkpads we've had to look at getting the correct Intel ISH firmware loaded (and we're working on getting that upstream to linux-firmware). Is that needed on the Ideapads for the feature to work well or not?

Mark

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Hi Mark,

On Wed, 2025-08-06 at 15:02 -0400, Mark Pearson wrote:
> Hi Rong,
> 
> On Tue, Aug 5, 2025, at 10:01 AM, Rong Zhang wrote:
> > Currently, the auto brightness mode of keyboard backlight maps to
> > brightness=0 in LED classdev. The only method to switch to such a mode
> > is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
> > is a multiplexed brightness value; writing 0 simply results in the
> > backlight being turned off.
> > 
> > With brightness processing code decoupled from LED classdev, we can now
> > fully support the auto brightness mode. In this mode, the keyboard
> > backlight is controlled by the EC according to the ambient light sensor
> > (ALS).
> > 
> > To utilize this, a sysfs node is exposed to the userspace:
> > /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
> > to align with dell-laptop, which provides a similar feature.
> > 
> > Signed-off-by: Rong Zhang <i@rong.moe>
> > ---
> >  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
> >  2 files changed, 75 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
> > b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > index 5ec0dee9e707..a2b78aa60aaa 100644
> > --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > @@ -50,3 +50,15 @@ Description:
> >  		Controls whether the "always on USB charging" feature is
> >  		enabled or not. This feature enables charging USB devices
> >  		even if the computer is not turned on.
> > +
> > +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
> > +Date:		July 2025
> > +KernelVersion:	6.17
> > +Contact:	platform-driver-x86@vger.kernel.org
> > +Description:
> > +		This file allows to control the automatic keyboard
> > +		illumination mode on some systems that have an ambient
> > +		light sensor. Write 1 to this file to enable the auto
> > +		mode, 0 to disable it. In this mode, the actual
> > +		brightness level is not available and reading the
> > +		"brightness" file always returns 0.
> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c 
> > b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > index 5014c1d0b633..49f2fc68add4 100644
> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct 
> > ideapad_private *priv)
> >  	ideapad_kbd_bl_notify_known(priv, brightness);
> >  }
> > 
> > +static ssize_t als_enabled_show(struct device *dev,
> > +				struct device_attribute *attr,
> > +				char *buf)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
> > ideapad_private, kbd_bl.led);
> > +	int hw_brightness;
> > +
> > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> > +	if (hw_brightness < 0)
> > +		return hw_brightness;
> > +
> > +	return sysfs_emit(buf, "%d\n", hw_brightness == 
> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> > +}
> > +
> > +static ssize_t als_enabled_store(struct device *dev,
> > +				 struct device_attribute *attr,
> > +				 const char *buf, size_t count)
> > +{
> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
> > ideapad_private, kbd_bl.led);
> > +	bool state;
> > +	int err;
> > +
> > +	err = kstrtobool(buf, &state);
> > +	if (err)
> > +		return err;
> > +
> > +	/*
> > +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
> > +	 * impossible to disable it without setting another brightness value.
> > +	 * Set the brightness to 0 when disabling is requested.
> > +	 */
> > +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? 
> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
> > +	if (err)
> > +		return err;
> > +
> > +	/* Both HW brightness values map to 0 in the LED classdev. */
> > +	ideapad_kbd_bl_notify_known(priv, 0);
> > +
> > +	return count;
> > +}
> > +
> > +static DEVICE_ATTR_RW(als_enabled);
> > +
> > +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
> > +	&dev_attr_als_enabled.attr,
> > +	NULL,
> > +};
> > +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
> > +
> >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> >  {
> >  	int brightness, err;
> > @@ -1722,10 +1773,20 @@ 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)) {
> > +	switch (priv->kbd_bl.type) {
> > +	case KBD_BL_TRISTATE_AUTO:
> > +		/* The sysfs node will be 
> > /sys/class/leds/platform::kbd_backlight/als_enabled */
> > +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
> > +		fallthrough;
> > +	case KBD_BL_TRISTATE:
> >  		priv->kbd_bl.led.max_brightness = 2;
> > -	} else {
> > +		break;
> > +	case KBD_BL_STANDARD:
> >  		priv->kbd_bl.led.max_brightness = 1;
> > +		break;
> > +	default:
> > +		/* This has already been validated by ideapad_check_features(). */
> > +		unreachable();
> >  	}
> > 
> >  	brightness = ideapad_kbd_bl_brightness_get(priv);
> > -- 
> > 2.50.1
> 
> We're looking to implement this feature on the Thinkpads, so this change is timely :)

Whoo, it's good to hear that.

> I did wonder if we should be making changes at the LED class level? Something similar to LED_BRIGHT_HW_CHANGED maybe as a way to advertise that auto mode is supported and some hooks to support that in sysfs?

To the best of my knowledge, there is already an ideal model to fit the
auto brightness mode, which is private LED trigger.

To utilize it, these are four pieces of the puzzle:

(1) implement a private LED trigger (see leds-cros_ec and
    leds-turris-omnia, for example)
(2) turn on/off the auto brightness mode when the activate/deactivate
    hooks are called
(3) switch to the private LED trigger/the "none" trigger when such mode
    is turned on/off by the HW (i.e., when Fn+Space is pressed)
(4) notifying the userspace of the HW-triggered LED trigger change

I'd finished (1) and (2) in my early experiments and verified their
functionality. However, I eventually realized the dilemma that pressing
Fn+Space would bring everything into an inconsistent state because of
the lack of (3).

For (3), when the HW turns on the auto brightness mode, we need:

   mutex_lock(&led_cdev->led_access);
   
   down_write(&led_cdev->trigger_lock);
   led_trigger_set(led_cdev, <THE PRIVATE LED TRIGGER>);
   up_write(&led_cdev->trigger_lock);
   
   mutex_unlock(&led_cdev->led_access);

When off, we need:

   mutex_lock(&led_cdev->led_access);
   
   led_trigger_remove(led_cdev);
   
   mutex_unlock(&led_cdev->led_access);

I never thought of (4) at that moment. Therefore, I eventually doubted
whether it was worth so much overhead and turned to the method in the
current patch.

Think twice now, I think it is worth implementing (1)-(3) as long as
(4) can be addressed. I just found that both led_trigger_set() and
led_trigger_remove() send a uevent once the trigger is changed [1], and
verified this using `udevadm monitor'. We have collected all four
pieces of the puzzle, hooray!

If you are OK with the private LED trigger approach, I will adopt it in
[PATCH v2].

[1]: commit 52c47742f79d ("leds: triggers: send uevent when changing
triggers")

> I know it would be more work, but I'm guessing this is going to be a common feature across multiple vendors it might need doing at a common layer.

CC'ing LED class maintainers.

Private LED triggers currently have two users: leds-cros_ec and leds-
turris-omnia. Their private triggers are respectively named "chromeos-
auto" and "omnia-mcu".

I agree that this is going to be a common feature. A generic name for
such a feature helps userspace [2] identify it. What about introducing
a namespace for private LED triggers, so that we can name these
triggers like "hw-driven:driver-specific-name"?

[2]: AFAIK, KDE Plasma already includes kbd_backlight in its battery
panel (Plasma 5) or brightness panel (Plasma 6).

> As a note - on the Thinkpads we've had to look at getting the correct Intel ISH firmware loaded (and we're working on getting that upstream to linux-firmware). Is that needed on the Ideapads for the feature to work well or not?

My device (ThinkBook 14 G7+ ASP) has an AMD Ryzen CPU, so the answer
about Intel ISH firmware is apparent :P

It has two sensor hubs [3]. The ALS sensor is under the AMD Sensor
Fusion Hub (SFH). The auto brightness mode requires the amd_sfh driver
to be loaded to work properly, but does *not* need the kernel to load
the firmware. More details below.

* AMD Sensor Fusion Hub 1.1 (1022:164a, driver: amd_sfh -> hid-sensor-
hub):
`` amd_sfh registers a standard HID sensor hub virtual device, which is
then used by hid-sensor-hub.
`` Checking the source code of amd_sfh, it doesn't use the firmware
subsystem, so SFH1.1 seems to have the firmware built into the
platform.
`` Firmware version: 0xb000026.

-- Ambient Light Sensor (ALS, driver: hid-sensor-als):
``` hid-sensor-als registers an IIO device. It can be monitored via
iio-sensor-proxy [4].
``` The EC can't collect data from it until amd_sfh is loaded. Manually
unloading (rmmod) amd_sfh also breaks the data availability.

* Ideapad HID sensor hub (IDEA5003/048D:5003, driver: i2c-hid-acpi
-> hid-sensor-hub):
`` No IIO sensor is registered because all HID Usages used to pass
sensor values are vendor-specific.
`` The only way to monitor it is HIDRAW.

-- Human Presence Detection sensor (HPD, driver: hid-sensor-custom):
``` sensor-model=BIOMETRIC_HUMAN_DETECTION
``` friendly-name=AMS_TMF882X HOD V2010 Sensor
``` sensor-description=2.4 HOR0.0.19
``` The EC uses it to wake the device from S0ix (s2idle) on human
approach.
``` I've managed to figure out how to parse its reports to get the
distance between the human body and the device, as well as its
confidence.

-- Unknown sensor (driver: hid-sensor-custom):
``` sensor-model=LENOVO
``` friendly-name=Lenovo AMS_HPD V0302 Sensor
``` sensor-manufacturer=LENOVO
``` It reports an increasing number periodically.

-- Unknown sensor (driver: hid-sensor-custom):
``` sensor-model=Lenovo Customized Gest
``` friendly-name=Lenovo AMS_GESTRUE V0209 Sensor
``` sensor-manufacturer=LENOVO
``` It never sends any HID report.

[3]: Maybe this is a workaround so that the EC can collect data from
the HPD sensor in S0ix, otherwise this is so weird to have two separate
sensor hubs since AMD SFH also supports HPD sensors. But the wake-on-
human-presence feature is already weird anyway - my device wakes itself
when I am napping at the desk :-/ Zzz...
[4]: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy

I will just stop here as this somehow becomes off-topic. If you need
more information about my device (or if you can provide some
information for me, big thanks \o/), feel free to email me in private.

> Mark

Thanks,
Rong

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Mark Pearson]

Thanks Rong,

On Thu, Aug 7, 2025, at 10:50 AM, Rong Zhang wrote:
> Hi Mark,
>
> On Wed, 2025-08-06 at 15:02 -0400, Mark Pearson wrote:
>> Hi Rong,
>> 
>> On Tue, Aug 5, 2025, at 10:01 AM, Rong Zhang wrote:
>> > Currently, the auto brightness mode of keyboard backlight maps to
>> > brightness=0 in LED classdev. The only method to switch to such a mode
>> > is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
>> > is a multiplexed brightness value; writing 0 simply results in the
>> > backlight being turned off.
>> > 
>> > With brightness processing code decoupled from LED classdev, we can now
>> > fully support the auto brightness mode. In this mode, the keyboard
>> > backlight is controlled by the EC according to the ambient light sensor
>> > (ALS).
>> > 
>> > To utilize this, a sysfs node is exposed to the userspace:
>> > /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
>> > to align with dell-laptop, which provides a similar feature.
>> > 
>> > Signed-off-by: Rong Zhang <i@rong.moe>
>> > ---
>> >  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
>> >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
>> >  2 files changed, 75 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
>> > b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > index 5ec0dee9e707..a2b78aa60aaa 100644
>> > --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
>> > @@ -50,3 +50,15 @@ Description:
>> >  		Controls whether the "always on USB charging" feature is
>> >  		enabled or not. This feature enables charging USB devices
>> >  		even if the computer is not turned on.
>> > +
>> > +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
>> > +Date:		July 2025
>> > +KernelVersion:	6.17
>> > +Contact:	platform-driver-x86@vger.kernel.org
>> > +Description:
>> > +		This file allows to control the automatic keyboard
>> > +		illumination mode on some systems that have an ambient
>> > +		light sensor. Write 1 to this file to enable the auto
>> > +		mode, 0 to disable it. In this mode, the actual
>> > +		brightness level is not available and reading the
>> > +		"brightness" file always returns 0.
>> > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c 
>> > b/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > index 5014c1d0b633..49f2fc68add4 100644
>> > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
>> > @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct 
>> > ideapad_private *priv)
>> >  	ideapad_kbd_bl_notify_known(priv, brightness);
>> >  }
>> > 
>> > +static ssize_t als_enabled_show(struct device *dev,
>> > +				struct device_attribute *attr,
>> > +				char *buf)
>> > +{
>> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
>> > ideapad_private, kbd_bl.led);
>> > +	int hw_brightness;
>> > +
>> > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
>> > +	if (hw_brightness < 0)
>> > +		return hw_brightness;
>> > +
>> > +	return sysfs_emit(buf, "%d\n", hw_brightness == 
>> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
>> > +}
>> > +
>> > +static ssize_t als_enabled_store(struct device *dev,
>> > +				 struct device_attribute *attr,
>> > +				 const char *buf, size_t count)
>> > +{
>> > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
>> > +	struct ideapad_private *priv = container_of(led_cdev, struct 
>> > ideapad_private, kbd_bl.led);
>> > +	bool state;
>> > +	int err;
>> > +
>> > +	err = kstrtobool(buf, &state);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	/*
>> > +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
>> > +	 * impossible to disable it without setting another brightness value.
>> > +	 * Set the brightness to 0 when disabling is requested.
>> > +	 */
>> > +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? 
>> > KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
>> > +	if (err)
>> > +		return err;
>> > +
>> > +	/* Both HW brightness values map to 0 in the LED classdev. */
>> > +	ideapad_kbd_bl_notify_known(priv, 0);
>> > +
>> > +	return count;
>> > +}
>> > +
>> > +static DEVICE_ATTR_RW(als_enabled);
>> > +
>> > +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
>> > +	&dev_attr_als_enabled.attr,
>> > +	NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
>> > +
>> >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>> >  {
>> >  	int brightness, err;
>> > @@ -1722,10 +1773,20 @@ 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)) {
>> > +	switch (priv->kbd_bl.type) {
>> > +	case KBD_BL_TRISTATE_AUTO:
>> > +		/* The sysfs node will be 
>> > /sys/class/leds/platform::kbd_backlight/als_enabled */
>> > +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
>> > +		fallthrough;
>> > +	case KBD_BL_TRISTATE:
>> >  		priv->kbd_bl.led.max_brightness = 2;
>> > -	} else {
>> > +		break;
>> > +	case KBD_BL_STANDARD:
>> >  		priv->kbd_bl.led.max_brightness = 1;
>> > +		break;
>> > +	default:
>> > +		/* This has already been validated by ideapad_check_features(). */
>> > +		unreachable();
>> >  	}
>> > 
>> >  	brightness = ideapad_kbd_bl_brightness_get(priv);
>> > -- 
>> > 2.50.1
>> 
>> We're looking to implement this feature on the Thinkpads, so this change is timely :)
>
> Whoo, it's good to hear that.
>
>> I did wonder if we should be making changes at the LED class level? Something similar to LED_BRIGHT_HW_CHANGED maybe as a way to advertise that auto mode is supported and some hooks to support that in sysfs?
>
> To the best of my knowledge, there is already an ideal model to fit the
> auto brightness mode, which is private LED trigger.
>
> To utilize it, these are four pieces of the puzzle:
>
> (1) implement a private LED trigger (see leds-cros_ec and
>     leds-turris-omnia, for example)
> (2) turn on/off the auto brightness mode when the activate/deactivate
>     hooks are called
> (3) switch to the private LED trigger/the "none" trigger when such mode
>     is turned on/off by the HW (i.e., when Fn+Space is pressed)
> (4) notifying the userspace of the HW-triggered LED trigger change
>
> I'd finished (1) and (2) in my early experiments and verified their
> functionality. However, I eventually realized the dilemma that pressing
> Fn+Space would bring everything into an inconsistent state because of
> the lack of (3).
>
> For (3), when the HW turns on the auto brightness mode, we need:
>
>    mutex_lock(&led_cdev->led_access);
>   
>    down_write(&led_cdev->trigger_lock);
>    led_trigger_set(led_cdev, <THE PRIVATE LED TRIGGER>);
>    up_write(&led_cdev->trigger_lock);
>   
>    mutex_unlock(&led_cdev->led_access);
>
> When off, we need:
>
>    mutex_lock(&led_cdev->led_access);
>   
>    led_trigger_remove(led_cdev);
>   
>    mutex_unlock(&led_cdev->led_access);
>
> I never thought of (4) at that moment. Therefore, I eventually doubted
> whether it was worth so much overhead and turned to the method in the
> current patch.
>
> Think twice now, I think it is worth implementing (1)-(3) as long as
> (4) can be addressed. I just found that both led_trigger_set() and
> led_trigger_remove() send a uevent once the trigger is changed [1], and
> verified this using `udevadm monitor'. We have collected all four
> pieces of the puzzle, hooray!
>
> If you are OK with the private LED trigger approach, I will adopt it in
> [PATCH v2].
>
> [1]: commit 52c47742f79d ("leds: triggers: send uevent when changing
> triggers")
>
I'm not a LED expert, but your proposal (including details below) looks sensible to me.

>> I know it would be more work, but I'm guessing this is going to be a common feature across multiple vendors it might need doing at a common layer.
>
> CC'ing LED class maintainers.
>
excellent idea :)

> Private LED triggers currently have two users: leds-cros_ec and leds-
> turris-omnia. Their private triggers are respectively named "chromeos-
> auto" and "omnia-mcu".
>
> I agree that this is going to be a common feature. A generic name for
> such a feature helps userspace [2] identify it. What about introducing
> a namespace for private LED triggers, so that we can name these
> triggers like "hw-driven:driver-specific-name"?
>
> [2]: AFAIK, KDE Plasma already includes kbd_backlight in its battery
> panel (Plasma 5) or brightness panel (Plasma 6).
>
>> As a note - on the Thinkpads we've had to look at getting the correct Intel ISH firmware loaded (and we're working on getting that upstream to linux-firmware). Is that needed on the Ideapads for the feature to work well or not?
>
> My device (ThinkBook 14 G7+ ASP) has an AMD Ryzen CPU, so the answer
> about Intel ISH firmware is apparent :P
>
Ah...yeah - that won't apply.

> It has two sensor hubs [3]. The ALS sensor is under the AMD Sensor
> Fusion Hub (SFH). The auto brightness mode requires the amd_sfh driver
> to be loaded to work properly, but does *not* need the kernel to load
> the firmware. More details below.
>
> * AMD Sensor Fusion Hub 1.1 (1022:164a, driver: amd_sfh -> hid-sensor-
> hub):
> `` amd_sfh registers a standard HID sensor hub virtual device, which is
> then used by hid-sensor-hub.
> `` Checking the source code of amd_sfh, it doesn't use the firmware
> subsystem, so SFH1.1 seems to have the firmware built into the
> platform.
> `` Firmware version: 0xb000026.
>
> -- Ambient Light Sensor (ALS, driver: hid-sensor-als):
> ``` hid-sensor-als registers an IIO device. It can be monitored via
> iio-sensor-proxy [4].
> ``` The EC can't collect data from it until amd_sfh is loaded. Manually
> unloading (rmmod) amd_sfh also breaks the data availability.
>
> * Ideapad HID sensor hub (IDEA5003/048D:5003, driver: i2c-hid-acpi
> -> hid-sensor-hub):
> `` No IIO sensor is registered because all HID Usages used to pass
> sensor values are vendor-specific.
> `` The only way to monitor it is HIDRAW.
>
> -- Human Presence Detection sensor (HPD, driver: hid-sensor-custom):
> ``` sensor-model=BIOMETRIC_HUMAN_DETECTION
> ``` friendly-name=AMS_TMF882X HOD V2010 Sensor
> ``` sensor-description=2.4 HOR0.0.19
> ``` The EC uses it to wake the device from S0ix (s2idle) on human
> approach.
> ``` I've managed to figure out how to parse its reports to get the
> distance between the human body and the device, as well as its
> confidence.
>
> -- Unknown sensor (driver: hid-sensor-custom):
> ``` sensor-model=LENOVO
> ``` friendly-name=Lenovo AMS_HPD V0302 Sensor
> ``` sensor-manufacturer=LENOVO
> ``` It reports an increasing number periodically.
>
> -- Unknown sensor (driver: hid-sensor-custom):
> ``` sensor-model=Lenovo Customized Gest
> ``` friendly-name=Lenovo AMS_GESTRUE V0209 Sensor
> ``` sensor-manufacturer=LENOVO
> ``` It never sends any HID report.
>
I'll double check if/when we have any of this on the Thinkpads...I don't think we do, but I'm sure we will at some point.

> [3]: Maybe this is a workaround so that the EC can collect data from
> the HPD sensor in S0ix, otherwise this is so weird to have two separate
> sensor hubs since AMD SFH also supports HPD sensors. But the wake-on-
> human-presence feature is already weird anyway - my device wakes itself
> when I am napping at the desk :-/ Zzz...
> [4]: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy
>
> I will just stop here as this somehow becomes off-topic. If you need
> more information about my device (or if you can provide some
> information for me, big thanks \o/), feel free to email me in private.
>
As side notes
 - we are looking at the HPD stuff too...but that's a topic for another thread ;)
 - If your system is waking itself - try the AMD debug tool (https://git.kernel.org/pub/scm/linux/kernel/git/superm1/amd-debug-tools.git) and you might be able to figure out which device is waking you up.
I'll discuss the sensorhubs with the AMD folk too to get their input - I'm a bit behind on that. I'll ping you off thread if we can do something there (and feel free to directly nag me if I don't send anything in the next couple of weeks...it's a bit hectic right now)

Thanks for all the details
Mark

Re: [PATCH 2/2] platform/x86: ideapad-laptop: Fully support auto kbd backlight

[Rong Zhang]

Hi All,

On Fri, 2025-08-08 at 13:57 -0400, Mark Pearson wrote:
> Thanks Rong,
> 
> On Thu, Aug 7, 2025, at 10:50 AM, Rong Zhang wrote:
> > Hi Mark,
> > 
> > On Wed, 2025-08-06 at 15:02 -0400, Mark Pearson wrote:
> > > Hi Rong,
> > > 
> > > On Tue, Aug 5, 2025, at 10:01 AM, Rong Zhang wrote:
> > > > Currently, the auto brightness mode of keyboard backlight maps to
> > > > brightness=0 in LED classdev. The only method to switch to such a mode
> > > > is by pressing the manufacturer-defined shortcut (Fn+Space). However, 0
> > > > is a multiplexed brightness value; writing 0 simply results in the
> > > > backlight being turned off.
> > > > 
> > > > With brightness processing code decoupled from LED classdev, we can now
> > > > fully support the auto brightness mode. In this mode, the keyboard
> > > > backlight is controlled by the EC according to the ambient light sensor
> > > > (ALS).
> > > > 
> > > > To utilize this, a sysfs node is exposed to the userspace:
> > > > /sys/class/leds/platform::kbd_backlight/als_enabled. The name is chosen
> > > > to align with dell-laptop, which provides a similar feature.
> > > > 
> > > > Signed-off-by: Rong Zhang <i@rong.moe>
> > > > ---
> > > >  .../ABI/testing/sysfs-platform-ideapad-laptop | 12 ++++
> > > >  drivers/platform/x86/lenovo/ideapad-laptop.c  | 65 ++++++++++++++++++-
> > > >  2 files changed, 75 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop 
> > > > b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > > > index 5ec0dee9e707..a2b78aa60aaa 100644
> > > > --- a/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > > > +++ b/Documentation/ABI/testing/sysfs-platform-ideapad-laptop
> > > > @@ -50,3 +50,15 @@ Description:
> > > >  		Controls whether the "always on USB charging" feature is
> > > >  		enabled or not. This feature enables charging USB devices
> > > >  		even if the computer is not turned on.
> > > > +
> > > > +What:		/sys/class/leds/platform::kbd_backlight/als_enabled
> > > > +Date:		July 2025
> > > > +KernelVersion:	6.17
> > > > +Contact:	platform-driver-x86@vger.kernel.org
> > > > +Description:
> > > > +		This file allows to control the automatic keyboard
> > > > +		illumination mode on some systems that have an ambient
> > > > +		light sensor. Write 1 to this file to enable the auto
> > > > +		mode, 0 to disable it. In this mode, the actual
> > > > +		brightness level is not available and reading the
> > > > +		"brightness" file always returns 0.
> > > > diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c 
> > > > b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > > > index 5014c1d0b633..49f2fc68add4 100644
> > > > --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> > > > +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> > > > @@ -1712,6 +1712,57 @@ static void ideapad_kbd_bl_notify(struct 
> > > > ideapad_private *priv)
> > > >  	ideapad_kbd_bl_notify_known(priv, brightness);
> > > >  }
> > > > 
> > > > +static ssize_t als_enabled_show(struct device *dev,
> > > > +				struct device_attribute *attr,
> > > > +				char *buf)
> > > > +{
> > > > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > > > +	struct ideapad_private *priv = container_of(led_cdev, struct 
> > > > ideapad_private, kbd_bl.led);
> > > > +	int hw_brightness;
> > > > +
> > > > +	hw_brightness = ideapad_kbd_bl_hw_brightness_get(priv);
> > > > +	if (hw_brightness < 0)
> > > > +		return hw_brightness;
> > > > +
> > > > +	return sysfs_emit(buf, "%d\n", hw_brightness == 
> > > > KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> > > > +}
> > > > +
> > > > +static ssize_t als_enabled_store(struct device *dev,
> > > > +				 struct device_attribute *attr,
> > > > +				 const char *buf, size_t count)
> > > > +{
> > > > +	struct led_classdev *led_cdev = dev_get_drvdata(dev);
> > > > +	struct ideapad_private *priv = container_of(led_cdev, struct 
> > > > ideapad_private, kbd_bl.led);
> > > > +	bool state;
> > > > +	int err;
> > > > +
> > > > +	err = kstrtobool(buf, &state);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	/*
> > > > +	 * Auto (ALS) mode uses a predefined HW brightness value. It is
> > > > +	 * impossible to disable it without setting another brightness value.
> > > > +	 * Set the brightness to 0 when disabling is requested.
> > > > +	 */
> > > > +	err = ideapad_kbd_bl_hw_brightness_set(priv, state ? 
> > > > KBD_BL_AUTO_MODE_HW_BRIGHTNESS : 0);
> > > > +	if (err)
> > > > +		return err;
> > > > +
> > > > +	/* Both HW brightness values map to 0 in the LED classdev. */
> > > > +	ideapad_kbd_bl_notify_known(priv, 0);
> > > > +
> > > > +	return count;
> > > > +}
> > > > +
> > > > +static DEVICE_ATTR_RW(als_enabled);
> > > > +
> > > > +static struct attribute *ideapad_kbd_bl_als_attrs[] = {
> > > > +	&dev_attr_als_enabled.attr,
> > > > +	NULL,
> > > > +};
> > > > +ATTRIBUTE_GROUPS(ideapad_kbd_bl_als);
> > > > +
> > > >  static int ideapad_kbd_bl_init(struct ideapad_private *priv)
> > > >  {
> > > >  	int brightness, err;
> > > > @@ -1722,10 +1773,20 @@ 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)) {
> > > > +	switch (priv->kbd_bl.type) {
> > > > +	case KBD_BL_TRISTATE_AUTO:
> > > > +		/* The sysfs node will be 
> > > > /sys/class/leds/platform::kbd_backlight/als_enabled */
> > > > +		priv->kbd_bl.led.groups = ideapad_kbd_bl_als_groups;
> > > > +		fallthrough;
> > > > +	case KBD_BL_TRISTATE:
> > > >  		priv->kbd_bl.led.max_brightness = 2;
> > > > -	} else {
> > > > +		break;
> > > > +	case KBD_BL_STANDARD:
> > > >  		priv->kbd_bl.led.max_brightness = 1;
> > > > +		break;
> > > > +	default:
> > > > +		/* This has already been validated by ideapad_check_features(). */
> > > > +		unreachable();
> > > >  	}
> > > > 
> > > >  	brightness = ideapad_kbd_bl_brightness_get(priv);
> > > > -- 
> > > > 2.50.1
> > > 
> > > We're looking to implement this feature on the Thinkpads, so this change is timely :)
> > 
> > Whoo, it's good to hear that.
> > 
> > > I did wonder if we should be making changes at the LED class level? Something similar to LED_BRIGHT_HW_CHANGED maybe as a way to advertise that auto mode is supported and some hooks to support that in sysfs?
> > 
> > To the best of my knowledge, there is already an ideal model to fit the
> > auto brightness mode, which is private LED trigger.
> > 
> > To utilize it, these are four pieces of the puzzle:
> > 
> > (1) implement a private LED trigger (see leds-cros_ec and
> >     leds-turris-omnia, for example)
> > (2) turn on/off the auto brightness mode when the activate/deactivate
> >     hooks are called
> > (3) switch to the private LED trigger/the "none" trigger when such mode
> >     is turned on/off by the HW (i.e., when Fn+Space is pressed)
> > (4) notifying the userspace of the HW-triggered LED trigger change
> > 
> > I'd finished (1) and (2) in my early experiments and verified their
> > functionality. However, I eventually realized the dilemma that pressing
> > Fn+Space would bring everything into an inconsistent state because of
> > the lack of (3).
> > 
> > For (3), when the HW turns on the auto brightness mode, we need:
> > 
> >    mutex_lock(&led_cdev->led_access);
> >   
> >    down_write(&led_cdev->trigger_lock);
> >    led_trigger_set(led_cdev, <THE PRIVATE LED TRIGGER>);
> >    up_write(&led_cdev->trigger_lock);
> >   
> >    mutex_unlock(&led_cdev->led_access);
> > 
> > When off, we need:
> > 
> >    mutex_lock(&led_cdev->led_access);
> >   
> >    led_trigger_remove(led_cdev);
> >   
> >    mutex_unlock(&led_cdev->led_access);

After some careful consideration, I propose a new API for this:

static int __led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig,
			     bool hw_triggered)
{
	[...]
	if (led_cdev->trigger) {
	[...]
		if (trig || !hw_triggered)
			led_set_brightness(led_cdev, LED_OFF);
	}
	[...]
}

int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
{
	return __led_trigger_set(led_cdev, trig, false);
}

void led_trigger_notify_hw_control_changed(struct led_classdev *led_cdev, bool activate)
{
	/* Callers are expected to acquire led_cdev->led_access first. */
	/* Hold led_cdev->trigger_lock, release when finish. */

	hc_trig = /* Search for the trigger named after led_cdev->hw_control_trigger. */

	/* We have 6 possible situations to handle: */

	/* Hardware just activated hw control mode, while "none" is active */
	if (activate && !led_cdev->trigger)
		__led_trigger_set(led_cdev, hc_trig, true);

	/* Hardware just deactivated hw control mode, while hc_trig is active */
	else if (!activate && led_cdev->trigger == hc_trig)
		__led_trigger_set(led_cdev, NULL, true);

	/* Hardware just activated hw control mode, while hc_trig is active */
	/* Hardware just activated hw control mode, while another trigger is active */
	/* Hardware just deactivated hw control mode, while "none" is active */
	/* Hardware just deactivated hw control mode, while another trigger is active */
}

void led_trigger_notify_hw_control_changed_fast(struct led_classdev *led_cdev,
						struct led_trigger *hc_trig, bool activate)
{
	/*
	 * LED drivers with private trigger may call this, eliminating the need
	 * to search for hc_trig. Everything else is the same as
	 * led_trigger_notify_hw_control_changed.
	 */
}

I will send an RFC patchset for this when ready (probably in the next
week). For now, let me explain the new API:

1. If a keyboard is capable of adjusting its backlight automatically
according to ALS, a private trigger can excellently represent this
capability. However, most keyboards with auto backlight can also switch
it on and off upon user input, making the activation state of such
private triggers meaningless noise. This is a major blocker. If we
provide an API for HW-triggered LED trigger transition, I expect
various (future or existing) drivers for keyboards/laptops can adopt
it.

2. When HW deactivates hw control mode on its own, it may choose a
specific brightness instead of simply turning off the LED. We must keep
it unchanged, or else brightness cycling will be broken. In the case of
my ThinkBook, pressing Fn+Space cycles the keyboard backlight in the
following order: auto => low => high => off => auto. @Mark, how is the
case for IdeaPad/ThinkPad?

3. Only "hc_trig <=> none" transition is possible. Should we also
reactivate the current trigger if it is neither hw control trigger nor
"none"?

4. The API are void functions, since LED drivers are always responsible
for participating in the (de)activation sequence of hw control mode and
should collect enough information about the current status during their
participation.

5. The LED driver needs to be able to handle the activation sequence
even if the hardware is currently under hw control mode, and vice
versa. This shouldn't be a big problem since I suppose most LED
hardware is capable of entering/leaving hw control mode idempotently.
In case any driver needs it, the new API doesn't touch
led_cdev->led_access so that LED drivers can protect their housekeeping
work, preparing for the idempotence.

6. I am not very familiar with ledtrig-netdev. In my glance, it always
checks the HW state and initializes its options accordingly on
activation, so it is apparently non-idempotent. I think this is not a
big deal since there shouldn't be any NIC that would switch hw control
state for its LED on its own.

> > I never thought of (4) at that moment. Therefore, I eventually doubted
> > whether it was worth so much overhead and turned to the method in the
> > current patch.
> > 
> > Think twice now, I think it is worth implementing (1)-(3) as long as
> > (4) can be addressed. I just found that both led_trigger_set() and
> > led_trigger_remove() send a uevent once the trigger is changed [1], and
> > verified this using `udevadm monitor'. We have collected all four
> > pieces of the puzzle, hooray!
> > 
> > If you are OK with the private LED trigger approach, I will adopt it in
> > [PATCH v2].
> > 
> > [1]: commit 52c47742f79d ("leds: triggers: send uevent when changing
> > triggers")
> > 
> I'm not a LED expert, but your proposal (including details below) looks sensible to me.

I will first incorporate this patch into the RFC patch mentioned above
to provide a reference usage and demonstrate the simplicity of this API
in use. When the discussion is settled, I will submit them separately.

> > > I know it would be more work, but I'm guessing this is going to be a common feature across multiple vendors it might need doing at a common layer.
> > 
> > CC'ing LED class maintainers.
> > 
> excellent idea :)
> 
> > Private LED triggers currently have two users: leds-cros_ec and leds-
> > turris-omnia. Their private triggers are respectively named "chromeos-
> > auto" and "omnia-mcu".
> > 
> > I agree that this is going to be a common feature. A generic name for
> > such a feature helps userspace [2] identify it. What about introducing
> > a namespace for private LED triggers, so that we can name these
> > triggers like "hw-driven:driver-specific-name"?
> > 
> > [2]: AFAIK, KDE Plasma already includes kbd_backlight in its battery
> > panel (Plasma 5) or brightness panel (Plasma 6).

When designing the above API, I realized that such a simple naming
trick has inherent flaws: it doesn't make sense for dual-role triggers.

The "netdev" trigger is either a software trigger or a hardware-driven
trigger, depending on the underlying LED hardware and trigger options.
It has an attribute "offloaded" for its dual-role capability.

Thus, I propose a read-only attribute "trigger_may_offload". It is
exposed when led_cdev->hw_control_trigger is defined. Its value depends
on the state of the hw control trigger:

- Offloaded: "[hw_control_trigger]"
- Active but not offloaded: "<hw_control_trigger>"
- Inactive: "hw_control_trigger"

In this way, it perfectly answers two questions (Which trigger? What is
its state?) from userspace at once. It can also be easily extended if
we (unfortunately) meet some HW with more than one possible hw control
trigger.

A new method led_trigger->offloaded() also needs to be introduced to
make the attribute useful. This should be easy.

I will send another RFC patchset for this when ready. This should be
independent of the one for led_trigger_notify_hw_control_changed since
their functionalities are orthogonal.

> > > As a note - on the Thinkpads we've had to look at getting the correct Intel ISH firmware loaded (and we're working on getting that upstream to linux-firmware). Is that needed on the Ideapads for the feature to work well or not?
> > 
> > My device (ThinkBook 14 G7+ ASP) has an AMD Ryzen CPU, so the answer
> > about Intel ISH firmware is apparent :P
> > 
> Ah...yeah - that won't apply.
> 
> > It has two sensor hubs [3]. The ALS sensor is under the AMD Sensor
> > Fusion Hub (SFH). The auto brightness mode requires the amd_sfh driver
> > to be loaded to work properly, but does *not* need the kernel to load
> > the firmware. More details below.
> > 
> > * AMD Sensor Fusion Hub 1.1 (1022:164a, driver: amd_sfh -> hid-sensor-
> > hub):
> > `` amd_sfh registers a standard HID sensor hub virtual device, which is
> > then used by hid-sensor-hub.
> > `` Checking the source code of amd_sfh, it doesn't use the firmware
> > subsystem, so SFH1.1 seems to have the firmware built into the
> > platform.
> > `` Firmware version: 0xb000026.
> > 
> > -- Ambient Light Sensor (ALS, driver: hid-sensor-als):
> > ``` hid-sensor-als registers an IIO device. It can be monitored via
> > iio-sensor-proxy [4].
> > ``` The EC can't collect data from it until amd_sfh is loaded. Manually
> > unloading (rmmod) amd_sfh also breaks the data availability.
> > 
> > * Ideapad HID sensor hub (IDEA5003/048D:5003, driver: i2c-hid-acpi
> > -> hid-sensor-hub):
> > `` No IIO sensor is registered because all HID Usages used to pass
> > sensor values are vendor-specific.
> > `` The only way to monitor it is HIDRAW.
> > 
> > -- Human Presence Detection sensor (HPD, driver: hid-sensor-custom):
> > ``` sensor-model=BIOMETRIC_HUMAN_DETECTION
> > ``` friendly-name=AMS_TMF882X HOD V2010 Sensor
> > ``` sensor-description=2.4 HOR0.0.19
> > ``` The EC uses it to wake the device from S0ix (s2idle) on human
> > approach.
> > ``` I've managed to figure out how to parse its reports to get the
> > distance between the human body and the device, as well as its
> > confidence.
> > 
> > -- Unknown sensor (driver: hid-sensor-custom):
> > ``` sensor-model=LENOVO
> > ``` friendly-name=Lenovo AMS_HPD V0302 Sensor
> > ``` sensor-manufacturer=LENOVO
> > ``` It reports an increasing number periodically.
> > 
> > -- Unknown sensor (driver: hid-sensor-custom):
> > ``` sensor-model=Lenovo Customized Gest
> > ``` friendly-name=Lenovo AMS_GESTRUE V0209 Sensor
> > ``` sensor-manufacturer=LENOVO
> > ``` It never sends any HID report.
> > 
> I'll double check if/when we have any of this on the Thinkpads...I don't think we do, but I'm sure we will at some point.
> 
> > [3]: Maybe this is a workaround so that the EC can collect data from
> > the HPD sensor in S0ix, otherwise this is so weird to have two separate
> > sensor hubs since AMD SFH also supports HPD sensors. But the wake-on-
> > human-presence feature is already weird anyway - my device wakes itself
> > when I am napping at the desk :-/ Zzz...
> > [4]: https://gitlab.freedesktop.org/hadess/iio-sensor-proxy
> > 
> > I will just stop here as this somehow becomes off-topic. If you need
> > more information about my device (or if you can provide some
> > information for me, big thanks \o/), feel free to email me in private.
> > 

Hi Mark,

> As side notes
>  - we are looking at the HPD stuff too...but that's a topic for another thread ;)

So glad to hear that. I definitely want to write a driver for IDEA5003
sensor hub, but due to the lack of documentation and time, I put it
aside for the time being.

>  - If your system is waking itself - try the AMD debug tool (https://git.kernel.org/pub/scm/linux/kernel/git/superm1/amd-debug-tools.git) and you might be able to figure out which device is waking you up.
> I'll discuss the sensorhubs with the AMD folk too to get their input - I'm a bit behind on that. I'll ping you off thread if we can do something there (and feel free to directly nag me if I don't send anything in the next couple of weeks...it's a bit hectic right now)

Thanks for your information. I had done some tests with amd-debug-tools
before, so I knew the wakeup interrupt was from EC due to the wake-on-
human-presence feature. I've disabled it somehow with Lenovo BaiYing[1]
and it's persistent. I didn't mention these in the previous reply
because I thought these were just trivial details. But again, thanks
for your information.

[1]: I suppose writing 0 to
/sys/bus/acpi/devices/IDEA5003:00/physical_node/0018:048D:5003.0002/HID
-SENSOR-*/enable_sensor can achieve the same effect.

> Thanks for all the details
> Mark

Thanks,
Rong