Re: [PATCH] leds: core: Omit set_brightness error message for a LED supporting hw trigger only

[Heiner Kallweit <hkallweit1@xxxxxxxxx>]

On 11.04.2024 16:26, Lee Jones wrote:
> On Fri, 05 Apr 2024, Heiner Kallweit wrote:
> 
>> If both set_brightness functions return -ENOTSUPP, then the LED doesn't
>> support setting a fixed brightness value, and the error message isn't
>> helpful. This can be the case e.g. for LEDs supporting a specific hw
>> trigger only.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>>  drivers/leds/led-core.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/leds/led-core.c b/drivers/leds/led-core.c
>> index 89c9806cc..9485f799f 100644
>> --- a/drivers/leds/led-core.c
>> +++ b/drivers/leds/led-core.c
>> @@ -127,7 +127,8 @@ static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
>>  	ret = __led_set_brightness(led_cdev, value);
>>  	if (ret == -ENOTSUPP)
>>  		ret = __led_set_brightness_blocking(led_cdev, value);
>> -	if (ret < 0 &&
>> +	/* Don't emit error message if LED supports a hw trigger like netdev only */
>> +	if (ret < 0 && ret != -ENOTSUPP &&
>>  	    /* LED HW might have been unplugged, therefore don't warn */
>>  	    !(ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING) &&
>>  	    (led_cdev->flags & LED_HW_PLUGGABLE)))
> 
> This function is already pretty messy.
> 
> How about something like:
> 
> static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
>                                                   unsigned int value)
> {
>         int ret;
> 
>         ret = __led_set_brightness(led_cdev, value);
>         if (ret == -ENOTSUPP) {
>                 ret = __led_set_brightness_blocking(led_cdev, value);
>                 if (ret == -ENOTSUPP)
>                         /* No back-end support to set a fixed brightness value */
>                         return;
>         }
> 
>         if (ret == -ENODEV && (led_cdev->flags & LED_UNREGISTERING))
>                 /* LED HW might have been unplugged, therefore don't warn */
>                 return;
> 
>         if (ret < 0 && led_cdev->flags & LED_HW_PLUGGABLE)
>                 dev_err(led_cdev->dev, "Setting an LED's brightness failed (%d)\n", ret);
>  }
> 
I think this would be a functional change. If:
ret = -ENODEV
led_cdev->flags & LED_UNREGISTERING is true
led_cdev->flags & LED_HW_PLUGGABLE is false

Current code would warn, proposed new code not.

But I like the idea. Proposal:

static void set_brightness_delayed_set_brightness(struct led_classdev *led_cdev,
                                                  unsigned int value)
{
        int ret;

        ret = __led_set_brightness(led_cdev, value);
        if (ret == -ENOTSUPP) {
                ret = __led_set_brightness_blocking(led_cdev, value);
                if (ret == -ENOTSUPP)
                        /* No back-end support to set a fixed brightness value */
                        return;
        }

        /* LED HW might have been unplugged, therefore don't warn */
        if (ret == -ENODEV && led_cdev->flags & LED_UNREGISTERING &&
            led_cdev->flags & LED_HW_PLUGGABLE)
                return;

        if (ret < 0)
                dev_err(led_cdev->dev,
                        "Setting an LED's brightness failed (%d)\n", ret);
}

Fine with me if you submit the patch.