Re: [PATCH v2] leds: led-triggers: Improvements for default trigger

[Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx>]

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