Message-ID: <zwckkfd6mzzjxfpitojcmhokhjbtc4u3brf34pcu4phdlipf3z@uijstw7daze2>
Date: Thu, 3 Apr 2025 17:01:46 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org, 
	Benjamin Larsson <benjamin.larsson@...exis.eu>, 
	AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Lorenzo Bianconi <lorenzo@...nel.org>
Subject: Re: [RESEND PATCH v11] pwm: airoha: Add support for EN7581 SoC

Hello,

On Thu, Apr 03, 2025 at 12:50:27PM +0200, Christian Marangi wrote:
> On Thu, Apr 03, 2025 at 11:58:48AM +0200, Uwe Kleine-König wrote:
> > > +	if (hwpwm >= AIROHA_PWM_NUM_GPIO)
> > > +		offset -= AIROHA_PWM_NUM_GPIO;
> > > +
> > > +	/* One FLASH_MAP register handles 8 pins */
> > > +	*shift = do_div(offset, AIROHA_PWM_PINS_PER_FLASH_MAP);
> > > +	*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(*shift);
> > > +
> > > +	if (offset < AIROHA_PWM_NUM_GPIO)
> > > +		*addr = AIROHA_PWM_REG_GPIO_FLASH_MAP(offset);
> > > +	else
> > > +		*addr = AIROHA_PWM_REG_SIPO_FLASH_MAP(offset);
> > 
> > A single if would be a bit more straight forward. Something like:
> > 
> > 	static void airoha_pwm_get_flash_map_addr_and_shift(unsigned int hwpwm,
> > 							    unsigned int *addr, unsigned int *shift)
> > 	{
> > 		u64 offset = hwpwm;
> > 	
> > 		if (hwpwm >= AIROHA_PWM_NUM_GPIO) {
> > 			unsigned sipohwpwm = hwpwm - AIROHA_PWM_NUM_GPIO;
> > 
> > 			*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(sipohwpwm % AIROHA_PWM_PINS_PER_FLASH_MAP)
> > 			*addr = AIROHA_PWM_REG_SIPO_FLASH_MAP(sipohwpwm);
> > 		} else {
> > 			*shift = AIROHA_PWM_REG_GPIO_FLASH_MAP_SHIFT(hwpwm % AIROHA_PWM_PINS_PER_FLASH_MAP)
> > 			*addr = AIROHA_PWM_REG_GPIO_FLASH_MAP(hwpwm);
> > 		}
> > 	}
> > 
> 
> I think you missed the do_div that do side effect on offset. Also that
> needs to be / AIROHA_PWM_PINS_PER_FLASH_MAP.

Ack. Luckily you got the idea anyhow. This double effect of do_div() is
easy to miss, so getting rid of them sounds sensible.

> > > +		if (duty_ns == bucket->duty_ns &&
> > > +		    period_ns == bucket->period_ns)
> > > +			return i;
> > 
> > If period_ns == 4010 and bucket->period_ns == 4000 you're not
> > considering *bucket even though it has the right period setting.
> > (period_ns is the requested period and not the expected period actually
> > implemented by HW, right?)
> > 
> 
> Doesn't that requires a different generator? The value we store in the
> bucket is the requested period yes.

No it doesn't. If I understood right the possible periods are: 4ms, 8ms,
..., so a request to do 4.01ms will be mapped to 4ms which allows to
share the generator.

> > > +				  AIROHA_PWM_GPIO_FLASH_EN << shift);
> > > +		return;
> > > +	}
> > > +
> > > +	regmap_update_bits(pc->regmap, addr,
> > > +			   AIROHA_PWM_GPIO_FLASH_SET_ID << shift,
> > > +			   FIELD_PREP(AIROHA_PWM_GPIO_FLASH_SET_ID, index) << shift);
> > 
> > Huh, I'd prefer:
> > 
> > 	regmap_update_bits(pc->regmap, addr,
> > 			   AIROHA_PWM_GPIO_FLASH_SET_ID(hwpwm % 8)
> > 			   FIELD_PREP(AIROHA_PWM_GPIO_FLASH_SET_ID(hwpwm % 8), index));
> > 
> > (That probably doesn't work out of the box because of the
> > __builtin_constant_p check on mask, so you might need a local
> > alternative to FIELD_PREP without that check.)
> 
> Honestly it's not worth to introduce custom FIELD_PREP for this. Yes the
> problem is that FIELD_PREP requires constant mask so hwpwm % 8 is
> problematic. An old implementation had stuff in define but resulted in
> very ugly and confusing define and macro. The shift and FIELD_PREP
> permits a much cleaner description in the define part at the cost of
> that additional << shift needed.
> 
> Hope you can understand why I think it's better to keep it this way.

OK, I can live with that.

> > > +	bucket = airoha_pwm_consume_generator(pc, duty_ns, period_ns,
> > > +					      hwpwm);
> > > +	if (bucket < 0)
> > > +		return -EBUSY;
> > > +
> > > +	/*
> > > +	 * For SIPO reinit is always needed to trigger the shift register chip
> > > +	 * and apply the new flash configuration.
> > 
> > I don't understand that sentence. What is the shift register chip? What
> > is a flash configuration?
> > 
> 
> The SoC can have attached a shift register chip to supports multiple LEDs.
> The handling of this chip and comunication is done internally to the SoC
> and it's exposed to register with these additional register.
> 
> When such channel are used with an assumed shift register, to apply the
> new configuration in airoha_pwm_config_flash_map(), the shift register
> chip needs to be reinit to actually refresh the chip internal register
> with the new "flash configuration" (aka the values for
> AIROHA_PWM_GPIO_FLASH_SET_ID)
> 
> Will add more comments to this to make it more clear.

Sounds good.

> > > +	state->enabled = FIELD_GET(AIROHA_PWM_GPIO_FLASH_EN, val >> shift);
> > > +	if (!state->enabled)
> > > +		return 0;
> > > +
> > > +	state->polarity = PWM_POLARITY_NORMAL;
> > > +
> > > +	bucket = FIELD_GET(AIROHA_PWM_GPIO_FLASH_SET_ID, val >> shift);
> > > +	state->period = pc->buckets[bucket].period_ns;
> > 
> > Does .period_ns hold the requested or the actual period? You should
> > return the latter.
> 
> Problem is that putting .period_ns here cause error in the PWM_DEBUG
> validations. This is caused by the conversion error of the various / and
> * done to convert tick to ns. Also on applying the configuration we
> already do all the validation to make sure the request value is the
> expected one.

Then there is a bug somewhere. I wouldn't completely rule out it's in
the PWM_DEBUG logic, but my bet is on your driver then.
 
> For the initial values, there is airoha_pwm_fill_bucket that read the
> current PWM values at boot and fill the buckets with the current values.
> 
> This is the compromise I found to handle both pre-configured bucket and
> also handle the division errors in the ns - tick conversion.
> 
> Hope this is acceptable, do you have hint on better handling this?

I'd wait for the next iteration of your patch and then take a deeper
look in the maths involved. It should be possible to make PWM_DEBUG
happy and still report the actual configuration.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists