Re: [PATCH] led-triggers: accept "default" written to sysfs trigger attr

[Jacek Anaszewski <jacek.anaszewski@gmail.com>]

On 3/11/25 11:27, Craig McQueen wrote:
> On Mon, 10 Mar 2025 05:50:23 +1100 Jacek Anaszewski  wrote:
>   > On 3/9/25 12:33, Craig McQueen wrote:
>   > > On Sat, 08 Mar 2025 04:10:49 +1100 Jacek Anaszewski  wrote:
>   > >   > On 3/7/25 17:50, Jacek Anaszewski wrote:
>   > >   > > Hi Craig,
>   > >   > >
>   > >   > > On 3/6/25 23:55, Craig McQueen wrote:
>   > >   > >> If the text "default" is written to the LED's sysfs 'trigger' attr, then
>   > >   > >> call led_trigger_set_default() to set the LED to its default trigger.
>   > >   > >> ---
>   > >   > >>   drivers/leds/led-triggers.c | 5 +++++
>   > >   > >>   1 file changed, 5 insertions(+)
>   > >   > >>
>   > >   > >> diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
>   > >   > >> index b2d40f87a5ff..f2bc3bb5062d 100644
>   > >   > >> --- a/drivers/leds/led-triggers.c
>   > >   > >> +++ b/drivers/leds/led-triggers.c
>   > >   > >> @@ -54,6 +54,11 @@ ssize_t led_trigger_write(struct file *filp, struct
>   > >   > >> kobject *kobj,
>   > >   > >>           goto unlock;
>   > >   > >>       }
>   > >   > >> +    if (sysfs_streq(buf, "default")) {
>   > >   > >> +        led_trigger_set_default(led_cdev);
>   > >   > >> +        goto unlock;
>   > >   > >> +    }
>   > >   > >> +
>   > >   > >>       down_read(&triggers_list_lock);
>   > >   > >>       list_for_each_entry(trig, &trigger_list, next_trig) {
>   > >   > >>           if (sysfs_streq(buf, trig->name) &&
>   > >   > >> trigger_relevant(led_cdev, trig)) {
>   > >   > >
>   > >   > > Makes sense for me, this would be the second half of the feature that is
>   > >   > > now available only from DT level.
>   > >   > >
>   > >   > > Reviewed-by: Jacek Anaszewski jacek.anaszewski@gmail.com>
>   > >   > >
>   > >   >
>   > >   > But after re-thinking it - we need to return -EINVAL in case
>   > >   > LED class device does not define default trigger, so that the user
>   > >   > had proper feedback.
>   > >   >
>   > >   > So, led_trigger_set_default() needs to be extended to return error
>   > >   > in case of !led_cdev->default_trigger or !found.
>   > >
>   > > In systems I've worked on, some LEDs have a default trigger, while others don't. I.e. it seems normal for an LED to have a default trigger of "none". I don't think of this as an error condition, but a normal operation to set an LED's trigger back to "none".
>   > >
>   > > The not-found case is an interesting corner case. It might be that a kernel module that provides a trigger is presently not loaded, so the trigger is not currently available -- but will be available if the kernel module is loaded again.
>   >
>   > Fair enough.
>   > It would be good to add this description to the entry related to
>   > "trigger" file in Documentation/ABI/testing/sysfs-class-led.
>   
> 
> I tried to update that document. But I wasn't sure what the required
> format is, when I'm not adding a new attribute but (slightly) modifying
> the behaviour of an existing attribute. Should I add a note to the existing
> /sys/class/leds/<led>/trigger description, or should I add a new

Just extend existing documentation of the /sys/class/leds/<led>/trigger
file. The intention is to provide a description of the new use case
you're adding for that file.

Now I've come to the conclusion, that led_trigger_format() should add
also "default" as of the strings accepted by the file on write,
similarly as it adds "none". However "default" would never be displayed
in brackets, as the selected one of course. Instead the exact default
trigger or "none" would be put in brackets, as it is done currently.

That way it would be self-documenting from the user perspective.

> /sys/class/leds/<led>/trigger entry at the bottom of the document,
> describing the modified behaviour?
> 

-- 
Best regards,
Jacek Anaszewski