Add support for nuvoton ma35d1 pwm controller

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

Signed-off-by Chi-Wen Weng <[email protected]>

drivers/pwm/pwm-ma35d1.c new file +18 -0

+ */
+
+#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)

drivers/pwm/pwm-ma35d1.c new file +21 -0

+
+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;

drivers/pwm/pwm-ma35d1.c new file +14 -0

+	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);

Hello,

drivers/pwm/pwm-ma35d1.c line 6

Quoted 13 lines · Chi-Wen Weng

[...]

drivers/pwm/pwm-ma35d1.c new file +6 -0

new file mode 100644
index 000000000000..380f17b20a3d
@@ -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 <[email protected]>

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.

Quoted 18 lines

drivers/pwm/pwm-ma35d1.c new file +18 -0

+ */
+
+#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)

drivers/pwm/pwm-ma35d1.c line 34

Quoted 10 lines

drivers/pwm/pwm-ma35d1.c new file +10 -0

+
+/* 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.

drivers/pwm/pwm-ma35d1.c line 37

Quoted 3 lines

drivers/pwm/pwm-ma35d1.c new file +2 -0

+#define MA35D1_PWM_MAX_COUNT           0xFFFF
+#define MA35D1_PWM_TOTAL_CHANNELS      6

s/TOTAL/NUM/

drivers/pwm/pwm-ma35d1.c line 41

Quoted 4 lines

drivers/pwm/pwm-ma35d1.c new file +4 -0

+
+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.

drivers/pwm/pwm-ma35d1.c line 47

Quoted 6 lines

drivers/pwm/pwm-ma35d1.c new file +6 -0

+};
+
+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.

Quoted 29 lines

drivers/pwm/pwm-ma35d1.c new file +29 -0

+
+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 */

drivers/pwm/pwm-ma35d1.c line 77

Quoted 2 lines

drivers/pwm/pwm-ma35d1.c new file +2 -0

+		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?

drivers/pwm/pwm-ma35d1.c line 85

Quoted 7 lines

drivers/pwm/pwm-ma35d1.c new file +7 -0

+	} 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.)

drivers/pwm/pwm-ma35d1.c line 94

Quoted 9 lines

drivers/pwm/pwm-ma35d1.c new file +9 -0

+	}
+
+	/* 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.

Quoted 15 lines

drivers/pwm/pwm-ma35d1.c new file +14 -0

+	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);

drivers/pwm/pwm-ma35d1.c line 146

Quoted 10 lines

drivers/pwm/pwm-ma35d1.c new file +10 -0

+
+	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.

drivers/pwm/pwm-ma35d1.c line 150

Quoted 4 lines

drivers/pwm/pwm-ma35d1.c new file +4 -0

+
+	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);

Quoted 4 lines

drivers/pwm/pwm-ma35d1.c new file +4 -0

+
+	chip->ops = &nuvoton_pwm_ops;
+	chip->atomic = true;
+

drivers/pwm/pwm-ma35d1.c line 160

Quoted 6 lines

drivers/pwm/pwm-ma35d1.c new file +6 -0

+	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