| Message ID | 015a5fbd5cf449bcb2d8fdf2305d7b6bf7109844.1722261050.git.u.kleine-koenig@baylibre.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: New abstraction and userspace API | expand |
On 2024-07-29 10:34 a.m., Uwe Kleine-König wrote: > Provide API functions for consumers to work with waveforms. > > Note that one relevant difference between pwm_get_state() and > pwm_get_waveform*() is that the latter yields the actually configured > hardware state, while the former yields the last state passed to > pwm_apply*() and so doesn't account for hardware specific rounding. Hi, kernel test robot caught an issue with a pwm function while testing my iio driver: https://lore.kernel.org/linux-iio/20240819-ad7625_r1-v3-0-75d5217c76b5@baylibre.com/T/#m7b3118821c416240e0309a8c2bbc5c51ba4b0823 Looks like an issue with static inline versions of the consumer functions not being present after the #else in pwm.h? > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> > --- > drivers/pwm/core.c | 201 ++++++++++++++++++++++++++++++++++++++++++++ > include/linux/pwm.h | 6 +- > 2 files changed, 206 insertions(+), 1 deletion(-) > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index a8a0c12fef83..41e620944375 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -49,6 +49,30 @@ static void pwmchip_unlock(struct pwm_chip *chip) > > DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T)) > > +static bool pwm_wf_valid(const struct pwm_waveform *wf) > +{ > + /* > + * For now restrict waveforms to period_length_ns <= S64_MAX to provide > + * some space for future extensions. One possibility is to simplify > + * representing waveforms with inverted polarity using negative values > + * somehow. > + */ > + if (wf->period_length_ns > S64_MAX) > + return false; > + > + if (wf->duty_length_ns > wf->period_length_ns) > + return false; > + > + /* > + * .duty_offset_ns is supposed to be smaller than .period_length_ns, apart > + * from the corner case .duty_offset_ns = 0 + .period_length_ns = 0. > + */ > + if (wf->duty_offset_ns && wf->duty_offset_ns >= wf->period_length_ns) > + return false; > + > + return true; > +} > + > static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state) > { > if (wf->period_length_ns) { > @@ -95,6 +119,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf) > } > } > > +static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b) > +{ > + if (a->period_length_ns > b->period_length_ns) > + return 1; > + > + if (a->period_length_ns < b->period_length_ns) > + return -1; > + > + if (a->duty_length_ns > b->duty_length_ns) > + return 1; > + > + if (a->duty_length_ns < b->duty_length_ns) > + return -1; > + > + if (a->duty_offset_ns > b->duty_offset_ns) > + return 1; > + > + if (a->duty_offset_ns < b->duty_offset_ns) > + return -1; > + > + return 0; > +} > + > static int pwm_check_rounding(const struct pwm_waveform *wf, > const struct pwm_waveform *wf_rounded) > { > @@ -145,6 +192,160 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c > > #define WFHWSIZE 20 > > +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) > +{ > + struct pwm_chip *chip = pwm->chip; > + const struct pwm_ops *ops = chip->ops; > + struct pwm_waveform wf_req = *wf; > + char wfhw[WFHWSIZE]; > + int ret_tohw, ret_fromhw; > + > + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); > + > + if (!pwm_wf_valid(wf)) > + return -EINVAL; > + > + guard(pwmchip)(chip); > + > + if (!chip->operational) > + return -ENODEV; > + > + ret_tohw = __pwm_round_waveform_tohw(chip, pwm, wf, wfhw); > + if (ret_tohw < 0) > + return ret_tohw; > + > + ret_fromhw = __pwm_round_waveform_fromhw(chip, pwm, wfhw, wf); > + if (ret_fromhw < 0) > + return ret_fromhw; > + > + if (IS_ENABLED(CONFIG_PWM_DEBUG) && > + ret_tohw == 0 && pwm_check_rounding(&wf_req, wf)) > + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", > + wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, > + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns); > + > + return ret_tohw; > +} > + > +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) > +{ > + struct pwm_chip *chip = pwm->chip; > + const struct pwm_ops *ops = chip->ops; > + char wfhw[WFHWSIZE]; > + int err; > + > + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); > + > + guard(pwmchip)(chip); > + > + if (!chip->operational) > + return -ENODEV; > + > + err = __pwm_read_waveform(chip, pwm, &wfhw); > + if (err) > + return err; > + > + return __pwm_round_waveform_fromhw(chip, pwm, &wfhw, wf); > +} > + > +/* Called with the pwmchip lock held */ > +static int __pwm_set_waveform(struct pwm_device *pwm, > + const struct pwm_waveform *wf, > + bool exact) > +{ > + struct pwm_chip *chip = pwm->chip; > + const struct pwm_ops *ops = chip->ops; > + char wfhw[WFHWSIZE]; > + struct pwm_waveform wf_rounded; > + int err; > + > + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); > + > + if (!pwm_wf_valid(wf)) > + return -EINVAL; > + > + err = __pwm_round_waveform_tohw(chip, pwm, wf, &wfhw); > + if (err) > + return err; > + > + if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length_ns) { > + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded); > + if (err) > + return err; > + > + if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded)) > + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", > + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, > + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns); > + > + if (exact && pwmwfcmp(wf, &wf_rounded)) { > + dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n", > + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, > + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns); > + > + return 1; > + } > + } > + > + err = __pwm_write_waveform(chip, pwm, &wfhw); > + if (err) > + return err; > + > + /* update .state */ > + pwm_wf2state(wf, &pwm->state); > + > + if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length_ns) { > + struct pwm_waveform wf_set; > + > + err = __pwm_read_waveform(chip, pwm, &wfhw); > + if (err) > + /* maybe ignore? */ > + return err; > + > + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_set); > + if (err) > + /* maybe ignore? */ > + return err; > + > + if (pwmwfcmp(&wf_set, &wf_rounded) != 0) > + dev_err(&chip->dev, > + "Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n", > + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, > + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns, > + wf_set.duty_length_ns, wf_set.period_length_ns, wf_set.duty_offset_ns); > + } > + return 0; > +} > + > +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, > + const struct pwm_waveform *wf, bool exact) > +{ > + struct pwm_chip *chip = pwm->chip; > + int err; > + > + might_sleep(); > + > + guard(pwmchip)(chip); > + > + if (!chip->operational) > + return -ENODEV; > + > + if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) { > + /* > + * Catch any drivers that have been marked as atomic but > + * that will sleep anyway. > + */ > + non_block_start(); > + err = __pwm_set_waveform(pwm, wf, exact); > + non_block_end(); > + } else { > + err = __pwm_set_waveform(pwm, wf, exact); > + } > + > + return err; > +} > +EXPORT_SYMBOL_GPL(pwm_set_waveform_might_sleep); > + > static void pwm_apply_debug(struct pwm_device *pwm, > const struct pwm_state *state) > { > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 6a26a5210dab..40cef0bc0de7 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -358,7 +358,11 @@ static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data) > } > > #if IS_ENABLED(CONFIG_PWM) > -/* PWM user APIs */ > + > +/* PWM consumer APIs */ > +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf); > +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf); > +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact); > int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state); > int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state); > int pwm_adjust_config(struct pwm_device *pwm);
Hello, On Tue, Aug 20, 2024 at 11:06:41AM -0400, Trevor Gamblin wrote: > On 2024-07-29 10:34 a.m., Uwe Kleine-König wrote: > > Provide API functions for consumers to work with waveforms. > > > > Note that one relevant difference between pwm_get_state() and > > pwm_get_waveform*() is that the latter yields the actually configured > > hardware state, while the former yields the last state passed to > > pwm_apply*() and so doesn't account for hardware specific rounding. > > Hi, > > kernel test robot caught an issue with a pwm function while testing my iio > driver: https://lore.kernel.org/linux-iio/20240819-ad7625_r1-v3-0-75d5217c76b5@baylibre.com/T/#m7b3118821c416240e0309a8c2bbc5c51ba4b0823 > > Looks like an issue with static inline versions of the consumer functions > not being present after the #else in pwm.h? I already replied in the linked thread, but for completeness I'm repeating the gist here: I didn't provide the stubs on purpose. If you use the new functions, make sure your code depends on CONFIG_PWM. For more details (or if you want to discuss that) please look at/reply to the more verbose mail in the other thread. Best regards Uwe
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index a8a0c12fef83..41e620944375 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -49,6 +49,30 @@ static void pwmchip_unlock(struct pwm_chip *chip) DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T)) +static bool pwm_wf_valid(const struct pwm_waveform *wf) +{ + /* + * For now restrict waveforms to period_length_ns <= S64_MAX to provide + * some space for future extensions. One possibility is to simplify + * representing waveforms with inverted polarity using negative values + * somehow. + */ + if (wf->period_length_ns > S64_MAX) + return false; + + if (wf->duty_length_ns > wf->period_length_ns) + return false; + + /* + * .duty_offset_ns is supposed to be smaller than .period_length_ns, apart + * from the corner case .duty_offset_ns = 0 + .period_length_ns = 0. + */ + if (wf->duty_offset_ns && wf->duty_offset_ns >= wf->period_length_ns) + return false; + + return true; +} + static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state) { if (wf->period_length_ns) { @@ -95,6 +119,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf) } } +static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b) +{ + if (a->period_length_ns > b->period_length_ns) + return 1; + + if (a->period_length_ns < b->period_length_ns) + return -1; + + if (a->duty_length_ns > b->duty_length_ns) + return 1; + + if (a->duty_length_ns < b->duty_length_ns) + return -1; + + if (a->duty_offset_ns > b->duty_offset_ns) + return 1; + + if (a->duty_offset_ns < b->duty_offset_ns) + return -1; + + return 0; +} + static int pwm_check_rounding(const struct pwm_waveform *wf, const struct pwm_waveform *wf_rounded) { @@ -145,6 +192,160 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c #define WFHWSIZE 20 +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + struct pwm_waveform wf_req = *wf; + char wfhw[WFHWSIZE]; + int ret_tohw, ret_fromhw; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + if (!pwm_wf_valid(wf)) + return -EINVAL; + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + ret_tohw = __pwm_round_waveform_tohw(chip, pwm, wf, wfhw); + if (ret_tohw < 0) + return ret_tohw; + + ret_fromhw = __pwm_round_waveform_fromhw(chip, pwm, wfhw, wf); + if (ret_fromhw < 0) + return ret_fromhw; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && + ret_tohw == 0 && pwm_check_rounding(&wf_req, wf)) + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", + wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns); + + return ret_tohw; +} + +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + char wfhw[WFHWSIZE]; + int err; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + err = __pwm_read_waveform(chip, pwm, &wfhw); + if (err) + return err; + + return __pwm_round_waveform_fromhw(chip, pwm, &wfhw, wf); +} + +/* Called with the pwmchip lock held */ +static int __pwm_set_waveform(struct pwm_device *pwm, + const struct pwm_waveform *wf, + bool exact) +{ + struct pwm_chip *chip = pwm->chip; + const struct pwm_ops *ops = chip->ops; + char wfhw[WFHWSIZE]; + struct pwm_waveform wf_rounded; + int err; + + BUG_ON(WFHWSIZE < ops->sizeof_wfhw); + + if (!pwm_wf_valid(wf)) + return -EINVAL; + + err = __pwm_round_waveform_tohw(chip, pwm, wf, &wfhw); + if (err) + return err; + + if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length_ns) { + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded); + if (err) + return err; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded)) + dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n", + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns); + + if (exact && pwmwfcmp(wf, &wf_rounded)) { + dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n", + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns); + + return 1; + } + } + + err = __pwm_write_waveform(chip, pwm, &wfhw); + if (err) + return err; + + /* update .state */ + pwm_wf2state(wf, &pwm->state); + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length_ns) { + struct pwm_waveform wf_set; + + err = __pwm_read_waveform(chip, pwm, &wfhw); + if (err) + /* maybe ignore? */ + return err; + + err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_set); + if (err) + /* maybe ignore? */ + return err; + + if (pwmwfcmp(&wf_set, &wf_rounded) != 0) + dev_err(&chip->dev, + "Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n", + wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns, + wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns, + wf_set.duty_length_ns, wf_set.period_length_ns, wf_set.duty_offset_ns); + } + return 0; +} + +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, + const struct pwm_waveform *wf, bool exact) +{ + struct pwm_chip *chip = pwm->chip; + int err; + + might_sleep(); + + guard(pwmchip)(chip); + + if (!chip->operational) + return -ENODEV; + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) { + /* + * Catch any drivers that have been marked as atomic but + * that will sleep anyway. + */ + non_block_start(); + err = __pwm_set_waveform(pwm, wf, exact); + non_block_end(); + } else { + err = __pwm_set_waveform(pwm, wf, exact); + } + + return err; +} +EXPORT_SYMBOL_GPL(pwm_set_waveform_might_sleep); + static void pwm_apply_debug(struct pwm_device *pwm, const struct pwm_state *state) { diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 6a26a5210dab..40cef0bc0de7 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -358,7 +358,11 @@ static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data) } #if IS_ENABLED(CONFIG_PWM) -/* PWM user APIs */ + +/* PWM consumer APIs */ +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf); +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf); +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact); int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state); int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state); int pwm_adjust_config(struct pwm_device *pwm);
Provide API functions for consumers to work with waveforms. Note that one relevant difference between pwm_get_state() and pwm_get_waveform*() is that the latter yields the actually configured hardware state, while the former yields the last state passed to pwm_apply*() and so doesn't account for hardware specific rounding. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com> --- drivers/pwm/core.c | 201 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/pwm.h | 6 +- 2 files changed, 206 insertions(+), 1 deletion(-)