Message ID | 1412197710-17731-1-git-send-email-ryang@hach.com |
---|---|
State | Deferred |
Headers | show |
On Wed, Oct 01, 2014 at 03:08:30PM -0600, robert yang wrote: > From: ryang <ryang@hach.com> > > Signed-off-by: ryang <ryang@hach.com> > --- > 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
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(); + } 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); }