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@xxxxxxxxx>
> > > >
> > >
> > > 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
/sys/class/leds/<led>/trigger entry at the bottom of the document,
describing the modified behaviour?
--
Craig McQueen
