| Message ID | cover.1725635013.git.u.kleine-koenig@baylibre.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | pwm: New abstraction and userspace API | expand |
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
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.
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
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. 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