linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5] backlight: lp855x: Switch to atomic PWM API
@ 2021-11-03 17:38 Maíra Canal
  2021-11-04  6:55 ` Uwe Kleine-König
  0 siblings, 1 reply; 2+ messages in thread
From: Maíra Canal @ 2021-11-03 17:38 UTC (permalink / raw)
  To: daniel.thompson, lee.jones, jingoohan1, thierry.reding, u.kleine-koenig
  Cc: dri-devel, linux-fbdev, linux-kernel, linux-pwm

Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
replace it for the atomic PWM API.

Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
V1 -> V2: Initialize variable and simplify conditional loop
V2 -> V3: Fix assignment of NULL variable
V3 -> V4: Replace division for pwm_set_relative_duty_cycle
V4 -> V5: Fix overwrite of state.period
---
 drivers/video/backlight/lp855x_bl.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index e94932c69f54..e70a7b72dcf3 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
 
 static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 {
-	unsigned int period = lp->pdata->period_ns;
-	unsigned int duty = br * period / max_br;
 	struct pwm_device *pwm;
+	struct pwm_state state;
 
 	/* request pwm device with the consumer name */
 	if (!lp->pwm) {
@@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 
 		lp->pwm = pwm;
 
-		/*
-		 * FIXME: pwm_apply_args() should be removed when switching to
-		 * the atomic PWM API.
-		 */
-		pwm_apply_args(pwm);
+		pwm_init_state(lp->pwm, &state);
+		state.period = lp->pdata->period_ns;
+	} else {
+		pwm_get_state(lp->pwm, &state);
 	}
 
-	pwm_config(lp->pwm, duty, period);
-	if (duty)
-		pwm_enable(lp->pwm);
-	else
-		pwm_disable(lp->pwm);
+	pwm_set_relative_duty_cycle(&state, br, max_br);
+	state.enabled = state.duty_cycle;
+
+	pwm_apply_state(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)
-- 
2.31.1


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

* Re: [PATCH v5] backlight: lp855x: Switch to atomic PWM API
  2021-11-03 17:38 [PATCH v5] backlight: lp855x: Switch to atomic PWM API Maíra Canal
@ 2021-11-04  6:55 ` Uwe Kleine-König
  0 siblings, 0 replies; 2+ messages in thread
From: Uwe Kleine-König @ 2021-11-04  6:55 UTC (permalink / raw)
  To: Maíra Canal
  Cc: daniel.thompson, lee.jones, jingoohan1, thierry.reding,
	dri-devel, linux-fbdev, linux-kernel, linux-pwm

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

On Wed, Nov 03, 2021 at 02:38:05PM -0300, Maíra Canal wrote:
> Remove legacy PWM interface (pwm_config, pwm_enable, pwm_disable) and
> replace it for the atomic PWM API.
> 
> Signed-off-by: Maíra Canal <maira.canal@usp.br>
> ---
> V1 -> V2: Initialize variable and simplify conditional loop
> V2 -> V3: Fix assignment of NULL variable
> V3 -> V4: Replace division for pwm_set_relative_duty_cycle
> V4 -> V5: Fix overwrite of state.period
> ---
>  drivers/video/backlight/lp855x_bl.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index e94932c69f54..e70a7b72dcf3 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -233,9 +233,8 @@ static int lp855x_configure(struct lp855x *lp)
>  
>  static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  {
> -	unsigned int period = lp->pdata->period_ns;
> -	unsigned int duty = br * period / max_br;
>  	struct pwm_device *pwm;
> +	struct pwm_state state;
>  
>  	/* request pwm device with the consumer name */
>  	if (!lp->pwm) {
> @@ -245,18 +244,16 @@ static void lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  
>  		lp->pwm = pwm;
>  
> -		/*
> -		 * FIXME: pwm_apply_args() should be removed when switching to
> -		 * the atomic PWM API.
> -		 */
> -		pwm_apply_args(pwm);
> +		pwm_init_state(lp->pwm, &state);
> +		state.period = lp->pdata->period_ns;
> +	} else {
> +		pwm_get_state(lp->pwm, &state);
>  	}
>  
> -	pwm_config(lp->pwm, duty, period);
> -	if (duty)
> -		pwm_enable(lp->pwm);
> -	else
> -		pwm_disable(lp->pwm);
> +	pwm_set_relative_duty_cycle(&state, br, max_br);
> +	state.enabled = state.duty_cycle;
> +
> +	pwm_apply_state(lp->pwm, &state);

Looks mostly right, but only on a deeper look into the driver. The
reason is that in the unmodified code there is always explicitly
period=lp->pdata->period_ns; while after your change the period is unset
(and so the previously set period is used).

So either mention in the change log that this driver doesn't modify the
pwm settings in other places or preferably pick an equivalent conversion
(plus maybe an optimisation in a separate patch).

If you go the "equivalent conversion" path, please note that
pwm_set_relative_duty_cycle() isn't equivalent to br * period / max_br,
as it implements a different rounding.

Best regards
Uwe


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

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

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

end of thread, other threads:[~2021-11-04  6:56 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-03 17:38 [PATCH v5] backlight: lp855x: Switch to atomic PWM API Maíra Canal
2021-11-04  6:55 ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).