[PATCH v3 0/2] Add support for nuvoton ma35d1 pwm controller

[PATCH v3 0/2] Add support for nuvoton ma35d1 pwm controller

[Chi-Wen Weng]

This patch series adds pwm driver for the nuvoton ma35d1 ARMv8 SoC.
It includes DT binding documentation and the ma35d1 pwm driver.

v3:
  - Update nuvoton,ma35d1-pwm.yaml
    - Add maintainers entry
    - Increse "#pwm-cells" to 3
  - Update ma35d1 pwm driver
    - Make include header and macros definitions organized alphabetically
    - Rename macros REG_PWM_XXXX to MA35D1_REG_PWM_XXXX
    - Add macros for register address

v2 resend:
  - Remove wrong 'Reviewed-by' tags.

v2:
  - Update nuvoton,ma35d1-pwm.yaml
    - Fix 'maxItems' of 'reg' to 1.
    - Remove unused label
  - Update ma35d1 pwm driver
    - Remove MODULE_ALIAS()
    - Add chip->atomic = true


Chi-Wen Weng (2):
  Add dt-bindings for Nuvoton MA35D1 SoC PWM controller.
  pwm: Add Nuvoton MA35D1 PWM controller support

 .../bindings/pwm/nuvoton,ma35d1-pwm.yaml      |  45 +++++
 drivers/pwm/Kconfig                           |   9 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-ma35d1.c                      | 179 ++++++++++++++++++
 4 files changed, 234 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/nuvoton,ma35d1-pwm.yaml
 create mode 100644 drivers/pwm/pwm-ma35d1.c

-- 
2.25.1

[PATCH v3 1/2] dt-bindings: pwm: nuvoton: Add MA35D1 pwm

[Chi-Wen Weng]

Add dt-bindings for Nuvoton MA35D1 SoC PWM controller.

Signed-off-by: Chi-Wen Weng <cwweng.linux@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/pwm/nuvoton,ma35d1-pwm.yaml      | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/nuvoton,ma35d1-pwm.yaml

diff --git a/Documentation/devicetree/bindings/pwm/nuvoton,ma35d1-pwm.yaml b/Documentation/devicetree/bindings/pwm/nuvoton,ma35d1-pwm.yaml
new file mode 100644
index 000000000000..47a59bdd14d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/nuvoton,ma35d1-pwm.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/nuvoton,ma35d1-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Nuvoton MA35D1 PWM controller
+
+maintainers:
+  - Chi-Wen Weng <cwweng@nuvoton.com>
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    enum:
+      - nuvoton,ma35d1-pwm
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 3
+
+required:
+  - compatible
+  - reg
+  - clocks
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/nuvoton,ma35d1-clk.h>
+
+    pwm@40580000 {
+      compatible = "nuvoton,ma35d1-pwm";
+      reg = <0x40580000 0x400>;
+      clocks = <&clk EPWM0_GATE>;
+      #pwm-cells = <3>;
+    };
-- 
2.25.1

[PATCH v3 2/2] pwm: Add Nuvoton MA35D1 PWM controller support

[Chi-Wen Weng]

This commit adds a generic PWM framework driver for Nuvoton MA35D1 PWM controller.

Signed-off-by: Chi-Wen Weng <cwweng.linux@gmail.com>
---
 drivers/pwm/Kconfig      |   9 ++
 drivers/pwm/Makefile     |   1 +
 drivers/pwm/pwm-ma35d1.c | 179 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 189 insertions(+)
 create mode 100644 drivers/pwm/pwm-ma35d1.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0915c1e7df16..97b9e83af020 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -411,6 +411,15 @@ config PWM_LPSS_PLATFORM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-lpss-platform.
 
+config PWM_MA35D1
+	tristate "Nuvoton MA35D1 PWM support"
+	depends on ARCH_MA35 || COMPILE_TEST
+	help
+	  Generic PWM framework driver for Nuvoton MA35D1.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-ma35d1.
+
 config PWM_MESON
 	tristate "Amlogic Meson PWM driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9081e0c0e9e0..c1d3a1d8add0 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -36,6 +36,7 @@ obj-$(CONFIG_PWM_LPC32XX)	+= pwm-lpc32xx.o
 obj-$(CONFIG_PWM_LPSS)		+= pwm-lpss.o
 obj-$(CONFIG_PWM_LPSS_PCI)	+= pwm-lpss-pci.o
 obj-$(CONFIG_PWM_LPSS_PLATFORM)	+= pwm-lpss-platform.o
+obj-$(CONFIG_PWM_MA35D1)	+= pwm-ma35d1.o
 obj-$(CONFIG_PWM_MESON)		+= pwm-meson.o
 obj-$(CONFIG_PWM_MEDIATEK)	+= pwm-mediatek.o
 obj-$(CONFIG_PWM_MICROCHIP_CORE)	+= pwm-microchip-core.o
diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
new file mode 100644
index 000000000000..380f17b20a3d
--- /dev/null
+++ b/drivers/pwm/pwm-ma35d1.c
@@ -0,0 +1,179 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the Nuvoton MA35D1 PWM controller
+ *
+ * Copyright (C) 2024 Nuvoton Corporation
+ *               Chi-Wen Weng <cwweng@nuvoton.com>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/math64.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+
+/* The following are registers address offset for MA35D1 PWM controller */
+#define MA35D1_REG_PWM_CTL0            (0x00)
+#define MA35D1_REG_PWM_CNTEN           (0x20)
+#define MA35D1_REG_PWM_PERIOD0         (0x30)
+#define MA35D1_REG_PWM_CMPDAT0         (0x50)
+#define MA35D1_REG_PWM_WGCTL0          (0xB0)
+#define MA35D1_REG_PWM_POLCTL          (0xD4)
+#define MA35D1_REG_PWM_POEN            (0xD8)
+
+/* The following are register address of MA35D1 PWM controller */
+#define MA35D1_PWM_CH_REG_SIZE         4
+#define MA35D1_PWM_CMPDAT0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_CMPDAT0 + \
+					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
+#define MA35D1_PWM_CNTEN_ADDR(base)           ((base) + MA35D1_REG_PWM_CNTEN)
+#define MA35D1_PWM_PERIOD0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_PERIOD0 + \
+					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
+#define MA35D1_PWM_POEN_ADDR(base)            ((base) + MA35D1_REG_PWM_POEN)
+#define MA35D1_PWM_POLCTL_ADDR(base)          ((base) + MA35D1_REG_PWM_POLCTL)
+
+#define MA35D1_PWM_MAX_COUNT           0xFFFF
+#define MA35D1_PWM_TOTAL_CHANNELS      6
+
+struct nuvoton_pwm {
+	void __iomem *base;
+	u64 clkrate;
+};
+
+static inline struct nuvoton_pwm *to_nuvoton_pwm(struct pwm_chip *chip)
+{
+	return pwmchip_get_drvdata(chip);
+}
+
+static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			     const struct pwm_state *state)
+{
+	struct nuvoton_pwm *nvtpwm;
+	u32 ch = pwm->hwpwm;
+
+	nvtpwm = to_nuvoton_pwm(chip);
+	if (state->enabled) {
+		u64 duty_cycles, period_cycles;
+
+		/* Calculate the duty and period cycles */
+		duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
+						  state->duty_cycle, NSEC_PER_SEC);
+		if (duty_cycles > MA35D1_PWM_MAX_COUNT)
+			duty_cycles = MA35D1_PWM_MAX_COUNT;
+
+		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
+						    state->period, NSEC_PER_SEC);
+		if (period_cycles > MA35D1_PWM_MAX_COUNT)
+			period_cycles = MA35D1_PWM_MAX_COUNT;
+
+		/* Write the duty and period cycles to registers */
+		writel(duty_cycles, MA35D1_PWM_CMPDAT0_ADDR(nvtpwm->base, ch));
+		writel(period_cycles, MA35D1_PWM_PERIOD0_ADDR(nvtpwm->base, ch));
+		/* Enable counter */
+		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) | BIT(ch),
+		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
+		/* Enable output */
+		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) | BIT(ch),
+		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));
+	} else {
+		/* Disable counter */
+		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) & ~BIT(ch),
+		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
+		/* Disable output */
+		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) & ~BIT(ch),
+		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));
+	}
+
+	/* Set polarity state to register */
+	if (state->polarity == PWM_POLARITY_NORMAL)
+		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) & ~BIT(ch),
+		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
+	else
+		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) | BIT(ch),
+		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
+
+	return 0;
+}
+
+static int nuvoton_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct nuvoton_pwm *nvtpwm;
+	u32 duty_cycles, period_cycles, cnten, outen, polarity;
+	u32 ch = pwm->hwpwm;
+
+	nvtpwm = to_nuvoton_pwm(chip);
+
+	cnten = readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
+	outen = readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base));
+	duty_cycles = readl(MA35D1_PWM_CMPDAT0_ADDR(nvtpwm->base, ch));
+	period_cycles = readl(MA35D1_PWM_PERIOD0_ADDR(nvtpwm->base, ch));
+	polarity = readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) & BIT(ch);
+
+	state->enabled = (cnten & BIT(ch)) && (outen & BIT(ch));
+	state->polarity = polarity ? PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
+	state->duty_cycle = DIV64_U64_ROUND_UP((u64)duty_cycles * NSEC_PER_SEC, nvtpwm->clkrate);
+	state->period = DIV64_U64_ROUND_UP((u64)period_cycles * NSEC_PER_SEC, nvtpwm->clkrate);
+
+	return 0;
+}
+
+static const struct pwm_ops nuvoton_pwm_ops = {
+	.apply = nuvoton_pwm_apply,
+	.get_state = nuvoton_pwm_get_state,
+};
+
+static int nuvoton_pwm_probe(struct platform_device *pdev)
+{
+	struct pwm_chip *chip;
+	struct nuvoton_pwm *nvtpwm;
+	struct clk *clk;
+	int ret;
+
+	chip = devm_pwmchip_alloc(&pdev->dev, MA35D1_PWM_TOTAL_CHANNELS, sizeof(*nvtpwm));
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	nvtpwm = to_nuvoton_pwm(chip);
+
+	nvtpwm->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(nvtpwm->base))
+		return PTR_ERR(nvtpwm->base);
+
+	clk = devm_clk_get_enabled(&pdev->dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get the clock");
+
+	nvtpwm->clkrate = clk_get_rate(clk);
+	if (nvtpwm->clkrate > NSEC_PER_SEC)
+		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clock out of range");
+
+	chip->ops = &nuvoton_pwm_ops;
+	chip->atomic = true;
+
+	ret = devm_pwmchip_add(&pdev->dev, chip);
+	if (ret < 0)
+		return dev_err_probe(&pdev->dev, ret, "unable to add pwm chip");
+
+	return 0;
+}
+
+static const struct of_device_id nuvoton_pwm_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-pwm" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, nuvoton_pwm_of_match);
+
+static struct platform_driver nuvoton_pwm_driver = {
+	.probe = nuvoton_pwm_probe,
+	.driver = {
+		.name = "nuvoton-pwm",
+		.of_match_table = nuvoton_pwm_of_match,
+	},
+};
+module_platform_driver(nuvoton_pwm_driver);
+
+MODULE_AUTHOR("Chi-Wen Weng <cwweng@nuvoton.com>");
+MODULE_DESCRIPTION("Nuvoton MA35D1 PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.25.1

Re: [PATCH v3 2/2] pwm: Add Nuvoton MA35D1 PWM controller support

[Trevor Gamblin]

On 2024-12-06 03:55, Chi-Wen Weng wrote:
> This commit adds a generic PWM framework driver for Nuvoton MA35D1 PWM controller.
>
> Signed-off-by: Chi-Wen Weng <cwweng.linux@gmail.com>
Reviewed-by: Trevor Gamblin <tgamblin@baylibre.com>

Re: [PATCH v3 2/2] pwm: Add Nuvoton MA35D1 PWM controller support

[Uwe Kleine-König]

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

Hello,

On Fri, Dec 06, 2024 at 04:55:01PM +0800, Chi-Wen Weng wrote:
> [...]
> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
> new file mode 100644
> index 000000000000..380f17b20a3d
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c
> @@ -0,0 +1,179 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for the Nuvoton MA35D1 PWM controller
> + *
> + * Copyright (C) 2024 Nuvoton Corporation
> + *               Chi-Wen Weng <cwweng@nuvoton.com>

Please add some information here about the hardware. The idea is to get
some info about the device's capabilities. Please stick to the format
that many other drivers are using such that

	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-ma35d1.c

emits the info for your driver. Things to mention are: possible glitches
during .apply(); behaviour of the pin when the PWM is disabled (constant
signal? High-Z?)

Also a link to a reference manual would be awesome.

> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/math64.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +
> +/* The following are registers address offset for MA35D1 PWM controller */
> +#define MA35D1_REG_PWM_CTL0            (0x00)
> +#define MA35D1_REG_PWM_CNTEN           (0x20)
> +#define MA35D1_REG_PWM_PERIOD0         (0x30)
> +#define MA35D1_REG_PWM_CMPDAT0         (0x50)
> +#define MA35D1_REG_PWM_WGCTL0          (0xB0)
> +#define MA35D1_REG_PWM_POLCTL          (0xD4)
> +#define MA35D1_REG_PWM_POEN            (0xD8)
> +
> +/* The following are register address of MA35D1 PWM controller */
> +#define MA35D1_PWM_CH_REG_SIZE         4
> +#define MA35D1_PWM_CMPDAT0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_CMPDAT0 + \
> +					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
> +#define MA35D1_PWM_CNTEN_ADDR(base)           ((base) + MA35D1_REG_PWM_CNTEN)
> +#define MA35D1_PWM_PERIOD0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_PERIOD0 + \
> +					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
> +#define MA35D1_PWM_POEN_ADDR(base)            ((base) + MA35D1_REG_PWM_POEN)
> +#define MA35D1_PWM_POLCTL_ADDR(base)          ((base) + MA35D1_REG_PWM_POLCTL)

I would drop the base part in these defines and add them to the above
list sorted by address.

So something like:

#define MA35D1_REG_PWM_CTL0		0x00
#define MA35D1_REG_PWM_CNTEN		0x20
#define MA35D1_REG_PWM_PERIOD(ch)	(0x30 + 4 * (ch))
#define MA35D1_REG_PWM_CMPDAT(ch)	(0x50 + 4 * (ch))
#define MA35D1_REG_PWM_WGCTL0		0xB0
#define MA35D1_REG_PWM_POLCTL		0xD4
#define MA35D1_REG_PWM_POEN		0xD8

To make up for the missing base, I'd create wrappers for readl and
writel à la:

	static u32 nuvoton_pwm_readl(struct nuvoton_pwm *nvtpwm, unsigned int offset);
	static void nuvoton_pwm_writel(struct nuvoton_pwm *nvtpwm, unsigned int offset, u32 value);

As you see I wouldn't use a define for 4, because IMHO that hurts
readability. But I don't feel strong here.


> +#define MA35D1_PWM_MAX_COUNT           0xFFFF
> +#define MA35D1_PWM_TOTAL_CHANNELS      6

s/TOTAL/NUM/

> +
> +struct nuvoton_pwm {
> +	void __iomem *base;
> +	u64 clkrate;

clk_get_rate() returns an unsigned long value. Please stick to that for
.clkrate as there is no use to double the size of this struct on 32-bit
platforms just to store zeros and padding.

> +};
> +
> +static inline struct nuvoton_pwm *to_nuvoton_pwm(struct pwm_chip *chip)
> +{
> +	return pwmchip_get_drvdata(chip);
> +}

I slightly prefer nuvoton_pwm_from_chip() for this function's name to
have the same function prefix for all functions here.

> +
> +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct nuvoton_pwm *nvtpwm;
> +	u32 ch = pwm->hwpwm;
> +
> +	nvtpwm = to_nuvoton_pwm(chip);
> +	if (state->enabled) {
> +		u64 duty_cycles, period_cycles;
> +
> +		/* Calculate the duty and period cycles */
> +		duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> +						  state->duty_cycle, NSEC_PER_SEC);
> +		if (duty_cycles > MA35D1_PWM_MAX_COUNT)
> +			duty_cycles = MA35D1_PWM_MAX_COUNT;
> +
> +		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> +						    state->period, NSEC_PER_SEC);
> +		if (period_cycles > MA35D1_PWM_MAX_COUNT)
> +			period_cycles = MA35D1_PWM_MAX_COUNT;
> +
> +		/* Write the duty and period cycles to registers */
> +		writel(duty_cycles, MA35D1_PWM_CMPDAT0_ADDR(nvtpwm->base, ch));
> +		writel(period_cycles, MA35D1_PWM_PERIOD0_ADDR(nvtpwm->base, ch));
> +		/* Enable counter */
> +		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) | BIT(ch),
> +		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
> +		/* Enable output */
> +		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) | BIT(ch),
> +		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));

Can this result in glitches? E.g. is it possible to see a waveform with
the old period_cycles but the new duty_cycles output? If you switch
polarity, do you see the new settings with the wrong polarity for some
time? Setup polarity before enabling the counter and output? Maybe only
enable the counter when the output is enabled to be sure to emit the
first edge with the correct timing?

> +	} else {
> +		/* Disable counter */
> +		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) & ~BIT(ch),
> +		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
> +		/* Disable output */
> +		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) & ~BIT(ch),
> +		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));

Maybe add another wrapper for this kind of rmw operation. Also I suggest
to introduce a name for BIT(ch) here such that this can become:

	nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN, MA35D1_REG_PWM_POEN_EN(ch), 0);

(Assuming "EN0" is the name of the bit for channel 0 in
MA35D1_REG_PWM_POEN.)

> +	}
> +
> +	/* Set polarity state to register */
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) & ~BIT(ch),
> +		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
> +	else
> +		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) | BIT(ch),
> +		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));

You can skip setting up period if !state->enabled.

> +	return 0;
> +}
> +
> [...]
> +
> +static int nuvoton_pwm_probe(struct platform_device *pdev)
> +{
> +	struct pwm_chip *chip;
> +	struct nuvoton_pwm *nvtpwm;
> +	struct clk *clk;
> +	int ret;
> +
> +	chip = devm_pwmchip_alloc(&pdev->dev, MA35D1_PWM_TOTAL_CHANNELS, sizeof(*nvtpwm));
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	nvtpwm = to_nuvoton_pwm(chip);
> +
> +	nvtpwm->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(nvtpwm->base))
> +		return PTR_ERR(nvtpwm->base);
> +
> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
> +	if (IS_ERR(clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get the clock");

Please start all error messages with a capital letter and end them with
\n.

devm_clk_rate_exclusive_get(&pdev->dev, clk) here please.

> +
> +	nvtpwm->clkrate = clk_get_rate(clk);
> +	if (nvtpwm->clkrate > NSEC_PER_SEC)
> +		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clock out of range");

	return dev_err_probe(&pdev->dev, -EINVAL, "PWM clock out of range (%lu)\n", nvtpwm->clkrate);

> +
> +	chip->ops = &nuvoton_pwm_ops;
> +	chip->atomic = true;
> +
> +	ret = devm_pwmchip_add(&pdev->dev, chip);
> +	if (ret < 0)
> +		return dev_err_probe(&pdev->dev, ret, "unable to add pwm chip");
> +
> +	return 0;
> +}

Best regards
Uwe

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

Re: [PATCH v3 2/2] pwm: Add Nuvoton MA35D1 PWM controller support

[Chi-Wen Weng]

Hi Uwe,

Thanks for your reply.


On 2025/2/7 下午 05:28, Uwe Kleine-König wrote:
> Hello,
>
> On Fri, Dec 06, 2024 at 04:55:01PM +0800, Chi-Wen Weng wrote:
>> [...]
>> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
>> new file mode 100644
>> index 000000000000..380f17b20a3d
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-ma35d1.c
>> @@ -0,0 +1,179 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for the Nuvoton MA35D1 PWM controller
>> + *
>> + * Copyright (C) 2024 Nuvoton Corporation
>> + *               Chi-Wen Weng <cwweng@nuvoton.com>
> Please add some information here about the hardware. The idea is to get
> some info about the device's capabilities. Please stick to the format
> that many other drivers are using such that
>
> 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/pwm-ma35d1.c
>
> emits the info for your driver. Things to mention are: possible glitches
> during .apply(); behaviour of the pin when the PWM is disabled (constant
> signal? High-Z?)
>
> Also a link to a reference manual would be awesome.
Okay, I will add the information into driver.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/math64.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +
>> +/* The following are registers address offset for MA35D1 PWM controller */
>> +#define MA35D1_REG_PWM_CTL0            (0x00)
>> +#define MA35D1_REG_PWM_CNTEN           (0x20)
>> +#define MA35D1_REG_PWM_PERIOD0         (0x30)
>> +#define MA35D1_REG_PWM_CMPDAT0         (0x50)
>> +#define MA35D1_REG_PWM_WGCTL0          (0xB0)
>> +#define MA35D1_REG_PWM_POLCTL          (0xD4)
>> +#define MA35D1_REG_PWM_POEN            (0xD8)
>> +
>> +/* The following are register address of MA35D1 PWM controller */
>> +#define MA35D1_PWM_CH_REG_SIZE         4
>> +#define MA35D1_PWM_CMPDAT0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_CMPDAT0 + \
>> +					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
>> +#define MA35D1_PWM_CNTEN_ADDR(base)           ((base) + MA35D1_REG_PWM_CNTEN)
>> +#define MA35D1_PWM_PERIOD0_ADDR(base, ch)     ((base) + MA35D1_REG_PWM_PERIOD0 + \
>> +					       ((ch) * MA35D1_PWM_CH_REG_SIZE))
>> +#define MA35D1_PWM_POEN_ADDR(base)            ((base) + MA35D1_REG_PWM_POEN)
>> +#define MA35D1_PWM_POLCTL_ADDR(base)          ((base) + MA35D1_REG_PWM_POLCTL)
> I would drop the base part in these defines and add them to the above
> list sorted by address.
>
> So something like:
>
> #define MA35D1_REG_PWM_CTL0		0x00
> #define MA35D1_REG_PWM_CNTEN		0x20
> #define MA35D1_REG_PWM_PERIOD(ch)	(0x30 + 4 * (ch))
> #define MA35D1_REG_PWM_CMPDAT(ch)	(0x50 + 4 * (ch))
> #define MA35D1_REG_PWM_WGCTL0		0xB0
> #define MA35D1_REG_PWM_POLCTL		0xD4
> #define MA35D1_REG_PWM_POEN		0xD8
>
> To make up for the missing base, I'd create wrappers for readl and
> writel à la:
>
> 	static u32 nuvoton_pwm_readl(struct nuvoton_pwm *nvtpwm, unsigned int offset);
> 	static void nuvoton_pwm_writel(struct nuvoton_pwm *nvtpwm, unsigned int offset, u32 value);
>
> As you see I wouldn't use a define for 4, because IMHO that hurts
> readability. But I don't feel strong here.

It really can let my driver more readable. Thanks.


>
>> +#define MA35D1_PWM_MAX_COUNT           0xFFFF
>> +#define MA35D1_PWM_TOTAL_CHANNELS      6
> s/TOTAL/NUM/
Okay, I will modify it in next version.
>> +
>> +struct nuvoton_pwm {
>> +	void __iomem *base;
>> +	u64 clkrate;
> clk_get_rate() returns an unsigned long value. Please stick to that for
> .clkrate as there is no use to double the size of this struct on 32-bit
> platforms just to store zeros and padding.
Okay, I will modify it in next version.
>> +};
>> +
>> +static inline struct nuvoton_pwm *to_nuvoton_pwm(struct pwm_chip *chip)
>> +{
>> +	return pwmchip_get_drvdata(chip);
>> +}
> I slightly prefer nuvoton_pwm_from_chip() for this function's name to
> have the same function prefix for all functions here.
Okay, I will modify it in next version.
>> +
>> +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>> +			     const struct pwm_state *state)
>> +{
>> +	struct nuvoton_pwm *nvtpwm;
>> +	u32 ch = pwm->hwpwm;
>> +
>> +	nvtpwm = to_nuvoton_pwm(chip);
>> +	if (state->enabled) {
>> +		u64 duty_cycles, period_cycles;
>> +
>> +		/* Calculate the duty and period cycles */
>> +		duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
>> +						  state->duty_cycle, NSEC_PER_SEC);
>> +		if (duty_cycles > MA35D1_PWM_MAX_COUNT)
>> +			duty_cycles = MA35D1_PWM_MAX_COUNT;
>> +
>> +		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
>> +						    state->period, NSEC_PER_SEC);
>> +		if (period_cycles > MA35D1_PWM_MAX_COUNT)
>> +			period_cycles = MA35D1_PWM_MAX_COUNT;
>> +
>> +		/* Write the duty and period cycles to registers */
>> +		writel(duty_cycles, MA35D1_PWM_CMPDAT0_ADDR(nvtpwm->base, ch));
>> +		writel(period_cycles, MA35D1_PWM_PERIOD0_ADDR(nvtpwm->base, ch));
>> +		/* Enable counter */
>> +		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) | BIT(ch),
>> +		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
>> +		/* Enable output */
>> +		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) | BIT(ch),
>> +		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));
> Can this result in glitches? E.g. is it possible to see a waveform with
> the old period_cycles but the new duty_cycles output? If you switch
> polarity, do you see the new settings with the wrong polarity for some
> time? Setup polarity before enabling the counter and output? Maybe only
> enable the counter when the output is enabled to be sure to emit the
> first edge with the correct timing?

Yes, enable counter should be after the output is enabled. Will fix in 
next version.


>> +	} else {
>> +		/* Disable counter */
>> +		writel(readl(MA35D1_PWM_CNTEN_ADDR(nvtpwm->base)) & ~BIT(ch),
>> +		       MA35D1_PWM_CNTEN_ADDR(nvtpwm->base));
>> +		/* Disable output */
>> +		writel(readl(MA35D1_PWM_POEN_ADDR(nvtpwm->base)) & ~BIT(ch),
>> +		       MA35D1_PWM_POEN_ADDR(nvtpwm->base));
> Maybe add another wrapper for this kind of rmw operation. Also I suggest
> to introduce a name for BIT(ch) here such that this can become:
>
> 	nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN, MA35D1_REG_PWM_POEN_EN(ch), 0);
>
> (Assuming "EN0" is the name of the bit for channel 0 in
> MA35D1_REG_PWM_POEN.)
Okay, I will modify it next version.
>> +	}
>> +
>> +	/* Set polarity state to register */
>> +	if (state->polarity == PWM_POLARITY_NORMAL)
>> +		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) & ~BIT(ch),
>> +		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
>> +	else
>> +		writel(readl(MA35D1_PWM_POLCTL_ADDR(nvtpwm->base)) | BIT(ch),
>> +		       MA35D1_PWM_POLCTL_ADDR(nvtpwm->base));
> You can skip setting up period if !state->enabled.
Yes, it should be skip if !state->enabled.
>> +	return 0;
>> +}
>> +
>> [...]
>> +
>> +static int nuvoton_pwm_probe(struct platform_device *pdev)
>> +{
>> +	struct pwm_chip *chip;
>> +	struct nuvoton_pwm *nvtpwm;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	chip = devm_pwmchip_alloc(&pdev->dev, MA35D1_PWM_TOTAL_CHANNELS, sizeof(*nvtpwm));
>> +	if (IS_ERR(chip))
>> +		return PTR_ERR(chip);
>> +
>> +	nvtpwm = to_nuvoton_pwm(chip);
>> +
>> +	nvtpwm->base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(nvtpwm->base))
>> +		return PTR_ERR(nvtpwm->base);
>> +
>> +	clk = devm_clk_get_enabled(&pdev->dev, NULL);
>> +	if (IS_ERR(clk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(clk), "unable to get the clock");
> Please start all error messages with a capital letter and end them with
> \n.
>
> devm_clk_rate_exclusive_get(&pdev->dev, clk) here please.
Okay, I will modify it in next version.
>> +
>> +	nvtpwm->clkrate = clk_get_rate(clk);
>> +	if (nvtpwm->clkrate > NSEC_PER_SEC)
>> +		return dev_err_probe(&pdev->dev, -EINVAL, "pwm clock out of range");
> 	return dev_err_probe(&pdev->dev, -EINVAL, "PWM clock out of range (%lu)\n", nvtpwm->clkrate);
Okay, I will modify it in next version.
>> +
>> +	chip->ops = &nuvoton_pwm_ops;
>> +	chip->atomic = true;
>> +
>> +	ret = devm_pwmchip_add(&pdev->dev, chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(&pdev->dev, ret, "unable to add pwm chip");
>> +
>> +	return 0;
>> +}
> Best regards
> Uwe


Thanks.

Chi-Wen Weng