diff mbox series

[RESEND,v2,2/2] pwm: Add Nuvoton MA35D1 PWM controller support

Message ID 20241024104309.169510-3-cwweng.linux@gmail.com
State New
Headers show
Series Add support for nuvoton ma35d1 pwm controller | expand

Commit Message

Chi-Wen Weng Oct. 24, 2024, 10:43 a.m. UTC
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 | 169 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 179 insertions(+)
 create mode 100644 drivers/pwm/pwm-ma35d1.c

Comments

Trevor Gamblin Nov. 20, 2024, 4:49 p.m. UTC | #1
Hello,

On 2024-10-24 06:43, 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>
> ---
>   drivers/pwm/Kconfig      |   9 +++
>   drivers/pwm/Makefile     |   1 +
>   drivers/pwm/pwm-ma35d1.c | 169 +++++++++++++++++++++++++++++++++++++++
>   3 files changed, 179 insertions(+)
>   create mode 100644 drivers/pwm/pwm-ma35d1.c
I don't see a MAINTAINERS entry? That needs to be added in the bindings 
patch first, and then it should be updated to list this driver file.
>
> 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..0c4eec4a0b07
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c
> @@ -0,0 +1,169 @@
> +// 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/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/math64.h>
These should be organized alphabetically.
> +
> +/* The following are registers for PWM controller */
> +#define REG_PWM_CTL0            (0x00)
> +#define REG_PWM_CNTEN           (0x20)
> +#define REG_PWM_PERIOD0         (0x30)
> +#define REG_PWM_CMPDAT0         (0x50)
> +#define REG_PWM_WGCTL0          (0xB0)
> +#define REG_PWM_POLCTL          (0xD4)
> +#define REG_PWM_POEN            (0xD8)

These too, I think - it will make it more readable for others.

You should also prefix all of your macros to be more explicit about 
their use, e.g. MA35D1_REG_PWM_CTL0. That way it's clearer that they're 
specific to this driver and not from elsewhere.

> +
> +#define PWM_TOTAL_CHANNELS      6
> +#define PWM_CH_REG_SIZE         4
And these.
> +
> +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;
> +	unsigned int 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 > 0xFFFF)
> +			duty_cycles = 0xFFFF;
It would be good to create a macro for this value 0xFFFF, e.g. 
MA35D1_MAX_PWM_RATE.
> +
> +		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> +						    state->period, NSEC_PER_SEC);
> +		if (period_cycles > 0xFFFF)
> +			period_cycles = 0xFFFF;
> +
> +		/* Write the duty and period cycles to registers */
> +		writel(duty_cycles, nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));
> +		writel(period_cycles, nvtpwm->base + REG_PWM_PERIOD0 + (ch * PWM_CH_REG_SIZE));

Since you are using things like

nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE)

and similar so frequently, I suggest creating more macros for these 
sorts of accesses to improve readability, e.g.

#define MA35D1_PWM_CMPDAT0_ADDR(base, ch) 	base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));

or even

#define MA35D1_PWM_CMPDAT0_ADDR(nvtpwm, ch) 	nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));

and then use those instead.

> +		/* Enable counter */
> +		writel(readl(nvtpwm->base + REG_PWM_CNTEN) | BIT(ch),
> +		       nvtpwm->base + REG_PWM_CNTEN);
Same for cases like this.
> +		/* Enable output */
> +		writel(readl(nvtpwm->base + REG_PWM_POEN) | BIT(ch),
> +		       nvtpwm->base + REG_PWM_POEN);
> +	} else {
> +		/* Disable counter */
> +		writel(readl(nvtpwm->base + REG_PWM_CNTEN) & ~BIT(ch),
> +		       nvtpwm->base + REG_PWM_CNTEN);
> +		/* Disable output */
> +		writel(readl(nvtpwm->base + REG_PWM_POEN) & ~BIT(ch),
> +		       nvtpwm->base + REG_PWM_POEN);
> +	}
> +
> +	/* Set polarity state to register */
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		writel(readl(nvtpwm->base + REG_PWM_POLCTL) & ~BIT(ch),
> +		       nvtpwm->base + REG_PWM_POLCTL);
> +	else
> +		writel(readl(nvtpwm->base + REG_PWM_POLCTL) | BIT(ch),
> +		       nvtpwm->base + REG_PWM_POLCTL);
> +
> +	return 0;
> +}
> +
> +static int nuvoton_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 struct pwm_state *state)
> +{
> +	struct nuvoton_pwm *nvtpwm;
> +	unsigned int duty_cycles, period_cycles, cnten, outen, polarity;
> +	unsigned int ch = pwm->hwpwm;
> +
> +	nvtpwm = to_nuvoton_pwm(chip);
> +
> +	cnten = readl(nvtpwm->base + REG_PWM_CNTEN);
> +	outen = readl(nvtpwm->base + REG_PWM_POEN);
> +	duty_cycles = readl(nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));
> +	period_cycles = readl(nvtpwm->base + REG_PWM_PERIOD0 + (ch * PWM_CH_REG_SIZE));
> +	polarity = readl(nvtpwm->base + REG_PWM_POLCTL) & 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, 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;
> +}

As a final note, please be aware that there is a change to the PWM 
subsystem coming in 6.13. You don't need to do anything now, but it's 
worth considering how the driver might change once that hits mainline.

You can find the changes here: 
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git/log/?h=pwm/duty_offset 
.

> +
> +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");
Uwe Kleine-König Nov. 20, 2024, 10:03 p.m. UTC | #2
hello,

On Wed, Nov 20, 2024 at 11:49:48AM -0500, Trevor Gamblin wrote:
> On 2024-10-24 06:43, Chi-Wen Weng wrote:
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pwm.h>
> > +#include <linux/io.h>
> > +#include <linux/clk.h>
> > +#include <linux/math64.h>
> These should be organized alphabetically.
> > +
> > +/* The following are registers for PWM controller */
> > +#define REG_PWM_CTL0            (0x00)
> > +#define REG_PWM_CNTEN           (0x20)
> > +#define REG_PWM_PERIOD0         (0x30)
> > +#define REG_PWM_CMPDAT0         (0x50)
> > +#define REG_PWM_WGCTL0          (0xB0)
> > +#define REG_PWM_POLCTL          (0xD4)
> > +#define REG_PWM_POEN            (0xD8)
> 
> These too, I think - it will make it more readable for others.

Keeping the registers in address order is the usual thing to do, so
please keep the order of these.

Otherwise I agree to Trevor's comments. Thanks for taking the time to
look at this patch.

Best regards
Uwe
diff mbox series

Patch

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..0c4eec4a0b07
--- /dev/null
+++ b/drivers/pwm/pwm-ma35d1.c
@@ -0,0 +1,169 @@ 
+// 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/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/math64.h>
+
+/* The following are registers for PWM controller */
+#define REG_PWM_CTL0            (0x00)
+#define REG_PWM_CNTEN           (0x20)
+#define REG_PWM_PERIOD0         (0x30)
+#define REG_PWM_CMPDAT0         (0x50)
+#define REG_PWM_WGCTL0          (0xB0)
+#define REG_PWM_POLCTL          (0xD4)
+#define REG_PWM_POEN            (0xD8)
+
+#define PWM_TOTAL_CHANNELS      6
+#define PWM_CH_REG_SIZE         4
+
+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;
+	unsigned int 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 > 0xFFFF)
+			duty_cycles = 0xFFFF;
+
+		period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
+						    state->period, NSEC_PER_SEC);
+		if (period_cycles > 0xFFFF)
+			period_cycles = 0xFFFF;
+
+		/* Write the duty and period cycles to registers */
+		writel(duty_cycles, nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));
+		writel(period_cycles, nvtpwm->base + REG_PWM_PERIOD0 + (ch * PWM_CH_REG_SIZE));
+		/* Enable counter */
+		writel(readl(nvtpwm->base + REG_PWM_CNTEN) | BIT(ch),
+		       nvtpwm->base + REG_PWM_CNTEN);
+		/* Enable output */
+		writel(readl(nvtpwm->base + REG_PWM_POEN) | BIT(ch),
+		       nvtpwm->base + REG_PWM_POEN);
+	} else {
+		/* Disable counter */
+		writel(readl(nvtpwm->base + REG_PWM_CNTEN) & ~BIT(ch),
+		       nvtpwm->base + REG_PWM_CNTEN);
+		/* Disable output */
+		writel(readl(nvtpwm->base + REG_PWM_POEN) & ~BIT(ch),
+		       nvtpwm->base + REG_PWM_POEN);
+	}
+
+	/* Set polarity state to register */
+	if (state->polarity == PWM_POLARITY_NORMAL)
+		writel(readl(nvtpwm->base + REG_PWM_POLCTL) & ~BIT(ch),
+		       nvtpwm->base + REG_PWM_POLCTL);
+	else
+		writel(readl(nvtpwm->base + REG_PWM_POLCTL) | BIT(ch),
+		       nvtpwm->base + REG_PWM_POLCTL);
+
+	return 0;
+}
+
+static int nuvoton_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+				 struct pwm_state *state)
+{
+	struct nuvoton_pwm *nvtpwm;
+	unsigned int duty_cycles, period_cycles, cnten, outen, polarity;
+	unsigned int ch = pwm->hwpwm;
+
+	nvtpwm = to_nuvoton_pwm(chip);
+
+	cnten = readl(nvtpwm->base + REG_PWM_CNTEN);
+	outen = readl(nvtpwm->base + REG_PWM_POEN);
+	duty_cycles = readl(nvtpwm->base + REG_PWM_CMPDAT0 + (ch * PWM_CH_REG_SIZE));
+	period_cycles = readl(nvtpwm->base + REG_PWM_PERIOD0 + (ch * PWM_CH_REG_SIZE));
+	polarity = readl(nvtpwm->base + REG_PWM_POLCTL) & 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, 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");