[Linux-stm32] [PATCH v4 0/7] pwm: New abstraction and userspace API

[Uwe Kleine-König]

Hello David,

On Fri, Sep 06, 2024 at 02:06:18PM -0500, David Lechner wrote:
> On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> > Hello,
> > 
> > here comes v4 of the series to add support for duty offset in PWM
> > waveforms. For a single PWM output there is no gain, but if you have a
> > chip with two (or more) outputs and both operate with the same period,
> > you can generate an output like:
> > 
> >                ______         ______         ______         ______
> >    PWM #0  ___/      \_______/      \_______/      \_______/      \_______
> >                  __             __             __             __
> >    PWM #1  _____/  \___________/  \___________/  \___________/  \_________
> >               ^              ^              ^              ^
> > 
> 
> While working on an ADC driver that uses these new waveform APIs, we came
> across a case where we wanted wf->duty_offset_ns >= wf->period_length_ns,
> which is currently not allowed. [1]
> 
>                 ______         ______         ______         ______
>     PWM #0  ___/      \_______/      \_______/      \_______/      \_______
>                                __             __             __
>     PWM #1  __________________/  \___________/  \___________/  \___________
>                             ^              ^              ^              

I restricted to waveforms with .duty_offset_ns < .period_length_ns
because the signal you want is only periodic once PWM #1 begins to
toggle. Given that the pwm subsystem is about periodic signals and has a
wide range of behaviours at the moments the configuration is changed, I
think it's little sensible today to consider reliably implementing
offsets bigger than the period length.

Does your hardware really behave like that?

> We worked around it by setting:
> 
> 	wf->duty_offset_ns = DESIRED_NS % wf->period_length_ns
> 
> Having PWM #1 trigger too early just causes the first sample data
> read to be invalid data.
> 
> But even if we allowed wf->duty_offset_ns >= wf->period_length_ns,
> this offset wouldn't matter because there currently isn't a way to
> enable two PWM outputs at exactly the same time.

Yup, that's another challenge. Up to now it's even special knowledge
about the used pwm chip that with configuring two pwms with the same
period, the period starts are synced and you don't get:


                   ______         ______         ______         ______
     PWM #0  _____/      \_______/      \_______/      \_______/      \_______
                  ^              ^              ^              ^
                 __             __             __             __
     PWM #1  ___/  \___________/  \___________/  \___________/  \___________
              ^              ^              ^              ^              

> >  - The functions pwm_set_waveform_might_sleep() and
> >    pwm_round_waveform_might_sleep() have an unusual return value
> >    convention: They return 0 on success, 1 if the requested waveform
> >    cannot be implemented without breaking the rounding rules, or a
> >    negative errno if an error occurs.
> >    Fabrice rightfully pointed out this to be surprised by this and
> >    suggested to use at least a define for it.
> > 
> >    I couldn't find a decision that I'm entirely happy with here. My
> >    conflicts are:
> > 
> >     - I want a constant that now and in the future only means "cannot be
> >       done due to the rounding rules in the pwm framework". So the
> >       options are:
> >         * Introduce a new ESOMETHING and return -ESOMETHING
> >           I think that's hard to motivate and also myself doubt this
> >           would be sensible. As below, the question for a good name is
> >           unresolved.
> >         * return 1
> >           This is what was done in the earlier revisions and also here.
> > 
> >     - When keeping the return 1 semantics (and also for a new
> >       ESOMETHING):
> >       I agree that a name instead of a plain 1 would be nice, but I
> >       couldn't come up with a name I liked. Given that this can be
> >       introduced later without breaking anything, I don't consider that
> >       very urgent.
> >       My candidates were PWM_REQUIRES_BROKEN_ROUNDING,
> >       PWM_REQUIRES_FORBIDDEN_ROUNDING and PWM_ERR_ROUNDING.
> >       These are too long or/and imprecise.
> >       If you have a good idea, please tell.
> 
> To avoid using the return value for status flags, we could introduce
> an optional output parameter. Consumers where best effort is good
> enough can just pass NULL and consumers that care can pass an unsigned
> int to receive the status flag. This could even be a bitmap of multiple
> flags if it would be useful to know which rule(s) could not be met.

Which rule couldn't be met is obvious when you look at the resulting
waveform because the lowlevel driver is supposed to give you the
smallest possible value for the relevant parameter if rounding down
doesn't work.

So if you request

	.period_length_ns = 3000
	.duty_length_ns = 2
	.duty_offset_ns = 10

and your hardware can do 3000 ns period but the smallest duty_length is
10, it is supposed to write

	.period_length_ns = 3000
	.duty_length_ns = 10
	.duty_offset_ns = 10

in the waveform parameter and return 1.

My intuitive reaction is that (another) output parameter is worse than
the return value semantics I came up with. After some more thought I
wonder if the wish to have something PWM specific is the problem, and
just picking one of the available error constants (ERANGE?) is the nice
way out. Alternatively return 0 in this case and let the caller work out
themselves that not all values were rounded down?!

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://st-md-mailman.stormreply.com/pipermail/linux-stm32/attachments/20240908/3368468d/attachment.sig>