* [PATCH v2] leds: led-triggers: Improvements for default trigger
@ 2025-03-12 1:11 Craig McQueen
2025-03-12 19:40 ` Jacek Anaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Craig McQueen @ 2025-03-12 1:11 UTC (permalink / raw)
To: linux-leds; +Cc: Craig McQueen
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/<led> 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/<led>/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;
+ }
+
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;
+
+ /* Remove trigger. */
+ if (!strcmp(led_cdev->default_trigger, "none")) {
+ led_trigger_remove(led_cdev);
+ return;
+ }
+
down_read(&triggers_list_lock);
down_write(&led_cdev->trigger_lock);
list_for_each_entry(trig, &trigger_list, next_trig) {
--
2.48.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] leds: led-triggers: Improvements for default trigger
2025-03-12 1:11 [PATCH v2] leds: led-triggers: Improvements for default trigger Craig McQueen
@ 2025-03-12 19:40 ` Jacek Anaszewski
2025-03-12 23:56 ` Craig McQueen
0 siblings, 1 reply; 4+ messages in thread
From: Jacek Anaszewski @ 2025-03-12 19:40 UTC (permalink / raw)
To: Craig McQueen, linux-leds
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/<led> 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/<led>/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.
> 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.
> + /* 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.
> + 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] leds: led-triggers: Improvements for default trigger
2025-03-12 19:40 ` Jacek Anaszewski
@ 2025-03-12 23:56 ` Craig McQueen
2025-03-13 23:07 ` Jacek Anaszewski
0 siblings, 1 reply; 4+ messages in thread
From: Craig McQueen @ 2025-03-12 23:56 UTC (permalink / raw)
To: Jacek Anaszewski; +Cc: linux-leds
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.
> > 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.
> > + /* 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.
> > + return;
> > + }
> > +
> > down_read(&triggers_list_lock);
> > down_write(&led_cdev->trigger_lock);
> > list_for_each_entry(trig, &trigger_list, next_trig) {
--
Craig McQueen
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] leds: led-triggers: Improvements for default trigger
2025-03-12 23:56 ` Craig McQueen
@ 2025-03-13 23:07 ` Jacek Anaszewski
0 siblings, 0 replies; 4+ messages in thread
From: Jacek Anaszewski @ 2025-03-13 23:07 UTC (permalink / raw)
To: Craig McQueen; +Cc: linux-leds
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-13 23:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-12 1:11 [PATCH v2] leds: led-triggers: Improvements for default trigger Craig McQueen
2025-03-12 19:40 ` Jacek Anaszewski
2025-03-12 23:56 ` Craig McQueen
2025-03-13 23:07 ` Jacek Anaszewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox