Re: [PATCH v12] pwm: airoha: Add support for EN7581 SoC

[Christian Marangi <ansuelsmth@gmail.com>]

On Fri, May 09, 2025 at 09:43:55PM +0200, Uwe Kleine-König wrote:
> Hello Christian,
> 
> On Fri, May 09, 2025 at 03:03:23PM +0200, Christian Marangi wrote:
> > thanks a lot for the review. I was just starting reviewing some patch on
> > patchwork so I could remove some work from you but you were faster...
> 
> You're still invited to comment ...
> 

Ok will check!

> > On Fri, May 09, 2025 at 12:39:37PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Apr 07, 2025 at 07:35:53PM +0200, Christian Marangi wrote:
> > > > +static void airoha_pwm_get_ticks_from_ns(u64 period_ns, u32 *period_tick,
> > > > +					 u64 duty_ns, u32 *duty_tick)
> > > > +{
> > > > +	u64 tmp_duty_tick;
> > > > +
> > > > +	*period_tick = div_u64(period_ns, AIROHA_PWM_PERIOD_TICK_NS);
> > > > +
> > > > +	tmp_duty_tick = mul_u64_u64_div_u64(duty_ns, AIROHA_PWM_DUTY_FULL,
> > > > +					    period_ns);
> > > 
> > > So period can be set to multiples of 4 ms. If you request
> > > 
> > > 	.period_ns = 11999 ns
> > > 	.duty_ns = 4016 ns
> > > 
> > > the hardware should configure 
> > > 
> > > 	.period = 8000 ns
> > > 	.duty_cycle = 4015.6862745098038 ns (i.e. 128/255 * period)
> > > 
> > > corresponding to period_tick = 2 and duty_tick = 128.
> > > 
> > > However you calculate duty_tick = 85.
> > > 
> > > I would expect that with having PWM_DEBUG enabled you get a warning when
> > > you do:
> > > 
> > > 	pwmset -P 8000 -D 4016
> > > 	pwmset -P 11999 -D 4016
> > > 
> > 
> > I addressed all the other comments but this is the only thing that I'm
> > confused about.
> > 
> > Where 85 comes from?
> 
> 4016 * 255 / 11999 -> 85. The problem I suspected is that duty_tick is
> calculated using the requested period value instead of the real one.
> 
> > I tested your command and I can correctly observe the values getting
> > set to the expected tick.
> 
> I didn't recheck in detail and now I'm unsure if that really happens
> because period_ns might be already round to a multiple of 4 ms?
>  

Yes it it. And that was actually a problem I notice in v11 that was
causing idempotent problem.

It's done in .apply with 

period_ns = rounddown(state->period, AIROHA_PWM_PERIOD_TICK_NS);

We normalize the value to 4ns step. Sooo with 11999, in
airoha_pwm_config the actual value used is 8000.

> > And I don't have the idempotent warning from PWM debug.
> > 
> > With period = 8000 and duty to 4016, period tick is set to 0x2 and duty
> > is set to 0x80 (128)
> > 
> > When PWM debug repply the configuration and read the state, it gets
> > period 0x8000 and duty 4015,687.
> > 
> > And those are the expected values. Am I missing something here?
> 
> If you don't get the warning, you most probably only miss that I
> misjudged the code :-)

I both tested with your combo values and with a loop and pwm-leds from 0
to 255 and no warning.

I will send v13, finger crossed everything is good!

> 
> Best regards
> Uwe



-- 
	Ansuel