Re: [PATCH v10 2/3] pwm: rp1: Add RP1 PWM controller driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] pwm: rp1: channel control register state loss across suspend
--

commit b77b6f077f5aa3ca8ba16d313a2e9952c97e47f7
Author: Naushir Patuck <naush@raspberrypi.com>

pwm: rp1: Add RP1 PWM controller driver
    
The Raspberry Pi RP1 southbridge features an embedded PWM controller with 4
output channels. This patch adds the supporting driver.

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

[ ... ]

> +static int rp1_pwm_request(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	u32 value, polarity;
> +
> +	/* init channel to reset defaults, preserving the polarity bit */
> +	value = readl(rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));
> +	polarity = FIELD_GET(RP1_PWM_CHAN_CTRL_POLARITY, value);
> +	value = u32_replace_bits(RP1_PWM_CHAN_CTRL_DEFAULT, polarity, RP1_PWM_CHAN_CTRL_POLARITY);
> +	writel(value, rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));

[Severity: Medium]
Will this channel control mode configuration be lost if the hardware loses power
or is reset during system suspend? 

The required MODE_TE_MS and FIFO_POP_MASK settings are initialized here, but
they do not appear to be restored anywhere on resume.

> +
> +	return 0;
> +}

[ ... ]

> +static int rp1_pwm_write_waveform(struct pwm_chip *chip,
> +				  struct pwm_device *pwm,
> +				  const void *_wfhw)
> +{
> +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> +	u32 value, ctrl;
> +
> +	/* set polarity */
> +	value = readl(rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));
> +	if (!wfhw->inverted_polarity)
> +		value &= ~RP1_PWM_CHAN_CTRL_POLARITY;
> +	else
> +		value |= RP1_PWM_CHAN_CTRL_POLARITY;
> +	writel(value, rp1->base + RP1_PWM_CHAN_CTRL(pwm->hwpwm));

[Severity: Medium]
If the channel control register loses its state across suspend, could this
read-modify-write operation preserve the corrupted state?

If a PWM consumer calls pwm_apply_state() upon resume, it would invoke
rp1_pwm_write_waveform() and read the potentially zeroed state, modify only the
polarity bit, and write the corrupted state back. This would cause the PWM
channel to fail to output correctly since the proper mode settings are missing.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ee0aa89faa14e0073f1271693bae28678652998b.1790606188.git.andrea.porta@suse.com?part=2