Re: [PATCH v6 2/3] pwm: add Andes PWM driver support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver fails to update the hardware PARK level to the inactive state when disabling the PWM channel, leaving the output stuck ON or at the wrong polarity.
--

commit 6823cc7ba3d3233c8090db2e53dc0a9b52b74a25
Author: Ben Zong-You Xie <ben717@andestech.com>

pwm: add Andes PWM driver support

Add a driver for the PWM controller found in Andes AE350 platforms and
QiLai SoCs.

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

[ ... ]

> +static int andes_pwm_config(struct pwm_chip *chip, unsigned int channel,
> +			    const struct pwm_state *state)
> +{

[ ... ]

> +	/*
> +	 * A zero-length phase means a 0% or 100% relative duty cycle, which the
> +	 * hardware cannot run. Emit the matching constant level by parking the
> +	 * channel: high_cycles == 0 stays low, low_cycles == 0 stays high.
> +	 */
> +	if (!high_cycles)
> +		return andes_pwm_park(chip, channel, false);
> +	if (!low_cycles)
> +		return andes_pwm_park(chip, channel, true);

[Severity: High]
When configuring a 100 percent duty cycle, this code sets the PARK bit to the
active state to emulate the output. If the channel is subsequently disabled,
could the output remain stuck ON?

> +
> +	/*
> +	 * If changing the clock source here, the output will not be the old one

[ ... ]

> +static int andes_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			   const struct pwm_state *state)
> +{
> +	unsigned int channel = pwm->hwpwm;
> +
> +	if (!state->enabled) {
> +		if (pwm->state.enabled)
> +			andes_pwm_enable(chip, channel, false);
> +
> +		return 0;
> +	}

[Severity: High]
Does this early return path disable the channel but fail to update the 
hardware's PARK bit to reflect the inactive state of the current polarity?

If the PWM was previously configured to 100 percent duty cycle where the PARK
bit was set active, it appears bypassing the PARK bit configuration on the
disable path leaves the output driven at the active level. 

Additionally, if the polarity changes while the channel is disabled, will the
hardware's inactive level fail to update?

> +
> +	return andes_pwm_config(chip, channel, state);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625-andes-pwm-v6-0-3aef11711017@andestech.com?part=2