* [PATCH v2] pwm: mediatek: Fix duty and period setting
@ 2025-07-24 21:00 Uwe Kleine-König
2025-07-28 10:31 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 2+ messages in thread
From: Uwe Kleine-König @ 2025-07-24 21:00 UTC (permalink / raw)
To: Matthias Brugger, AngeloGioacchino Del Regno, John Crispin
Cc: linux-pwm, linux-arm-kernel, linux-mediatek
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.
Fixes: caf065f8fd58 ("pwm: Add MediaTek PWM support")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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
--
2.50.0
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] pwm: mediatek: Fix duty and period setting
2025-07-24 21:00 [PATCH v2] pwm: mediatek: Fix duty and period setting Uwe Kleine-König
@ 2025-07-28 10:31 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 2+ messages in thread
From: AngeloGioacchino Del Regno @ 2025-07-28 10:31 UTC (permalink / raw)
To: Uwe Kleine-König, Matthias Brugger, John Crispin
Cc: linux-pwm, linux-arm-kernel, linux-mediatek
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@collabora.com>
Cheers,
Angelo
>
> Fixes: caf065f8fd58 ("pwm: Add MediaTek PWM support")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-07-28 10:40 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-24 21:00 [PATCH v2] pwm: mediatek: Fix duty and period setting Uwe Kleine-König
2025-07-28 10:31 ` AngeloGioacchino Del Regno
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).