[PATCH v3 2/2] pwm: Add Nuvoton MA35D1 PWM controller support
Chi-Wen Weng
cwweng.linux at gmail.com
Wed Feb 12 02:41:20 PST 2025
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 at 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
More information about the linux-arm-kernel
mailing list