On Mon, May 31, 2021 at 06:46:04AM +0200, Roman Beranek wrote:
> The reason why we wait before gating the clock is to allow for the PWM
> to finish its cycle and stop. But it won't stop unless the EN bit is
> disabled.
> 
> Signed-off-by: Roman Beranek <roman.beranek@prusa3d.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 3721b9894cf6..2777abe66f79 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -303,6 +303,8 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  	if (state->enabled)
>  		ctrl |= BIT_CH(PWM_EN, pwm->hwpwm);
> +	else
> +		ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);

Catching the case !state->enabled even earlier would make sense.
Otherwise you might see a needless glitch after

	pwm_apply_state(mypwm, { .period = A, .duty_cycle = B, .enabled = true });
	pwm_apply_state(mypwm, { .period = C, .duty_cycle = D, .enabled = false });

which might make C+D visible on the PWM before disabling.

>  
>  	sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
>  
> @@ -325,7 +327,6 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	spin_lock(&sun4i_pwm->ctrl_lock);
>  	ctrl = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>  	ctrl &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> -	ctrl &= ~BIT_CH(PWM_EN, pwm->hwpwm);
>  	sun4i_pwm_writel(sun4i_pwm, ctrl, PWM_CTRL_REG);
>  	spin_unlock(&sun4i_pwm->ctrl_lock);

So the comment

  /* We need a full period to elapse before disabling the channel. */

is wrong?

Best regards
Uwe

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