[PATCH 0/3] leds: pwm: some cleanups

[PATCH 0/3] leds: pwm: some cleanups

[Uwe Kleine-König]

Hello,

note that this series is only compile tested, as I don't have a hardware
that uses this driver.

Best regards
Uwe

Uwe Kleine-König (3):
  leds: pwm: simplify if condition
  leds: pwm: convert to atomic PWM API
  leds: pwm: don't set the brightness during .probe

 drivers/leds/leds-pwm.c | 47 ++++++++++-------------------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

-- 
2.24.0

[PATCH 1/3] leds: pwm: simplify if condition

[Uwe Kleine-König]

.pwm_period_ns is an unsigned integer. So when led->pwm_period_ns > 0
is false, we now assign 0 to a value that is already 0, so it doesn't
hurt and we can skip checking the actual value.

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

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 8b6965a563e9..b72fd89ff390 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -102,7 +102,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 	pwm_get_args(led_data->pwm, &pargs);
 
 	led_data->period = pargs.period;
-	if (!led_data->period && (led->pwm_period_ns > 0))
+	if (!led_data->period)
 		led_data->period = led->pwm_period_ns;
 
 	ret = devm_led_classdev_register(dev, &led_data->cdev);
-- 
2.24.0

Re: [PATCH 1/3] leds: pwm: simplify if condition

[Jeff LaBundy]

Hi Uwe,

On Fri, Jan 24, 2020 at 05:54:07PM +0100, Uwe Kleine-König wrote:
> .pwm_period_ns is an unsigned integer. So when led->pwm_period_ns > 0
> is false, we now assign 0 to a value that is already 0, so it doesn't
> hurt and we can skip checking the actual value.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/leds/leds-pwm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 8b6965a563e9..b72fd89ff390 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -102,7 +102,7 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  	pwm_get_args(led_data->pwm, &pargs);
>  
>  	led_data->period = pargs.period;
> -	if (!led_data->period && (led->pwm_period_ns > 0))
> +	if (!led_data->period)
>  		led_data->period = led->pwm_period_ns;
>  
>  	ret = devm_led_classdev_register(dev, &led_data->cdev);
> -- 
> 2.24.0
> 

Having tested this series with the pwm-iqs620a driver in development
(v5) and some actual hardware (IQS620AEV04 connected to an n-channel
MOSFET and an LED), let me add:

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

Kind regards,
Jeff LaBundy

[PATCH 2/3] leds: pwm: convert to atomic PWM API

[Uwe Kleine-König]

pwm_config(), pwm_enable() and pwm_disable() should get removed in the
long run. So update the driver to use the atomic API that is here to
stay.

A few side effects:

 - led_pwm_set() now returns an error when setting the PWM fails.
 - During .probe() the PWM isn't disabled implicitly by pwm_apply_args()
   any more.

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

diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index b72fd89ff390..9111cdede0ee 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -22,9 +22,8 @@
 struct led_pwm_data {
 	struct led_classdev	cdev;
 	struct pwm_device	*pwm;
+	struct pwm_state	pwmstate;
 	unsigned int		active_low;
-	unsigned int		period;
-	int			duty;
 };
 
 struct led_pwm_priv {
@@ -32,44 +31,29 @@ struct led_pwm_priv {
 	struct led_pwm_data leds[0];
 };
 
-static void __led_pwm_set(struct led_pwm_data *led_dat)
-{
-	int new_duty = led_dat->duty;
-
-	pwm_config(led_dat->pwm, new_duty, led_dat->period);
-
-	if (new_duty == 0)
-		pwm_disable(led_dat->pwm);
-	else
-		pwm_enable(led_dat->pwm);
-}
-
 static int led_pwm_set(struct led_classdev *led_cdev,
 		       enum led_brightness brightness)
 {
 	struct led_pwm_data *led_dat =
 		container_of(led_cdev, struct led_pwm_data, cdev);
 	unsigned int max = led_dat->cdev.max_brightness;
-	unsigned long long duty =  led_dat->period;
+	unsigned long long duty = led_dat->pwmstate.period;
 
 	duty *= brightness;
 	do_div(duty, max);
 
 	if (led_dat->active_low)
-		duty = led_dat->period - duty;
-
-	led_dat->duty = duty;
-
-	__led_pwm_set(led_dat);
+		duty = led_dat->pwmstate.period - duty;
 
-	return 0;
+	led_dat->pwmstate.duty_cycle = duty;
+	led_dat->pwmstate.enabled = duty > 0;
+	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
 }
 
 static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 		       struct led_pwm *led, struct fwnode_handle *fwnode)
 {
 	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
-	struct pwm_args pargs;
 	int ret;
 
 	led_data->active_low = led->active_low;
@@ -93,17 +77,10 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
 
 	led_data->cdev.brightness_set_blocking = led_pwm_set;
 
-	/*
-	 * FIXME: pwm_apply_args() should be removed when switching to the
-	 * atomic PWM API.
-	 */
-	pwm_apply_args(led_data->pwm);
-
-	pwm_get_args(led_data->pwm, &pargs);
+	pwm_init_state(led_data->pwm, &led_data->pwmstate);
 
-	led_data->period = pargs.period;
-	if (!led_data->period)
-		led_data->period = led->pwm_period_ns;
+	if (!led_data->pwmstate.period)
+		led_data->pwmstate.period = led->pwm_period_ns;
 
 	ret = devm_led_classdev_register(dev, &led_data->cdev);
 	if (ret == 0) {
-- 
2.24.0

Re: [PATCH 2/3] leds: pwm: convert to atomic PWM API

[Jeff LaBundy]

Hi Uwe,

On Fri, Jan 24, 2020 at 05:54:08PM +0100, Uwe Kleine-König wrote:
> pwm_config(), pwm_enable() and pwm_disable() should get removed in the
> long run. So update the driver to use the atomic API that is here to
> stay.
> 
> A few side effects:
> 
>  - led_pwm_set() now returns an error when setting the PWM fails.
>  - During .probe() the PWM isn't disabled implicitly by pwm_apply_args()
>    any more.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
>  drivers/leds/leds-pwm.c | 41 +++++++++--------------------------------
>  1 file changed, 9 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index b72fd89ff390..9111cdede0ee 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -22,9 +22,8 @@
>  struct led_pwm_data {
>  	struct led_classdev	cdev;
>  	struct pwm_device	*pwm;
> +	struct pwm_state	pwmstate;
>  	unsigned int		active_low;
> -	unsigned int		period;
> -	int			duty;
>  };
>  
>  struct led_pwm_priv {
> @@ -32,44 +31,29 @@ struct led_pwm_priv {
>  	struct led_pwm_data leds[0];
>  };
>  
> -static void __led_pwm_set(struct led_pwm_data *led_dat)
> -{
> -	int new_duty = led_dat->duty;
> -
> -	pwm_config(led_dat->pwm, new_duty, led_dat->period);
> -
> -	if (new_duty == 0)
> -		pwm_disable(led_dat->pwm);
> -	else
> -		pwm_enable(led_dat->pwm);
> -}
> -
>  static int led_pwm_set(struct led_classdev *led_cdev,
>  		       enum led_brightness brightness)
>  {
>  	struct led_pwm_data *led_dat =
>  		container_of(led_cdev, struct led_pwm_data, cdev);
>  	unsigned int max = led_dat->cdev.max_brightness;
> -	unsigned long long duty =  led_dat->period;
> +	unsigned long long duty = led_dat->pwmstate.period;
>  
>  	duty *= brightness;
>  	do_div(duty, max);
>  
>  	if (led_dat->active_low)
> -		duty = led_dat->period - duty;
> -
> -	led_dat->duty = duty;
> -
> -	__led_pwm_set(led_dat);
> +		duty = led_dat->pwmstate.period - duty;
>  
> -	return 0;
> +	led_dat->pwmstate.duty_cycle = duty;
> +	led_dat->pwmstate.enabled = duty > 0;
> +	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
>  }
>  
>  static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  		       struct led_pwm *led, struct fwnode_handle *fwnode)
>  {
>  	struct led_pwm_data *led_data = &priv->leds[priv->num_leds];
> -	struct pwm_args pargs;
>  	int ret;
>  
>  	led_data->active_low = led->active_low;
> @@ -93,17 +77,10 @@ static int led_pwm_add(struct device *dev, struct led_pwm_priv *priv,
>  
>  	led_data->cdev.brightness_set_blocking = led_pwm_set;
>  
> -	/*
> -	 * FIXME: pwm_apply_args() should be removed when switching to the
> -	 * atomic PWM API.
> -	 */
> -	pwm_apply_args(led_data->pwm);
> -
> -	pwm_get_args(led_data->pwm, &pargs);
> +	pwm_init_state(led_data->pwm, &led_data->pwmstate);
>  
> -	led_data->period = pargs.period;
> -	if (!led_data->period)
> -		led_data->period = led->pwm_period_ns;
> +	if (!led_data->pwmstate.period)
> +		led_data->pwmstate.period = led->pwm_period_ns;
>  
>  	ret = devm_led_classdev_register(dev, &led_data->cdev);
>  	if (ret == 0) {
> -- 
> 2.24.0
> 

Using the same setup described in my response to patch [1/3], I checked
the following:

* LED appears as expected with brightness continuously swept between 0
  and 255 at fixed intervals
* I2C traffic appears as expected for random cases of brightness being
  changed between 0 and !0 and back to 0

And so:

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

Kind regards,
Jeff LaBundy

Re: [PATCH 2/3] leds: pwm: convert to atomic PWM API

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

Hi!

> On Fri, Jan 24, 2020 at 05:54:08PM +0100, Uwe Kleine-König wrote:
> > pwm_config(), pwm_enable() and pwm_disable() should get removed in the
> > long run. So update the driver to use the atomic API that is here to
> > stay.
> > 
> > A few side effects:
> > 
> >  - led_pwm_set() now returns an error when setting the PWM fails.
> >  - During .probe() the PWM isn't disabled implicitly by pwm_apply_args()
> >    any more.
> > 
> > Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>

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

Thanks for patches, thanks for testing.

1,2 applied to -for-next.

AFAICT 3/3 should really get the default-state: keep support before
being merged?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 2/3] leds: pwm: convert to atomic PWM API

[Uwe Kleine-König]

[-- Attachment #1.1: Type: text/plain, Size: 1038 bytes --]

Hi Pavel,

On 2/26/20 3:35 PM, Pavel Machek wrote:
>> On Fri, Jan 24, 2020 at 05:54:08PM +0100, Uwe Kleine-König wrote:
>>> pwm_config(), pwm_enable() and pwm_disable() should get removed in the
>>> long run. So update the driver to use the atomic API that is here to
>>> stay.
>>>
>>> A few side effects:
>>>
>>>  - led_pwm_set() now returns an error when setting the PWM fails.
>>>  - During .probe() the PWM isn't disabled implicitly by pwm_apply_args()
>>>    any more.
>>>
>>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> 
>> And so:
>>
>> Tested-by: Jeff LaBundy <jeff@labundy.com>
> 
> Thanks for patches, thanks for testing.
> 
> 1,2 applied to -for-next.
> 
> AFAICT 3/3 should really get the default-state: keep support before
> being merged?

Maybe I miss something, but I wonder why default-state: keep support
isn't implemented in the core but in each led driver instead. (And as
soon as someone implements it in the core, patch 3 is the right thing to
do.)

Best regards
Uwe


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Uwe Kleine-König]

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

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

[Jeff LaBundy]

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

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

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |