Re: [PATCH RFC v3 11/11] platform/x86: ideapad-laptop: Fully support auto keyboard backlight

[Ilpo Järvinen]

On Sun, 19 Jul 2026, 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 private hardware control trigger "ideapad-auto" is
> added, with the event handling procedure calling the
> led_trigger_notify_hw_control_changed() interface to activate/deactivate
> the private trigger according to the current LED trigger state.
> 
> Meanwhile, block brightness changes on exit to prevent the side effect
> of LED device unregistration when the private trigger is active from
> resetting the brightness to zero, so that we can retain the state of
> auto mode among boots.
> 
> Signed-off-by: Rong Zhang <i@xxxxxxxx>
> ---
> Changes in v3:
> - Address concerns from Sashiko
>   - Fix a race condition in ideapad_kbd_bl_led_cdev_brightness_set()
>   - Fix trigger re-registration of ideapad_kbd_bl_auto_trigger
>   - https://sashiko.dev/#/patchset/20260618-leds-trigger-hw-changed-v2-0-c28c44053cf3%40rong.moe
> - Make registration failures of ideapad_kbd_bl_auto_trigger non-fatal
> ---
>  drivers/platform/x86/lenovo/ideapad-laptop.c | 112 ++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/platform/x86/lenovo/ideapad-laptop.c b/drivers/platform/x86/lenovo/ideapad-laptop.c
> index 66e16abda5e3..253d2962b927 100644
> --- a/drivers/platform/x86/lenovo/ideapad-laptop.c
> +++ b/drivers/platform/x86/lenovo/ideapad-laptop.c
> @@ -1714,9 +1714,58 @@ static int ideapad_kbd_bl_led_cdev_brightness_set(struct led_classdev *led_cdev,
>  {
>  	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
>  
> +	/*
> +	 * When deinitializing: It must be the side effect of led_cdev
> +	 * unregistration when our private trigger is active. We've set
> +	 * LED_RETAIN_AT_SHUTDOWN to retain led_cdev brightness level.
> +	 * To do the same for auto mode, gate changes and return early.
> +	 */
> +	if (unlikely(!priv->kbd_bl.initialized))

This too would need include, but I think addressing some earlier include 
request will cover it.

> +		return 0;
> +
>  	return ideapad_kbd_bl_brightness_set(priv, brightness);
>  }
>  
> +static bool ideapad_kbd_bl_auto_trigger_offloaded(struct led_classdev *led_cdev)
> +{
> +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);

Add include for container_of().

> +
> +	return atomic_read(&priv->kbd_bl.last_hw_brightness) == KBD_BL_AUTO_MODE_HW_BRIGHTNESS;
> +}
> +
> +static int ideapad_kbd_bl_auto_trigger_activate(struct led_classdev *led_cdev)
> +{
> +	struct ideapad_private *priv = container_of(led_cdev, struct ideapad_private, kbd_bl.led);
> +
> +	return ideapad_kbd_bl_hw_brightness_set(priv, KBD_BL_AUTO_MODE_HW_BRIGHTNESS);
> +}
> +
> +static struct led_hw_trigger_type ideapad_kbd_bl_auto_trigger_type;
> +
> +static struct led_trigger ideapad_kbd_bl_auto_trigger = {
> +	.name = "ideapad-auto",
> +	.trigger_type = &ideapad_kbd_bl_auto_trigger_type,
> +	.activate = ideapad_kbd_bl_auto_trigger_activate,
> +	.offloaded = ideapad_kbd_bl_auto_trigger_offloaded,
> +};
> +
> +static bool ideapad_kbd_bl_auto_trigger_registered;
> +
> +static void ideapad_kbd_bl_notify_hw_control(struct ideapad_private *priv,
> +					     int hw_brightness, int last_hw_brightness)
> +{
> +	bool hw_control, last_hw_control;
> +
> +	if (priv->kbd_bl.type != KBD_BL_TRISTATE_AUTO)
> +		return;
> +
> +	hw_control = hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS;
> +	last_hw_control = last_hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS;
> +
> +	if (hw_control != last_hw_control)
> +		led_trigger_notify_hw_control_changed(&priv->kbd_bl.led, hw_control);
> +}
> +
>  static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  {
>  	int hw_brightness, brightness, last_hw_brightness;
> @@ -1738,6 +1787,8 @@ static void ideapad_kbd_bl_notify(struct ideapad_private *priv)
>  	if (hw_brightness == last_hw_brightness)
>  		return;
>  
> +	ideapad_kbd_bl_notify_hw_control(priv, hw_brightness, last_hw_brightness);
> +
>  	led_classdev_notify_brightness_hw_changed(&priv->kbd_bl.led, brightness);
>  }
>  
> @@ -1768,6 +1819,24 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  
>  	switch (priv->kbd_bl.type) {
>  	case KBD_BL_TRISTATE_AUTO:
> +		priv->kbd_bl.led.max_brightness = 2;
> +
> +		if (!ideapad_kbd_bl_auto_trigger_registered) {
> +			dev_warn(&priv->platform_device->dev,
> +				 "Could not provide LED trigger %s for keyboard backlight\n",
> +				 ideapad_kbd_bl_auto_trigger.name);
> +			break;
> +		}
> +
> +		priv->kbd_bl.led.flags             |= LED_TRIG_HW_CHANGED;
> +		priv->kbd_bl.led.hw_control_trigger = ideapad_kbd_bl_auto_trigger.name;
> +		priv->kbd_bl.led.trigger_type       = &ideapad_kbd_bl_auto_trigger_type;

I'm skeptical aligning makes things better here.

> +
> +		/* Hardware remembers the last brightness level, including auto mode. */
> +		if (hw_brightness == KBD_BL_AUTO_MODE_HW_BRIGHTNESS)
> +			priv->kbd_bl.led.default_trigger = ideapad_kbd_bl_auto_trigger.name;
> +
> +		break;
>  	case KBD_BL_TRISTATE:
>  		priv->kbd_bl.led.max_brightness = 2;
>  		break;
> @@ -1779,13 +1848,22 @@ static int ideapad_kbd_bl_init(struct ideapad_private *priv)
>  		unreachable();
>  	}
>  
> -	err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
> -	if (err)
> -		return err;
> +	/* Queue notifications, as kbd_bl.initialized is about to be set. */
> +	guard(mutex)(&priv->kbd_bl.notif_mutex);
>  
> +	/*
> +	 * Setting kbd_bl.initialized after led_classdev_register() could lead
> +	 * to race conditions in ideapad_kbd_bl_led_cdev_brightness_set() where
> +	 * kbd_bl.initialized is checked, so set it now. It can be reverted back
> +	 * if the LED classdev failed to register.
> +	 */
>  	priv->kbd_bl.initialized = true;
>  
> -	return 0;
> +	err = led_classdev_register(&priv->platform_device->dev, &priv->kbd_bl.led);
> +	if (err)
> +		priv->kbd_bl.initialized = false;
> +
> +	return err;
>  }
>  
>  static void ideapad_kbd_bl_exit(struct ideapad_private *priv)
> @@ -2612,17 +2690,30 @@ static int __init ideapad_laptop_init(void)
>  {
>  	int err;
>  
> +	err = led_trigger_register(&ideapad_kbd_bl_auto_trigger);
> +	if (err) {
> +		pr_warn("Failed to register LED trigger %s: %d\n",

include missing.

> +			ideapad_kbd_bl_auto_trigger.name, err);
> +	} else {
> +		ideapad_kbd_bl_auto_trigger_registered = true;
> +	}
> +
>  	err = ideapad_wmi_driver_register();
>  	if (err)
> -		return err;
> +		goto err_ledtrig;
>  
>  	err = platform_driver_register(&ideapad_acpi_driver);
> -	if (err) {
> -		ideapad_wmi_driver_unregister();
> -		return err;
> -	}
> +	if (err)
> +		goto err_wmi;
>  
>  	return 0;
> +
> +err_wmi:
> +	ideapad_wmi_driver_unregister();
> +err_ledtrig:
> +	if (ideapad_kbd_bl_auto_trigger_registered)
> +		led_trigger_unregister(&ideapad_kbd_bl_auto_trigger);
> +	return err;
>  }
>  module_init(ideapad_laptop_init)
>  
> @@ -2630,6 +2721,9 @@ static void __exit ideapad_laptop_exit(void)
>  {
>  	ideapad_wmi_driver_unregister();
>  	platform_driver_unregister(&ideapad_acpi_driver);

Why is the order not the reverse of the init order?

> +
> +	if (ideapad_kbd_bl_auto_trigger_registered)
> +		led_trigger_unregister(&ideapad_kbd_bl_auto_trigger);
>  }
>  module_exit(ideapad_laptop_exit)
>  
> 
> 

-- 
 i.