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

[Trevor Gamblin]

On 2024-09-06 11:42, 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  _____/  \___________/  \___________/  \___________/  \_________
>                ^              ^              ^              ^
>
> The opportunity for a new abstraction is/should be used to also improve
> precision of the API functions, which implies that the rules for
> lowlevel drivers are more strict for the new callbacks.
>
> Changes since v3, which is available from
> https://lore.kernel.org/linux-pwm/cover.1722261050.git.u.kleine-koenig@baylibre.com/
> include:
>
>   - Drop PWM_IOCTL_GET_NUM_PWMS ioctl (patch #4), suggestion by David
>     Lechner
>
>   - Define members of userspace API struct using __u32 instead of
>     unsigned int; thanks to Kent Gibson for the suggestion (patch #4)
>
>   - Ensure that pwm_apply_state_{atomic,might_sleep}() don't return 1
>     Noticed by Fabrice Gasnier
>
>   - Rebased to current pwm/for-next.
>     (fixing a missing +1 noticed by Fabrice)
>
>   - Dropped Tested-by: from Trevor
>     While the axi-pwmgen driver didn't change, there were considerable
>     changes in the core. So I dropped it.
>
>   - add some missing EXPORT_SYMBOL_GPL for the new API functions
>
>   - Add kernel doc comments for the new API functions
>
>   - debug message in stm32_pwm_round_waveform_fromhw (another suggestion
>     by Fabrice)
>
> I also updated the branch pwm/chardev in
> https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git to
> match this v4.
>
> I'm aware of two issues in this series:
>
>   - Triggers a lockdep warning in the pwm-meson driver. This affects the
>     new pwm locking vs the clk framework's prepare lock. I think this is
>     a false positive and to fix it, only changes in the clk framework are
>     necessary.
>
>   - 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.

PWM_ERR_ROUNDING doesn't seem too bad.

What about something like PWM_REQ_INVALID, PWM_WF_INVALID, or 
PWM_WF_REQ_INVALID?

>
> Still I'd like to get that forward and (unless a new and relevant issue
> pops up until then) intend to put it into next after the next merge
> window. Nevertheless I'm open for suggestions to further improve this
> series.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (7):
>    pwm: Add more locking
>    pwm: New abstraction for PWM waveforms
>    pwm: Provide new consumer API functions for waveforms
>    pwm: Add support for pwmchip devices for faster and easier userspace
>      access
>    pwm: Add tracing for waveform callbacks
>    pwm: axi-pwmgen: Implementation of the waveform callbacks
>    pwm: stm32: Implementation of the waveform callbacks
>
>   drivers/pwm/core.c           | 867 +++++++++++++++++++++++++++++++++--
>   drivers/pwm/pwm-axi-pwmgen.c | 154 +++++--
>   drivers/pwm/pwm-stm32.c      | 612 ++++++++++++++++---------
>   include/linux/pwm.h          |  58 ++-
>   include/trace/events/pwm.h   | 134 +++++-
>   include/uapi/linux/pwm.h     |  24 +
>   6 files changed, 1545 insertions(+), 304 deletions(-)
>   create mode 100644 include/uapi/linux/pwm.h
>
> base-commit: cf6631b07b907d4c644ca42f7cc234e7149290a2