[PATCH RESEND v2 2/2] pwm: Add Nuvoton MA35D1 PWM controller support
Chi-Wen Weng
cwweng.linux at gmail.com
Thu Dec 5 21:56:11 PST 2024
Hi Trevor,
Thanks for your suggestions.
On 2024/11/21 上午 12:49, Trevor Gamblin wrote:
> 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 at 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 at 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 at nuvoton.com>");
>> +MODULE_DESCRIPTION("Nuvoton MA35D1 PWM driver");
>> +MODULE_LICENSE("GPL");
Per Uwe's commet, I will keep the registers in address order.
Otherwise I will modify in next version.
Thanks.
Chi-Wen Weng
More information about the linux-arm-kernel
mailing list