[PATCH] pwm: visconti: Fix and simplify period calculation

[PATCH] pwm: visconti: Fix and simplify period calculation

[Uwe Kleine-König]

With the original code a request for period = 65536000 ns and period =
32768000 ns yields the same register settings (which results in 32768000
ns) because the value for pwmc0 was miscalculated.

Also simplify using that fls(0) is 0.

Fixes: 721b595744f1 ("pwm: visconti: Add Toshiba Visconti SoC PWM support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/pwm/pwm-visconti.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
index 46d903786366..af4e37d3e3a6 100644
--- a/drivers/pwm/pwm-visconti.c
+++ b/drivers/pwm/pwm-visconti.c
@@ -82,17 +82,14 @@ static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return -ERANGE;
 
 	/*
-	 * PWMC controls a divider that divides the input clk by a
-	 * power of two between 1 and 8. As a smaller divider yields
-	 * higher precision, pick the smallest possible one.
+	 * PWMC controls a divider that divides the input clk by a power of two
+	 * between 1 and 8. As a smaller divider yields higher precision, pick
+	 * the smallest possible one. As period is at most 0xffff << 3, pwmc0 is
+	 * in the intended range [0..3].
 	 */
-	if (period > 0xffff) {
-		pwmc0 = ilog2(period >> 16);
-		if (WARN_ON(pwmc0 > 3))
-			return -EINVAL;
-	} else {
-		pwmc0 = 0;
-	}
+	pwmc0 = fls(period >> 16);
+	if (WARN_ON(pwmc0 > 3))
+		return -EINVAL;
 
 	period >>= pwmc0;
 	duty_cycle >>= pwmc0;
-- 
2.30.2

RE: [PATCH] pwm: visconti: Fix and simplify period calculation

[nobuhiro1.iwamatsu]

Hi Uwe,

> -----Original Message-----
> From: Uwe Kleine-König [mailto:u.kleine-koenig@pengutronix.de]
> Sent: Tuesday, April 27, 2021 12:04 AM
> To: iwamatsu nobuhiro(岩松 信洋 □ＳＷＣ◯ＡＣＴ) <nobuhiro1.iwamatsu@toshiba.co.jp>; Thierry Reding
> <thierry.reding@gmail.com>; Lee Jones <lee.jones@linaro.org>
> Cc: linux-pwm@vger.kernel.org; kernel@pengutronix.de
> Subject: [PATCH] pwm: visconti: Fix and simplify period calculation
> 
> With the original code a request for period = 65536000 ns and period =
> 32768000 ns yields the same register settings (which results in 32768000
> ns) because the value for pwmc0 was miscalculated.
> 

I see, the current code uses ilog2(), which causes this problem.
Thank for fixing.

> Also simplify using that fls(0) is 0.
> 
> Fixes: 721b595744f1 ("pwm: visconti: Add Toshiba Visconti SoC PWM support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Acked-by: Nobuhiro Iwamatsu <nobuhiro1.iwamatsu@toshiba.co.jp>

> ---
>  drivers/pwm/pwm-visconti.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-visconti.c b/drivers/pwm/pwm-visconti.c
> index 46d903786366..af4e37d3e3a6 100644
> --- a/drivers/pwm/pwm-visconti.c
> +++ b/drivers/pwm/pwm-visconti.c
> @@ -82,17 +82,14 @@ static int visconti_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  		return -ERANGE;
> 
>  	/*
> -	 * PWMC controls a divider that divides the input clk by a
> -	 * power of two between 1 and 8. As a smaller divider yields
> -	 * higher precision, pick the smallest possible one.
> +	 * PWMC controls a divider that divides the input clk by a power of two
> +	 * between 1 and 8. As a smaller divider yields higher precision, pick
> +	 * the smallest possible one. As period is at most 0xffff << 3, pwmc0 is
> +	 * in the intended range [0..3].
>  	 */
> -	if (period > 0xffff) {
> -		pwmc0 = ilog2(period >> 16);
> -		if (WARN_ON(pwmc0 > 3))
> -			return -EINVAL;
> -	} else {
> -		pwmc0 = 0;
> -	}
> +	pwmc0 = fls(period >> 16);
> +	if (WARN_ON(pwmc0 > 3))
> +		return -EINVAL;
> 
>  	period >>= pwmc0;
>  	duty_cycle >>= pwmc0;
> --
> 2.30.2

Re: [PATCH] pwm: visconti: Fix and simplify period calculation

[Thierry Reding]

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

On Mon, Apr 26, 2021 at 05:03:50PM +0200, Uwe Kleine-König wrote:
> With the original code a request for period = 65536000 ns and period =
> 32768000 ns yields the same register settings (which results in 32768000
> ns) because the value for pwmc0 was miscalculated.
> 
> Also simplify using that fls(0) is 0.
> 
> Fixes: 721b595744f1 ("pwm: visconti: Add Toshiba Visconti SoC PWM support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/pwm/pwm-visconti.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH] pwm: visconti: Fix and simplify period calculation

[Uwe Kleine-König]

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

On Tue, May 25, 2021 at 06:29:57PM +0200, Thierry Reding wrote:
> On Mon, Apr 26, 2021 at 05:03:50PM +0200, Uwe Kleine-König wrote:
> > With the original code a request for period = 65536000 ns and period =
> > 32768000 ns yields the same register settings (which results in 32768000
> > ns) because the value for pwmc0 was miscalculated.
> > 
> > Also simplify using that fls(0) is 0.
> > 
> > Fixes: 721b595744f1 ("pwm: visconti: Add Toshiba Visconti SoC PWM support")
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/pwm-visconti.c | 17 +++++++----------
> >  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> Applied, thanks.

I see you applied to your for-next branch. I would have considered to
get this patch in before v5.13 given that the driver was introduced for
5.13-rc1.

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