[PATCH] pwm: mxs: fix period divider computation

[PATCH] pwm: mxs: fix period divider computation

[Gaetan Hug]

The driver computes which clock divider it sould be using from the
requested period. 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 - instead of 32, 64, 128.
This affects only the users requesting a period > 0.04369s.

Replace the computation with a look-up table.
---
 drivers/pwm/pwm-mxs.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index f75ecb0..c65e183 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -35,6 +35,8 @@
 #define  PERIOD_CDIV(div)	(((div) & 0x7) << 20)
 #define  PERIOD_CDIV_MAX	8
 
+static unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 64, 256, 1024};
+
 struct mxs_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
@@ -54,13 +56,13 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	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

[PATCH] pwm: mxs: fix period divider computation

[Gaetan Hug]

The driver computes which clock divider it sould be using from the
requested period. 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 - instead of 32, 64, 128.
This affects only the users requesting a period > 0.04369s.

Replace the computation with a look-up table.
---
 drivers/pwm/pwm-mxs.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
index f75ecb0..c65e183 100644
--- a/drivers/pwm/pwm-mxs.c
+++ b/drivers/pwm/pwm-mxs.c
@@ -35,6 +35,8 @@
 #define  PERIOD_CDIV(div)	(((div) & 0x7) << 20)
 #define  PERIOD_CDIV_MAX	8
 
+static unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 64, 256, 1024};
+
 struct mxs_pwm_chip {
 	struct pwm_chip chip;
 	struct clk *clk;
@@ -54,13 +56,13 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	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] pwm: mxs: fix period divider computation

[Shawn Guo]

On Wed, Feb 18, 2015 at 02:06:34PM +0100, Gaetan Hug wrote:
> The driver computes which clock divider it sould be using from the
> requested period. 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 - instead of 32, 64, 128.

Just checked i.MX28 Reference Manual, and yes, this is the case.

> This affects only the users requesting a period > 0.04369s.
> 
> Replace the computation with a look-up table.

Your SoB is missing here.  Otherwise,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
>  drivers/pwm/pwm-mxs.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c
> index f75ecb0..c65e183 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -35,6 +35,8 @@
>  #define  PERIOD_CDIV(div)	(((div) & 0x7) << 20)
>  #define  PERIOD_CDIV_MAX	8
>  
> +static unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 64, 256, 1024};
> +
>  struct mxs_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> @@ -54,13 +56,13 @@ static int mxs_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	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] pwm: mxs: fix period divider computation

[Zha, Qipeng]

Hi 

I got pwm subsystem git tree info as below from v3.19-rc6 MAINTAINER
And I clone it, found the version is v3.12
Is the tree current maintain tree ? or there is a new one, thanks.

git git://git.kernel.org/pub/scm/linux/kernel/git/thierry.reding/linux-pwm.git
 

Best wishes
Qipeng

-----Original Message-----
From: linux-pwm-owner@vger.kernel.org [mailto:linux-pwm-owner@vger.kernel.org] On Behalf Of Shawn Guo
Sent: Monday, March 02, 2015 8:32 PM
To: Gaetan Hug
Cc: thierry.reding@gmail.com; linux-pwm@vger.kernel.org; linux-kernel@vger.kernel.org; fabio.estevam@freescale.com
Subject: Re: [PATCH] pwm: mxs: fix period divider computation

On Wed, Feb 18, 2015 at 02:06:34PM +0100, Gaetan Hug wrote:
> The driver computes which clock divider it sould be using from the 
> requested period. 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 - instead of 32, 64, 128.

Just checked i.MX28 Reference Manual, and yes, this is the case.

> This affects only the users requesting a period > 0.04369s.
> 
> Replace the computation with a look-up table.

Your SoB is missing here.  Otherwise,

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> ---
>  drivers/pwm/pwm-mxs.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mxs.c b/drivers/pwm/pwm-mxs.c index 
> f75ecb0..c65e183 100644
> --- a/drivers/pwm/pwm-mxs.c
> +++ b/drivers/pwm/pwm-mxs.c
> @@ -35,6 +35,8 @@
>  #define  PERIOD_CDIV(div)	(((div) & 0x7) << 20)
>  #define  PERIOD_CDIV_MAX	8
>  
> +static unsigned const int cdiv[PERIOD_CDIV_MAX] = {1, 2, 4, 8, 16, 
> +64, 256, 1024};
> +
>  struct mxs_pwm_chip {
>  	struct pwm_chip chip;
>  	struct clk *clk;
> @@ -54,13 +56,13 @@ static int mxs_pwm_config(struct pwm_chip *chip, 
> struct pwm_device *pwm,
>  
>  	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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pwm: mxs: fix period divider computation

[Thierry Reding]

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

On Mon, Mar 02, 2015 at 08:32:09PM +0800, Shawn Guo wrote:
> On Wed, Feb 18, 2015 at 02:06:34PM +0100, Gaetan Hug wrote:
> > The driver computes which clock divider it sould be using from the
> > requested period. 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 - instead of 32, 64, 128.
> 
> Just checked i.MX28 Reference Manual, and yes, this is the case.
> 
> > This affects only the users requesting a period > 0.04369s.
> > 
> > Replace the computation with a look-up table.
> 
> Your SoB is missing here.  Otherwise,
> 
> Acked-by: Shawn Guo <shawn.guo@linaro.org>

Gaetan,

can you resend with your Signed-off-by added? Or at least provide it
here? I can't really apply this without one.

Thierry

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