| Message ID | 20221003015546.202308-3-doug@schmorgal.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: pxa: Fixes for enable/disable transitions | expand |
On Sun, Oct 02, 2022 at 06:55:43PM -0700, Doug Brown wrote: > When disabling PWM, the duty cycle needs to be set to 0. This prevents > the previous duty cycle from showing up momentarily when the clock is > re-enabled next time, and also prevents the output pin from being stuck > high when the clock is disabled. > > Because the clock has to be running in order to configure the duty > cycle, unconditionally enable it early in pxa_pwm_apply and account for > the correct enable count at the end. > > Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > Signed-off-by: Doug Brown <doug@schmorgal.com> > --- > drivers/pwm/pwm-pxa.c | 25 ++++++++++++++++--------- > 1 file changed, 16 insertions(+), 9 deletions(-) > > diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c > index 0ac052652c62..9ee9b41d62b8 100644 > --- a/drivers/pwm/pwm-pxa.c > +++ b/drivers/pwm/pwm-pxa.c > @@ -105,24 +105,31 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > const struct pwm_state *state) > { > struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip); > + u64 duty_cycle; > int err; > > if (state->polarity != PWM_POLARITY_NORMAL) > return -EINVAL; > > - if (!state->enabled) { > - if (pwm->state.enabled) > - clk_disable_unprepare(pc->clk); > + err = clk_prepare_enable(pc->clk); > + if (err) > + return err; > > - return 0; > - } > + duty_cycle = state->enabled ? state->duty_cycle : 0; > > - err = pxa_pwm_config(chip, pwm, state->duty_cycle, state->period); > - if (err) > + err = pxa_pwm_config(chip, pwm, duty_cycle, state->period); > + if (err) { > + clk_disable_unprepare(pc->clk); > return err; > + } > + > + if (state->enabled && !pwm->state.enabled) > + return 0; > + > + clk_disable_unprepare(pc->clk); > > - if (!pwm->state.enabled) > - return clk_prepare_enable(pc->clk); > + if (!state->enabled && pwm->state.enabled) > + clk_disable_unprepare(pc->clk); Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Best regards Uwe
Hi Uwe, On 10/19/2022 12:34 AM, Uwe Kleine-König wrote: > On Sun, Oct 02, 2022 at 06:55:43PM -0700, Doug Brown wrote: >> When disabling PWM, the duty cycle needs to be set to 0. This prevents >> the previous duty cycle from showing up momentarily when the clock is >> re-enabled next time, and also prevents the output pin from being stuck >> high when the clock is disabled. > > Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> It appears I messed up when writing the commit message for this patch, because I did some more testing today with raw register writes and the output pin definitely doesn't get stuck high when I disable the clock -- no matter where we are in the PWM cycle. Either way, it still makes sense to set the duty cycle to 0 first. Just wanted you to know that I'm not changing the patch at all -- just the commit message -- on v3, so I'm not carrying over your Reviewed-by tag on v3 of this patch. Sorry about that! Doug
diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c index 0ac052652c62..9ee9b41d62b8 100644 --- a/drivers/pwm/pwm-pxa.c +++ b/drivers/pwm/pwm-pxa.c @@ -105,24 +105,31 @@ static int pxa_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { struct pxa_pwm_chip *pc = to_pxa_pwm_chip(chip); + u64 duty_cycle; int err; if (state->polarity != PWM_POLARITY_NORMAL) return -EINVAL; - if (!state->enabled) { - if (pwm->state.enabled) - clk_disable_unprepare(pc->clk); + err = clk_prepare_enable(pc->clk); + if (err) + return err; - return 0; - } + duty_cycle = state->enabled ? state->duty_cycle : 0; - err = pxa_pwm_config(chip, pwm, state->duty_cycle, state->period); - if (err) + err = pxa_pwm_config(chip, pwm, duty_cycle, state->period); + if (err) { + clk_disable_unprepare(pc->clk); return err; + } + + if (state->enabled && !pwm->state.enabled) + return 0; + + clk_disable_unprepare(pc->clk); - if (!pwm->state.enabled) - return clk_prepare_enable(pc->clk); + if (!state->enabled && pwm->state.enabled) + clk_disable_unprepare(pc->clk); return 0; }
When disabling PWM, the duty cycle needs to be set to 0. This prevents the previous duty cycle from showing up momentarily when the clock is re-enabled next time, and also prevents the output pin from being stuck high when the clock is disabled. Because the clock has to be running in order to configure the duty cycle, unconditionally enable it early in pxa_pwm_apply and account for the correct enable count at the end. Suggested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Signed-off-by: Doug Brown <doug@schmorgal.com> --- drivers/pwm/pwm-pxa.c | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-)