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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The driver unconditionally clobbers PWM waveform generator and control registers in probe(), breaking seamless handover for active PWM channels.
--

commit 1c5d12cc5f94bf9ab4e36bc34c306ce883661f7e
Author: Chi-Wen Weng <cwweng@nuvoton.com>

pwm: Add Nuvoton MA35D1 PWM controller support

Add a PWM framework driver for the Nuvoton MA35D1 PWM controller.

> diff --git a/drivers/pwm/pwm-ma35d1.c b/drivers/pwm/pwm-ma35d1.c
> new file mode 100644
> index 0000000000000..c07eedeca035b
> --- /dev/null
> +++ b/drivers/pwm/pwm-ma35d1.c

[ ... ]

> +static void nuvoton_pwm_init(struct nuvoton_pwm *nvtpwm)
> +{
> +	u32 ctl1_mask = 0;
> +	u32 wgctl0_mask = 0;
> +	u32 wgctl0_val = 0;
> +	u32 wgctl1_mask = 0;
> +	u32 wgctl1_val = 0;
> +	int ch;
> +
> +	for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch++) {
> +		/* CNTTYPEn = 00: up counter type */
> +		ctl1_mask |= nuvoton_pwm_ctl1_cnttype_mask(ch);
> +
> +		/* CNTMODEn = 0: auto-reload mode */
> +		ctl1_mask |= MA35D1_PWM_CTL1_CNTMODE_MASK(ch);
> +
> +		/* ZPCTLn = 10: output high at zero point */
> +		wgctl0_mask |= nuvoton_pwm_wgctl_zero_mask(ch);
> +		wgctl0_val |= MA35D1_PWM_WGCTL_ZERO_HIGH(ch);
> +
> +		/* PRDPCTLn = 00: do nothing at period point */
> +		wgctl0_mask |= nuvoton_pwm_wgctl_period_mask(ch);
> +
> +		/* CMPUCTLn = 01: output low at compare up point */
> +		wgctl1_mask |= nuvoton_pwm_wgctl_cmp_up_mask(ch);
> +		wgctl1_val |= MA35D1_PWM_WGCTL_CMP_UP_LOW(ch);
> +
> +		/* CMPDCTLn = 00: do nothing at compare down point */
> +		wgctl1_mask |= nuvoton_pwm_wgctl_cmp_down_mask(ch);
> +	}
> +
> +	for (ch = 0; ch < MA35D1_PWM_NUM_CHANNELS; ch += 2) {
> +		/* OUTMODEn = 0: independent mode */
> +		ctl1_mask |= MA35D1_PWM_CTL1_OUTMODE_MASK(ch);
> +	}
> +
> +	nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CTL1, ctl1_mask, 0);
> +	nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL0,
> +			wgctl0_mask, wgctl0_val);
> +	nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_WGCTL1,
> +			wgctl1_mask, wgctl1_val);
> +}

[ ... ]

> +static int nuvoton_pwm_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	if (nvtpwm->clkrate > NSEC_PER_SEC)
> +		return dev_err_probe(dev, -EINVAL,
> +				     "PWM clock out of range (%lu)\n",
> +				     nvtpwm->clkrate);
> +
> +	nuvoton_pwm_init(nvtpwm);

[Severity: Medium]
Will this unconditionally clobber the waveform generator and control
registers for all channels during probe()?

If the bootloader has already enabled a PWM channel (such as for a regulator
or display backlight) with a different waveform configuration, altering these
global control registers while the channel is running might corrupt its output.

Also, since nuvoton_pwm_get_state() relies on PERIOD and CMPDAT values,
altering their hardware interpretation here means the PWM core will inherit
an incorrect initial state.

Should the driver check if channels are already active via CNTEN and POEN
before modifying their configuration to allow for a seamless handover?

> +
> +	chip->ops = &nuvoton_pwm_ops;
> +	chip->atomic = true;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617025925.2539334-1-cwweng.linux@gmail.com?part=2