Hello Thierry,

On Thu, Aug 19, 2021 at 03:36:26PM +0200, Thierry Reding wrote:
> On Thu, Jul 01, 2021 at 09:29:26AM +0200, Uwe Kleine-König wrote:
> > If a running PWM is reconfigured to disabled calling the ->config()
> > callback before disabling the hardware might result in a glitch where
> > the (maybe) new period and duty_cycle are visible on the output before
> > disabling the hardware.
> > 
> > So handle disabling before calling ->config(). Also exit early in this case
> > which is possible because period and duty_cycle don't matter for disabled PWMs.
> > In return however ->config has to be called even if state->period ==
> > pwm->state.period && state->duty_cycle != pwm->state.duty_cycle because setting
> > these might have been skipped in the previous call.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/pwm/core.c | 41 ++++++++++++++++++++++++-----------------
> >  1 file changed, 24 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> > index 3c72f8963073..20afe6d0bc5e 100644
> > --- a/drivers/pwm/core.c
> > +++ b/drivers/pwm/core.c
> > @@ -568,26 +568,33 @@ static int pwm_apply_legacy(struct pwm_chip *chip, struct pwm_device *pwm,
> >  		pwm->state.polarity = state->polarity;
> >  	}
> >  
> > -	if (state->period != pwm->state.period ||
> > -	    state->duty_cycle != pwm->state.duty_cycle) {
> > -		err = chip->ops->config(pwm->chip, pwm,
> > -					state->duty_cycle,
> > -					state->period);
> > -		if (err)
> > -			return err;
> > +	if (!state->enabled) {
> > +		if (pwm->state.enabled)
> > +			chip->ops->disable(chip, pwm);
> >  
> > -		pwm->state.period = state->period;
> > -		pwm->state.duty_cycle = state->duty_cycle;
> > +		return 0;
> >  	}
> >  
> > -	if (state->enabled != pwm->state.enabled) {
> > -		if (!pwm->state.enabled) {
> > -			err = chip->ops->enable(chip, pwm);
> > -			if (err)
> > -				return err;
> > -		} else {
> > -			chip->ops->disable(chip, pwm);
> > -		}
> > +	/*
> > +	 * We cannot skip calling ->config even if state->period ==
> > +	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
> > +	 * because we might have exited early in the last call to
> > +	 * pwm_apply_state because of !state->enabled and so the two values in
> > +	 * pwm->state might not be configured in hardware.
> > +	 */
> > +	err = chip->ops->config(pwm->chip, pwm,
> > +				state->duty_cycle,
> > +				state->period);
> > +	if (err)
> > +		return err;
> > +
> > +	pwm->state.period = state->period;
> > +	pwm->state.duty_cycle = state->duty_cycle;
> > +
> > +	if (!pwm->state.enabled) {
> > +		err = chip->ops->enable(chip, pwm);
> > +		if (err)
> > +			return err;
> >  	}
> 
> I thought we might have discussed this, but I can't find any record of
> it. How is this better than always configuring, whether the PWM is
> disabled or not?

The question is: Should we call .config() if state->enabled is false,
right?

> From an atomic point of view, the hardware state is expected to match
> the PWM state that was passed to ->apply() after it returns. That means
> that calling ->get_state() after ->apply() should return the same state
> that was passed to ->apply(). With the above that's no longer true. It
> doesn't actually matter, because legacy drivers don't support
> ->get_state(), but conceptually it's not mimicking the atomic API as
> well as it could.

In my book the value of .period and .duty_cycle is irrelevant when
state->enabled is false as it has no effect on the actual behaviour of
the hardware. And then calling .config() with all the calculations that
are done there just to have the values stored in hardware doesn't make
much sense for me. Still more because this isn't possible for all
hardwares as not all support the concept of "disabled" and have to
simulate it using .duty_cycle = 0.

For me it is ok to deviate here, callers of .get_state() have to be
aware of other hardware induced deviations anyhow. If having a match
here is really wanted, then I suggest to fixup in the framework instead
of adapting all drivers (if and when we will have a consumer reachable
function that makes use of .get_state()). So for example do something
like:

	ret = chip->ops->get_state(..., &state);
	if (ret)
		return ret;

	/* Normalize */
	if (!state->enabled) {
		state->period = pwm->state->period;
		state->duty_cycle = pwm->state->duty_cycle;
	}

but also assigning 0 instead of pwm->state->{period,duty_cycle} would
work for me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |