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

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

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@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