Re: [PATCH v2 5/5] pwm: imx27: wait till the duty cycle is applied

[Marco Felsch <m.felsch@pengutronix.de>]

On 20-09-28 10:04, Uwe Kleine-König wrote:
> On Fri, Sep 25, 2020 at 05:53:30PM +0200, Marco Felsch wrote:
> > Currently the driver don't check if the new state was applied or not.
> 
> s/don't/doesn't/
> 
> > This can cause glitches on the output pin if the new state disables the
> > PWM. In this case the PWM clocks are disabled before the new duty cycle
> > value gets applied.
> 
> Hmm, the problem that is addressed here is that .apply() might turn off
> the clock input for the counter before the inactive value is on the pin,
> right? So an alternative fix would be to not disable the clock, wouldn't
> it?

Yes, till the new state is applied.

> > The fix is to wait till the desired duty cycle was applied.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> > v2:
> > - new patch
> > 
> >  drivers/pwm/pwm-imx27.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> > 
> > diff --git a/drivers/pwm/pwm-imx27.c b/drivers/pwm/pwm-imx27.c
> > index 07c6a263a39c..ffa00bcd81da 100644
> > --- a/drivers/pwm/pwm-imx27.c
> > +++ b/drivers/pwm/pwm-imx27.c
> > @@ -222,6 +222,26 @@ static int pwm_imx27_get_fifo_slot(struct pwm_chip *chip,
> >  	return fifoav;
> >  }
> >  
> > +static int pwm_imx27_wait_till_applied(struct pwm_chip *chip,
> > +				       struct pwm_device *pwm)
> > +{
> > +	unsigned int attempts = 4;
> > +	unsigned int period_ms;
> > +	int busy_slots;
> > +
> > +	do {
> > +		busy_slots = pwm_imx27_get_fifo_slot(chip, pwm);
> > +		if (busy_slots == 0)
> > +			return 0;
> > +
> > +		period_ms = DIV_ROUND_UP(pwm_get_period(pwm),
> 
> I was glad you removed the call to pwm_get_state() from .apply(), now it is
> back in disguised form here :-\ 

I reused the code from pwm_imx27_get_fifo_slot().

> Also the value shouldn't change over the
> iteration of this loop, so determining it once should be enough.

Yes, you are right. I will change that.

> > +					 NSEC_PER_MSEC);
> > +		msleep(period_ms);
> > +	} while (attempts--);
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> >  static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  			   const struct pwm_state *state)
> >  {
> > @@ -277,6 +297,11 @@ static int pwm_imx27_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> >  		writel(duty_cycles, imx->mmio_base + MX3_PWMSAR);
> >  	else
> >  		writel(0, imx->mmio_base + MX3_PWMSAR);
> > +
> > +	ret = pwm_imx27_wait_till_applied(chip, pwm);
> > +	if (ret)
> > +		goto out;
> > +
> 
> The framework doesn't define (and this is a problem there) if .apply is
> supposed to sleep.

Current upstream driver sleeps as well if pwm_imx27_wait_fifo_slot()
waits. So this patch don't changes the bevhaviour.

Regards,
  Marco

> OTOH at least sun4i has a similar behaviour.
> Thierry, what is your thought on this?
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |



-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |