[PATCH 0/1] fix for pwm-mxs period divider computation

[PATCH 0/1] fix for pwm-mxs period divider computation

[Gaetan Hug]

Hello Thierry,

   I found a bug in the pwm-mxs.c driver for the i.MX28 platform.

   The driver computes from the desired period which clock divider it
should be using. This computation assumes that the link between the
register value and the actual divider value is raising 2 to the power of
the registry value.

    div = 1 << regvalue

   This is true only for the first 5 values out of 8. Next values are
64, 256 and, 1024.
   This affects only the user only if he requests a period > 0.04369s.

Gaetan Hug (1):
  fixing pwm-mxs period divider computation

 drivers/pwm/pwm-mxs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--
1.7.10.4

[PATCH 1/1] fixing pwm-mxs period divider computation

[Gaetan Hug]

---
 drivers/pwm/pwm-mxs.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index f75ecb0..bbb2f8a 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -52,15 +52,18 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	unsigned long rate;
 	unsigned long long c;
 
+    unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 64, 256, 1024};
+
 	rate = clk_get_rate(mxs->clk);
+
 	while (1) {
-		c = rate / (1 << div);
+		c = rate / cdiv[div];
 		c = c * period_ns;
 		do_div(c, 1000000000);
 		if (c < PERIOD_PERIOD_MAX)
 			break;
 		div++;
-		if (div > PERIOD_CDIV_MAX)
+		if (div >= PERIOD_CDIV_MAX)
 			return -EINVAL;
 	}
 
-- 
1.7.10.4

Re: [PATCH 0/1] fix for pwm-mxs period divider computation

[Thierry Reding]

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

On Wed, Feb 18, 2015 at 09:18:55AM +0100, Gaetan Hug wrote:
> Hello Thierry,
> 
>    I found a bug in the pwm-mxs.c driver for the i.MX28 platform.
> 
>    The driver computes from the desired period which clock divider it
> should be using. This computation assumes that the link between the
> register value and the actual divider value is raising 2 to the power of
> the registry value.
> 
>     div = 1 << regvalue
> 
>    This is true only for the first 5 values out of 8. Next values are
> 64, 256 and, 1024.
>    This affects only the user only if he requests a period > 0.04369s.

Can you put this description into the commit message of the proper
patch, please?

Also, the subject should be in the following format:

	pwm: mxs: Fix period divider computation

Thanks,
Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/1] fixing pwm-mxs period divider computation

[Thierry Reding]

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

On Wed, Feb 18, 2015 at 09:18:56AM +0100, Gaetan Hug wrote:
> ---
>  drivers/pwm/pwm-mxs.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Adding Shawn and Fabio, hence quoting the full patch.

> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index f75ecb0..bbb2f8a 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -52,15 +52,18 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	unsigned long rate;
>  	unsigned long long c;
>  
> +    unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 64, 256, 1024};

This could prbably be static const and could be in file scope rather
than in this function.

Thierry

> +
>  	rate = clk_get_rate(mxs->clk);
> +
>  	while (1) {
> -		c = rate / (1 << div);
> +		c = rate / cdiv[div];
>  		c = c * period_ns;
>  		do_div(c, 1000000000);
>  		if (c < PERIOD_PERIOD_MAX)
>  			break;
>  		div++;
> -		if (div > PERIOD_CDIV_MAX)
> +		if (div >= PERIOD_CDIV_MAX)
>  			return -EINVAL;
>  	}
>  
> -- 
> 1.7.10.4
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]