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