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

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

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

Hello,

On Tue, Jul 21, 2026 at 02:23:08PM +0800, Chi-Wen Weng wrote:
> +static int nuvoton_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			     const struct pwm_state *state)
> +{
> +	struct nuvoton_pwm *nvtpwm = nuvoton_pwm_from_chip(chip);
> +	unsigned int ch = pwm->hwpwm;
> +	u64 duty_cycles, period_cycles;
> +	u64 divisor;
> +	u32 cmpdat, period, prescale;
> +	int ret;
> +
> +	if (!state->enabled) {
> +		nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POEN,
> +				MA35D1_PWM_POEN_EN(ch), 0);

Does this mean the output pin goes High-Z on disable?

> +		nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_CNTEN,
> +				MA35D1_PWM_CNTEN_EN(ch), 0);
> +
> +		return 0;
> +	}
> +
> +	prescale = nuvoton_pwm_get_prescale(nvtpwm, ch);
> +	divisor = (u64)NSEC_PER_SEC * (prescale + 1);
> +
> +	period_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> +					    state->period, divisor);
> +	if (!period_cycles)
> +		return -EINVAL;
> +
> +	if (period_cycles > MA35D1_PWM_MAX_CYCLES)
> +		period_cycles = MA35D1_PWM_MAX_CYCLES;
> +
> +	duty_cycles = mul_u64_u64_div_u64(nvtpwm->clkrate,
> +					  state->duty_cycle, divisor);
> +	if (duty_cycles > period_cycles)
> +		duty_cycles = period_cycles;
> +
> +	ret = nuvoton_pwm_config_channel(nvtpwm, ch);
> +	if (ret)
> +		return ret;
> +
> +	if (state->polarity == PWM_POLARITY_NORMAL)
> +		nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POLCTL,
> +				MA35D1_PWM_POLCTL_INV(ch), 0);
> +	else
> +		nuvoton_pwm_rmw(nvtpwm, MA35D1_REG_PWM_POLCTL,
> +				MA35D1_PWM_POLCTL_INV(ch),
> +				MA35D1_PWM_POLCTL_INV(ch));
> +
> +	/*
> +	 * In up-counting mode the counter counts from 0 to PERIOD inclusive.
> +	 * With zero point high and compare-up point low actions:
> +	 * - CMPDAT = 0 produces 0% duty.
> +	 * - CMPDAT > PERIOD produces 100% duty.
> +	 *
> +	 * period_cycles is limited to 0xffff, so PERIOD is at most 0xfffe
> +	 * and a 100% duty cycle can be represented by CMPDAT = 0xffff.
> +	 */
> +	period = period_cycles - 1;
> +	cmpdat = duty_cycles;
> +
> +	nuvoton_pwm_writel(nvtpwm, MA35D1_REG_PWM_PERIOD(ch), period);
> +	nuvoton_pwm_writel(nvtpwm, MA35D1_REG_PWM_CMPDAT(ch), cmpdat);

This is racy, right? (i.e. if a period end happens between these two
writes, we see (probably) one period with the new period length and the
old duty_cycle.

The result of the two questions should be a comment describing the
behaviour, otherwise the code looks fine on a quick glance.

Best regards
Uwe

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