[PATCH v2] pwm: mediatek: Fix duty and period setting

[AngeloGioacchino Del Regno]

Il 24/07/25 23:00, Uwe Kleine-König ha scritto:
> The period generated by the hardware is
> 
> 	(PWMDWIDTH + 1) << CLKDIV) / freq
> 
> according to my tests with a signal analyser and also the documentation.
> 
> The current algorithm doesn't consider the `+ 1` part and so configures
> slightly too high periods. The same issue exists for the duty cycle
> setting. So subtract 1 from both the register values for period and
> duty cycle. If period is 0, bail out, if duty_cycle is 0, just disable
> the PWM which results in a constant low output.
> 
> Note that clk handling is dropped from pwm_mediatek_{en,dis}able to
> handle duty_cycle = 0, so the calls in pwm_mediatek_apply() have to be
> modified to do clk handling, too.

The changes LGTM, but please split those in two commits, one that fixes the +1
and one that changes the clock handling.

After which:

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno at collabora.com>

Cheers,
Angelo

> 
> Fixes: caf065f8fd58 ("pwm: Add MediaTek PWM support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at baylibre.com>
> ---
> Hello,
> 
> changes since (implicit) v1,
> https://lore.kernel.org/linux-pwm/20250710163705.2095418-2-u.kleine-koenig@baylibre.com/:
> 
>   - Drop superflous parenthesis from commit log
>   - To implement drop the enable register instead of bit 15 in the CON
>     register, adapt commit log accordingly.
> 
> Best regards
> Uwe
> 
>   drivers/pwm/pwm-mediatek.c | 76 ++++++++++++++++++++------------------
>   1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 6777c511622a..4460bbca2582 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -121,6 +121,26 @@ static inline void pwm_mediatek_writel(struct pwm_mediatek_chip *chip,
>   	writel(value, chip->regs + chip->soc->reg_offset[num] + offset);
>   }
>   
> +static void pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> +	u32 value;
> +
> +	value = readl(pc->regs);
> +	value |= BIT(pwm->hwpwm);
> +	writel(value, pc->regs);
> +}
> +
> +static void pwm_mediatek_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> +	u32 value;
> +
> +	value = readl(pc->regs);
> +	value &= ~BIT(pwm->hwpwm);
> +	writel(value, pc->regs);
> +}
> +
>   static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   			       int duty_ns, int period_ns)
>   {
> @@ -150,7 +170,10 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	do_div(resolution, clk_rate);
>   
>   	cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution);
> -	while (cnt_period > 8191) {
> +	if (!cnt_period)
> +		return -EINVAL;
> +
> +	while (cnt_period > 8192) {
>   		resolution *= 2;
>   		clkdiv++;
>   		cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
> @@ -173,9 +196,16 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	}
>   
>   	cnt_duty = DIV_ROUND_CLOSEST_ULL((u64)duty_ns * 1000, resolution);
> +
>   	pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
> -	pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
> -	pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
> +	pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period - 1);
> +
> +	if (cnt_duty) {
> +		pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty - 1);
> +		pwm_mediatek_enable(chip, pwm);
> +	} else {
> +		pwm_mediatek_disable(chip, pwm);
> +	}
>   
>   out:
>   	pwm_mediatek_clk_disable(chip, pwm);
> @@ -183,35 +213,6 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
>   	return ret;
>   }
>   
> -static int pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> -	u32 value;
> -	int ret;
> -
> -	ret = pwm_mediatek_clk_enable(chip, pwm);
> -	if (ret < 0)
> -		return ret;
> -
> -	value = readl(pc->regs);
> -	value |= BIT(pwm->hwpwm);
> -	writel(value, pc->regs);
> -
> -	return 0;
> -}
> -
> -static void pwm_mediatek_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> -{
> -	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
> -	u32 value;
> -
> -	value = readl(pc->regs);
> -	value &= ~BIT(pwm->hwpwm);
> -	writel(value, pc->regs);
> -
> -	pwm_mediatek_clk_disable(chip, pwm);
> -}
> -
>   static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   			      const struct pwm_state *state)
>   {
> @@ -221,8 +222,10 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   		return -EINVAL;
>   
>   	if (!state->enabled) {
> -		if (pwm->state.enabled)
> +		if (pwm->state.enabled) {
>   			pwm_mediatek_disable(chip, pwm);
> +			pwm_mediatek_clk_disable(chip, pwm);
> +		}
>   
>   		return 0;
>   	}
> @@ -231,8 +234,11 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>   	if (err)
>   		return err;
>   
> -	if (!pwm->state.enabled)
> -		err = pwm_mediatek_enable(chip, pwm);
> +	if (!pwm->state.enabled) {
> +		err = pwm_mediatek_clk_enable(chip, pwm);
> +		if (err < 0)
> +			return err;
> +	}
>   
>   	return err;
>   }
> 
> base-commit: a02b105fe9f2b82cbd13b13a98c2b9ffae4a7c27