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

[Uwe Kleine-König]

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

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature