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

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

Hi Uwe,

On 16:50 Tue 21 Apr     , Uwe Kleine-König wrote:
> 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.

Yup, already added, plus a couple other edge cases.
 
> 
> > > 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.

Confirmed, tested with the scope and a very long period time.

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

Sure.

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

Ack.

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

I totally agree, of course. From a practical perspective I take it as "even
if it's not ideal, you don't need to do further coding action on that side".

Many thanks,
Andrea
 

> 
> Best regards
> Uwe