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

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

Hi Uwe,

On 22:51 Fri 05 Jun     , Uwe Kleine-König wrote:
> 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 see. Since you said that teh current behaviour is also fine, I'd prefer
to stick with it to avoid complicating the code.

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

Already done :)

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

Sure!

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

Ack.

Regards,
Andrea

> 
> Best regards
> Uwe