| Message ID | 9c0f1616fca5b218336b9321bfefe7abb7e1749f.1697193646.git.sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve pwm-ir-tx precision | expand |
On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote: [...] > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index d2f9f690a9c1..93f166ab03c1 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -267,6 +267,7 @@ struct pwm_capture { > * @get_state: get the current PWM state. This function is only > * called once per PWM device when the PWM chip is > * registered. > + * @atomic: can the driver execute pwm_apply_state in atomic context > * @owner: helps prevent removal of modules exporting active PWMs > */ > struct pwm_ops { > @@ -278,6 +279,7 @@ struct pwm_ops { > const struct pwm_state *state); > int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, > struct pwm_state *state); > + bool atomic; > struct module *owner; > }; As I mentioned earlier, this really belongs in struct pwm_chip rather than struct pwm_ops. I know that Uwe said this is unlikely to happen, and that may be true, but at the same time it's not like I'm asking much. Whether you put this in struct pwm_ops or struct pwm_chip is about the same amount of code, and putting it into pwm_chip is much more flexible, so it's really a no-brainer. Thierry
On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote: > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote: > [...] > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > index d2f9f690a9c1..93f166ab03c1 100644 > > --- a/include/linux/pwm.h > > +++ b/include/linux/pwm.h > > @@ -267,6 +267,7 @@ struct pwm_capture { > > * @get_state: get the current PWM state. This function is only > > * called once per PWM device when the PWM chip is > > * registered. > > + * @atomic: can the driver execute pwm_apply_state in atomic context > > * @owner: helps prevent removal of modules exporting active PWMs > > */ > > struct pwm_ops { > > @@ -278,6 +279,7 @@ struct pwm_ops { > > const struct pwm_state *state); > > int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, > > struct pwm_state *state); > > + bool atomic; > > struct module *owner; > > }; > > As I mentioned earlier, this really belongs in struct pwm_chip rather > than struct pwm_ops. I know that Uwe said this is unlikely to happen, > and that may be true, but at the same time it's not like I'm asking > much. Whether you put this in struct pwm_ops or struct pwm_chip is > about the same amount of code, and putting it into pwm_chip is much > more flexible, so it's really a no-brainer. Happy to change this of course. I changed it and then changed it back after Uwe's comment, I'll fix this in the next version. One tiny advantage is that pwm_ops is static const while pwm_chip is allocated per-pwm, so will need instructions for setting the value. Having said that, the difference is tiny, it's a single bool. Sean
On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote: > On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote: > > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote: > > [...] > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > index d2f9f690a9c1..93f166ab03c1 100644 > > > --- a/include/linux/pwm.h > > > +++ b/include/linux/pwm.h > > > @@ -267,6 +267,7 @@ struct pwm_capture { > > > * @get_state: get the current PWM state. This function is only > > > * called once per PWM device when the PWM chip is > > > * registered. > > > + * @atomic: can the driver execute pwm_apply_state in atomic context > > > * @owner: helps prevent removal of modules exporting active PWMs > > > */ > > > struct pwm_ops { > > > @@ -278,6 +279,7 @@ struct pwm_ops { > > > const struct pwm_state *state); > > > int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, > > > struct pwm_state *state); > > > + bool atomic; > > > struct module *owner; > > > }; > > > > As I mentioned earlier, this really belongs in struct pwm_chip rather > > than struct pwm_ops. I know that Uwe said this is unlikely to happen, > > and that may be true, but at the same time it's not like I'm asking > > much. Whether you put this in struct pwm_ops or struct pwm_chip is > > about the same amount of code, and putting it into pwm_chip is much > > more flexible, so it's really a no-brainer. > > Happy to change this of course. I changed it and then changed it back after > Uwe's comment, I'll fix this in the next version. > > One tiny advantage is that pwm_ops is static const while pwm_chip is > allocated per-pwm, so will need instructions for setting the value. Having > said that, the difference is tiny, it's a single bool. Yeah, it's typically a single assignment, so from a code point of view it should be pretty much the same. I suppose from an instruction level point of view, yes, this might add a teeny-tiny bit of overhead. On the other hand it lets us do interesting things like initialize chip->atomic = !regmap_might_sleep() for those drivers that use regmap and then not worry about it any longer. Given that, I'm also wondering if we should try to keep the terminology a bit more consistent. "Atomic" is somewhat overloaded because ->apply() and ->get_state() are part of the "atomic" PWM API (in the sense that applying changes are done as a single, atomic operation, rather than in the sense of "non-sleeping" operation). So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird. On the other hand it's a bit tedious to convert all existing users to pwm_apply_state_might_sleep(). Perhaps as a compromise we can add pwm_apply_state_might_sleep() and make pwm_apply_state() a (deprecated) alias for that, so that existing drivers can be converted one by one. Eventually we would then end up with both pwm_apply_state_might_sleep() and pwm_apply_state_atomic(), which has the nice side-effect of these being unambiguous. That doesn't get rid of the ambiguity of that _atomic() suffix, but I can probably live with that one. It's used for this same meaning in other contexts and if we add a _might_sleep() variant it becomes clearer how the two are different. Anyway, the bottom line is that I'd prefer the "atomic" field to be renamed "might_sleep". It'd also be nice to add the new _might_sleep() variant since you're already changing all of this anyway. No need to mass-convert all the drivers to the _might_sleep() variant yet, though, since we can have a transitional alias for that. Of course feel free to give it a shot if you feel like it. Thierry
Hello, On Fri, Oct 13, 2023 at 05:34:34PM +0200, Thierry Reding wrote: > On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote: > > On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote: > > > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote: > > > [...] > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > > index d2f9f690a9c1..93f166ab03c1 100644 > > > > --- a/include/linux/pwm.h > > > > +++ b/include/linux/pwm.h > > > > @@ -267,6 +267,7 @@ struct pwm_capture { > > > > * @get_state: get the current PWM state. This function is only > > > > * called once per PWM device when the PWM chip is > > > > * registered. > > > > + * @atomic: can the driver execute pwm_apply_state in atomic context > > > > * @owner: helps prevent removal of modules exporting active PWMs > > > > */ > > > > struct pwm_ops { > > > > @@ -278,6 +279,7 @@ struct pwm_ops { > > > > const struct pwm_state *state); > > > > int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, > > > > struct pwm_state *state); > > > > + bool atomic; > > > > struct module *owner; > > > > }; > > > > > > As I mentioned earlier, this really belongs in struct pwm_chip rather > > > than struct pwm_ops. I know that Uwe said this is unlikely to happen, > > > and that may be true, but at the same time it's not like I'm asking > > > much. Whether you put this in struct pwm_ops or struct pwm_chip is > > > about the same amount of code, and putting it into pwm_chip is much > > > more flexible, so it's really a no-brainer. > > > > Happy to change this of course. I changed it and then changed it back after > > Uwe's comment, I'll fix this in the next version. > > > > One tiny advantage is that pwm_ops is static const while pwm_chip is > > allocated per-pwm, so will need instructions for setting the value. Having > > said that, the difference is tiny, it's a single bool. > > Yeah, it's typically a single assignment, so from a code point of view > it should be pretty much the same. I suppose from an instruction level > point of view, yes, this might add a teeny-tiny bit of overhead. > > On the other hand it lets us do interesting things like initialize > chip->atomic = !regmap_might_sleep() for those drivers that use regmap > and then not worry about it any longer. > > Given that, I'm also wondering if we should try to keep the terminology > a bit more consistent. "Atomic" is somewhat overloaded because ->apply() > and ->get_state() are part of the "atomic" PWM API (in the sense that > applying changes are done as a single, atomic operation, rather than in > the sense of "non-sleeping" operation). > > So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird. > On the other hand it's a bit tedious to convert all existing users to > pwm_apply_state_might_sleep(). > > Perhaps as a compromise we can add pwm_apply_state_might_sleep() and > make pwm_apply_state() a (deprecated) alias for that, so that existing > drivers can be converted one by one. To throw in my green for our bike shed: I'd pick pwm_apply_state_cansleep() to match what gpio does (with gpiod_set_value_cansleep()). (Though I have to admit that semantically Thierry's might_sleep is nicer as it matches might_sleep().) If we don't want to have an explicit indicator for the atomic/fast variant (again similar to the gpio framework), maybe we can drop "_state" which I think is somehow redundant and go for: pwm_apply (fast) pwm_apply_cansleep (sleeping) pwm_apply_state (compat alias for pwm_apply_cansleep()) (maybe replace cansleep with might_sleep). Similar for pwm_get_state() we could use the opportunity and make pwm_get() actually call ->get_state() and introduce pwm_get_lastapplied() with the semantic of todays pwm_get_state(). Do we need a pwm_get_cansleep/might_sleep()? Best regards Uwe
Hello, On Fri, Oct 13, 2023 at 08:04:49PM +0200, Uwe Kleine-König wrote: > On Fri, Oct 13, 2023 at 05:34:34PM +0200, Thierry Reding wrote: > > On Fri, Oct 13, 2023 at 03:58:30PM +0100, Sean Young wrote: > > > On Fri, Oct 13, 2023 at 01:51:40PM +0200, Thierry Reding wrote: > > > > On Fri, Oct 13, 2023 at 11:46:14AM +0100, Sean Young wrote: > > > > [...] > > > > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > > > > > index d2f9f690a9c1..93f166ab03c1 100644 > > > > > --- a/include/linux/pwm.h > > > > > +++ b/include/linux/pwm.h > > > > > @@ -267,6 +267,7 @@ struct pwm_capture { > > > > > * @get_state: get the current PWM state. This function is only > > > > > * called once per PWM device when the PWM chip is > > > > > * registered. > > > > > + * @atomic: can the driver execute pwm_apply_state in atomic context > > > > > * @owner: helps prevent removal of modules exporting active PWMs > > > > > */ > > > > > struct pwm_ops { > > > > > @@ -278,6 +279,7 @@ struct pwm_ops { > > > > > const struct pwm_state *state); > > > > > int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, > > > > > struct pwm_state *state); > > > > > + bool atomic; > > > > > struct module *owner; > > > > > }; > > > > > > > > As I mentioned earlier, this really belongs in struct pwm_chip rather > > > > than struct pwm_ops. I know that Uwe said this is unlikely to happen, > > > > and that may be true, but at the same time it's not like I'm asking > > > > much. Whether you put this in struct pwm_ops or struct pwm_chip is > > > > about the same amount of code, and putting it into pwm_chip is much > > > > more flexible, so it's really a no-brainer. > > > > > > Happy to change this of course. I changed it and then changed it back after > > > Uwe's comment, I'll fix this in the next version. > > > > > > One tiny advantage is that pwm_ops is static const while pwm_chip is > > > allocated per-pwm, so will need instructions for setting the value. Having > > > said that, the difference is tiny, it's a single bool. > > > > Yeah, it's typically a single assignment, so from a code point of view > > it should be pretty much the same. I suppose from an instruction level > > point of view, yes, this might add a teeny-tiny bit of overhead. > > > > On the other hand it lets us do interesting things like initialize > > chip->atomic = !regmap_might_sleep() for those drivers that use regmap > > and then not worry about it any longer. Sure. > > Given that, I'm also wondering if we should try to keep the terminology > > a bit more consistent. "Atomic" is somewhat overloaded because ->apply() > > and ->get_state() are part of the "atomic" PWM API (in the sense that > > applying changes are done as a single, atomic operation, rather than in > > the sense of "non-sleeping" operation). Good point. > > So pwm_apply_state_atomic() is then doubly atomic, which is a bit weird. > > On the other hand it's a bit tedious to convert all existing users to > > pwm_apply_state_might_sleep(). > > > > Perhaps as a compromise we can add pwm_apply_state_might_sleep() and > > make pwm_apply_state() a (deprecated) alias for that, so that existing > > drivers can be converted one by one. > > To throw in my green for our bike shed: I'd pick > > pwm_apply_state_cansleep() > > to match what gpio does (with gpiod_set_value_cansleep()). (Though I > have to admit that semantically Thierry's might_sleep is nicer as it > matches might_sleep().) I have to agree here. "can" is shorter and clearer than "might", since "can" expresses capability. Having said that the mixture of nomenclature is not great, so there is very little between them. > If we don't want to have an explicit indicator for the atomic/fast > variant (again similar to the gpio framework), maybe we can drop > "_state" which I think is somehow redundant and go for: > > pwm_apply (fast) > pwm_apply_cansleep (sleeping) > pwm_apply_state (compat alias for pwm_apply_cansleep()) > > (maybe replace cansleep with might_sleep). This is very nice. :) There are callsites in 15 files, might as well rename it and do away with pwm_apply_state() > Similar for pwm_get_state() > we could use the opportunity and make > > pwm_get() > > actually call ->get_state() and introduce > > pwm_get_lastapplied() > > with the semantic of todays pwm_get_state(). Do we need a > pwm_get_cansleep/might_sleep()? Not all drivers implement .get_state(), how would we handle those? Thanks, Sean
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index dc66e3405bf5..241510ba1823 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -489,24 +489,15 @@ static void pwm_apply_state_debug(struct pwm_device *pwm, } /** - * pwm_apply_state() - atomically apply a new state to a PWM device + * pwm_apply_state_unchecked() - atomically apply a new state to a PWM device * @pwm: PWM device * @state: new state to apply */ -int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) +static int pwm_apply_state_unchecked(struct pwm_device *pwm, const struct pwm_state *state) { struct pwm_chip *chip; int err; - /* - * Some lowlevel driver's implementations of .apply() make use of - * mutexes, also with some drivers only returning when the new - * configuration is active calling pwm_apply_state() from atomic context - * is a bad idea. So make it explicit that calling this function might - * sleep. - */ - might_sleep(); - if (!pwm || !state || !state->period || state->duty_cycle > state->period) return -EINVAL; @@ -535,8 +526,57 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) return 0; } + +/** + * pwm_apply_state() - atomically apply a new state to a PWM device + * Cannot be used in atomic context. + * @pwm: PWM device + * @state: new state to apply + */ +int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) +{ + /* + * Some lowlevel driver's implementations of .apply() make use of + * mutexes, also with some drivers only returning when the new + * configuration is active calling pwm_apply_state() from atomic context + * is a bad idea. So make it explicit that calling this function might + * sleep. + */ + might_sleep(); + + if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->ops->atomic) { + int err; + + /* + * Catch any sleeping drivers when atomic is set. + */ + non_block_start(); + err = pwm_apply_state_unchecked(pwm, state); + non_block_end(); + + return err; + } + + return pwm_apply_state_unchecked(pwm, state); +} EXPORT_SYMBOL_GPL(pwm_apply_state); +/** + * pwm_apply_state_atomic() - atomically apply a new state to a PWM device + * Can be used from atomic context. + * @pwm: PWM device + * @state: new state to apply + */ +int pwm_apply_state_atomic(struct pwm_device *pwm, + const struct pwm_state *state) +{ + WARN_ONCE(!pwm->chip->ops->atomic, + "sleeping pwm driver used in atomic context"); + + return pwm_apply_state_unchecked(pwm, state); +} +EXPORT_SYMBOL_GPL(pwm_apply_state_atomic); + /** * pwm_capture() - capture and report a PWM signal * @pwm: PWM device diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c index d7311614c846..96797a33d8c6 100644 --- a/drivers/pwm/pwm-renesas-tpu.c +++ b/drivers/pwm/pwm-renesas-tpu.c @@ -11,7 +11,6 @@ #include <linux/init.h> #include <linux/ioport.h> #include <linux/module.h> -#include <linux/mutex.h> #include <linux/of.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> diff --git a/include/linux/pwm.h b/include/linux/pwm.h index d2f9f690a9c1..93f166ab03c1 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -267,6 +267,7 @@ struct pwm_capture { * @get_state: get the current PWM state. This function is only * called once per PWM device when the PWM chip is * registered. + * @atomic: can the driver execute pwm_apply_state in atomic context * @owner: helps prevent removal of modules exporting active PWMs */ struct pwm_ops { @@ -278,6 +279,7 @@ struct pwm_ops { const struct pwm_state *state); int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state); + bool atomic; struct module *owner; }; @@ -310,6 +312,7 @@ struct pwm_chip { #if IS_ENABLED(CONFIG_PWM) /* PWM user APIs */ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state); +int pwm_apply_state_atomic(struct pwm_device *pwm, const struct pwm_state *state); int pwm_adjust_config(struct pwm_device *pwm); /** @@ -380,6 +383,17 @@ static inline void pwm_disable(struct pwm_device *pwm) pwm_apply_state(pwm, &state); } +/** + * pwm_is_atomic() - is pwm_apply_state_atomic() supported? + * @pwm: PWM device + * + * Returns: true pwm_apply_state_atomic() can be called from atomic context. + */ +static inline bool pwm_is_atomic(struct pwm_device *pwm) +{ + return pwm->chip->ops->atomic; +} + /* PWM provider APIs */ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); @@ -408,16 +422,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev, struct fwnode_handle *fwnode, const char *con_id); #else +static inline bool pwm_is_atomic(struct pwm_device *pwm) +{ + return false; +} + static inline int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state) { might_sleep(); - return -ENOTSUPP; + return -EOPNOTSUPP; +} + +static inline int pwm_apply_state_atomic(struct pwm_device *pwm, + const struct pwm_state *state) +{ + return -EOPNOTSUPP; } static inline int pwm_adjust_config(struct pwm_device *pwm) { - return -ENOTSUPP; + return -EOPNOTSUPP; } static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
Some drivers require sleeping, for example if the pwm device is connected over i2c. The pwm-ir-tx requires precise timing, and sleeping causes havoc with the generated IR signal when sleeping occurs. This patch makes it possible to use pwm when the driver does not sleep, by introducing the pwm_can_sleep() function. Signed-off-by: Sean Young <sean@mess.org> --- drivers/pwm/core.c | 62 ++++++++++++++++++++++++++++------- drivers/pwm/pwm-renesas-tpu.c | 1 - include/linux/pwm.h | 29 ++++++++++++++-- 3 files changed, 78 insertions(+), 14 deletions(-)