[3/3] leds: pwm: don't set the brightness during .probe

The explicit call to led_pwm_set() prevents that the led's state can
stay as configured by the bootloader.

Note that brightness is always 0 here as the led_pwm driver doesn't
provide a .brightness_get() callback and so the LED was disabled
unconditionally.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/leds/leds-pwm.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Hi Uwe,

Thank you for your work on this series.

On Fri, Jan 24, 2020 at 05:54:09PM +0100, Uwe Kleine-König wrote:
> The explicit call to led_pwm_set() prevents that the led's state can
> stay as configured by the bootloader.
> 
> Note that brightness is always 0 here as the led_pwm driver doesn't
> provide a .brightness_get() callback and so the LED was disabled
> unconditionally.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/leds/leds-pwm.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 9111cdede0ee..de74e1c8d234 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -83,13 +83,11 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  		led_data->pwmstate.period = led->pwm_period_ns;
>  
>  	ret = devm_led_classdev_register(dev, &led_data->cdev);
> -	if (ret == 0) {
> +	if (ret == 0)
>  		priv->num_leds++;
> -		led_pwm_set(&led_data->cdev, led_data->cdev.brightness);
> -	} else {
> +	else
>  		dev_err(dev, "failed to register PWM led for %s: %d\n",
>  			led->name, ret);
> -	}
>  
>  	return ret;
>  }
> -- 
> 2.24.0
> 

My only concern here is whether or not there happen to be some hardware
out in the world whose PWM-related registers power on to a random state
and unknowingly depended on brightness being forced to zero.

The other folks on the thread probably have some better view into this,
but I wonder if the safest option would be to adopt the "default-state"
property from leds/common.txt, and only forgo the call to led_pwm_set()
if the property is set to "keep".

I did test this and can confirm that the LED's state prior to the driver
being loaded is preserved. However as you've warned, the brightness read
back from user space is zero and does not match the actual brightness of
the LED.

Understanding that it's more work, I wonder if this patch is not safe if
we don't also add a brightness_get callback in case there were any cases
where user space makes some decisions based on brightness == 0?

At any rate this patch does what is advertised, and so:

Tested-by: Jeff LaBundy <jeff@labundy.com>

Kind regards,
Jeff LaBundy

Hello Jeff,

On Sun, Jan 26, 2020 at 07:42:39PM +0000, Jeff LaBundy wrote:
> My only concern here is whether or not there happen to be some hardware
> out in the world whose PWM-related registers power on to a random state
> and unknowingly depended on brightness being forced to zero.

This might happen, and is (AFAIK) the same for other LED drivers.

> The other folks on the thread probably have some better view into this,
> but I wonder if the safest option would be to adopt the "default-state"
> property from leds/common.txt, and only forgo the call to led_pwm_set()
> if the property is set to "keep".

I think it would be sane to add support for parsing the default-state
property to the LED core. I was surprised to learn that this for now is
only implemented in a few LED drivers.

> I did test this and can confirm that the LED's state prior to the driver
> being loaded is preserved. However as you've warned, the brightness read
> back from user space is zero and does not match the actual brightness of
> the LED.

That is something that the core should handle, too. If there is no
.get_brightness callback and 0 is assumed, calling .set_brightness to 0
to ensure a right assumption sounds like the right thing to me.
 
> Understanding that it's more work, I wonder if this patch is not safe if
> we don't also add a brightness_get callback in case there were any cases
> where user space makes some decisions based on brightness == 0?
> 
> At any rate this patch does what is advertised, and so:
> 
> Tested-by: Jeff LaBundy <jeff@labundy.com>

Thanks for your feedback.

Best regards
Uwe