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

[Rong Zhang]

Hi Ilpo,

Thanks for reviewing the series :)

On Tue, 2026-07-21 at 20:14 +0300, Ilpo Järvinen wrote:
> 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?

Thanks for discovering it. Since it exists before the series, I guess I
will submit a fixup patch for it separately so that it don't have to
wait for an RFC series.

And ACK to all other comments in this and previous replies. Will fix
them when I resubmit the series.

Thanks,
Rong

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