Re: [RFC PATCH 1/9] leds: Load trigger modules on-demand if used as hw control trigger

[Rong Zhang]

Hi Andrew,

On Sat, 2026-04-11 at 22:42 +0200, Andrew Lunn wrote:
> > Hi maintainers - a gentle nudge on this one. Would like some
> > direction if we can move ahead with the series. If not, what is
> > required?
>  
> 
> 
> 
> > On our side I know Vishnu has implemented the thinkpad driver using
> > this and confirmed the design works there too.
>  
> 
> 
> 
> > Would like to get this upstream so we can start working with the
> > user-space folk on updating their pieces.
> 
> I'm not the Maintainer here, so i would not be too unhappy if i was
> ignored.

Thanks for sharing your thoughts.

> 
> I've had some time to think about this. I guess you are going to go
> with a Linux LED, whatever. With that assumption in place, a trigger
> makes sense. But i think it needs to be a special sort of trigger, and
> a special sort of LED.
> 
> This is not a general purpose LED, which is a basic assumption for
> Linux LEDs. You cannot make it blink for netdev packets, shift lock,
> heartbeat, etc, because Linux does not control it, the EC does. Linux
> can ask the EC to set it to some state, but the EC might change it,
> and notify Linux afterwards. That is not the normal behaviour of a
> Linux LED.

Such a behavior is mostly handled by brightness_hw_changed (with
CONFIG_LEDS_BRIGHTNESS_HW_CHANGED=y). It's already there for 9 years.
More details below.

The problem here is more about how to represent the ALS-based auto
brightness mode and how to notify userspace of its (de)activation. The
series demonstrates an approach to achieve both goals.

> 
> Also, it does not make sense to allow the trigger to be bound to any
> other LED.

I used a private trigger in PATCH 9/9. It can never be bound to any
other LEDs.

> 
> The trigger and the LED are tightly bound together. So i would put
> them in the same driver. There are triggers which live outside of
> drivers/leds/trigger/. So it is not a problem it lives somewhere
> else. It also solves the ordering problem you had, getting the trigger
> registered. Since it lives in the same driver as the LED, you know it
> is registered, you do it before registering the LED.

I intentionally included PATCH 9/9 in the series to demonstrate how to
use the new interface. It registers a private trigger before registering
the LED, so there is no ordering problem.

> 
> I think you need some additional logic in the core. This LED must have
> this trigger. I would look at using the is_visible() attribute
> callback to make the trigger file in sysfs invisible for this LED, so
> it cannot be changed. 
> 

Doing so breaks userspace. The LED device has been already there and
worked with LED triggers for so many years. Not having 100% pure
software control is not a convictive reason to break userspace from my
perspective. Of course I am not a maintainer too so that's just my
personal thoughts.

Moreover, LEDs without 100% pure software control are already there for
at least 9 years. All LEDs with the LED_BRIGHT_HW_CHANGED flag set fall
into this category. They've worked with LED triggers for 9 years. See
also commit 0cb8eb30d425 ("leds: class: Add new optional
brightness_hw_changed attribute")

All these LEDs are keyboard backlight. As long as the user doesn't press
the shortcut (e.g., Fn+Space), all these LEDs works with any LED
triggers perfectly. If an user presses the shortcut, they know what they
are doing. If an user enables a trigger by writing to the trigger file,
they know they shouldn't press the shortcut afterward.

> I would somehow get this trigger attached to the
> LED when it is created. 
> 

I've done this in PATCH 9/9.

> The trigger_relevant() call needs to be
> extended so that you cannot attach this trigger to any other LED.

trigger_relevant() already supports this.

I set .trigger_type to the same value on both the private trigger and
the LED device in PATCH 9/9. Doing so is enough to prevent the trigger
from being attached to any other LED. See also commit 93690cdf3060
("leds: trigger: add support for LED-private device triggers").

> 
> I would also think about naming. The behaviour is i guess specific to
> X86 laptops? Do Apple laptops have the same behaviour? What about
> Snapdragon X Series laptops? Chromebooks? USB keyboards? What about
> things like the Unihertz Titan 2?

   $ grep -rl LED_BRIGHT_HW_CHANGED
   Documentation/leds/leds-class.rst
   drivers/hid/hid-lg-g15.c
   drivers/leds/led-class.c
   drivers/platform/x86/dell/dell-laptop.c
   drivers/platform/x86/system76_acpi.c
   drivers/platform/x86/lenovo/ideapad-laptop.c
   drivers/platform/x86/lenovo/thinkpad_acpi.c
   drivers/platform/x86/samsung-galaxybook.c
   drivers/platform/x86/asus-wmi.c
   drivers/platform/x86/lg-laptop.c
   drivers/platform/x86/toshiba_acpi.c
   drivers/platform/arm64/lenovo-thinkpad-t14s.c
   include/linux/leds.h

There are arm64 laptops and Logitech's USB keyboards that change their
backlight brightness without consulting with the OS. If these devices
support auto brightness, they will benefit from this series too.

Also, the series aims to eliminate problems on naming. Userspace doesn't
need to know the trigger's name beforehand. Instead, the new interface
provides the name to userspace, see PATCH 6/9.

> 
> Somebody in the future might produce a generic solution. Linux
> controls the LED. The Linux input system gets the key presses and
> sends them to the trigger. You can bind an iio ambient sensor to the
> trigger, etc. So you should leave the name space open for other
> implementations.

I agree with that, but we are already living in a world that most Fn
keys on keyboards are always translated before reaching the OS. I don't
think we will have a keyboard that behave like the ideal way any soon,
especially considering that nowadays some compact keyboards even send
Meta+Shift+S instead of SysRq when the Fn key combination for screenshot
is pressed -- they can't even work with magic SysRq. And in an ideal
world the Copilot key (sends RCtrl+RShift+F23) shouldn't appear at
all...

Thanks,
Rong

> 
> 	 Andrew