Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/3] leds: pwm: some cleanups
@ 2020-01-24 16:54 Uwe Kleine-König
  2020-01-24 16:54 ` [PATCH 1/3] leds: pwm: simplify if condition Uwe Kleine-König
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2020-01-24 16:54 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Jeff LaBundy, Dan Murphy, Thierry Reding, linux-leds, linux-pwm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/3] leds: pwm: simplify if condition
  2020-01-24 16:54 [PATCH 0/3] leds: pwm: some cleanups Uwe Kleine-König
@ 2020-01-24 16:54 ` Uwe Kleine-König
  2020-01-26 18:55   ` Jeff LaBundy
  2020-01-24 16:54 ` [PATCH 2/3] leds: pwm: convert to atomic PWM API Uwe Kleine-König
  2020-01-24 16:54 ` [PATCH 3/3] leds: pwm: don't set the brightness during .probe Uwe Kleine-König
  2 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2020-01-24 16:54 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Jeff LaBundy, Dan Murphy, Thierry Reding, linux-leds, linux-pwm

.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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 2/3] leds: pwm: convert to atomic PWM API
  2020-01-24 16:54 [PATCH 0/3] leds: pwm: some cleanups Uwe Kleine-König
  2020-01-24 16:54 ` [PATCH 1/3] leds: pwm: simplify if condition Uwe Kleine-König
@ 2020-01-24 16:54 ` Uwe Kleine-König
  2020-01-26 19:15   ` Jeff LaBundy
  2020-01-24 16:54 ` [PATCH 3/3] leds: pwm: don't set the brightness during .probe Uwe Kleine-König
  2 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2020-01-24 16:54 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Jeff LaBundy, Dan Murphy, Thierry Reding, linux-leds, linux-pwm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 3/3] leds: pwm: don't set the brightness during .probe
  2020-01-24 16:54 [PATCH 0/3] leds: pwm: some cleanups Uwe Kleine-König
  2020-01-24 16:54 ` [PATCH 1/3] leds: pwm: simplify if condition Uwe Kleine-König
  2020-01-24 16:54 ` [PATCH 2/3] leds: pwm: convert to atomic PWM API Uwe Kleine-König
@ 2020-01-24 16:54 ` Uwe Kleine-König
  2020-01-26 19:42   ` Jeff LaBundy
  2 siblings, 1 reply; 8+ messages in thread
From: Uwe Kleine-König @ 2020-01-24 16:54 UTC (permalink / raw)
  To: Jacek Anaszewski, Pavel Machek
  Cc: Jeff LaBundy, Dan Murphy, Thierry Reding, linux-leds, linux-pwm

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


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] leds: pwm: simplify if condition
  2020-01-24 16:54 ` [PATCH 1/3] leds: pwm: simplify if condition Uwe Kleine-König
@ 2020-01-26 18:55   ` Jeff LaBundy
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff LaBundy @ 2020-01-26 18:55 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Thierry Reding,
	linux-leds, linux-pwm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/3] leds: pwm: convert to atomic PWM API
  2020-01-24 16:54 ` [PATCH 2/3] leds: pwm: convert to atomic PWM API Uwe Kleine-König
@ 2020-01-26 19:15   ` Jeff LaBundy
  0 siblings, 0 replies; 8+ messages in thread
From: Jeff LaBundy @ 2020-01-26 19:15 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Thierry Reding,
	linux-leds, linux-pwm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] leds: pwm: don't set the brightness during .probe
  2020-01-24 16:54 ` [PATCH 3/3] leds: pwm: don't set the brightness during .probe Uwe Kleine-König
@ 2020-01-26 19:42   ` Jeff LaBundy
  2020-01-27  7:41     ` Uwe Kleine-König
  0 siblings, 1 reply; 8+ messages in thread
From: Jeff LaBundy @ 2020-01-26 19:42 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Thierry Reding,
	linux-leds, linux-pwm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 3/3] leds: pwm: don't set the brightness during .probe
  2020-01-26 19:42   ` Jeff LaBundy
@ 2020-01-27  7:41     ` Uwe Kleine-König
  0 siblings, 0 replies; 8+ messages in thread
From: Uwe Kleine-König @ 2020-01-27  7:41 UTC (permalink / raw)
  To: Jeff LaBundy
  Cc: Jacek Anaszewski, Pavel Machek, Dan Murphy, Thierry Reding,
	linux-leds, linux-pwm

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-24 16:54 [PATCH 0/3] leds: pwm: some cleanups Uwe Kleine-König
2020-01-24 16:54 ` [PATCH 1/3] leds: pwm: simplify if condition Uwe Kleine-König
2020-01-26 18:55   ` Jeff LaBundy
2020-01-24 16:54 ` [PATCH 2/3] leds: pwm: convert to atomic PWM API Uwe Kleine-König
2020-01-26 19:15   ` Jeff LaBundy
2020-01-24 16:54 ` [PATCH 3/3] leds: pwm: don't set the brightness during .probe Uwe Kleine-König
2020-01-26 19:42   ` Jeff LaBundy
2020-01-27  7:41     ` Uwe Kleine-König

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org
	public-inbox-index linux-leds

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git