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

[Uwe Kleine-König]

Hello Christian,

On Mon, Sep 08, 2025 at 08:48:38PM +0200, Christian Marangi wrote:
> On Fri, Aug 01, 2025 at 11:15:41AM +0200, Uwe Kleine-König wrote:
> > On Tue, Jul 08, 2025 at 04:50:52PM +0200, Christian Marangi wrote:
> > > +	duty_ticks = airoha_pwm_get_duty_ticks_from_ns(period_ns, duty_ns);
> > 
> > As duty_ticks depends on the selected period_ticks, I think the bucket
> > selection algorithm is still wrong.
> > 
> > Consider a request to implement
> > 
> > 	period_ns = 256 ms
> > 	duty_ns = 128 ms
> > 
> > which at first correctly results in
> > 
> > 	period_ticks = 64
> > 	duty_ticks = 127
> > 
> > If however all buckets are used and we only find one with say 62 period
> > ticks we get period_ns = 248 and with that duty_ticks should better be
> > 131 and not 127.
>
> sorry for checking this only now and maybe we need to catch this again.

no need to be sorry here. Taking time for replies is fine for me.

> Maybe we are getting confused here but itsn't this already handled by
> the upper condition?
> 
> 		/* Ignore bucket with invalid configs */
> 		if (bucket_period_ticks > period_ticks ||
> 		    bucket_duty_ticks > duty_ticks)
> 			continue;
> 
> 		/*
> 		 * Search for a bucket closer to the requested period/duty
> 		 * that has the maximal possible period that isn't bigger
> 		 * than the requested period. For that period pick the maximal
> 		 * duty cycle that isn't bigger than the requested duty_cycle.
> 		 */
> 		if (bucket_period_ticks > best_period_ticks ||
> 		    (bucket_period_ticks == best_period_ticks &&
> 		     bucket_duty_ticks > best_duty_ticks)) {
> 			best_period_ticks = bucket_period_ticks;
> 			best_duty_ticks = bucket_duty_ticks;
> 			best = i;
> 		}
> 
> We first limit for a bucket that doesn't got over both period and duty
> and then we search for period and best duty. This should account for
> never exceeding a duty since both period and duty are precalculated for
> the current bucket and even if duty depends on period, again it's
> precalculated. Am I missing something?

Let me describe the issue in more detail:

The period length is configured in the AIROHA_PWM_WAVE_GEN_CYCLE
register in multiples of 4 ms. The duty length is configured in the
AIROHA_PWM_GPIO_FLASH_PRD_HIGH register in multiples of
$period_length/255.

So if you calcultate the number of multiples you need for duty_ns = 128
ms based on the assumption that period_ns = 256 ms the result becomes
wrong when you are forced to switch to period_ns = 248.

So to implement a request for period = 256 ms (64 ticks) and duty_cycle
= 128 ms (127.5 duty ticks) having the choice between the two buckets:

 a) period_ticks = 62; duty_ticks = 127
    (period = 248 ms, duty_cycle = 123.51372549019608 ms)
 b) period_ticks = 62; duty_ticks = 131
    (period = 248 ms, duty_cycle = 127.40392156862744 ms)

b) is the better one despite 127 duty_ticks would be an exact match for
period_ticks = 64. So the issue is that the "Ignore bucket with invalid
configs" kicks out b). That's wrong because

	bucket_duty_ticks > duty_ticks

doesn't imply

	bucket_duty > duty

.

Best regards
Uwe

Attachment: signature.asc
Description: PGP signature