Signed-off-by: Alban Bedel <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]> ---
>> 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
On Tue, 6 Nov 2012 07:47:22 -0200
Alexandre Pereira da Silva <[email protected]> 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
Signed-off-by: Alban Bedel <[email protected]>
---
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
On Wed, Nov 7, 2012 at 1:25 PM, Alban Bedel
<[email protected]> wrote:
> Signed-off-by: Alban Bedel <[email protected]>
Acked-by: Alexandre Pereira da Silva <[email protected]>
> ---
> 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
>
On 07/11/12 16:25, Alban Bedel wrote:
> Signed-off-by: Alban Bedel <[email protected]>
> ---
> 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));
On Thu, 08 Nov 2012 10:51:35 +0100
Roland Stigge <[email protected]> wrote:
> On 07/11/12 16:25, Alban Bedel wrote:
> > Signed-off-by: Alban Bedel <[email protected]>
> > ---
> > 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
On 08/11/12 11:33, Alban Bedel wrote:
> On Thu, 08 Nov 2012 10:51:35 +0100
> Roland Stigge <[email protected]> wrote:
>
>> On 07/11/12 16:25, Alban Bedel wrote:
>>> Signed-off-by: Alban Bedel <[email protected]>
>>> ---
>>> 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
On Thu, 08 Nov 2012 11:44:48 +0100
Roland Stigge <[email protected]> wrote:
> On 08/11/12 11:33, Alban Bedel wrote:
> > On Thu, 08 Nov 2012 10:51:35 +0100
> > Roland Stigge <[email protected]> wrote:
> >
> >> On 07/11/12 16:25, Alban Bedel wrote:
> >>> Signed-off-by: Alban Bedel <[email protected]>
> >>> ---
> >>> 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
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 <[email protected]>
---
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
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
On Thu, Nov 8, 2012 at 11:12 AM, Roland Stigge <[email protected]> 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.