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
Attachment:
signature.asc
Description: PGP signature
