| Message ID | 20210308093600.25455-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: Prevent a glitch in compat code | expand |
On Mon, Mar 08, 2021 at 10:36:00AM +0100, Uwe Kleine-König wrote: > When a PWM is to be disabled, configuring the duty cycle and > period before actually disabling the hardware might result in either a > glitch or a delay. So check for disabling first and return early in this > case. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/pwm/core.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 4058d3c86a45..4604ca3e0e62 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -597,6 +597,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > pwm->state.polarity = state->polarity; > } > > + if (!state->enabled && pwm->state.enabled) { > + chip->ops->disable(chip, pwm); > + pwm->state.enabled = false; > + return 0; I don't think we can return early here because otherwise if consumers happen to modify the period along with the enabled state, the changes to the period will get lost. Can we just omit the "return 0;" here? > + } > + > if (state->period != pwm->state.period || > state->duty_cycle != pwm->state.duty_cycle) { > int duty_cycle, period; > @@ -620,16 +626,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > pwm->state.period = state->period; > } > > - if (state->enabled != pwm->state.enabled) { > - if (state->enabled) { > - err = chip->ops->enable(chip, pwm); > - if (err) > - return err; > - } else { > - chip->ops->disable(chip, pwm); > - } > + if (!pwm->state.enabled) { If we don't return early above, we'll have to change this here as well to ensure we don't gratuituously enable the PWM. Instead this would basically become the inverse of what we do for disable above, which is also nicely symmetrical (i.e. disable, then change period/duty to avoid a glitch and change period/duty then enable to avoid the glitch). Thierry
On Mon, Mar 22, 2021 at 12:01:01PM +0100, Thierry Reding wrote: > On Mon, Mar 08, 2021 at 10:36:00AM +0100, Uwe Kleine-König wrote: > > When a PWM is to be disabled, configuring the duty cycle and > > period before actually disabling the hardware might result in either a > > glitch or a delay. So check for disabling first and return early in this > > case. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/pwm/core.c | 20 +++++++++++--------- > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > index 4058d3c86a45..4604ca3e0e62 100644 > > --- a/drivers/pwm/core.c > > +++ b/drivers/pwm/core.c > > @@ -597,6 +597,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > pwm->state.polarity = state->polarity; > > } > > > > + if (!state->enabled && pwm->state.enabled) { > > + chip->ops->disable(chip, pwm); > > + pwm->state.enabled = false; > > + return 0; > > I don't think we can return early here because otherwise if consumers > happen to modify the period along with the enabled state, the changes > to the period will get lost. This however doesn't matter, because the output of a disabled PWM only depends on polarity. (And polarity is already cared for.) And if a driver calls pwm_enable() afterwards (or the equivalent in terms of pwm_apply_state) the period and duty_cycle will be picked up correctly. > Can we just omit the "return 0;" here? > > > + } > > + > > if (state->period != pwm->state.period || > > state->duty_cycle != pwm->state.duty_cycle) { > > int duty_cycle, period; > > @@ -620,16 +626,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > pwm->state.period = state->period; > > } > > > > - if (state->enabled != pwm->state.enabled) { > > - if (state->enabled) { > > - err = chip->ops->enable(chip, pwm); > > - if (err) > > - return err; > > - } else { > > - chip->ops->disable(chip, pwm); > > - } > > + if (!pwm->state.enabled) { > > If we don't return early above, we'll have to change this here as well > to ensure we don't gratuituously enable the PWM. Instead this would > basically become the inverse of what we do for disable above, which is > also nicely symmetrical (i.e. disable, then change period/duty to avoid > a glitch and change period/duty then enable to avoid the glitch). Either I don't understand you, or we're already there. Best regards Uwe
On Mon, Mar 22, 2021 at 12:11:31PM +0100, Uwe Kleine-König wrote: > On Mon, Mar 22, 2021 at 12:01:01PM +0100, Thierry Reding wrote: > > On Mon, Mar 08, 2021 at 10:36:00AM +0100, Uwe Kleine-König wrote: > > > When a PWM is to be disabled, configuring the duty cycle and > > > period before actually disabling the hardware might result in either a > > > glitch or a delay. So check for disabling first and return early in this > > > case. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/pwm/core.c | 20 +++++++++++--------- > > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > index 4058d3c86a45..4604ca3e0e62 100644 > > > --- a/drivers/pwm/core.c > > > +++ b/drivers/pwm/core.c > > > @@ -597,6 +597,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > > pwm->state.polarity = state->polarity; > > > } > > > > > > + if (!state->enabled && pwm->state.enabled) { > > > + chip->ops->disable(chip, pwm); > > > + pwm->state.enabled = false; > > > + return 0; > > > > I don't think we can return early here because otherwise if consumers > > happen to modify the period along with the enabled state, the changes > > to the period will get lost. > > This however doesn't matter, because the output of a disabled PWM only > depends on polarity. (And polarity is already cared for.) > > And if a driver calls pwm_enable() afterwards (or the equivalent in > terms of pwm_apply_state) the period and duty_cycle will be picked up > correctly. I see you marked my patch as "changes requested" in patchwork. However I'm convinced your feedback is wrong and so I still think the patch is safe to be applied unmodified. Best regards Uwe
On Sat, Apr 10, 2021 at 11:46:42PM +0200, Uwe Kleine-König wrote: > On Mon, Mar 22, 2021 at 12:11:31PM +0100, Uwe Kleine-König wrote: > > On Mon, Mar 22, 2021 at 12:01:01PM +0100, Thierry Reding wrote: > > > On Mon, Mar 08, 2021 at 10:36:00AM +0100, Uwe Kleine-König wrote: > > > > When a PWM is to be disabled, configuring the duty cycle and > > > > period before actually disabling the hardware might result in either a > > > > glitch or a delay. So check for disabling first and return early in this > > > > case. > > > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > > --- > > > > drivers/pwm/core.c | 20 +++++++++++--------- > > > > 1 file changed, 11 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > > > > index 4058d3c86a45..4604ca3e0e62 100644 > > > > --- a/drivers/pwm/core.c > > > > +++ b/drivers/pwm/core.c > > > > @@ -597,6 +597,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) > > > > pwm->state.polarity = state->polarity; > > > > } > > > > > > > > + if (!state->enabled && pwm->state.enabled) { > > > > + chip->ops->disable(chip, pwm); > > > > + pwm->state.enabled = false; > > > > + return 0; > > > > > > I don't think we can return early here because otherwise if consumers > > > happen to modify the period along with the enabled state, the changes > > > to the period will get lost. > > > > This however doesn't matter, because the output of a disabled PWM only > > depends on polarity. (And polarity is already cared for.) > > > > And if a driver calls pwm_enable() afterwards (or the equivalent in > > terms of pwm_apply_state) the period and duty_cycle will be picked up > > correctly. > > I see you marked my patch as "changes requested" in patchwork. However > I'm convinced your feedback is wrong and so I still think the patch > is safe to be applied unmodified. After sleeping about this I understood you're right. I will rework the patch. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 4058d3c86a45..4604ca3e0e62 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -597,6 +597,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) pwm->state.polarity = state->polarity; } + if (!state->enabled && pwm->state.enabled) { + chip->ops->disable(chip, pwm); + pwm->state.enabled = false; + return 0; + } + if (state->period != pwm->state.period || state->duty_cycle != pwm->state.duty_cycle) { int duty_cycle, period; @@ -620,16 +626,12 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) pwm->state.period = state->period; } - if (state->enabled != pwm->state.enabled) { - if (state->enabled) { - err = chip->ops->enable(chip, pwm); - if (err) - return err; - } else { - chip->ops->disable(chip, pwm); - } + if (!pwm->state.enabled) { + err = chip->ops->enable(chip, pwm); + if (err) + return err; - pwm->state.enabled = state->enabled; + pwm->state.enabled = true; } }
When a PWM is to be disabled, configuring the duty cycle and period before actually disabling the hardware might result in either a glitch or a delay. So check for disabling first and return early in this case. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/pwm/core.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)