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

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

Uwe Kleine-König 於 2026/7/16 下午 11:29 寫道:
> Hello,
>
> On Wed, Jun 17, 2026 at 10:59:25AM +0800, Chi-Wen Weng wrote:
>> +#include <linux/bits.h>
>> +#include <linux/clk.h>
>> +#include <linux/io.h>
>> +#include <linux/math64.h>
>> +#include <linux/mod_devicetable.h>
> Please don't include that file, <linux/platform_device.h> should pull in
> the things you need from that file.
>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pwm.h>
>> +
>> +#define MA35D1_REG_PWM_CTL0			0x00
>> +#define MA35D1_REG_PWM_CTL1			0x04
>> +#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_WGCTL1			0xb4
>> +#define MA35D1_REG_PWM_POLCTL			0xd4
>> +#define MA35D1_REG_PWM_POEN			0xd8
>> +
>> +#define MA35D1_PWM_CTL1_CNTMODE_MASK(ch)	BIT(16 + (ch))
>> +#define MA35D1_PWM_CTL1_OUTMODE_MASK(ch)	BIT(24 + ((ch) / 2))
>> +
>> +#define MA35D1_PWM_WGCTL_ACTION_MASK		0x3
>> +#define MA35D1_PWM_WGCTL_ACTION_LOW		1
>> +#define MA35D1_PWM_WGCTL_ACTION_HIGH		2
> If you make this:
>
> 	#define MA35D1_PWM_WGCTL_ACTION(ch)		GENMASK(2 * (ch) + 2, 2 * (ch))
> 	#define MA35D1_PWM_WGCTL_ACTION_LOW		1
> 	#define MA35D1_PWM_WGCTL_ACTION_HIGH		2
>
> you can drop the static inlines below.
>
>> +
>> +#define MA35D1_PWM_WGCTL_ZERO_HIGH(ch)		\
>> +	(MA35D1_PWM_WGCTL_ACTION_HIGH << (2 * (ch)))
>> +#define MA35D1_PWM_WGCTL_CMP_UP_LOW(ch)		\
>> +	(MA35D1_PWM_WGCTL_ACTION_LOW << (2 * (ch)))
>> +
>> +#define MA35D1_PWM_CNTEN_EN(ch)			BIT(ch)
>> +#define MA35D1_PWM_POEN_EN(ch)			BIT(ch)
>> +#define MA35D1_PWM_POLCTL_INV(ch)		BIT(ch)
>> +
>> +#define MA35D1_PWM_MAX_CMPDAT			0xffff
>> +#define MA35D1_PWM_MAX_PERIOD			0xfffe
>> +#define MA35D1_PWM_MAX_PERIOD_CYCLES		(MA35D1_PWM_MAX_PERIOD + 1)
> This is irritating with similar names and different values/semantic.
>
>> +#define MA35D1_PWM_NUM_CHANNELS			6
>> +
>> [...]
>> +static int nuvoton_pwm_probe(struct platform_device *pdev)
>> +{
>> [...]
>> +	nuvoton_pwm_init(nvtpwm);
> This clobbers what the hardware is doing. The idea here is to not modify
> the hardware settings at probe time to keep e.g. a backlight configured
> as it was setup by the bootloader and only modify on explicit calls to
> .apply().
>
>> +
>> +	chip->ops = &nuvoton_pwm_ops;
>> +	chip->atomic = true;
>> +
>> +	ret = devm_pwmchip_add(dev, chip);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Unable to add PWM chip\n");
>> +
>> +	return 0;
>> +}
> Best regards
> Uwe

Hi Uwe,

Thanks for the review.

On the include, I will drop <linux/mod_devicetable.h> as
<linux/platform_device.h> already provides what is needed here.

For the WGCTL helpers, I will rework this to avoid the extra static inline
helpers and compute the 2-bit action field mask/value locally when 
configuring
a channel. The WGCTL action field is 2 bits wide, so I will use the bit 
range
[2 * ch + 1 : 2 * ch] for the mask.

I will also rename the maximum value definitions to make the semantics 
clearer.
The intent is to keep PERIOD below the 16-bit register field maximum so that
CMPDAT can be programmed greater than PERIOD to generate a 100% duty cycle.

Most importantly, you are right about the probe-time initialization. The 
driver
should not reconfigure CTL1/WGCTL for all channels during probe, as that can
disturb a PWM output already configured and enabled by firmware, such as a
backlight.

In the next version I will remove the controller initialization from 
probe().
Instead, the driver will configure only the channel being changed from the
.apply() callback. The disable path will only clear POENn and CNTENn and 
will
not touch the waveform/control configuration.

Best regards,
Chi-Wen