[PATCH v3 5/5] pwm: airoha: Add support for EN7581 SoC

[Uwe Kleine-König]

Hello Benjamin,

On Tue, Sep 03, 2024 at 01:58:30PM +0200, Benjamin Larsson wrote:
> On 2024-09-03 12:46, Uwe Kleine-König wrote:
> > Would you please add a "Limitations" paragraph here covering the
> > following questions:
> > 
> >   - How does the hardware behave on changes of configuration (does it
> >     complete the currently running period? Are there any glitches?)
> >   - How does the hardware behave on disabling?
> > 
> > Please stick to the format used in several other drivers such that
> > 
> > 	sed -rn '/Limitations:/,/\*\/?$/p' drivers/pwm/*.c
> > 
> > emits the informations.
> 
> The answer to your questions are currently unknown. Is this information
> needed for a merge of the driver ?

It would be very welcome and typically isn't that hard to work out if
you have an LED connected to the output or a similar means to observe
the output. An oscilloscope makes it still easier.

For example to check if the current period is completed configure the
PWM with period = 1s and duty_cycle = 0 disabling the LED. (I leave it
as an exercise for the reader what to do if duty_cycle = 0 enables the
LED :-) Then do:

	pwm_apply_might_sleep(mypwm, &(struct pwm_state){
		.period = NSEC_PER_SEC,
		.duty_cycle = NSEC_PER_SEC,
		.enabled = true,
	});
	pwm_apply_might_sleep(mypwm, &(struct pwm_state){
		.period = NSEC_PER_SEC,
		.duty_cycle = 0,
		.enabled = true,
	});

Iff that enables the LED for a second, the period is completed. The
question about glitches is a bit harder to answer, but with a tool like
memtool should be possible to answer. Alternatively add delays and
printk output to .apply() in the critical places.

> > > +#define airoha_pwm_sgpio_clear(pc, offset, mask)				\
> > > +	airoha_pwm_sgpio_rmw((pc), (offset), (mask), 0)
> > > +#define airoha_pwm_flash_set(pc, offset, val)					\
> > > +	airoha_pwm_flash_rmw((pc), (offset), 0, (val))
> > > +#define airoha_pwm_flash_clear(pc, offset, mask)				\
> > > +	airoha_pwm_flash_rmw((pc), (offset), (mask), 0)
> > > +
> > > +static int airoha_pwm_get_waveform(struct airoha_pwm *pc, u64 duty_ns,
> > > +				   u64 period_ns)
> > Given that "waveform" will gain some specific semantic soon, but this
> > usage is different, I'd like to see this function renamed.
> 
> I suggest pwm_generator. Is that acceptable ?

This function determines the register values to write for a given
duty_ns + period_ns combination, right? airoha_pwm_calc_bucket_config()?
 
> > If you limit the number of requested pwm devices to 8, the code gets a
> > tad simpler (because you can map a fixed bucket to each pwm device and
> > don't need to search during .apply()) and each consumer has maximal
> > freedom to configure its device.
> 
> The main use for this solution is for led-dimming which uses the same timing
> among groups of leds. Most (of our) products have more then 8 leds in total,
> with a limit of only 8 pwm devices it would not be possible to use the
> mainline driver with our hardware. I suggest that the current logic is kept
> but properly documented in the limitations section.

Fine for me.

Best regards
Uwe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-mediatek/attachments/20240903/8945edeb/attachment-0001.sig>