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: 3719 bytes --]

Hello Andrea,

On Mon, Apr 20, 2026 at 06:27:45PM +0200, Andrea della Porta wrote:
> On 12:50 Fri 17 Apr     , Uwe Kleine-König wrote:
> > What happens if sync is asserted while a disabled channel didn't
> > complete the last period yet?
> 
> The output stops immediately without waiting for the current period to finish.

This is a good info for the Limitations block.

> > Maybe it's worth to test the following procedure for updating duty and
> > period:
> > 
> > 	disable channel
> > 	configure duty
> > 	configure period
> > 	enable
> > 	set update flag
> > 
> > Assumint disable is delayed until the end of the currently running
> > period, the effect of this procedure might be that no glitch happens if
> > the update flag is asserted before the currently running period ends and
> > the anormality is reduced to a longer inactive state if the updates are
> > not that lucky (in contrast to more severe glitches).
> 
> The disable isn't delayed as explained above. Setting just the new period/duty
> (which do not depend on the sync bit) correctly waits for the end of the current
> period without noticeable glitches (tested with a scope).

So if you happen to change both and one is done before the end of the
current period and the other shortly afterwards (which might happen as
those are configured in two different registers and the update trigger
isn't used), you get a mixed output for one cycle, right? If yes, please
also mention that in the Limitations paragraph.

> > > Let's say that teh user want 10 tick period, we have to use
> > > 9 instead to account for the extra tick at the end, so that the complete period
> > > contains that extra tick?
> > 
> > I would describe that a bit differently, but in general: yes.
> > 
> > The more straight forward description is that setting
> > 
> > 	RP1_PWM_RANGE(pwm->hwpwm) := x
> > 
> > results in a period of x + 1 ticks.
> 
> Exactly. So whatever the user request I have to subtract one from the value
> to be written to the RANGE register.

Unless the calculation is already rounded to 0, in that case don't
subtract 1 and let the tohw callback return 1.

> > > This also means that if we ask for 100% duty cycle, the output waveform will
> > > have the high part of the signal lasting one tick less than expected.a I guess
> > > this is the accepted compromise.
> > 
> > I assume you considered something like:
> > 
> > 	RP1_PWM_RANGE(pwm->hwpwm) := 17
> > 	RP1_PWM_DUTY(pwm->hwpwm) := 18
> > 
> > to get a 100% relative duty?
> 
> Ah right! It's working fine and I've got 100% duty. So at hw register level
> the duty can be greater that the period.

In that case please make sure to not use the maximal value for
RP1_PWM_RANGE(pwm->hwpwm) to ensure that for each (possible) period
length a 100% relative duty cycle can be configured.

> > My (not so well articulated) point is: Please be stringent about clock
> > handling to not bank up technical dept more than necessary and such that
> > the driver can be made unbindable if and when syscons grow
> > that feature. Optionally wail at the syscon guys :-)
> 
> Hmmm not sure I've understood your point: is it a requirement that the driver
> must be unbindable? In this case I should avoid registering the syscon. Or
> should I just provide a .remove callback in case there will be a way to
> unregister the syscon (even if this callback will not be called as of now)?

It's a requirement to properly manage the resources you allocate. If a
driver isn't unbindable due to restrictions of other subsystems that's
unfortunate and I don't like it, but I wouldn't block a patch because of
that.

Best regards
Uwe

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