Re: [PATCH v9 2/5] pwm: sun8i: Add H616 PWM support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] goto mixing with scoped_guard() cleanup helpers
--

commit d272817b7f4a414d55278ac7371a0119d48346da
Author: Richard Genoud <richard.genoud@bootlin.com>

pwm: sun8i: Add H616 PWM support

Add waveform support for the six-channel Allwinner H616 PWM controller.
Each pair of outputs shares a source mux, gate and power-of-two divider;
each channel has its own prescaler and period/duty registers. Model the
shared pair clocks in CCF and program the channel-local dividers directly.

> diff --git a/drivers/pwm/pwm-sun8i.c b/drivers/pwm/pwm-sun8i.c
> new file mode 100644
> index 0000000000000..7e6c39815c183
> --- /dev/null
> +++ b/drivers/pwm/pwm-sun8i.c
[ ... ]
> +static int sun8i_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> +	struct sun8i_pwm_channel *chan = &sun8i_chip->channels[pwm->hwpwm];
> +	unsigned int idx = pwm->hwpwm;
> +	struct clk_hw *parent = sun8i_chip->pairs[SUN8I_PWM_PAIR_IDX(idx)].hw;
> +	bool was_enabled;
> +	int ret;
> +
> +	scoped_guard(spinlock_irqsave, &sun8i_chip->lock) {
> +		was_enabled =
> +			sun8i_pwm_channel_is_enabled_locked(sun8i_chip, idx);
> +	}
> +
> +	chan->pair_clk = clk_hw_get_clk(parent, NULL);
> +	if (IS_ERR(chan->pair_clk)) {
> +		ret = PTR_ERR(chan->pair_clk);
> +		goto err_clear_clock;
> +	}

[Severity: Low]
This isn't a bug, but does this mix goto-based error handling and scope-based
cleanup helpers?

The cleanup subsystem guidelines recommend avoiding mixing goto labels with
cleanup helpers like scoped_guard() in the same function. Could the resources
that need goto cleanup be converted to use scope-based cleanup, or the
scoped_guard() converted to regular locking?

[ ... ]
> +static int sun8i_pwm_prepare_update(struct pwm_chip *chip, unsigned int idx,
> +				    const struct sun8i_pwm_waveform *wfhw,
> +				  struct sun8i_pwm_waveform *old)
> +{
> +	struct sun8i_pwm_chip *sun8i_chip = sun8i_pwm_from_chip(chip);
> +	struct sun8i_pwm_channel *chan = &sun8i_chip->channels[idx];
> +	unsigned long rate, bus_rate;
> +	bool had_exclusive, stop;
> +	u32 pccr;
> +	int ret, restore_ret;
> +
> +	scoped_guard(spinlock_irqsave, &sun8i_chip->lock)
> +		had_exclusive = chan->rate_exclusive;
> +
> +	if (!had_exclusive) {
> +		ret = clk_rate_exclusive_get(chan->pair_clk);
> +		if (ret)
> +			return ret;
> +		scoped_guard(spinlock_irqsave, &sun8i_chip->lock)
> +			chan->rate_exclusive = true;
> +	}
> +
> +	/* Sample the old rate only after excluding other rate changes. */
> +	rate = sun8i_pwm_get_pair_rate(sun8i_chip, idx);
> +	if (!rate || rate > U32_MAX) {
> +		ret = -ERANGE;
> +		goto put_rate;
> +	}

[Severity: Low]
Is it intentional to mix goto-based cleanup and scope-based cleanup helpers
here?

Similar to sun8i_pwm_request(), this function mixes scoped_guard() with goto
error handling paths, which goes against the cleanup subsystem guidelines.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-h616-pwm-v8-v9-0-37bec6201ff5@gmail.com?part=2