On 3/13/25 00:56, Craig McQueen wrote:
On Thu, 13 Mar 2025 06:40:35 +1100 Jacek Anaszewski wrote:
> Hi Craig,
>
> Thanks for the update.
>
> On 3/12/25 02:11, Craig McQueen wrote:
> > Accept "default" written to sysfs trigger attr.
> > 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.
> >
> > If the default trigger is set to "none", then led_trigger_set_default()
> > will remove a trigger. This is in contrast to the default trigger being
> > unset, in which case led_trigger_set_default() does nothing.
> > ---
> > Documentation/ABI/testing/sysfs-class-led | 6 ++++++
> > drivers/leds/led-triggers.c | 26 ++++++++++++++++++++++-
> > 2 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-led b/Documentation/ABI/testing/sysfs-class-led
> > index 2e24ac3bd7ef..0313b82644f2 100644
> > --- a/Documentation/ABI/testing/sysfs-class-led
> > +++ b/Documentation/ABI/testing/sysfs-class-led
> > @@ -72,6 +72,12 @@ Description:
> > /sys/class/leds/ once a given trigger is selected. For
> > their documentation see `sysfs-class-led-trigger-*`.
> >
> > + Writing "none" removes the trigger for this LED.
> > +
> > + Writing "default" sets the trigger to the LED's default trigger
> > + (which would often be configured in the device tree for the
> > + hardware).
> > +
> > What: /sys/class/leds//inverted
> > Date: January 2011
> > KernelVersion: 2.6.38
> > diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
> > index b2d40f87a5ff..00c898af9969 100644
> > --- a/drivers/leds/led-triggers.c
> > +++ b/drivers/leds/led-triggers.c
> > @@ -49,11 +49,21 @@ ssize_t led_trigger_write(struct file *filp, struct kobject *kobj,
> > goto unlock;
> > }
> >
> > + /* Empty string. Do nothing. */
> > + if (sysfs_streq(buf, "")) {
> > + goto unlock;
> > + }
> > +
>
> Do we need this? It seems to be the same case as any other arbitrary
> string, for which we obviously don't have special handling.
I'll defer to you on this. I can remove it.
I don't see much gain in it, let's skip it.
> > if (sysfs_streq(buf, "none")) {
> > led_trigger_remove(led_cdev);
> > 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)) {
> > @@ -98,6 +108,9 @@ static int led_trigger_format(char *buf, size_t size,
> > int len = led_trigger_snprintf(buf, size, "%s",
> > led_cdev->trigger ? "none" : "[none]");
> >
> > + if (led_cdev->default_trigger && led_cdev->default_trigger[0])
> > + len += led_trigger_snprintf(buf + len, size - len, " default");
> > +
> > list_for_each_entry(trig, &trigger_list, next_trig) {
> > bool hit;
> >
> > @@ -278,9 +291,20 @@ void led_trigger_set_default(struct led_classdev *led_cdev)
> > struct led_trigger *trig;
> > bool found = false;
> >
> > - if (!led_cdev->default_trigger)
> > + /* NULL pointer or empty string. Do nothing. */
> > + if (!led_cdev->default_trigger || !led_cdev->default_trigger[0])
> > return;
> >
> > + /* This case isn't sensible. Do nothing. */
> > + if (!strcmp(led_cdev->default_trigger, "default"))
> > + return;
>
> This is rather validation of the default trigger name obtained from DT,
> which, if at all, should be done after parsing DT property in
> led_classdev_register_ext(). I'd skip it anyway.
In a separate patch I've submitted for the uleds driver, I want to add the
ability for users to set a default trigger for a userspace LED. Maybe I should
add extra validation of the trigger name in that uleds driver patch.
I'd do that even in a separate patch.
> > + /* Remove trigger. */
> > + if (!strcmp(led_cdev->default_trigger, "none")) {
> > + led_trigger_remove(led_cdev);
>
> This will be handled be led_trigger_set() called from
> led_match_default_trigger() below.
I added this because users should have a way to get led_trigger_set_default()
to reset a LED's trigger to none, i.e. to have a default trigger of "none".
If led_cdev->default_trigger is NULL then the function exits early, so that
doesn't achieve it.
Without the proposed change, if led_cdev->default_trigger is "none", then
it will look for a trigger literally named "none", and won't find it, so it will
follow the !found code path, which isn't helpful.
So, it seems beneficial to add explicit handling of "none" to remove any
existing trigger.
Makes sense.
> > + return;
> > + }
> > +
> > down_read(&triggers_list_lock);
> > down_write(&led_cdev->trigger_lock);
> > list_for_each_entry(trig, &trigger_list, next_trig) {
--
Best regards,
Jacek Anaszewski
