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

[Andrea della Porta <andrea.porta@suse.com>]

Hi Uwe,

On 23:28 Thu 04 Jun     , Uwe Kleine-König wrote:
> 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.

<...snip...>

> > > > +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.

Right. The condition should be: (period_ticks < 2).

> 
> > > > +	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.

Perfect, so the current behavior is fine.

> 
> 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.)

I think this is what it's currently doing in this patch iteration. I stand
corrected here when I said that period_ticks should be at max U32_MAX-1: logically
it can be U32_MAX but it's getting 'translated' into U32_MAX-1 as the value which 
is fed to the register. So the period is still U32_MAX, accounting for the extra
tick at the end. So I guess we're on the same page.

> 
> > > > +		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.

Ack. I'll set the polarity first so there will be no uncovered corner
case.

> 
> 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.)

I was just curious about sashiko saying it's violating the pwm framework
expectations, while according to your words it seems there's no constraints.

BTW, I've tried to install sashiko and use it on my patches but it's obvious
that some custom settings are in order, since all I can get is some error
aborting the review after 3 attempts. Any chance you can share your Settings.toml
or any customization so I can test it in advance before submitting the new patchset
or do you recommend just throwing the new V5 at your script?

Many thanks,
Andrea

> 
> > > [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