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

["Uwe Kleine-König" <u.kleine-koenig@baylibre.com>]

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

Hello Andrea,

On Thu, Jun 04, 2026 at 03:39:44PM +0200, Andrea della Porta wrote:
> On 15:47 Wed 03 Jun     , sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
> > - [High] Missing `pwmchip_remove()` on the `of_syscon_register_regmap()` error path leaves a dangling PWM chip.
> > - [High] When disabling a PWM channel, the driver fails to clear the polarity bit, causing the output to erroneously remain HIGH if it was previously inverted.
> > - [High] Registering a devm-managed regmap with the permanent syscon registry creates a teardown asymmetry.
> > - [High] Hardware channel control registers (`CHAN_CTRL`) are not restored across system suspend/resume.
> > - [Medium] Non-monotonic period limit checking in `rp1_pwm_round_waveform_tohw()` causes larger requested periods to result in smaller hardware periods.
> > - [Medium] Calculating `period_length_ns` from hardware state suffers from a 32-bit integer overflow before casting to u64, resulting in a zero period.
> > - [Medium] Calculating inverted polarity duty length underflows if the hardware is configured for a 100% duty cycle.
> > - [Medium] Returning a positive integer (1) from `round_waveform_tohw` incorrectly propagates as an error.
> > --
> > 
> > commit 90d2019e94496c6949f2d04c2605c6a197b16d6a
> > 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, alongside an RPM interface to read the fan
> > speed on the Raspberry Pi 5. Add the supporting driver.
> > 
> > > diff --git a/drivers/pwm/pwm-rp1.c b/drivers/pwm/pwm-rp1.c
> > > new file mode 100644
> > > index 0000000000000..052c3ae770569
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-rp1.c
> > [ ... ]
> > > +static int rp1_pwm_round_waveform_tohw(struct pwm_chip *chip,
> > > +				       struct pwm_device *pwm,
> > > +				       const struct pwm_waveform *wf,
> > > +				       void *_wfhw)
> > > +{
> > > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > > +	u64 period_ticks, duty_ticks, offset_ticks;
> > > +	struct rp1_pwm_waveform *wfhw = _wfhw;
> > > +	u64 clk_rate = rp1->clk_rate;
> > > +	int ret = 0;
> > > +
> > > +	if (!wf->period_length_ns) {
> > > +		wfhw->enabled = false;
> > > +		return 0;
> > > +	}
> > > +
> > > +	period_ticks = mul_u64_u64_div_u64(wf->period_length_ns, clk_rate, NSEC_PER_SEC);
> > > +
> > > +	/*
> > > +	 * The period is limited to U32_MAX - 1
> > > +	 * to allow 100% duty cycle.
> > > +	 */
> > > +	if (period_ticks > U32_MAX) {
> > > +		period_ticks = U32_MAX - 1;
> > 
> > [Severity: Medium]
> > Does this limit check break monotonicity for large periods?
> > 
> > If the requested period translates to U32_MAX, this condition is false and
> > the period becomes U32_MAX - 1.
> > 
> > If it translates to U32_MAX + 1, it becomes U32_MAX - 1, which is then
> > subtracted by 1 later, yielding U32_MAX - 2. This makes a larger requested
> > period result in a smaller actual hardware period.
> 
> Ack. I will set period_ticks = U32_MAX in that if.
> 
> > 
> > > +	} else if (!period_ticks) {
> > > +		period_ticks = 2;
> > > +		ret = 1;
> > > +	}
> > 
> > [Severity: Medium]
> > Is returning a positive integer here intentional?
> > 
> > The PWM core subsystem expects round_waveform_tohw callbacks to return 0 on
> > success or a negative error code on failure. Returning 1 could propagate as
> > an error up to consumer drivers, breaking operation for very small periods.
> 
> It should return 1 in case the if any value is rounded, which in this case 
> is the period.

Ack. However it's a bit strange that you fixup period_ticks = 0 to 2
while 1 is kept as is. Either using 1 is invalid, then it should be
updated to two, too. Or it gives a shorter period than 2, then 0 should
be fixed up to 1 only.

> > > +	duty_ticks = mul_u64_u64_div_u64(wf->duty_length_ns, clk_rate, NSEC_PER_SEC);
> > [ ... ]
> > > +static int rp1_pwm_round_waveform_fromhw(struct pwm_chip *chip,
> > > +					 struct pwm_device *pwm,
> > > +					 const void *_wfhw,
> > > +					 struct pwm_waveform *wf)
> > > +{
> > > +	struct rp1_pwm *rp1 = pwmchip_get_drvdata(chip);
> > > +	const struct rp1_pwm_waveform *wfhw = _wfhw;
> > > +	u64 clk_rate = rp1->clk_rate;
> > > +	u32 ticks;
> > > +
> > > +	*wf = (struct pwm_waveform){ };
> > > +
> > > +	if (!wfhw->enabled)
> > > +		return 0;
> > > +
> > > +	wf->period_length_ns = DIV_ROUND_UP_ULL((u64)(wfhw->period_ticks + 1) * NSEC_PER_SEC,
> > > +						clk_rate);
> > 
> > [Severity: Medium]
> > Could this encounter a 32-bit integer overflow?
> > 
> > Since wfhw->period_ticks is a 32-bit integer, the addition of 1 happens in
> > 32-bit space before the cast to u64.
> > 
> > If the hardware register was configured to 0xffffffff by the bootloader for
> > a 100% duty cycle, 0xffffffff + 1 wraps around to 0, producing a period of 0.
> > 
> > Should the cast be placed before the addition?
> 
> Ack. 0xffffffff should be deemed as a non valid value though.
> 
> > 
> > > +
> > > +	if (!wfhw->inverted_polarity) {
> > > +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)wfhw->duty_ticks * NSEC_PER_SEC,
> > > +						      clk_rate);
> > > +	} else {
> > > +		ticks = wfhw->period_ticks + 1 - wfhw->duty_ticks;
> > > +		wf->duty_length_ns = DIV_ROUND_UP_ULL((u64)ticks * NSEC_PER_SEC, clk_rate);
> > 
> > [Severity: Medium]
> > Will this subtraction underflow if the hardware is configured for a 100%
> > duty cycle?
> > 
> > The driver allows duty_ticks to exceed period_ticks + 1 to achieve a 100%
> > duty cycle.
> 
> The driver is not allowing this since tohw() has this: 
> 
> duty_ticks = min(duty_ticks, period_ticks);
> 
> Anyway, if fromhw() is called first and the bootloader has configured 0xffffffff
> in the period register, should it returns an error? The period should always be
> at most U32_MAX-1.

I'm not sure if this triggers a PWM_DEBUG check, but I'd say the right
thing is to let fromhw() report what the actual setting even if tohw()
will never result in that setting.

Thinking again, the tohw() callback could be a bit more clever and also
use period_ticks = 0xffffffff, as this fine if duty_ticks is less than
this value and if duty_ticks = period_ticks = 0xffffffff you can still
configure the hardware using period_ticks = 0xfffffffe to achieve the
100% relative dutycycle. (But keeping the current behaviour is fine for
me, too.)

> > > +		wf->duty_offset_ns = wf->period_length_ns - wf->duty_length_ns;
> > > +	}
> > > +
> > > +	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;
> > > +
> > > +	/* early exit if disabled */
> > > +	regmap_read(rp1->regmap, RP1_PWM_GLB_CTRL, &ctrl);
> > > +	if (!wfhw->enabled) {
> > > +		ctrl &= ~RP1_PWM_GLB_CTRL_CHANNEL_ENABLE(pwm->hwpwm);
> > > +		goto exit_disable;
> > > +	}
> > 
> > [Severity: High]
> > Does skipping the polarity update here cause the output to erroneously
> > remain HIGH while disabled?
> > 
> > The early exit skips the read-modify-write block that updates
> > RP1_PWM_CHAN_CTRL_POLARITY.
> > 
> > The driver notes state that when disabled, the output is driven to 1 if
> > polarity is inversed. If a channel previously configured with inverted
> > polarity is disabled, it seems it will continually output a HIGH signal,
> > violating the PWM framework expectation.
> 
> I'm not sure whether an inverted polarity pin shoudl stay low when disabled.
> After all, the inactive state for a reversed pin is high.

Sashiko's concern is correctly stated, if you go from

	polarity = inversed, enabled

to

	polarity = normal, disabled

the output stays high, which is active for polarity = normal.

However the behaviour of a disabled PWM isn't specified, so any
behaviour is fine, the only objective is to save power. And if the
consumer relies on a constant inactive output, it's supposed to not
disable it.

For me both behaviours are fine. Making the hardware emit the inactive
level might prevent a surprise if the consumer isn't aware of the
missing guarantee, but being lazy and so surprise the consumer is also
fine as this might uncover that wrong assumption and allow the consumer
to be fixed.

(And not all PWM implementations allow to configure the output level, so
a guarantee cannot be given. Some go to 0 irrespective of the configured
polarity, some go to High-Z.)

> > [Severity: High]
> > Also, does registering a devm-managed regmap with the permanent syscon
> > registry create a teardown asymmetry?
> > 
> > The regmap is allocated with devm_regmap_init_mmio(). If the underlying
> > device is hotplug removed, devres will automatically free the regmap memory.
> > 
> > However, the syscon framework does not provide an unregister function,
> > leaving it with a dangling pointer in its static list that could cause a
> > use-after-free upon subsequent lookups.
> 
> The device is not hotpluggable and the driver cannot be unloaded/unbounded, so
> there should be no dangling pointer around.

ack.

Best regards
Uwe

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