diff mbox series

leds: core: Omit set_brightness error message for a LED supporting hw trigger only

Message ID 44177e37-9512-4044-8991-bb23b184bf37@gmail.com
State New
Headers show
Series leds: core: Omit set_brightness error message for a LED supporting hw trigger only | expand

Commit Message

Heiner Kallweit April 5, 2024, 8:58 p.m. UTC
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@gmail.com>
---
 drivers/leds/led-core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Lee Jones April 11, 2024, 2:26 p.m. UTC | #1
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@gmail.com>
> ---
>  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);
 }
Lee Jones April 11, 2024, 2:27 p.m. UTC | #2
On Thu, 11 Apr 2024, 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@gmail.com>
> > ---
> >  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);
>  }

If it looks reasonable to you, I'd e happy to submit it as a patch.
Heiner Kallweit April 11, 2024, 8:46 p.m. UTC | #3
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@gmail.com>
>> ---
>>  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.
Lee Jones May 2, 2024, 4:11 p.m. UTC | #4
On Thu, 11 Apr 2024, Heiner Kallweit wrote:

> 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@gmail.com>
> >> ---
> >>  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.

Sorry, it's been a busy few weeks.

Just so you know, this is still on my radar.

I plan on submitting this for the next cycle.
diff mbox series

Patch

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)))