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

Hello Andrea,

On Fri, Jun 05, 2026 at 06:49:17PM +0200, Andrea della Porta wrote:
> On 23:28 Thu 04 Jun     , Uwe Kleine-König wrote:
> > 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.

Not sure you got what I wrote. You can use the maximal value possible in
hardware, because for all but 100% relative duty cycle it works as
expected. You only need to map 100% relative duty cycle with that
maximal period to a different 100% relative duty cycle setting. I think
PWM_DEBUG even doesn't yell at you for that. (And if it does, that needs
fixing.)

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

You can, but as I wrote below, being lazy is also fine.
 
> > 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.

In doubt trust me :-)

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

Note that sashiko is not "my script", it was setup by Google engineers
and I have nothing to do with it (apart from benefitting from its review
feedback). Kevin (added to Cc) tried to setup a local instance with his
Claude plan, but (IIUC) it quickly ate his day's amount of tokens before
completing review of a series of only 4 patches.

So without knowing the size of Kevin's or your AI plan, probably it's
easier to just rely on the public instance. (And IMHO that's nothing to
be afraid of, just handle it like a human reviewer. For these you also
don't know what they will reply for your next revision.)

Best regards
Uwe

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