On 3/9/25 19:50, 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.
Forgot to answer to the below sequence.
1. LED has a default trigger "moduletrigger".
2. Module that provides that trigger "moduletrigger" is unloaded.
Now led_trigger_unregister() is called, the trigger is being removed
from trigger_list and LED class devices having it set, have their
trigger property set to NULL.
3. LED has trigger set to something else, "othertrigger".
LED class device trigger property is being initialized with a pointer
to the related struct led_trigger.
4. led_trigger_set_default() is called for that LED.
Will the LED's trigger be effectively "none", or stay at
"othertrigger"?
Will stay at "othertrigger", since led_trigger_set_default() will end up
in !found state.
5. Module that provides "moduletrigger" is loaded again.
Will the LED be connected to its default trigger "moduletrigger",
or remain at "none"?
Will remain at "othertrigger". led_trigger_register() would set default
trigger for LED class device only if no trigger is set for the LED,
and the name matches LED's default trigger
--
Best regards,
Jacek Anaszewski
