Re: [PATCH 1/1] Atmel-PWM: Fixed the PWM channel enable/disable issue which will cause the PWM output stop working after reenable. THe PWM channel status register is read to ensure the PWM channel is really enabled or disabled.

[Thierry Reding]

On Wed, Oct 01, 2014 at 03:08:30PM -0600, robert yang wrote:
> From: ryang <ryang@xxxxxxxx>
> 
> Signed-off-by: ryang <ryang@xxxxxxxx>
> ---
>  drivers/pwm/pwm-atmel.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

Seems like I never got around to review this. Sorry for that.

First, you need to change the commit subject and message. The first line
should be a short description of what the patch does, something like
this:

	pwm: atmel: Ensure channel is enabled or disabled

Followed by a more verbose description of the change. What I don't
understand here is under what circumstances it can happen that the
channel isn't getting enabled. Is this because some error happened and
therefore the register needs to be written again? Or is it just that we
need to wait for some time for the PWM to become enabled and we use the
PWM enable register for that?

> diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
> index 6e700a5..bab933a 100644
> --- a/drivers/pwm/pwm-atmel.c
> +++ b/drivers/pwm/pwm-atmel.c
> @@ -237,7 +237,10 @@ static int atmel_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	}
>  
>  	atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> -
> +	while (!(atmel_pwm_readl(atmel_pwm, PWM_SR) & (1 << pwm->hwpwm))) {
> +		atmel_pwm_writel(atmel_pwm, PWM_ENA, 1 << pwm->hwpwm);
> +		cpu_relax();
> +	}

So this indicates that the PWM_ENA register is used to enable multiple
PWM channels. Given that there is a PWM_DIS register too, I suspect that
this is a write-only register, so writing zeroes to an enable bit
doesn't actually cause the corresponding PWM to be disabled. That would
have been a nice explanation for why the original code could break.

>  	return 0;
>  }
>  
> @@ -246,7 +249,10 @@ static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
>  
>  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> -
> +	while (atmel_pwm_readl(atmel_pwm, PWM_SR) & (1 << pwm->hwpwm)) {
> +		atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
> +		cpu_relax();
> +	}
>  	clk_disable(atmel_pwm->clk);
>  }
>  

Same here. I don't really understand why this change is necessary. Maybe
you can provide a more detailed description of the error you're seeing.
Also adding Nicolas to Cc, perhaps he can help find out what the issue
is with the current code.

Thierry

Attachment: pgpZOUTKtCfc5.pgp
Description: PGP signature