[PATCH] pwm: lpc32xx - Fix the PWM polarity

[PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/pwm/pwm-lpc32xx.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index adb87f0..a2704b8 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	c = 256 * duty_ns;
 	do_div(c, period_ns);
-	duty_cycles = c;
+	if (c > 255)
+		c = 255;
+	if (c < 1)
+		c = 1;
+	duty_cycles = 256 - c;
 
 	writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
 		lpc32xx->base + (pwm->hwpwm << 2));
-- 
1.7.0.4

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Thierry Reding]

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

Cc'ing Roland and Alexandre. What do you guys think?

On Mon, Nov 05, 2012 at 05:48:45PM +0100, Alban Bedel wrote:
> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> ---
>  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> index adb87f0..a2704b8 100644
> --- a/drivers/pwm/pwm-lpc32xx.c
> +++ b/drivers/pwm/pwm-lpc32xx.c
> @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	c = 256 * duty_ns;
>  	do_div(c, period_ns);
> -	duty_cycles = c;
> +	if (c > 255)
> +		c = 255;
> +	if (c < 1)
> +		c = 1;
> +	duty_cycles = 256 - c;
>  
>  	writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
>  		lpc32xx->base + (pwm->hwpwm << 2));

Shouldn't duty_cycles rather be 255 - c, such that it can still be 0?

Thierry

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

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Roland Stigge]

On 05/11/12 22:03, Thierry Reding wrote:
> Cc'ing Roland and Alexandre. What do you guys think?
> 
> On Mon, Nov 05, 2012 at 05:48:45PM +0100, Alban Bedel wrote:
>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> --- 
>> drivers/pwm/pwm-lpc32xx.c |    6 +++++- 1 files changed, 5
>> insertions(+), 1 deletions(-)
>> 
>> diff --git a/drivers/pwm/pwm-lpc32xx.c
>> b/drivers/pwm/pwm-lpc32xx.c index adb87f0..a2704b8 100644 ---
>> a/drivers/pwm/pwm-lpc32xx.c +++ b/drivers/pwm/pwm-lpc32xx.c @@
>> -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip
>> *chip, struct pwm_device *pwm,
>> 
>> c = 256 * duty_ns; do_div(c, period_ns); -	duty_cycles = c; +	if
>> (c > 255) +		c = 255; +	if (c < 1) +		c = 1; +	duty_cycles = 256
>> - c;
>> 
>> writel(PWM_ENABLE | PWM_RELOADV(period_cycles) |
>> PWM_DUTY(duty_cycles), lpc32xx->base + (pwm->hwpwm << 2));
> 
> Shouldn't duty_cycles rather be 255 - c, such that it can still be
> 0?
> 
> Thierry

According to the Manual: [Low]/[High] = [PWM_DUTY] / [256-PWM_DUTY],
i.e., the PWM polarity inversion looks good.

However, as Thierry pointed out, the valid range 0..255 should be
maintained differently, maybe:

if (c > 255)
	c = 255;
duty_cycles = 255 - c;

?

Thanks,

Roland

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

On Tue, 6 Nov 2012 07:47:22 -0200
Alexandre Pereira da Silva <aletes.xgr@gmail.com> wrote:

> Can you test the 0 and 255 values on actual hardware and see the effective
> values?

0   ->   0%
1   ->  99%
128 ->  50%
255 ->   1%

So yes 0 mean 256.

> It may be handled as the RELOADV where 0 really means 256. If so, you can
> use the same logic I used originally on the frequency division.

I'll look at this and submit a new patch.

Alban

[PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/pwm/pwm-lpc32xx.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index adb87f0..0dc278d 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	c = 256 * duty_ns;
 	do_div(c, period_ns);
-	duty_cycles = c;
+	if (c == 0)
+		c = 256;
+	if (c > 255)
+		c = 255;
+	duty_cycles = 256 - c;
 
 	writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
 		lpc32xx->base + (pwm->hwpwm << 2));
-- 
1.7.0.4

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alexandre Pereira da Silva]

On Wed, Nov 7, 2012 at 1:25 PM, Alban Bedel
<alban.bedel@avionic-design.de> wrote:
> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>

Acked-by: Alexandre Pereira da Silva <aletes.xgr@gmail.com>

> ---
>  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> index adb87f0..0dc278d 100644
> --- a/drivers/pwm/pwm-lpc32xx.c
> +++ b/drivers/pwm/pwm-lpc32xx.c
> @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>
>         c = 256 * duty_ns;
>         do_div(c, period_ns);
> -       duty_cycles = c;
> +       if (c == 0)
> +               c = 256;
> +       if (c > 255)
> +               c = 255;
> +       duty_cycles = 256 - c;
>
>         writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
>                 lpc32xx->base + (pwm->hwpwm << 2));
> --
> 1.7.0.4
>

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Roland Stigge]

On 07/11/12 16:25, Alban Bedel wrote:
> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> ---
>  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> index adb87f0..0dc278d 100644
> --- a/drivers/pwm/pwm-lpc32xx.c
> +++ b/drivers/pwm/pwm-lpc32xx.c
> @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	c = 256 * duty_ns;
>  	do_div(c, period_ns);
> -	duty_cycles = c;
> +	if (c == 0)
> +		c = 256;
> +	if (c > 255)
> +		c = 255;
> +	duty_cycles = 256 - c;

Except for the range check (for the original c > 255), this results in:

	duty_cycles = 256 - c

except for (c == 0) where

	duty_cycles = 1

which actually is

	duty_cycles = (256 - c) - 255

(think with the original c)

i.e. nearly a polarity inversion in the case of (c == 0).

Why is the case (c == 0) so special here? Maybe you can document this,
if it is really intended?

>  
>  	writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
>  		lpc32xx->base + (pwm->hwpwm << 2));

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

On Thu, 08 Nov 2012 10:51:35 +0100
Roland Stigge <stigge@antcom.de> wrote:

> On 07/11/12 16:25, Alban Bedel wrote:
> > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> > ---
> >  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> > index adb87f0..0dc278d 100644
> > --- a/drivers/pwm/pwm-lpc32xx.c
> > +++ b/drivers/pwm/pwm-lpc32xx.c
> > @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >  
> >  	c = 256 * duty_ns;
> >  	do_div(c, period_ns);
> > -	duty_cycles = c;
> > +	if (c == 0)
> > +		c = 256;
> > +	if (c > 255)
> > +		c = 255;
> > +	duty_cycles = 256 - c;
> 
> Except for the range check (for the original c > 255), this results in:
> 
> 	duty_cycles = 256 - c
> 
> except for (c == 0) where
> 
> 	duty_cycles = 1

No it lead to duty_cycles = 0

> which actually is
> 
> 	duty_cycles = (256 - c) - 255
> 
> (think with the original c)
> 
> i.e. nearly a polarity inversion in the case of (c == 0).
> 
> Why is the case (c == 0) so special here? Maybe you can document this,
> if it is really intended?

It is intended, the formular for duty value in the register is:

duty = (256 - 256*duty_ns/period_ns) % 256

But the code avoid the modulo by clamping '256*duty_ns/period_ns' to 1-256.

Perhaps something like:

if (c > 255)
	c = 255;
duty_cycles = (256 - c) % 256;

would be easier to understand.

Alban

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Roland Stigge]

On 08/11/12 11:33, Alban Bedel wrote:
> On Thu, 08 Nov 2012 10:51:35 +0100
> Roland Stigge <stigge@antcom.de> wrote:
> 
>> On 07/11/12 16:25, Alban Bedel wrote:
>>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
>>> ---
>>>  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
>>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
>>> index adb87f0..0dc278d 100644
>>> --- a/drivers/pwm/pwm-lpc32xx.c
>>> +++ b/drivers/pwm/pwm-lpc32xx.c
>>> @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>>>  
>>>  	c = 256 * duty_ns;
>>>  	do_div(c, period_ns);
>>> -	duty_cycles = c;
>>> +	if (c == 0)
>>> +		c = 256;
>>> +	if (c > 255)
>>> +		c = 255;
>>> +	duty_cycles = 256 - c;
>>
>> Except for the range check (for the original c > 255), this results in:
>>
>> 	duty_cycles = 256 - c
>>
>> except for (c == 0) where
>>
>> 	duty_cycles = 1
> 
> No it lead to duty_cycles = 0

Let's do it step by step with the above code:

c == 0

>>> +	if (c == 0)
>>> +		c = 256;

c == 256

>>> +	if (c > 255)
>>> +		c = 255;

c == 255

>>> +	duty_cycles = 256 - c;

c == 1

See?

> 
>> which actually is
>>
>> 	duty_cycles = (256 - c) - 255
>>
>> (think with the original c)
>>
>> i.e. nearly a polarity inversion in the case of (c == 0).
>>
>> Why is the case (c == 0) so special here? Maybe you can document this,
>> if it is really intended?
> 
> It is intended, the formular for duty value in the register is:
> 
> duty = (256 - 256*duty_ns/period_ns) % 256

Where does this modulo defined? In the Manual, there is sth. like this
defined for RELOADV (tables 606+607), but not for DUTY.

Maybe I missed sth. in the manual. Link or hint appreciated!

Thanks,

Roland

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

On Thu, 08 Nov 2012 11:44:48 +0100
Roland Stigge <stigge@antcom.de> wrote:

> On 08/11/12 11:33, Alban Bedel wrote:
> > On Thu, 08 Nov 2012 10:51:35 +0100
> > Roland Stigge <stigge@antcom.de> wrote:
> > 
> >> On 07/11/12 16:25, Alban Bedel wrote:
> >>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> >>> ---
> >>>  drivers/pwm/pwm-lpc32xx.c |    6 +++++-
> >>>  1 files changed, 5 insertions(+), 1 deletions(-)
> >>>
> >>> diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
> >>> index adb87f0..0dc278d 100644
> >>> --- a/drivers/pwm/pwm-lpc32xx.c
> >>> +++ b/drivers/pwm/pwm-lpc32xx.c
> >>> @@ -51,7 +51,11 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> >>>  
> >>>  	c = 256 * duty_ns;
> >>>  	do_div(c, period_ns);
> >>> -	duty_cycles = c;
> >>> +	if (c == 0)
> >>> +		c = 256;
> >>> +	if (c > 255)
> >>> +		c = 255;
> >>> +	duty_cycles = 256 - c;
> >>
> >> Except for the range check (for the original c > 255), this results in:
> >>
> >> 	duty_cycles = 256 - c
> >>
> >> except for (c == 0) where
> >>
> >> 	duty_cycles = 1
> > 
> > No it lead to duty_cycles = 0
> 
> Let's do it step by step with the above code:
> 
> c == 0
> 
> >>> +	if (c == 0)
> >>> +		c = 256;
> 
> c == 256
> 
> >>> +	if (c > 255)
> >>> +		c = 255;
> 
> c == 255
> 
> >>> +	duty_cycles = 256 - c;
> 
> c == 1
> 
> See?

Right, my bad.
 
> > 
> >> which actually is
> >>
> >> 	duty_cycles = (256 - c) - 255
> >>
> >> (think with the original c)
> >>
> >> i.e. nearly a polarity inversion in the case of (c == 0).
> >>
> >> Why is the case (c == 0) so special here? Maybe you can document this,
> >> if it is really intended?
> > 
> > It is intended, the formular for duty value in the register is:
> > 
> > duty = (256 - 256*duty_ns/period_ns) % 256
> 
> Where does this modulo defined? In the Manual, there is sth. like this
> defined for RELOADV (tables 606+607), but not for DUTY.
> 
> Maybe I missed sth. in the manual. Link or hint appreciated!

The manual doesn't mention this explicitly but you can see that without
the modulo when duty_ns==0 DUTY would be 256, but the register is only
8 bits wide (ie. modulo 256). I made a few test and looked at the PWM
output on a scope they confirm this:

 DUTY     HIGH LEVEL
  1         99.9%
  25        90.0%
  128       50.0%
  220       10.0%
  255        0.1%
  0          0.0%

I'll resubmit the patch with the clamping in the correct order.

Alban

[PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alban Bedel]

The duty cycles value goes from 1 (99% HIGH) to 256 (0% HIGH) but it
is stored modulo 256 in the register as it is only 8 bits wide.

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 drivers/pwm/pwm-lpc32xx.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/pwm/pwm-lpc32xx.c b/drivers/pwm/pwm-lpc32xx.c
index adb87f0..2590f8d 100644
--- a/drivers/pwm/pwm-lpc32xx.c
+++ b/drivers/pwm/pwm-lpc32xx.c
@@ -51,7 +51,9 @@ static int lpc32xx_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	c = 256 * duty_ns;
 	do_div(c, period_ns);
-	duty_cycles = c;
+	if (c > 255)
+		c = 255;
+	duty_cycles = 256 - c;
 
 	writel(PWM_ENABLE | PWM_RELOADV(period_cycles) | PWM_DUTY(duty_cycles),
 		lpc32xx->base + (pwm->hwpwm << 2));
-- 
1.7.0.4

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Roland Stigge]

On 08/11/12 12:23, Alban Bedel wrote:
>>> It is intended, the formular for duty value in the register is:
>>>
>>> duty = (256 - 256*duty_ns/period_ns) % 256
>>
>> Where does this modulo defined? In the Manual, there is sth. like this
>> defined for RELOADV (tables 606+607), but not for DUTY.
>>
>> Maybe I missed sth. in the manual. Link or hint appreciated!
> 
> The manual doesn't mention this explicitly but you can see that without
> the modulo when duty_ns==0 DUTY would be 256, but the register is only
> 8 bits wide (ie. modulo 256). I made a few test and looked at the PWM
> output on a scope they confirm this:
> 
>  DUTY     HIGH LEVEL
>   1         99.9%
>   25        90.0%
>   128       50.0%
>   220       10.0%
>   255        0.1%
>   0          0.0%
> 
> I'll resubmit the patch with the clamping in the correct order.

Thanks for measuring. With this, your resubmitted patch make much more
sense now.

Roland

Re: [PATCH] pwm: lpc32xx - Fix the PWM polarity

[Alexandre Pereira da Silva]

On Thu, Nov 8, 2012 at 11:12 AM, Roland Stigge <stigge@antcom.de> wrote:
> On 08/11/12 12:23, Alban Bedel wrote:
>>>> It is intended, the formular for duty value in the register is:
>>>>
>>>> duty = (256 - 256*duty_ns/period_ns) % 256
>>>
>>> Where does this modulo defined? In the Manual, there is sth. like this
>>> defined for RELOADV (tables 606+607), but not for DUTY.
>>>
>>> Maybe I missed sth. in the manual. Link or hint appreciated!
>>
>> The manual doesn't mention this explicitly but you can see that without
>> the modulo when duty_ns==0 DUTY would be 256, but the register is only
>> 8 bits wide (ie. modulo 256). I made a few test and looked at the PWM
>> output on a scope they confirm this:
>>
>>  DUTY     HIGH LEVEL
>>   1         99.9%
>>   25        90.0%
>>   128       50.0%
>>   220       10.0%
>>   255        0.1%
>>   0          0.0%
>>
>> I'll resubmit the patch with the clamping in the correct order.
>
> Thanks for measuring. With this, your resubmitted patch make much more
> sense now.
>
> Roland

Alban,

I think you should include this measurements on the source code as
comments, for future reference.

Thanks.