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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The driver fails to update the inactive output state (PARK bit) when disabling the PWM channel, ignoring polarity changes and leaving the output at the active level if previously at 100% duty cycle.
- [High] Emulating 0% or 100% duty cycles leaves RELOAD registers stale and breaks `pwm_get_state` reporting by corrupting polarity, enabled status, and duty cycle calculations.
--

--- Patch [2]: [PATCH v7 2/3] pwm: add Andes PWM driver support ---
commit 76e1cbe61be3ddd82abebdaef695376f3fc6a571
Author: Ben Zong-You Xie <[email protected]>

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..8dafce7c467c1
> --- /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]
Does using the PARK bit to emulate 100% and 0% duty cycles cause incorrect
states to be reported by andes_pwm_get_state() later?

If a channel is emulating a 100% duty cycle, andes_pwm_park() modifies the
PARK bit and disables the channel without updating the RELOAD registers. 

When andes_pwm_get_state() is later called, it uses the PARK bit to deduce
polarity, which incorrectly evaluates to PWM_POLARITY_INVERSED. Additionally,
the enabled status will be evaluated as false since the channel was physically
disabled, and the unupdated RELOAD register will cause stale period and duty
cycle values to be returned.

[ ... ]
> +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]
When disabling the PWM channel, does the driver need to update the PARK bit
to ensure the output goes to the correct inactive state based on polarity?

If a channel is configured to a 100% duty cycle with NORMAL polarity,
andes_pwm_config() will have set the PARK bit to 1. If the PWM is
subsequently disabled, andes_pwm_apply() clears the PWM enable bit but does
not reset the PARK bit. 

Since a disabled channel continues to drive the PARK level, might this leave
the disabled channel driving the active state (HIGH) instead of the expected
inactive state (LOW)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2