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

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

[-- Attachment #1: Type: text/plain, Size: 9482 bytes --]

On Wed, 22 Jul 2026, Rong Zhang wrote:

> 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@rong.moe>
> > > ---
> > > 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.

A separate patch works. I assume this series won't make it into this 
cycle.

> 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)
> > >  
> > > 
> > > 
> 

-- 
 i.