Re: [PATCH] leds: trigger: input-events: Restore previous brightness on input

[Николай Пластинин]

> Should we be overloading 'blink_brightness' like this?   It would be cleaner to
> allocate a small per-LED state struct in the 'activate()' callback, store it in
> 'led_cdev->trigger_data', and free it in a 'deactivate()' callback.

Agreed, I'll keep the saved level in a small per-LED struct allocated in
activate() and freed in deactivate().

> Please avoid accessing 'led_cdevs' and 'trig_list' directly from outside the LED
> core.  As you suggested in the commit message, adding a helper in
> 'led-triggers.c' would be much preferred.

Will do: v2 adds led_trigger_walk_leds() to led-triggers.c and the
trigger uses that instead of walking the list itself.

Thanks for the review, v2 to follow.

Nikolay

вт, 29 сент. 2026 г. в 17:11, Lee Jones <lee@xxxxxxxxxx>:
>
> On Thu, 24 Sep 2026, Nikolay Plastinin via B4 Relay wrote:
>
> > From: Nikolay Plastinin <plaztininikolai@xxxxxxxxx>
> >
> > The trigger turns the LEDs back on with LED_FULL, which the LED core
> > clamps to max_brightness. For keyboard backlights with several levels,
> > such as asus::kbd_backlight, this means the backlight always comes back
> > at maximum after being idle instead of at the level the user picked.
> >
> > Remember the brightness in blink_brightness before turning the LEDs off
> > and use it when turning them back on, as the netdev trigger does.
> > Brightness changes made while the trigger is active are picked up too.
> > This needs per-LED handling, so convert the trigger from the simple
> > trigger API to a regular trigger with an activate callback.
> >
> > Tested on an ASUS ROG Flow X13 GV302XV: with the keyboard backlight at
> > level 1, it now comes back at level 1 after going idle instead of at 3.
> > A level changed with the brightness hotkeys while the trigger is active
> > is restored after the next idle period as well.
> >
> > Assisted-by: Claude Opus 5.5
> > Signed-off-by: Nikolay Plastinin <plaztininikolai@xxxxxxxxx>
> > ---
> > This came up while looking at a keyboard backlight idle timeout for ASUS
> > laptops in asusctl:
> > https://github.com/OpenGamingCollective/asusctl/issues/98
> >
> > The trigger walks trig->led_cdevs under rcu_read_lock() the same way
> > led_trigger_event() does. I can add a helper in led-triggers.c instead
> > if that is preferred.
> > ---
> >  drivers/leds/trigger/ledtrig-input-events.c | 56 +++++++++++++++++++++++++----
> >  1 file changed, 50 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/leds/trigger/ledtrig-input-events.c b/drivers/leds/trigger/ledtrig-input-events.c
> > index d057b2a23..d1ef14ae8 100644
> > --- a/drivers/leds/trigger/ledtrig-input-events.c
> > +++ b/drivers/leds/trigger/ledtrig-input-events.c
> > @@ -10,6 +10,7 @@
> >  #include <linux/leds.h>
> >  #include <linux/module.h>
> >  #include <linux/moduleparam.h>
> > +#include <linux/rculist.h>
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/workqueue.h>
> > @@ -28,7 +29,48 @@ static struct input_events_data {
> >       unsigned long led_off_time;
> >  } input_events_data;
> >
> > -static struct led_trigger *input_events_led_trigger;
> > +static int input_events_activate(struct led_classdev *led_cdev)
> > +{
> > +     struct input_events_data *data = &input_events_data;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&data->lock, flags);
> > +
> > +     if (led_cdev->brightness)
> > +             led_cdev->blink_brightness = led_cdev->brightness;
>
> Should we be overloading 'blink_brightness' like this?   It would be cleaner to
> allocate a small per-LED state struct in the 'activate()' callback, store it in
> 'led_cdev->trigger_data', and free it in a 'deactivate()' callback.
>
> > +     if (!led_cdev->blink_brightness)
> > +             led_cdev->blink_brightness = led_cdev->max_brightness;
> > +
> > +     led_set_brightness(led_cdev, data->led_on ? led_cdev->blink_brightness : LED_OFF);
> > +
> > +     spin_unlock_irqrestore(&data->lock, flags);
> > +
> > +     return 0;
> > +}
> > +
> > +static struct led_trigger input_events_led_trigger = {
> > +     .name = "input-events",
> > +     .activate = input_events_activate,
> > +};
> > +
> > +/* Must be called with input_events_data.lock held */
> > +static void input_events_set_leds(bool on)
> > +{
> > +     struct led_classdev *led_cdev;
> > +
> > +     rcu_read_lock();
> > +     list_for_each_entry_rcu(led_cdev, &input_events_led_trigger.led_cdevs, trig_list) {
>
> Please avoid accessing 'led_cdevs' and 'trig_list' directly from outside the LED
> core.  As you suggested in the commit message, adding a helper in
> 'led-triggers.c' would be much preferred.
>
> > +             /*
> > +              * Remember the brightness the LED was last set to, so that it
> > +              * is restored instead of max brightness on the next input event.
> > +              */
> > +             if (led_cdev->brightness)
> > +                     led_cdev->blink_brightness = led_cdev->brightness;
> > +
> > +             led_set_brightness(led_cdev, on ? led_cdev->blink_brightness : LED_OFF);
> > +     }
> > +     rcu_read_unlock();
> > +}
> >
> >  static void led_input_events_work(struct work_struct *work)
> >  {
> > @@ -42,7 +84,7 @@ static void led_input_events_work(struct work_struct *work)
> >        * running before a new event pushed led_off_time back.
> >        */
> >       if (time_after_eq(jiffies, data->led_off_time)) {
> > -             led_trigger_event(input_events_led_trigger, LED_OFF);
> > +             input_events_set_leds(false);
> >               data->led_on = false;
> >       }
> >
> > @@ -59,7 +101,7 @@ static void input_events_event(struct input_handle *handle, unsigned int type,
> >       spin_lock_irqsave(&data->lock, flags);
> >
> >       if (!data->led_on) {
> > -             led_trigger_event(input_events_led_trigger, LED_FULL);
> > +             input_events_set_leds(true);
> >               data->led_on = true;
> >       }
> >       data->led_off_time = jiffies + led_off_delay;
> > @@ -138,11 +180,13 @@ static int __init input_events_init(void)
> >       INIT_DELAYED_WORK(&input_events_data.work, led_input_events_work);
> >       spin_lock_init(&input_events_data.lock);
> >
> > -     led_trigger_register_simple("input-events", &input_events_led_trigger);
> > +     ret = led_trigger_register(&input_events_led_trigger);
> > +     if (ret)
> > +             return ret;
> >
> >       ret = input_register_handler(&input_events_handler);
> >       if (ret) {
> > -             led_trigger_unregister_simple(input_events_led_trigger);
> > +             led_trigger_unregister(&input_events_led_trigger);
> >               return ret;
> >       }
> >
> > @@ -153,7 +197,7 @@ static void __exit input_events_exit(void)
> >  {
> >       input_unregister_handler(&input_events_handler);
> >       cancel_delayed_work_sync(&input_events_data.work);
> > -     led_trigger_unregister_simple(input_events_led_trigger);
> > +     led_trigger_unregister(&input_events_led_trigger);
> >  }
> >
> >  module_init(input_events_init);
> >
> > ---
> > base-commit: f475845eaf3d749114a63270bf2efea459e14dd2
> > change-id: 20260924-leds-input-events-brightness-a76ae9a77b85
> >
> > Best regards,
> > --
> > Nikolay Plastinin <plaztininikolai@xxxxxxxxx>
> >
> >
>
> --
> Lee Jones