* Re: [PATCH] pwm: atmel: Fix disabling of PWM channels
2016-05-13 11:09 [PATCH] pwm: atmel: Fix disabling of PWM channels Guillermo Rodriguez
@ 2016-06-09 16:36 ` Guillermo Rodriguez Garcia
2016-06-10 8:29 ` Alexandre Belloni
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Guillermo Rodriguez Garcia @ 2016-06-09 16:36 UTC (permalink / raw)
To: Thierry Reding, linux-kernel, linux-pwm, Nicolas Ferre
Cc: Guillermo Rodriguez
Hello,
Any comments on this patch?
Guillermo
2016-05-13 13:09 GMT+02:00 Guillermo Rodriguez <guille.rodriguez@gmail.com>:
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
>
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
>
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
> drivers/pwm/pwm-atmel.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> mutex_unlock(&atmel_pwm->isr_lock);
> atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>
> + /*
> + * Wait for the PWM channel disable operation to be effective before
> + * stopping the clock.
> + */
> + timeout = jiffies + 2 * HZ;
> + while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> + time_before(jiffies, timeout)) {
> + usleep_range(10, 100);
> + }
> +
> clk_disable(atmel_pwm->clk);
> }
>
> --
> 1.7.9.5
>
--
Guillermo Rodriguez Garcia
guille.rodriguez@gmail.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: pwm: atmel: Fix disabling of PWM channels
2016-05-13 11:09 [PATCH] pwm: atmel: Fix disabling of PWM channels Guillermo Rodriguez
2016-06-09 16:36 ` Guillermo Rodriguez Garcia
@ 2016-06-10 8:29 ` Alexandre Belloni
2016-06-13 10:31 ` Guillermo Rodriguez Garcia
2016-06-13 9:50 ` Alexandre Belloni
2016-07-11 10:16 ` [PATCH] " Thierry Reding
3 siblings, 1 reply; 6+ messages in thread
From: Alexandre Belloni @ 2016-06-10 8:29 UTC (permalink / raw)
To: Guillermo Rodriguez
Cc: Thierry Reding, linux-kernel, linux-pwm, Nicolas Ferre
Hi,
On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
>
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
>
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
> drivers/pwm/pwm-atmel.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> mutex_unlock(&atmel_pwm->isr_lock);
> atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>
> + /*
> + * Wait for the PWM channel disable operation to be effective before
> + * stopping the clock.
> + */
> + timeout = jiffies + 2 * HZ;
> + while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> + time_before(jiffies, timeout)) {
> + usleep_range(10, 100);
> + }
> +
While this seems good, can you tell on which SoC you observed that? I'd
like to understand whether this is only on v1 or v2 or both.
Thanks!
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: pwm: atmel: Fix disabling of PWM channels
2016-06-10 8:29 ` Alexandre Belloni
@ 2016-06-13 10:31 ` Guillermo Rodriguez Garcia
0 siblings, 0 replies; 6+ messages in thread
From: Guillermo Rodriguez Garcia @ 2016-06-13 10:31 UTC (permalink / raw)
To: linux-kernel, linux-pwm
(resending to MLs for the record -- previous message bounced)
El viernes, 10 de junio de 2016, Alexandre Belloni
<alexandre.belloni@free-electrons.com> escribió:
>
> Hi,
>
> On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> > When disabling a PWM channel, the PWM clock was being stopped
> > immediately after writing to PWM_DIS. As a result, the disabling
> > of the PWM channel did not complete properly, and the PWM output
> > might be left at the wrong level.
> >
> > Fix this by waiting for the channel to be effectively disabled
> > (by checking the PWM_SR register) before disabling the clock.
> >
> > Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> > ---
> > drivers/pwm/pwm-atmel.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> > index 0e4bd4e..a714434 100644
> > --- a/drivers/pwm/pwm-atmel.c
> > +++ b/drivers/pwm/pwm-atmel.c
> > @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> > mutex_unlock(&atmel_pwm->isr_lock);
> > atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> >
> > + /*
> > + * Wait for the PWM channel disable operation to be effective before
> > + * stopping the clock.
> > + */
> > + timeout = jiffies + 2 * HZ;
> > + while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> > + time_before(jiffies, timeout)) {
> > + usleep_range(10, 100);
> > + }
> > +
>
> While this seems good, can you tell on which SoC you observed that? I'd
> like to understand whether this is only on v1 or v2 or both.
I observed it on a sama5d3, however this article from atmel's KB would
seem to suggest that it is not specific to this SoC:
http://atmel.force.com/support/pkb_mobile#/articles/en_US/FAQ/Why-PWM-Channel-Polarity-Inversion
Guillermo
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: pwm: atmel: Fix disabling of PWM channels
2016-05-13 11:09 [PATCH] pwm: atmel: Fix disabling of PWM channels Guillermo Rodriguez
2016-06-09 16:36 ` Guillermo Rodriguez Garcia
2016-06-10 8:29 ` Alexandre Belloni
@ 2016-06-13 9:50 ` Alexandre Belloni
2016-07-11 10:16 ` [PATCH] " Thierry Reding
3 siblings, 0 replies; 6+ messages in thread
From: Alexandre Belloni @ 2016-06-13 9:50 UTC (permalink / raw)
To: Guillermo Rodriguez
Cc: Thierry Reding, linux-kernel, linux-pwm, Nicolas Ferre
On 13/05/2016 at 13:09:37 +0200, Guillermo Rodriguez wrote :
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
>
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
>
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
> ---
> drivers/pwm/pwm-atmel.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 0e4bd4e..a714434 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -271,6 +271,16 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> mutex_unlock(&atmel_pwm->isr_lock);
> atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
>
> + /*
> + * Wait for the PWM channel disable operation to be effective before
> + * stopping the clock.
> + */
> + timeout = jiffies + 2 * HZ;
> + while ((atmel_pwm_readl(atmel_pwm, PWM_SR)& (1 << pwm->hwpwm)) &&
> + time_before(jiffies, timeout)) {
> + usleep_range(10, 100);
> + }
> +
> clk_disable(atmel_pwm->clk);
> }
>
--
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] pwm: atmel: Fix disabling of PWM channels
2016-05-13 11:09 [PATCH] pwm: atmel: Fix disabling of PWM channels Guillermo Rodriguez
` (2 preceding siblings ...)
2016-06-13 9:50 ` Alexandre Belloni
@ 2016-07-11 10:16 ` Thierry Reding
3 siblings, 0 replies; 6+ messages in thread
From: Thierry Reding @ 2016-07-11 10:16 UTC (permalink / raw)
To: Guillermo Rodriguez; +Cc: linux-kernel, linux-pwm, Nicolas Ferre
[-- Attachment #1: Type: text/plain, Size: 635 bytes --]
On Fri, May 13, 2016 at 01:09:37PM +0200, Guillermo Rodriguez wrote:
> When disabling a PWM channel, the PWM clock was being stopped
> immediately after writing to PWM_DIS. As a result, the disabling
> of the PWM channel did not complete properly, and the PWM output
> might be left at the wrong level.
>
> Fix this by waiting for the channel to be effectively disabled
> (by checking the PWM_SR register) before disabling the clock.
>
> Signed-off-by: Guillermo Rodriguez <guille.rodriguez@gmail.com>
> ---
> drivers/pwm/pwm-atmel.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
Applied, thanks.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread