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

["Uwe Kleine-König" <ukleinek@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]