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

["Uwe Kleine-König" <ukleinek@kernel.org>]

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

Hello Andrea,

one thing I forgot to ask: Is there a public reference manual covering
the hardware. If yes, please add a link at the top of the driver.

On Thu, Apr 16, 2026 at 12:30:43PM +0200, Andrea della Porta wrote:
> On 19:31 Fri 10 Apr     , Uwe Kleine-König wrote:
> > I assume there is a glitch if I update two channels and the old
> > configuration of the first channel ends while I'm in the middle of
> > configuring the second?
> 
> The configuration registers are per-channel but the update flag is global.
> I don't have details of the hw insights, my best guess is that anything that
> you set in the registers before updating the flag will take effect, so there
> should be no glitches.

Would be great if you could test that. (Something along the lines of:
configure a very short period and wait a bit to be sure the short
configuration is active. Configure something with a long period and wait
shortly to be sure that the long period started, then change the duty,
toggle the update bit and modify a 2nd channel without toggling update
again. Then check the output of the 2nd channel after the first
channel's period ended.

> > > +	if (ticks > U32_MAX)
> > > +		ticks = U32_MAX;
> > > +	wfhw->period_ticks = ticks;
> > 
> > What happens if wf->period_length_ns > 0 but ticks == 0?
> 
> I've added a check, returning 1 to signal teh round-up, and a minimum tick of 1
> in this case.

Sounds good. Are you able to verify that there is no +1 missing in the
calculation, e.g. using 1 as register value really gives you a period of
1 tick and not 2?

> > > +	if (wf->duty_offset_ns + wf->duty_length_ns >= wf->period_length_ns) {
> > 
> > The maybe surprising effect here is that in the two cases
> > 
> > 	wf->duty_offset_ns == wf->period_length_ns and wf->duty_length_ns == 0
> > 
> > and
> > 	
> > 	wf->duty_length_ns == wf->period_length_ns and wf->duty_offset_ns == 0
> > 
> > you're configuring inverted polarity. I doesn't matter technically
> > because the result is the same, but for consumers still using pwm_state
> > this is irritating. That's why pwm-stm32 uses inverted polarity only if
> > also wf->duty_length_ns and wf->duty_offset_ns are non-zero.

Please align to the pwm-stm32 algorithm (as of
https://patch.msgid.link/c5e7767cee821b5f6e00f95bd14a5e13015646fb.1776264104.git.u.kleine-koenig@baylibre.com)
here to decide when to select inverted polarity.

> > > +	}
> > > +
> > > +	return 0;
> > > +
> > > +err_disable_clk:
> > > +	clk_disable_unprepare(rp1->clk);
> > > +
> > > +	return ret;
> > > +}
> > 
> > On remove you miss to balance the call to clk_prepare_enable() (if no
> > failed call to clk_prepare_enable() in rp1_pwm_resume() happend).
> 
> Since this driver now exports a syscon, it's only builtin (=Y) so
> it cannot be unloaded.
> I've also avoided the .remove callback via .suppress_bind_attrs.

Oh no, please work cleanly here and make the driver unbindable. This
yields better code quality and also helps during development and
debugging.

Best regards
Uwe

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