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

[Chi-Wen Weng <cwweng.linux@gmail.com>]

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@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");

Per Uwe's commet, I will keep the registers in address order.

Otherwise I will modify in next version.


Thanks.

Chi-Wen Weng