[PATCH v2 0/8] pwm: mediatek: Convert to waveform API

[PATCH v2 0/8] pwm: mediatek: Convert to waveform API

[Uwe Kleine-König]

Hello,

here comes v2 of my effort to convert the pwm-mediatek driver to the new
waveform API. Changes since (implicit) v1 (available at
https://lore.kernel.org/linux-pwm/cover.1751994509.git.u.kleine-koenig@baylibre.com/):

 - Rebase on top of the latest fix for period and duty setting
   (https://lore.kernel.org/linux-pwm/20250724210041.2513590-2-u.kleine-koenig@baylibre.com/)

 - Don't report a disabled PWM when the PWM is disabled. Though that
   seems counter intuitive, this is the only way to allow the consumer
   to query the minimal period. This wasn't necessary in v1 because back
   then the driver claimed to provide a non-disabled setting for
   duty_cycle=0 .

As before the patches in the middle don't serve a functional purpose
because the last patch removes the changes again. Still I consider it
useful because it reduces the last patch to what is needed in such a
conversion.

Best regards
Uwe

Uwe Kleine-König (8):
  pwm: mediatek: Simplify representation of channel offsets
  pwm: mediatek: Introduce and use a few more register defines
  pwm: mediatek: Rework parameters for clk helper function
  pwm: mediatek: Initialize clks when the hardware is enabled at probe
    time
  pwm: mediatek: Implement .get_state() callback
  pwm: mediatek: Fix various issues in the .apply() callback
  pwm: mediatek: Lock and cache clock rate
  pwm: mediatek: Convert to waveform API

 drivers/pwm/pwm-mediatek.c | 454 ++++++++++++++++++++++++++-----------
 1 file changed, 319 insertions(+), 135 deletions(-)


base-commit: 68b9272ca7ac948b71aba482ef8244dee8032f46
prerequisite-patch-id: ec97edc20a72c4e283e74cec97c8cc6036e2fd01
-- 
2.50.0

[PATCH v2 1/8] pwm: mediatek: Simplify representation of channel offsets

[Uwe Kleine-König]

The general register layout contains some per-chip registers starting at
offset 0 and then at a higher address there are n nearly identical and
equidistant blocks for the registers of the n channels.

This allows to represent the offsets of per-channel registers as $base +
i * $width instead of listing all (or too many) offsets explicitly in an
array. So for a small additional effort in pwm_mediatek_writel() the
three arrays with the channel offsets can be dropped.

The size changes according to bloat-o-meter are:

	add/remove: 0/3 grow/shrink: 1/0 up/down: 12/-96 (-84)
	Function                                     old     new   delta
	pwm_mediatek_apply                           696     708     +12
	mtk_pwm_reg_offset_v3                         32       -     -32
	mtk_pwm_reg_offset_v2                         32       -     -32
	mtk_pwm_reg_offset_v1                         32       -     -32
	Total: Before=5347, After=5263, chg -1.57%

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 58 ++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 28 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 4460bbca2582..4dfe657957bf 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -38,7 +38,8 @@ struct pwm_mediatek_of_data {
 	unsigned int num_pwms;
 	bool pwm45_fixup;
 	u16 pwm_ck_26m_sel_reg;
-	const unsigned int *reg_offset;
+	unsigned int chanreg_base;
+	unsigned int chanreg_width;
 };
 
 /**
@@ -57,19 +58,6 @@ struct pwm_mediatek_chip {
 	const struct pwm_mediatek_of_data *soc;
 };
 
-static const unsigned int mtk_pwm_reg_offset_v1[] = {
-	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
-};
-
-static const unsigned int mtk_pwm_reg_offset_v2[] = {
-	0x0080, 0x00c0, 0x0100, 0x0140, 0x0180, 0x01c0, 0x0200, 0x0240
-};
-
-/* PWM IP Version 3.0.2 */
-static const unsigned int mtk_pwm_reg_offset_v3[] = {
-	0x0100, 0x0200, 0x0300, 0x0400, 0x0500, 0x0600, 0x0700, 0x0800
-};
-
 static inline struct pwm_mediatek_chip *
 to_pwm_mediatek_chip(struct pwm_chip *chip)
 {
@@ -118,7 +106,8 @@ static inline void pwm_mediatek_writel(struct pwm_mediatek_chip *chip,
 				       unsigned int num, unsigned int offset,
 				       u32 value)
 {
-	writel(value, chip->regs + chip->soc->reg_offset[num] + offset);
+	writel(value, chip->regs + chip->soc->chanreg_base +
+	       num * chip->soc->chanreg_width + offset);
 }
 
 static void pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -306,86 +295,99 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 static const struct pwm_mediatek_of_data mt2712_pwm_data = {
 	.num_pwms = 8,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt6795_pwm_data = {
 	.num_pwms = 7,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7622_pwm_data = {
 	.num_pwms = 6,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7623_pwm_data = {
 	.num_pwms = 5,
 	.pwm45_fixup = true,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7628_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = true,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7629_pwm_data = {
 	.num_pwms = 1,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7981_pwm_data = {
 	.num_pwms = 3,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v2,
+	.chanreg_base = 0x80,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7986_pwm_data = {
 	.num_pwms = 2,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt7988_pwm_data = {
 	.num_pwms = 8,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v2,
+	.chanreg_base = 0x80,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt8183_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt8365_pwm_data = {
 	.num_pwms = 3,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt8516_pwm_data = {
 	.num_pwms = 5,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.chanreg_base = 0x10,
+	.chanreg_width = 0x40,
 };
 
 static const struct pwm_mediatek_of_data mt6991_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = false,
 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL_V3,
-	.reg_offset = mtk_pwm_reg_offset_v3,
+	.chanreg_base = 0x100,
+	.chanreg_width = 0x100,
 };
 
 static const struct of_device_id pwm_mediatek_of_match[] = {
-- 
2.50.0

[PATCH v2 2/8] pwm: mediatek: Introduce and use a few more register defines

[Uwe Kleine-König]

Instead of using a magic constant for bound checking, derive the numbers
from appropriate register defines.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 4dfe657957bf..eb348337454d 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -7,6 +7,7 @@
  *
  */
 
+#include <linux/bitfield.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
@@ -21,19 +22,19 @@
 
 /* PWM registers and bits definitions */
 #define PWMCON			0x00
+#define PWMCON_CLKDIV			GENMASK(2, 0)
 #define PWMHDUR			0x04
 #define PWMLDUR			0x08
 #define PWMGDUR			0x0c
 #define PWMWAVENUM		0x28
 #define PWMDWIDTH		0x2c
+#define PWMDWIDTH_PERIOD		GENMASK(12, 0)
 #define PWM45DWIDTH_FIXUP	0x30
 #define PWMTHRES		0x30
 #define PWM45THRES_FIXUP	0x34
 #define PWM_CK_26M_SEL_V3	0x74
 #define PWM_CK_26M_SEL		0x210
 
-#define PWM_CLK_DIV_MAX		7
-
 struct pwm_mediatek_of_data {
 	unsigned int num_pwms;
 	bool pwm45_fixup;
@@ -162,14 +163,14 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!cnt_period)
 		return -EINVAL;
 
-	while (cnt_period > 8192) {
+	while (cnt_period - 1 > FIELD_MAX(PWMDWIDTH_PERIOD)) {
 		resolution *= 2;
 		clkdiv++;
 		cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
 						   resolution);
 	}
 
-	if (clkdiv > PWM_CLK_DIV_MAX) {
+	if (clkdiv > FIELD_MAX(PWMCON_CLKDIV)) {
 		dev_err(pwmchip_parent(chip), "period of %d ns not supported\n", period_ns);
 		ret = -EINVAL;
 		goto out;
-- 
2.50.0

[PATCH v2 3/8] pwm: mediatek: Rework parameters for clk helper function

[Uwe Kleine-König]

Convert pwm_mediatek_clk_enable() and pwm_mediatek_clk_disable() to take
lower level parameters. This enables these functions to be used in the next
commit when there is no valid pwm_chip and pwm_device yet.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index eb348337454d..16ea42be4c18 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -65,10 +65,9 @@ to_pwm_mediatek_chip(struct pwm_chip *chip)
 	return pwmchip_get_drvdata(chip);
 }
 
-static int pwm_mediatek_clk_enable(struct pwm_chip *chip,
-				   struct pwm_device *pwm)
+static int pwm_mediatek_clk_enable(struct pwm_mediatek_chip *pc,
+				   unsigned int hwpwm)
 {
-	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
 	int ret;
 
 	ret = clk_prepare_enable(pc->clk_top);
@@ -79,7 +78,7 @@ static int pwm_mediatek_clk_enable(struct pwm_chip *chip,
 	if (ret < 0)
 		goto disable_clk_top;
 
-	ret = clk_prepare_enable(pc->clk_pwms[pwm->hwpwm]);
+	ret = clk_prepare_enable(pc->clk_pwms[hwpwm]);
 	if (ret < 0)
 		goto disable_clk_main;
 
@@ -93,12 +92,10 @@ static int pwm_mediatek_clk_enable(struct pwm_chip *chip,
 	return ret;
 }
 
-static void pwm_mediatek_clk_disable(struct pwm_chip *chip,
-				     struct pwm_device *pwm)
+static void pwm_mediatek_clk_disable(struct pwm_mediatek_chip *pc,
+				     unsigned int hwpwm)
 {
-	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
-
-	clk_disable_unprepare(pc->clk_pwms[pwm->hwpwm]);
+	clk_disable_unprepare(pc->clk_pwms[hwpwm]);
 	clk_disable_unprepare(pc->clk_main);
 	clk_disable_unprepare(pc->clk_top);
 }
@@ -141,7 +138,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 resolution;
 	int ret;
 
-	ret = pwm_mediatek_clk_enable(chip, pwm);
+	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
 	if (ret < 0)
 		return ret;
 
@@ -198,7 +195,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 out:
-	pwm_mediatek_clk_disable(chip, pwm);
+	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
 
 	return ret;
 }
@@ -206,6 +203,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			      const struct pwm_state *state)
 {
+	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
 	int err;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
@@ -214,7 +212,7 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (!state->enabled) {
 		if (pwm->state.enabled) {
 			pwm_mediatek_disable(chip, pwm);
-			pwm_mediatek_clk_disable(chip, pwm);
+			pwm_mediatek_clk_disable(pc, pwm->hwpwm);
 		}
 
 		return 0;
@@ -225,7 +223,7 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		return err;
 
 	if (!pwm->state.enabled) {
-		err = pwm_mediatek_clk_enable(chip, pwm);
+		err = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
 		if (err < 0)
 			return err;
 	}
-- 
2.50.0

[PATCH v2 4/8] pwm: mediatek: Initialize clks when the hardware is enabled at probe time

[Uwe Kleine-König]

When a PWM is already configured by the bootloader (e.g. to power a
backlight), the clk enable count must be increased to keep clock usage
balanced. So check which PWMs are enabled during probe and enable the
respective clocks.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 47 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 16ea42be4c18..7d0a6f41d671 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -235,6 +235,49 @@ static const struct pwm_ops pwm_mediatek_ops = {
 	.apply = pwm_mediatek_apply,
 };
 
+static int pwm_mediatek_init_used_clks(struct pwm_mediatek_chip *pc)
+{
+	const struct pwm_mediatek_of_data *soc = pc->soc;
+	unsigned int hwpwm;
+	u32 enabled, handled = 0;
+	int ret;
+
+	ret = clk_prepare_enable(pc->clk_top);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(pc->clk_main);
+	if (ret)
+		goto err_enable_main;
+
+	enabled = readl(pc->regs) & GENMASK(soc->num_pwms - 1, 0);
+
+	while (enabled & ~handled) {
+		hwpwm = ilog2(enabled & ~handled);
+
+		ret = pwm_mediatek_clk_enable(pc, hwpwm);
+		if (ret) {
+			while (handled) {
+				hwpwm = ilog2(handled);
+
+				pwm_mediatek_clk_disable(pc, hwpwm);
+				handled &= ~BIT(hwpwm);
+			}
+
+			break;
+		}
+
+		handled |= BIT(hwpwm);
+	}
+
+	clk_disable_unprepare(pc->clk_main);
+err_enable_main:
+
+	clk_disable_unprepare(pc->clk_top);
+
+	return ret;
+}
+
 static int pwm_mediatek_probe(struct platform_device *pdev)
 {
 	struct pwm_chip *chip;
@@ -282,6 +325,10 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 					     "Failed to get %s clock\n", name);
 	}
 
+	ret = pwm_mediatek_init_used_clks(pc);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "Failed to initialize used clocks\n");
+
 	chip->ops = &pwm_mediatek_ops;
 
 	ret = devm_pwmchip_add(&pdev->dev, chip);
-- 
2.50.0

[PATCH v2 5/8] pwm: mediatek: Implement .get_state() callback

[Uwe Kleine-König]

The registers can be read out just fine on an MT8365. In the assumption
that this works on all supported devices, a .get_state() callback can be
implemented. This enables consumers to make use of pwm_get_state_hw() and
improves the usefulness of /sys/kernel/debug/pwm.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 70 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 7d0a6f41d671..05263a17d55d 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -31,6 +31,7 @@
 #define PWMDWIDTH_PERIOD		GENMASK(12, 0)
 #define PWM45DWIDTH_FIXUP	0x30
 #define PWMTHRES		0x30
+#define PWMTHRES_DUTY			GENMASK(12, 0)
 #define PWM45THRES_FIXUP	0x34
 #define PWM_CK_26M_SEL_V3	0x74
 #define PWM_CK_26M_SEL		0x210
@@ -108,6 +109,13 @@ static inline void pwm_mediatek_writel(struct pwm_mediatek_chip *chip,
 	       num * chip->soc->chanreg_width + offset);
 }
 
+static inline u32 pwm_mediatek_readl(struct pwm_mediatek_chip *chip,
+				     unsigned int num, unsigned int offset)
+{
+	return readl(chip->regs + chip->soc->chanreg_base +
+		     num * chip->soc->chanreg_width + offset);
+}
+
 static void pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 {
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
@@ -231,8 +239,70 @@ static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return err;
 }
 
+static int pwm_mediatek_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				  struct pwm_state *state)
+{
+	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
+	int ret;
+	u32 enable;
+	u32 reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
+
+	if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
+		/*
+		 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
+		 * from the other PWMs on MT7623.
+		 */
+		reg_width = PWM45DWIDTH_FIXUP;
+		reg_thres = PWM45THRES_FIXUP;
+	}
+
+	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
+	if (ret < 0)
+		return ret;
+
+	enable = readl(pc->regs);
+	if (enable & BIT(pwm->hwpwm)) {
+		u32 clkdiv, cnt_period, cnt_duty;
+		unsigned long clk_rate;
+
+		clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
+		if (!clk_rate) {
+			ret = -EINVAL;
+			goto out;
+		}
+
+		state->enabled = true;
+		state->polarity = PWM_POLARITY_NORMAL;
+
+		clkdiv = FIELD_GET(PWMCON_CLKDIV,
+				   pwm_mediatek_readl(pc, pwm->hwpwm, PWMCON));
+		cnt_period = FIELD_GET(PWMDWIDTH_PERIOD,
+				       pwm_mediatek_readl(pc, pwm->hwpwm, reg_width));
+		cnt_duty = FIELD_GET(PWMTHRES_DUTY,
+				     pwm_mediatek_readl(pc, pwm->hwpwm, reg_thres));
+
+		/*
+		 * cnt_period is a 13 bit value, NSEC_PER_SEC is 30 bits wide
+		 * and clkdiv is less than 8, so the multiplication doesn't
+		 * overflow an u64.
+		 */
+		state->period =
+			DIV_ROUND_UP_ULL((u64)cnt_period * NSEC_PER_SEC << clkdiv, clk_rate);
+		state->duty_cycle =
+			DIV_ROUND_UP_ULL((u64)cnt_duty * NSEC_PER_SEC << clkdiv, clk_rate);
+	} else {
+		state->enabled = false;
+	}
+
+out:
+	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
+
+	return ret;
+}
+
 static const struct pwm_ops pwm_mediatek_ops = {
 	.apply = pwm_mediatek_apply,
+	.get_state = pwm_mediatek_get_state,
 };
 
 static int pwm_mediatek_init_used_clks(struct pwm_mediatek_chip *pc)
-- 
2.50.0

[PATCH v2 6/8] pwm: mediatek: Fix various issues in the .apply() callback

[Uwe Kleine-König]

duty_cycle and period were silently cast from u64 to int losing
relevant bits. Dividing by the result of a division (resolution) looses
precision. clkdiv was determined using a loop while it can be done
without one. Also too low period values were not catched.

Improve all these issues. Handling period and duty_cycle being u64 now
requires a bit more care to prevent overflows, so mul_u64_u64_div_u64()
is used.

The changes implemented in this change also align the chosen hardware
settings to match the usual PWM rules (i.e. round down instead round
nearest) and so .apply() also matches .get_state() silencing several
warnings with PWM_DEBUG=y. While this probably doesn't result in
problems, this aspect makes this change---though it might be considered
a fix---unsuitable for backporting.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 71 +++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 28 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index 05263a17d55d..c0a97d18e673 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -137,13 +137,13 @@ static void pwm_mediatek_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 }
 
 static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			       int duty_ns, int period_ns)
+			       u64 duty_ns, u64 period_ns)
 {
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
-	u32 clkdiv = 0, cnt_period, cnt_duty, reg_width = PWMDWIDTH,
-	    reg_thres = PWMTHRES;
+	u32 clkdiv, enable;
+	u32 reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
+	u64 cnt_period, cnt_duty;
 	unsigned long clk_rate;
-	u64 resolution;
 	int ret;
 
 	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
@@ -151,7 +151,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		return ret;
 
 	clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
-	if (!clk_rate) {
+	/*
+	 * With the clk running with not more than 1 GHz the calculations below
+	 * won't overflow
+	 */
+	if (!clk_rate || clk_rate > 1000000000) {
 		ret = -EINVAL;
 		goto out;
 	}
@@ -160,27 +164,40 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (pc->soc->pwm_ck_26m_sel_reg)
 		writel(0, pc->regs + pc->soc->pwm_ck_26m_sel_reg);
 
-	/* Using resolution in picosecond gets accuracy higher */
-	resolution = (u64)NSEC_PER_SEC * 1000;
-	do_div(resolution, clk_rate);
-
-	cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000, resolution);
-	if (!cnt_period)
-		return -EINVAL;
-
-	while (cnt_period - 1 > FIELD_MAX(PWMDWIDTH_PERIOD)) {
-		resolution *= 2;
-		clkdiv++;
-		cnt_period = DIV_ROUND_CLOSEST_ULL((u64)period_ns * 1000,
-						   resolution);
-	}
-
-	if (clkdiv > FIELD_MAX(PWMCON_CLKDIV)) {
-		dev_err(pwmchip_parent(chip), "period of %d ns not supported\n", period_ns);
-		ret = -EINVAL;
+	cnt_period = mul_u64_u64_div_u64(period_ns, clk_rate, NSEC_PER_SEC);
+	if (cnt_period == 0) {
+		ret = -ERANGE;
 		goto out;
 	}
 
+	if (cnt_period > FIELD_MAX(PWMDWIDTH_PERIOD) + 1) {
+		if (cnt_period >= ((FIELD_MAX(PWMDWIDTH_PERIOD) + 1) << FIELD_MAX(PWMCON_CLKDIV))) {
+			clkdiv = FIELD_MAX(PWMCON_CLKDIV);
+			cnt_period = FIELD_MAX(PWMDWIDTH_PERIOD) + 1;
+		} else {
+			clkdiv = ilog2(cnt_period) - ilog2(FIELD_MAX(PWMDWIDTH_PERIOD));
+			cnt_period >>= clkdiv;
+		}
+	} else {
+		clkdiv = 0;
+	}
+
+	cnt_duty = mul_u64_u64_div_u64(duty_ns, clk_rate, NSEC_PER_SEC) >> clkdiv;
+	if (cnt_duty > cnt_period)
+		cnt_duty = cnt_period;
+
+	if (cnt_duty) {
+		cnt_duty -= 1;
+		enable = BIT(pwm->hwpwm);
+	} else {
+		enable = 0;
+	}
+
+	cnt_period -= 1;
+
+	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld @%lu -> CON: %x, PERIOD: %llx, DUTY: %llx\n",
+		pwm->hwpwm, duty_ns, period_ns, clk_rate, clkdiv, cnt_period, cnt_duty);
+
 	if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
 		/*
 		 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
@@ -190,13 +207,11 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		reg_thres = PWM45THRES_FIXUP;
 	}
 
-	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 - 1);
+	pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
 
-	if (cnt_duty) {
-		pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty - 1);
+	if (enable) {
+		pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
 		pwm_mediatek_enable(chip, pwm);
 	} else {
 		pwm_mediatek_disable(chip, pwm);
-- 
2.50.0

[PATCH v2 7/8] pwm: mediatek: Lock and cache clock rate

[Uwe Kleine-König]

This simplifies error handling and reduces the amount of clk_get_rate()
calls.

While touching the clk handling also allocate the clock array as part of
driver data and lock the clock rate to ensure that the output doesn't
change unexpectedly.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 64 +++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index c0a97d18e673..e3db54995f7b 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -49,15 +49,18 @@ struct pwm_mediatek_of_data {
  * @regs: base address of PWM chip
  * @clk_top: the top clock generator
  * @clk_main: the clock used by PWM core
- * @clk_pwms: the clock used by each PWM channel
  * @soc: pointer to chip's platform data
+ * @clk_pwms: the clock and clkrate used by each PWM channel
  */
 struct pwm_mediatek_chip {
 	void __iomem *regs;
 	struct clk *clk_top;
 	struct clk *clk_main;
-	struct clk **clk_pwms;
 	const struct pwm_mediatek_of_data *soc;
+	struct {
+		struct clk *clk;
+		unsigned long rate;
+	} clk_pwms[];
 };
 
 static inline struct pwm_mediatek_chip *
@@ -79,12 +82,28 @@ static int pwm_mediatek_clk_enable(struct pwm_mediatek_chip *pc,
 	if (ret < 0)
 		goto disable_clk_top;
 
-	ret = clk_prepare_enable(pc->clk_pwms[hwpwm]);
+	ret = clk_prepare_enable(pc->clk_pwms[hwpwm].clk);
 	if (ret < 0)
 		goto disable_clk_main;
 
+	if (!pc->clk_pwms[hwpwm].rate) {
+		pc->clk_pwms[hwpwm].rate = clk_get_rate(pc->clk_pwms[hwpwm].clk);
+
+		/*
+		 * With the clk running with not more than 1 GHz the
+		 * calculations in .apply() won't overflow.
+		 */
+		if (!pc->clk_pwms[hwpwm].rate ||
+		    pc->clk_pwms[hwpwm].rate > 1000000000) {
+			ret = -EINVAL;
+			goto disable_clk_hwpwm;
+		}
+	}
+
 	return 0;
 
+disable_clk_hwpwm:
+	clk_disable_unprepare(pc->clk_pwms[hwpwm].clk);
 disable_clk_main:
 	clk_disable_unprepare(pc->clk_main);
 disable_clk_top:
@@ -96,7 +115,7 @@ static int pwm_mediatek_clk_enable(struct pwm_mediatek_chip *pc,
 static void pwm_mediatek_clk_disable(struct pwm_mediatek_chip *pc,
 				     unsigned int hwpwm)
 {
-	clk_disable_unprepare(pc->clk_pwms[hwpwm]);
+	clk_disable_unprepare(pc->clk_pwms[hwpwm].clk);
 	clk_disable_unprepare(pc->clk_main);
 	clk_disable_unprepare(pc->clk_top);
 }
@@ -150,15 +169,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	if (ret < 0)
 		return ret;
 
-	clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
-	/*
-	 * With the clk running with not more than 1 GHz the calculations below
-	 * won't overflow
-	 */
-	if (!clk_rate || clk_rate > 1000000000) {
-		ret = -EINVAL;
-		goto out;
-	}
+	clk_rate = pc->clk_pwms[pwm->hwpwm].rate;
 
 	/* Make sure we use the bus clock and not the 26MHz clock */
 	if (pc->soc->pwm_ck_26m_sel_reg)
@@ -217,7 +228,6 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		pwm_mediatek_disable(chip, pwm);
 	}
 
-out:
 	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
 
 	return ret;
@@ -280,11 +290,7 @@ static int pwm_mediatek_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		u32 clkdiv, cnt_period, cnt_duty;
 		unsigned long clk_rate;
 
-		clk_rate = clk_get_rate(pc->clk_pwms[pwm->hwpwm]);
-		if (!clk_rate) {
-			ret = -EINVAL;
-			goto out;
-		}
+		clk_rate = pc->clk_pwms[pwm->hwpwm].rate;
 
 		state->enabled = true;
 		state->polarity = PWM_POLARITY_NORMAL;
@@ -309,7 +315,6 @@ static int pwm_mediatek_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 		state->enabled = false;
 	}
 
-out:
 	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
 
 	return ret;
@@ -373,7 +378,8 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 
 	soc = of_device_get_match_data(&pdev->dev);
 
-	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_pwms, sizeof(*pc));
+	chip = devm_pwmchip_alloc(&pdev->dev, soc->num_pwms,
+				  sizeof(*pc) + soc->num_pwms * sizeof(*pc->clk_pwms));
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 	pc = to_pwm_mediatek_chip(chip);
@@ -384,11 +390,6 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->regs))
 		return PTR_ERR(pc->regs);
 
-	pc->clk_pwms = devm_kmalloc_array(&pdev->dev, soc->num_pwms,
-				    sizeof(*pc->clk_pwms), GFP_KERNEL);
-	if (!pc->clk_pwms)
-		return -ENOMEM;
-
 	pc->clk_top = devm_clk_get(&pdev->dev, "top");
 	if (IS_ERR(pc->clk_top))
 		return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_top),
@@ -404,10 +405,15 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 
 		snprintf(name, sizeof(name), "pwm%d", i + 1);
 
-		pc->clk_pwms[i] = devm_clk_get(&pdev->dev, name);
-		if (IS_ERR(pc->clk_pwms[i]))
-			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i]),
+		pc->clk_pwms[i].clk = devm_clk_get(&pdev->dev, name);
+		if (IS_ERR(pc->clk_pwms[i].clk))
+			return dev_err_probe(&pdev->dev, PTR_ERR(pc->clk_pwms[i].clk),
 					     "Failed to get %s clock\n", name);
+
+		ret = devm_clk_rate_exclusive_get(&pdev->dev, pc->clk_pwms[i].clk);
+		if (ret)
+			return dev_err_probe(&pdev->dev, ret,
+					     "Failed to lock clock rate for %s\n", name);
 	}
 
 	ret = pwm_mediatek_init_used_clks(pc);
-- 
2.50.0

[PATCH v2 8/8] pwm: mediatek: Convert to waveform API

[Uwe Kleine-König]

Implement the new waveform callbacks which makes the usage of this
hardware more flexible and allows to use it via the pwm character
device.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-mediatek.c | 283 +++++++++++++++++++++----------------
 1 file changed, 164 insertions(+), 119 deletions(-)

diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index e3db54995f7b..8ad289b06352 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -135,50 +135,53 @@ static inline u32 pwm_mediatek_readl(struct pwm_mediatek_chip *chip,
 		     num * chip->soc->chanreg_width + 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,
-			       u64 duty_ns, u64 period_ns)
+struct pwm_mediatek_waveform {
+	u32 enable;
+	u32 con;
+	u32 width;
+	u32 thres;
+};
+
+static int pwm_mediatek_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *pwm,
+					    const struct pwm_waveform *wf, void *_wfhw)
 {
+	struct pwm_mediatek_waveform *wfhw = _wfhw;
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
 	u32 clkdiv, enable;
-	u32 reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
 	u64 cnt_period, cnt_duty;
 	unsigned long clk_rate;
-	int ret;
+	int ret = 0;
 
-	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
-	if (ret < 0)
-		return ret;
+	if (wf->period_length_ns == 0) {
+		*wfhw = (typeof(*wfhw)){
+			.con = 0,
+		};
+
+		return 0;
+	}
+
+	if (!pc->clk_pwms[pwm->hwpwm].rate) {
+		struct clk *clk = pc->clk_pwms[pwm->hwpwm].clk;
+
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			return ret;
+
+		pc->clk_pwms[pwm->hwpwm].rate = clk_get_rate(clk);
+
+		clk_disable_unprepare(clk);
+
+		if (pc->clk_pwms[pwm->hwpwm].rate == 0 ||
+		    pc->clk_pwms[pwm->hwpwm].rate > 1000000000)
+			return -EINVAL;
+	}
 
 	clk_rate = pc->clk_pwms[pwm->hwpwm].rate;
 
-	/* Make sure we use the bus clock and not the 26MHz clock */
-	if (pc->soc->pwm_ck_26m_sel_reg)
-		writel(0, pc->regs + pc->soc->pwm_ck_26m_sel_reg);
-
-	cnt_period = mul_u64_u64_div_u64(period_ns, clk_rate, NSEC_PER_SEC);
+	cnt_period = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
 	if (cnt_period == 0) {
-		ret = -ERANGE;
-		goto out;
+		cnt_period = 1;
+		ret = 1;
 	}
 
 	if (cnt_period > FIELD_MAX(PWMDWIDTH_PERIOD) + 1) {
@@ -193,7 +196,7 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		clkdiv = 0;
 	}
 
-	cnt_duty = mul_u64_u64_div_u64(duty_ns, clk_rate, NSEC_PER_SEC) >> clkdiv;
+	cnt_duty = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC) >> clkdiv;
 	if (cnt_duty > cnt_period)
 		cnt_duty = cnt_period;
 
@@ -206,123 +209,165 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	cnt_period -= 1;
 
-	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld @%lu -> CON: %x, PERIOD: %llx, DUTY: %llx\n",
-		pwm->hwpwm, duty_ns, period_ns, clk_rate, clkdiv, cnt_period, cnt_duty);
+	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld @%lu -> ENABLE: %x, CON: %x, PERIOD: %llx, DUTY: %llx\n",
+		pwm->hwpwm, wf->duty_length_ns, wf->period_length_ns, clk_rate,
+		enable, clkdiv, cnt_period, cnt_duty);
 
-	if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
-		/*
-		 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
-		 * from the other PWMs on MT7623.
-		 */
-		reg_width = PWM45DWIDTH_FIXUP;
-		reg_thres = PWM45THRES_FIXUP;
-	}
-
-	pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, BIT(15) | clkdiv);
-	pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, cnt_period);
-
-	if (enable) {
-		pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, cnt_duty);
-		pwm_mediatek_enable(chip, pwm);
-	} else {
-		pwm_mediatek_disable(chip, pwm);
-	}
-
-	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
+	*wfhw = (typeof(*wfhw)){
+		.enable = enable,
+		.con = clkdiv,
+		.width = cnt_period,
+		.thres = cnt_duty,
+	};
 
 	return ret;
 }
 
-static int pwm_mediatek_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			      const struct pwm_state *state)
+static int pwm_mediatek_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+					      const void *_wfhw, struct pwm_waveform *wf)
 {
+	const struct pwm_mediatek_waveform *wfhw = _wfhw;
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
-	int err;
+	u32 clkdiv, cnt_period, cnt_duty;
+	unsigned long clk_rate;
 
-	if (state->polarity != PWM_POLARITY_NORMAL)
-		return -EINVAL;
+	/*
+	 * When _wfhw was populated, the clock was on, so .rate is
+	 * already set appropriately.
+	 */
+	clk_rate = pc->clk_pwms[pwm->hwpwm].rate;
 
-	if (!state->enabled) {
-		if (pwm->state.enabled) {
-			pwm_mediatek_disable(chip, pwm);
-			pwm_mediatek_clk_disable(pc, pwm->hwpwm);
-		}
+	clkdiv = FIELD_GET(PWMCON_CLKDIV, wfhw->con);
+	cnt_period = FIELD_GET(PWMDWIDTH_PERIOD, wfhw->width);
+	cnt_duty = FIELD_GET(PWMTHRES_DUTY, wfhw->thres);
 
-		return 0;
-	}
+	/*
+	 * cnt_period is a 13 bit value, NSEC_PER_SEC is 30 bits wide
+	 * and clkdiv is less than 8, so the multiplication doesn't
+	 * overflow an u64.
+	 */
+	*wf = (typeof(*wf)){
+		.period_length_ns =
+			DIV_ROUND_UP_ULL((u64)(cnt_period + 1) * NSEC_PER_SEC << clkdiv, clk_rate),
+		.duty_length_ns =
+			wfhw->enable ?
+			DIV_ROUND_UP_ULL((u64)(cnt_duty + 1) * NSEC_PER_SEC << clkdiv, clk_rate) : 0,
+		.duty_offset_ns = 0,
+	};
+	dev_dbg(&chip->dev, "pwm#%u: ENABLE: %x, CLKDIV: %x, PERIOD: %x, DUTY: %x @%lu -> %lld/%lld\n",
+		pwm->hwpwm, wfhw->enable, clkdiv, cnt_period, cnt_duty, clk_rate,
+		wf->duty_length_ns, wf->period_length_ns);
 
-	err = pwm_mediatek_config(chip, pwm, state->duty_cycle, state->period);
-	if (err)
-		return err;
-
-	if (!pwm->state.enabled) {
-		err = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
-		if (err < 0)
-			return err;
-	}
-
-	return err;
+	return 0;
 }
 
-static int pwm_mediatek_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
-				  struct pwm_state *state)
+static int pwm_mediatek_read_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm, void *_wfhw)
 {
+	struct pwm_mediatek_waveform *wfhw = _wfhw;
 	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
-	int ret;
-	u32 enable;
+	u32 enable, clkdiv, cnt_period, cnt_duty;
 	u32 reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
-
-	if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
-		/*
-		 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
-		 * from the other PWMs on MT7623.
-		 */
-		reg_width = PWM45DWIDTH_FIXUP;
-		reg_thres = PWM45THRES_FIXUP;
-	}
+	int ret;
 
 	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
 	if (ret < 0)
 		return ret;
 
 	enable = readl(pc->regs);
-	if (enable & BIT(pwm->hwpwm)) {
-		u32 clkdiv, cnt_period, cnt_duty;
-		unsigned long clk_rate;
-
-		clk_rate = pc->clk_pwms[pwm->hwpwm].rate;
-
-		state->enabled = true;
-		state->polarity = PWM_POLARITY_NORMAL;
-
-		clkdiv = FIELD_GET(PWMCON_CLKDIV,
-				   pwm_mediatek_readl(pc, pwm->hwpwm, PWMCON));
-		cnt_period = FIELD_GET(PWMDWIDTH_PERIOD,
-				       pwm_mediatek_readl(pc, pwm->hwpwm, reg_width));
-		cnt_duty = FIELD_GET(PWMTHRES_DUTY,
-				     pwm_mediatek_readl(pc, pwm->hwpwm, reg_thres));
 
+	if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
 		/*
-		 * cnt_period is a 13 bit value, NSEC_PER_SEC is 30 bits wide
-		 * and clkdiv is less than 8, so the multiplication doesn't
-		 * overflow an u64.
+		 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
+		 * from the other PWMs on MT7623.
 		 */
-		state->period =
-			DIV_ROUND_UP_ULL((u64)cnt_period * NSEC_PER_SEC << clkdiv, clk_rate);
-		state->duty_cycle =
-			DIV_ROUND_UP_ULL((u64)cnt_duty * NSEC_PER_SEC << clkdiv, clk_rate);
-	} else {
-		state->enabled = false;
+		reg_width = PWM45DWIDTH_FIXUP;
+		reg_thres = PWM45THRES_FIXUP;
 	}
 
+	clkdiv = FIELD_GET(PWMCON_CLKDIV, pwm_mediatek_readl(pc, pwm->hwpwm, PWMCON));
+	cnt_period = FIELD_GET(PWMDWIDTH_PERIOD, pwm_mediatek_readl(pc, pwm->hwpwm, reg_width));
+	cnt_duty = FIELD_GET(PWMTHRES_DUTY, pwm_mediatek_readl(pc, pwm->hwpwm, reg_thres));
+
+	*wfhw = (typeof(*wfhw)){
+		.enable = enable & BIT(pwm->hwpwm),
+		.con = BIT(15) | clkdiv,
+		.width = cnt_period,
+		.thres = cnt_duty,
+	};
+
+	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
+
+	return ret;
+}
+
+static int pwm_mediatek_write_waveform(struct pwm_chip *chip,
+				       struct pwm_device *pwm, const void *_wfhw)
+{
+	const struct pwm_mediatek_waveform *wfhw = _wfhw;
+	struct pwm_mediatek_chip *pc = to_pwm_mediatek_chip(chip);
+	u32 ctrl;
+	int ret;
+
+	ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
+	if (ret < 0)
+		return ret;
+
+	ctrl = readl(pc->regs);
+
+	if (wfhw->con & BIT(15)) {
+		u32 reg_width = PWMDWIDTH, reg_thres = PWMTHRES;
+
+		if (pc->soc->pwm45_fixup && pwm->hwpwm > 2) {
+			/*
+			 * PWM[4,5] has distinct offset for PWMDWIDTH and PWMTHRES
+			 * from the other PWMs on MT7623.
+			 */
+			reg_width = PWM45DWIDTH_FIXUP;
+			reg_thres = PWM45THRES_FIXUP;
+		}
+
+		if (!(ctrl & BIT(pwm->hwpwm))) {
+			ctrl |= BIT(pwm->hwpwm);
+			writel(ctrl, pc->regs);
+
+			/*
+			 * The clks are already on, just increasing the usage
+			 * counter doesn't fail.
+			 */
+			ret = pwm_mediatek_clk_enable(pc, pwm->hwpwm);
+			if (unlikely(ret < 0))
+				goto out;
+		}
+
+		/* Make sure we use the bus clock and not the 26MHz clock */
+		if (pc->soc->pwm_ck_26m_sel_reg)
+			writel(0, pc->regs + pc->soc->pwm_ck_26m_sel_reg);
+
+		pwm_mediatek_writel(pc, pwm->hwpwm, PWMCON, wfhw->con);
+		pwm_mediatek_writel(pc, pwm->hwpwm, reg_width, wfhw->width);
+		pwm_mediatek_writel(pc, pwm->hwpwm, reg_thres, wfhw->thres);
+	} else {
+		if (ctrl & BIT(pwm->hwpwm)) {
+			ctrl &= ~BIT(pwm->hwpwm);
+			writel(ctrl, pc->regs);
+
+			pwm_mediatek_clk_disable(pc, pwm->hwpwm);
+		}
+	}
+
+out:
 	pwm_mediatek_clk_disable(pc, pwm->hwpwm);
 
 	return ret;
 }
 
 static const struct pwm_ops pwm_mediatek_ops = {
-	.apply = pwm_mediatek_apply,
-	.get_state = pwm_mediatek_get_state,
+	.sizeof_wfhw = sizeof(struct pwm_mediatek_waveform),
+	.round_waveform_tohw = pwm_mediatek_round_waveform_tohw,
+	.round_waveform_fromhw = pwm_mediatek_round_waveform_fromhw,
+	.read_waveform = pwm_mediatek_read_waveform,
+	.write_waveform = pwm_mediatek_write_waveform,
 };
 
 static int pwm_mediatek_init_used_clks(struct pwm_mediatek_chip *pc)
-- 
2.50.0

Re: [PATCH v2 8/8] pwm: mediatek: Convert to waveform API

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> Implement the new waveform callbacks which makes the usage of this
> hardware more flexible and allows to use it via the pwm character
> device.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

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

Re: [PATCH v2 7/8] pwm: mediatek: Lock and cache clock rate

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> This simplifies error handling and reduces the amount of clk_get_rate()
> calls.
> 
> While touching the clk handling also allocate the clock array as part of
> driver data and lock the clock rate to ensure that the output doesn't
> change unexpectedly.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>


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

Re: [PATCH v2 4/8] pwm: mediatek: Initialize clks when the hardware is enabled at probe time

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> When a PWM is already configured by the bootloader (e.g. to power a
> backlight), the clk enable count must be increased to keep clock usage
> balanced. So check which PWMs are enabled during probe and enable the
> respective clocks.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

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

Re: [PATCH v2 6/8] pwm: mediatek: Fix various issues in the .apply() callback

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> duty_cycle and period were silently cast from u64 to int losing
> relevant bits. Dividing by the result of a division (resolution) looses
> precision. clkdiv was determined using a loop while it can be done
> without one. Also too low period values were not catched.
> 
> Improve all these issues. Handling period and duty_cycle being u64 now
> requires a bit more care to prevent overflows, so mul_u64_u64_div_u64()
> is used.
> 
> The changes implemented in this change also align the chosen hardware
> settings to match the usual PWM rules (i.e. round down instead round
> nearest) and so .apply() also matches .get_state() silencing several
> warnings with PWM_DEBUG=y. While this probably doesn't result in
> problems, this aspect makes this change---though it might be considered
> a fix---unsuitable for backporting.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

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

Re: [PATCH v2 3/8] pwm: mediatek: Rework parameters for clk helper function

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> Convert pwm_mediatek_clk_enable() and pwm_mediatek_clk_disable() to take
> lower level parameters. This enables these functions to be used in the next
> commit when there is no valid pwm_chip and pwm_device yet.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

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

Re: [PATCH v2 2/8] pwm: mediatek: Introduce and use a few more register defines

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> Instead of using a magic constant for bound checking, derive the numbers
> from appropriate register defines.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

Awesome.

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

Re: [PATCH v2 5/8] pwm: mediatek: Implement .get_state() callback

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> The registers can be read out just fine on an MT8365. In the assumption
> that this works on all supported devices, a .get_state() callback can be
> implemented. This enables consumers to make use of pwm_get_state_hw() and
> improves the usefulness of /sys/kernel/debug/pwm.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

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

Re: [PATCH v2 1/8] pwm: mediatek: Simplify representation of channel offsets

[AngeloGioacchino Del Regno]

Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> The general register layout contains some per-chip registers starting at
> offset 0 and then at a higher address there are n nearly identical and
> equidistant blocks for the registers of the n channels.
> 
> This allows to represent the offsets of per-channel registers as $base +
> i * $width instead of listing all (or too many) offsets explicitly in an
> array. So for a small additional effort in pwm_mediatek_writel() the
> three arrays with the channel offsets can be dropped.
> 
> The size changes according to bloat-o-meter are:
> 
> 	add/remove: 0/3 grow/shrink: 1/0 up/down: 12/-96 (-84)
> 	Function                                     old     new   delta
> 	pwm_mediatek_apply                           696     708     +12
> 	mtk_pwm_reg_offset_v3                         32       -     -32
> 	mtk_pwm_reg_offset_v2                         32       -     -32
> 	mtk_pwm_reg_offset_v1                         32       -     -32
> 	Total: Before=5347, After=5263, chg -1.57%
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>

What if we do, instead...

struct pwm_mediatek_regs {
	u16 pwm_ck_26m_sel_reg;
	u16 chan_base;
	u16 chan_width;
};

struct pwm_mediatek_regs pwm_v1_reg_data = {
	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
	.chan_base = 0x10,
	.chan_width = 0x40,
};

static const struct pwm_mediatek_of_data mt7986_pwm_data = {
	....
	.reg_data = &pwm_v1_reg_data,
};

...that should reduce the bloat even more :-)

Cheers,
Angelo

> ---
>   drivers/pwm/pwm-mediatek.c | 58 ++++++++++++++++++++------------------
>   1 file changed, 30 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
> index 4460bbca2582..4dfe657957bf 100644
> --- a/drivers/pwm/pwm-mediatek.c
> +++ b/drivers/pwm/pwm-mediatek.c
> @@ -38,7 +38,8 @@ struct pwm_mediatek_of_data {
>   	unsigned int num_pwms;
>   	bool pwm45_fixup;
>   	u16 pwm_ck_26m_sel_reg;
> -	const unsigned int *reg_offset;
> +	unsigned int chanreg_base;
> +	unsigned int chanreg_width;
>   };
>   
>   /**
> @@ -57,19 +58,6 @@ struct pwm_mediatek_chip {
>   	const struct pwm_mediatek_of_data *soc;
>   };
>   
> -static const unsigned int mtk_pwm_reg_offset_v1[] = {
> -	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
> -};
> -
> -static const unsigned int mtk_pwm_reg_offset_v2[] = {
> -	0x0080, 0x00c0, 0x0100, 0x0140, 0x0180, 0x01c0, 0x0200, 0x0240
> -};
> -
> -/* PWM IP Version 3.0.2 */
> -static const unsigned int mtk_pwm_reg_offset_v3[] = {
> -	0x0100, 0x0200, 0x0300, 0x0400, 0x0500, 0x0600, 0x0700, 0x0800
> -};
> -
>   static inline struct pwm_mediatek_chip *
>   to_pwm_mediatek_chip(struct pwm_chip *chip)
>   {
> @@ -118,7 +106,8 @@ static inline void pwm_mediatek_writel(struct pwm_mediatek_chip *chip,
>   				       unsigned int num, unsigned int offset,
>   				       u32 value)
>   {
> -	writel(value, chip->regs + chip->soc->reg_offset[num] + offset);
> +	writel(value, chip->regs + chip->soc->chanreg_base +
> +	       num * chip->soc->chanreg_width + offset);
>   }
>   
>   static void pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> @@ -306,86 +295,99 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
>   static const struct pwm_mediatek_of_data mt2712_pwm_data = {
>   	.num_pwms = 8,
>   	.pwm45_fixup = false,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt6795_pwm_data = {
>   	.num_pwms = 7,
>   	.pwm45_fixup = false,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7622_pwm_data = {
>   	.num_pwms = 6,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7623_pwm_data = {
>   	.num_pwms = 5,
>   	.pwm45_fixup = true,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7628_pwm_data = {
>   	.num_pwms = 4,
>   	.pwm45_fixup = true,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7629_pwm_data = {
>   	.num_pwms = 1,
>   	.pwm45_fixup = false,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7981_pwm_data = {
>   	.num_pwms = 3,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v2,
> +	.chanreg_base = 0x80,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7986_pwm_data = {
>   	.num_pwms = 2,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt7988_pwm_data = {
>   	.num_pwms = 8,
>   	.pwm45_fixup = false,
> -	.reg_offset = mtk_pwm_reg_offset_v2,
> +	.chanreg_base = 0x80,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt8183_pwm_data = {
>   	.num_pwms = 4,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt8365_pwm_data = {
>   	.num_pwms = 3,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt8516_pwm_data = {
>   	.num_pwms = 5,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> -	.reg_offset = mtk_pwm_reg_offset_v1,
> +	.chanreg_base = 0x10,
> +	.chanreg_width = 0x40,
>   };
>   
>   static const struct pwm_mediatek_of_data mt6991_pwm_data = {
>   	.num_pwms = 4,
>   	.pwm45_fixup = false,
>   	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL_V3,
> -	.reg_offset = mtk_pwm_reg_offset_v3,
> +	.chanreg_base = 0x100,
> +	.chanreg_width = 0x100,
>   };
>   
>   static const struct of_device_id pwm_mediatek_of_match[] = {

Re: [PATCH v2 1/8] pwm: mediatek: Simplify representation of channel offsets

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

Hallo AngeloGioacchino,

On Mon, Aug 04, 2025 at 10:50:01AM +0200, AngeloGioacchino Del Regno wrote:
> Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> > The general register layout contains some per-chip registers starting at
> > offset 0 and then at a higher address there are n nearly identical and
> > equidistant blocks for the registers of the n channels.
> > 
> > This allows to represent the offsets of per-channel registers as $base +
> > i * $width instead of listing all (or too many) offsets explicitly in an
> > array. So for a small additional effort in pwm_mediatek_writel() the
> > three arrays with the channel offsets can be dropped.
> > 
> > The size changes according to bloat-o-meter are:
> > 
> > 	add/remove: 0/3 grow/shrink: 1/0 up/down: 12/-96 (-84)
> > 	Function                                     old     new   delta
> > 	pwm_mediatek_apply                           696     708     +12
> > 	mtk_pwm_reg_offset_v3                         32       -     -32
> > 	mtk_pwm_reg_offset_v2                         32       -     -32
> > 	mtk_pwm_reg_offset_v1                         32       -     -32
> > 	Total: Before=5347, After=5263, chg -1.57%
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> 
> What if we do, instead...
> 
> struct pwm_mediatek_regs {
> 	u16 pwm_ck_26m_sel_reg;
> 	u16 chan_base;
> 	u16 chan_width;
> };
> 
> struct pwm_mediatek_regs pwm_v1_reg_data = {
> 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> 	.chan_base = 0x10,
> 	.chan_width = 0x40,
> };
> 
> static const struct pwm_mediatek_of_data mt7986_pwm_data = {
> 	....
> 	.reg_data = &pwm_v1_reg_data,
> };
> 
> ...that should reduce the bloat even more :-)

Having the three u16 directly in pwm_mediatek_of_data is cheaper because
.reg_data is a pointer and so 64 bits wide (on arm64) and so bigger than
3xu16. Also having the data directly in pwm_mediatek_of_data saves one
indirection and so it should also be slightly faster.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/8] pwm: mediatek: Simplify representation of channel offsets

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 8887 bytes --]

On Mon, Aug 04, 2025 at 12:30:45PM +0200, Uwe Kleine-König wrote:
> Hallo AngeloGioacchino,
> 
> On Mon, Aug 04, 2025 at 10:50:01AM +0200, AngeloGioacchino Del Regno wrote:
> > Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
> > > The general register layout contains some per-chip registers starting at
> > > offset 0 and then at a higher address there are n nearly identical and
> > > equidistant blocks for the registers of the n channels.
> > > 
> > > This allows to represent the offsets of per-channel registers as $base +
> > > i * $width instead of listing all (or too many) offsets explicitly in an
> > > array. So for a small additional effort in pwm_mediatek_writel() the
> > > three arrays with the channel offsets can be dropped.
> > > 
> > > The size changes according to bloat-o-meter are:
> > > 
> > > 	add/remove: 0/3 grow/shrink: 1/0 up/down: 12/-96 (-84)
> > > 	Function                                     old     new   delta
> > > 	pwm_mediatek_apply                           696     708     +12
> > > 	mtk_pwm_reg_offset_v3                         32       -     -32
> > > 	mtk_pwm_reg_offset_v2                         32       -     -32
> > > 	mtk_pwm_reg_offset_v1                         32       -     -32
> > > 	Total: Before=5347, After=5263, chg -1.57%
> > > 
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > 
> > What if we do, instead...
> > 
> > struct pwm_mediatek_regs {
> > 	u16 pwm_ck_26m_sel_reg;
> > 	u16 chan_base;
> > 	u16 chan_width;
> > };
> > 
> > struct pwm_mediatek_regs pwm_v1_reg_data = {
> > 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
> > 	.chan_base = 0x10,
> > 	.chan_width = 0x40,
> > };
> > 
> > static const struct pwm_mediatek_of_data mt7986_pwm_data = {
> > 	....
> > 	.reg_data = &pwm_v1_reg_data,
> > };
> > 
> > ...that should reduce the bloat even more :-)
> 
> Having the three u16 directly in pwm_mediatek_of_data is cheaper because
> .reg_data is a pointer and so 64 bits wide (on arm64) and so bigger than
> 3xu16. Also having the data directly in pwm_mediatek_of_data saves one
> indirection and so it should also be slightly faster.

I took the time to complete your patch suggestion and the bloat-o-meter
output is:

add/remove: 4/3 grow/shrink: 1/0 up/down: 56/-96 (-40)
Function                                     old     new   delta
pwm_mediatek_apply                           776     808     +32
pwm_mediatek_v3_26m_reg_data                   -       6      +6
pwm_mediatek_v2_reg_data                       -       6      +6
pwm_mediatek_v1_reg_data                       -       6      +6
pwm_mediatek_v1_26m_reg_data                   -       6      +6
mtk_pwm_reg_offset_v3                         32       -     -32
mtk_pwm_reg_offset_v2                         32       -     -32
mtk_pwm_reg_offset_v1                         32       -     -32
Total: Before=5427, After=5387, chg -0.74%

See below for the complete patch for reference.

I tend to stick to my variant, also because then all info is in a single
struct which is nice for both the human reader and the generated code
(only on indirection).

Best regards
Uwe

---->8----
diff --git a/drivers/pwm/pwm-mediatek.c b/drivers/pwm/pwm-mediatek.c
index e4b595fc5a5e..ae8fa561ce2c 100644
--- a/drivers/pwm/pwm-mediatek.c
+++ b/drivers/pwm/pwm-mediatek.c
@@ -34,11 +34,38 @@
 
 #define PWM_CLK_DIV_MAX		7
 
+struct pwm_mediatek_regs {
+	u16 pwm_ck_26m_sel_reg;
+	u16 chan_base;
+	u16 chan_width;
+};
+
+struct pwm_mediatek_regs pwm_mediatek_v1_reg_data = {
+        .chan_base = 0x10,
+        .chan_width = 0x40,
+};
+
+struct pwm_mediatek_regs pwm_mediatek_v1_26m_reg_data = {
+        .pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
+        .chan_base = 0x10,
+        .chan_width = 0x40,
+};
+
+struct pwm_mediatek_regs pwm_mediatek_v2_reg_data = {
+        .chan_base = 0x80,
+        .chan_width = 0x40,
+};
+
+struct pwm_mediatek_regs pwm_mediatek_v3_26m_reg_data = {
+        .pwm_ck_26m_sel_reg = PWM_CK_26M_SEL_V3,
+        .chan_base = 0x100,
+        .chan_width = 0x100,
+};
+
 struct pwm_mediatek_of_data {
 	unsigned int num_pwms;
 	bool pwm45_fixup;
-	u16 pwm_ck_26m_sel_reg;
-	const unsigned int *reg_offset;
+	const struct pwm_mediatek_regs *reg_data;
 };
 
 /**
@@ -57,19 +84,6 @@ struct pwm_mediatek_chip {
 	const struct pwm_mediatek_of_data *soc;
 };
 
-static const unsigned int mtk_pwm_reg_offset_v1[] = {
-	0x0010, 0x0050, 0x0090, 0x00d0, 0x0110, 0x0150, 0x0190, 0x0220
-};
-
-static const unsigned int mtk_pwm_reg_offset_v2[] = {
-	0x0080, 0x00c0, 0x0100, 0x0140, 0x0180, 0x01c0, 0x0200, 0x0240
-};
-
-/* PWM IP Version 3.0.2 */
-static const unsigned int mtk_pwm_reg_offset_v3[] = {
-	0x0100, 0x0200, 0x0300, 0x0400, 0x0500, 0x0600, 0x0700, 0x0800
-};
-
 static inline struct pwm_mediatek_chip *
 to_pwm_mediatek_chip(struct pwm_chip *chip)
 {
@@ -118,7 +132,7 @@ static inline void pwm_mediatek_writel(struct pwm_mediatek_chip *chip,
 				       unsigned int num, unsigned int offset,
 				       u32 value)
 {
-	writel(value, chip->regs + chip->soc->reg_offset[num] + offset);
+	writel(value, chip->regs + chip->soc->reg_data->chan_base + num * chip->soc->reg_data->chan_width + offset);
 }
 
 static void pwm_mediatek_enable(struct pwm_chip *chip, struct pwm_device *pwm)
@@ -162,8 +176,8 @@ static int pwm_mediatek_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 	/* Make sure we use the bus clock and not the 26MHz clock */
-	if (pc->soc->pwm_ck_26m_sel_reg)
-		writel(0, pc->regs + pc->soc->pwm_ck_26m_sel_reg);
+	if (pc->soc->reg_data->pwm_ck_26m_sel_reg)
+		writel(0, pc->regs + pc->soc->reg_data->pwm_ck_26m_sel_reg);
 
 	/* Using resolution in picosecond gets accuracy higher */
 	resolution = (u64)NSEC_PER_SEC * 1000;
@@ -303,86 +317,79 @@ static int pwm_mediatek_probe(struct platform_device *pdev)
 static const struct pwm_mediatek_of_data mt2712_pwm_data = {
 	.num_pwms = 8,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt6795_pwm_data = {
 	.num_pwms = 7,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7622_pwm_data = {
 	.num_pwms = 6,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7623_pwm_data = {
 	.num_pwms = 5,
 	.pwm45_fixup = true,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7628_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = true,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7629_pwm_data = {
 	.num_pwms = 1,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7981_pwm_data = {
 	.num_pwms = 3,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v2,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7986_pwm_data = {
 	.num_pwms = 2,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt7988_pwm_data = {
 	.num_pwms = 8,
 	.pwm45_fixup = false,
-	.reg_offset = mtk_pwm_reg_offset_v2,
+	.reg_data = &pwm_mediatek_v2_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt8183_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt8365_pwm_data = {
 	.num_pwms = 3,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt8516_pwm_data = {
 	.num_pwms = 5,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
-	.reg_offset = mtk_pwm_reg_offset_v1,
+	.reg_data = &pwm_mediatek_v1_26m_reg_data,
 };
 
 static const struct pwm_mediatek_of_data mt6991_pwm_data = {
 	.num_pwms = 4,
 	.pwm45_fixup = false,
-	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL_V3,
-	.reg_offset = mtk_pwm_reg_offset_v3,
+	.reg_data = &pwm_mediatek_v3_26m_reg_data,
 };
 
 static const struct of_device_id pwm_mediatek_of_match[] = {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/8] pwm: mediatek: Simplify representation of channel offsets

[AngeloGioacchino Del Regno]

Il 04/08/25 15:14, Uwe Kleine-König ha scritto:
> On Mon, Aug 04, 2025 at 12:30:45PM +0200, Uwe Kleine-König wrote:
>> Hallo AngeloGioacchino,
>>
>> On Mon, Aug 04, 2025 at 10:50:01AM +0200, AngeloGioacchino Del Regno wrote:
>>> Il 25/07/25 17:45, Uwe Kleine-König ha scritto:
>>>> The general register layout contains some per-chip registers starting at
>>>> offset 0 and then at a higher address there are n nearly identical and
>>>> equidistant blocks for the registers of the n channels.
>>>>
>>>> This allows to represent the offsets of per-channel registers as $base +
>>>> i * $width instead of listing all (or too many) offsets explicitly in an
>>>> array. So for a small additional effort in pwm_mediatek_writel() the
>>>> three arrays with the channel offsets can be dropped.
>>>>
>>>> The size changes according to bloat-o-meter are:
>>>>
>>>> 	add/remove: 0/3 grow/shrink: 1/0 up/down: 12/-96 (-84)
>>>> 	Function                                     old     new   delta
>>>> 	pwm_mediatek_apply                           696     708     +12
>>>> 	mtk_pwm_reg_offset_v3                         32       -     -32
>>>> 	mtk_pwm_reg_offset_v2                         32       -     -32
>>>> 	mtk_pwm_reg_offset_v1                         32       -     -32
>>>> 	Total: Before=5347, After=5263, chg -1.57%
>>>>
>>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
>>>
>>> What if we do, instead...
>>>
>>> struct pwm_mediatek_regs {
>>> 	u16 pwm_ck_26m_sel_reg;
>>> 	u16 chan_base;
>>> 	u16 chan_width;
>>> };
>>>
>>> struct pwm_mediatek_regs pwm_v1_reg_data = {
>>> 	.pwm_ck_26m_sel_reg = PWM_CK_26M_SEL,
>>> 	.chan_base = 0x10,
>>> 	.chan_width = 0x40,
>>> };
>>>
>>> static const struct pwm_mediatek_of_data mt7986_pwm_data = {
>>> 	....
>>> 	.reg_data = &pwm_v1_reg_data,
>>> };
>>>
>>> ...that should reduce the bloat even more :-)
>>
>> Having the three u16 directly in pwm_mediatek_of_data is cheaper because
>> .reg_data is a pointer and so 64 bits wide (on arm64) and so bigger than
>> 3xu16. Also having the data directly in pwm_mediatek_of_data saves one
>> indirection and so it should also be slightly faster.
> 
> I took the time to complete your patch suggestion and the bloat-o-meter
> output is:
> 
> add/remove: 4/3 grow/shrink: 1/0 up/down: 56/-96 (-40)
> Function                                     old     new   delta
> pwm_mediatek_apply                           776     808     +32
> pwm_mediatek_v3_26m_reg_data                   -       6      +6
> pwm_mediatek_v2_reg_data                       -       6      +6
> pwm_mediatek_v1_reg_data                       -       6      +6
> pwm_mediatek_v1_26m_reg_data                   -       6      +6
> mtk_pwm_reg_offset_v3                         32       -     -32
> mtk_pwm_reg_offset_v2                         32       -     -32
> mtk_pwm_reg_offset_v1                         32       -     -32
> Total: Before=5427, After=5387, chg -0.74%
> 
> See below for the complete patch for reference.
> 
> I tend to stick to my variant, also because then all info is in a single
> struct which is nice for both the human reader and the generated code
> (only on indirection).
> 

With this amount of reasoning, I can only say ...

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

;-)

Cheers!
Angelo

> Best regards
> Uwe
> 

Re: [PATCH v2 0/8] pwm: mediatek: Convert to waveform API

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1989 bytes --]

Hello,

On Fri, Jul 25, 2025 at 05:45:04PM +0200, Uwe Kleine-König wrote:
> here comes v2 of my effort to convert the pwm-mediatek driver to the new
> waveform API. Changes since (implicit) v1 (available at
> https://lore.kernel.org/linux-pwm/cover.1751994509.git.u.kleine-koenig@baylibre.com/):
> 
>  - Rebase on top of the latest fix for period and duty setting
>    (https://lore.kernel.org/linux-pwm/20250724210041.2513590-2-u.kleine-koenig@baylibre.com/)
> 
>  - Don't report a disabled PWM when the PWM is disabled. Though that
>    seems counter intuitive, this is the only way to allow the consumer
>    to query the minimal period. This wasn't necessary in v1 because back
>    then the driver claimed to provide a non-disabled setting for
>    duty_cycle=0 .
> 
> As before the patches in the middle don't serve a functional purpose
> because the last patch removes the changes again. Still I consider it
> useful because it reduces the last patch to what is needed in such a
> conversion.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (8):
>   pwm: mediatek: Simplify representation of channel offsets
>   pwm: mediatek: Introduce and use a few more register defines
>   pwm: mediatek: Rework parameters for clk helper function
>   pwm: mediatek: Initialize clks when the hardware is enabled at probe
>     time
>   pwm: mediatek: Implement .get_state() callback
>   pwm: mediatek: Fix various issues in the .apply() callback
>   pwm: mediatek: Lock and cache clock rate
>   pwm: mediatek: Convert to waveform API

i pushed the first 7 patches to pwm/for-nexxt now. I found a twist in
the conversion to the waveform APIs that I first want to think about a
bit more. The problem is that a setting with a small duty cycle now
reports the PWM as disabled. While this is technically correct,
this isn't helpful to let the consumer find the smallest possible
period.

Will followup when I left the current rabbit hole.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]