Re: [PATCH v4 0/7] pwm: New abstraction and userspace API

[David Lechner <dlechner@baylibre.com>]

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  __________________/  \___________/  \___________/  \___________
                            ^              ^              ^              

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.

In the ADC application we work around both of these shortcomings by not
enabling the DMA that is triggered by PWM #1 until after both PWMs are
enabled. However, there may be similar applications in the future that
also need such an offset and synchronized enable that might not be so
easy to work around, so something to keep in mind.

[1]: https://lore.kernel.org/linux-iio/20240904-ad7625_r1-v4-2-78bc7dfb2b35@baylibre.com/


> 
>  - 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.