| Message ID | 734c9985a6f54d34d9ef20203ba7f962b572cb45.1701248996.git.sean@mess.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve pwm-ir-tx precision | expand |
On Wed, Nov 29, 2023 at 09:13:35AM +0000, Sean Young wrote: > Some pwm devices require sleeping, for example if the pwm device is > connected over i2c. However, many pwm devices could be used from atomic > context, e.g. memmory mapped pwm. This is useful for, for example, the > pwm-ir-tx driver which requires precise timing. Sleeping causes havoc > with the generated IR signal. > > Since not all pmw devices can support atomic context, we also add a > pwm_is_atomic() function to check if it is supported. s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/ > > Signed-off-by: Sean Young <sean@mess.org> > --- > Documentation/driver-api/pwm.rst | 9 +++++ > drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ > drivers/pwm/pwm-renesas-tpu.c | 1 - > include/linux/pwm.h | 29 ++++++++++++++- > 4 files changed, 87 insertions(+), 15 deletions(-) > > diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst > index f1d8197c8c43..1d4536fdf47c 100644 > --- a/Documentation/driver-api/pwm.rst > +++ b/Documentation/driver-api/pwm.rst > @@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: > > int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); > > +Some PWM devices can be used from atomic context. You can check if this is > +supported with:: > + > + bool pwm_is_atomic(struct pwm_device *pwm); This is now going to look a bit odd. I think it'd be best to change this to pwm_might_sleep() for consistency with the pwm_apply_might_sleep() function. Fine to keep the actual member variable as atomic, though, so we don't have to change the default for all drivers. > + > +If true, the PWM can be configured from atomic context with:: > + > + int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state); > + > This API controls both the PWM period/duty_cycle config and the > enable/disable state. > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index fc92ba622e56..63174e207400 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -463,24 +463,15 @@ static void pwm_apply_debug(struct pwm_device *pwm, > } > > /** > - * pwm_apply_might_sleep() - atomically apply a new state to a PWM device > + * pwm_apply_unchecked() - atomically apply a new state to a PWM device > * @pwm: PWM device > * @state: new state to apply > */ > -int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) > +static int pwm_apply_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_might_sleep() 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; > @@ -501,16 +492,64 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) > > pwm->state = *state; > > + return 0; > +} > + > +/** > + * pwm_apply_might_sleep() - 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_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) > +{ > + 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_might_sleep() 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->atomic) { > + /* > + * Catch any sleeping drivers when atomic is set. I think this is slightly confusing. What we're really trying to catch here is drivers that are marked as "atomic" but which still end up sleeping during ->apply(), right? So maybe something like this would be a bit more explicit: /* * Catch any drivers that have been marked as atomic but * that will sleep anyway. */ > +/** > + * pwm_apply_atomic() - apply a new state to a PWM device from atomic context > + * Not all pwm devices support this function, check with pwm_is_atomic(). s/pwm/PWM/ here... > + * @pwm: PWM device > + * @state: new state to apply > + */ > +int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state) > +{ > + WARN_ONCE(!pwm->chip->atomic, > + "sleeping pwm driver used in atomic context"); ... and in the warning message as well. > + > + return pwm_apply_unchecked(pwm, state); > +} > +EXPORT_SYMBOL_GPL(pwm_apply_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 4239f2c3e8b2..47ea92cd8c67 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 5c5c456948a4..f1fa1243e95a 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -286,6 +286,7 @@ struct pwm_ops { > * @npwm: number of PWMs controlled by this chip > * @of_xlate: request a PWM device given a device tree PWM specifier > * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier > + * @atomic: can the driver call pwm_apply_atomic in atomic context Maybe: "can the driver's ->apply() be called in atomic context"? > * @list: list node for internal use > * @pwms: array of PWM devices allocated by the framework > */ > @@ -299,6 +300,7 @@ struct pwm_chip { > struct pwm_device * (*of_xlate)(struct pwm_chip *chip, > const struct of_phandle_args *args); > unsigned int of_pwm_n_cells; > + bool atomic; > > /* only used internally by the PWM framework */ > struct list_head list; > @@ -308,6 +310,7 @@ struct pwm_chip { > #if IS_ENABLED(CONFIG_PWM) > /* PWM user APIs */ > 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); > > /** > @@ -378,6 +381,17 @@ static inline void pwm_disable(struct pwm_device *pwm) > pwm_apply_might_sleep(pwm, &state); > } > > +/** > + * pwm_is_atomic() - is pwm_apply_atomic() supported? > + * @pwm: PWM device > + * > + * Returns: true pwm_apply_atomic() can be called from atomic context. > + */ > +static inline bool pwm_is_atomic(struct pwm_device *pwm) As I mentioned above, I think it'd be more consistent to call this pwm_might_sleep(). > +{ > + return pwm->chip->atomic; > +} > + > /* PWM provider APIs */ > int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, > unsigned long timeout); > @@ -406,16 +420,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_might_sleep(struct pwm_device *pwm, > const struct pwm_state *state) > { > might_sleep(); > - return -ENOTSUPP; > + return -EOPNOTSUPP; > +} > + > +static inline int pwm_apply_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; > } What's wrong with ENOTSUPP? Thierry
Hi Thierry, On Fri, Dec 08, 2023 at 05:19:52PM +0100, Thierry Reding wrote: > On Wed, Nov 29, 2023 at 09:13:35AM +0000, Sean Young wrote: > > Some pwm devices require sleeping, for example if the pwm device is > > connected over i2c. However, many pwm devices could be used from atomic > > context, e.g. memmory mapped pwm. This is useful for, for example, the > > pwm-ir-tx driver which requires precise timing. Sleeping causes havoc > > with the generated IR signal. > > > > Since not all pmw devices can support atomic context, we also add a > > pwm_is_atomic() function to check if it is supported. > > s/i2c/I2C/ and s/pwm/PWM/ in the above. Also, s/memmory/memory/ Thanks for your detailed review. I agree with all your points, they are all nice improvements. Just a question at the bottom: > > > > > Signed-off-by: Sean Young <sean@mess.org> > > --- > > Documentation/driver-api/pwm.rst | 9 +++++ > > drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ > > drivers/pwm/pwm-renesas-tpu.c | 1 - > > include/linux/pwm.h | 29 ++++++++++++++- > > 4 files changed, 87 insertions(+), 15 deletions(-) > > > > diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst > > index f1d8197c8c43..1d4536fdf47c 100644 > > --- a/Documentation/driver-api/pwm.rst > > +++ b/Documentation/driver-api/pwm.rst > > @@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: > > > > int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); > > > > +Some PWM devices can be used from atomic context. You can check if this is > > +supported with:: > > + > > + bool pwm_is_atomic(struct pwm_device *pwm); > > This is now going to look a bit odd. I think it'd be best to change this > to pwm_might_sleep() for consistency with the pwm_apply_might_sleep() > function. Fine to keep the actual member variable as atomic, though, so > we don't have to change the default for all drivers. Agreed, I was struggling with finding good name and yours is much better, thanks. > +{ > > + return pwm->chip->atomic; > > +} > > + > > /* PWM provider APIs */ > > int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, > > unsigned long timeout); > > @@ -406,16 +420,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_might_sleep(struct pwm_device *pwm, > > const struct pwm_state *state) > > { > > might_sleep(); > > - return -ENOTSUPP; > > + return -EOPNOTSUPP; > > +} > > + > > +static inline int pwm_apply_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; > > } > > What's wrong with ENOTSUPP? This was found by checkpatch, see https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl#L4891-L4892 # ENOTSUPP is not a standard error code and should be avoided in new patches. # Folks usually mean EOPNOTSUPP (also called ENOTSUP), when they type ENOTSUPP. ENOTSUPP is not really widely used in the tree. So it was really done to keep checkpatch happy, please let me know what you would like me to do here. Thanks, Sean
diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst index f1d8197c8c43..1d4536fdf47c 100644 --- a/Documentation/driver-api/pwm.rst +++ b/Documentation/driver-api/pwm.rst @@ -43,6 +43,15 @@ After being requested, a PWM has to be configured using:: int pwm_apply_might_sleep(struct pwm_device *pwm, struct pwm_state *state); +Some PWM devices can be used from atomic context. You can check if this is +supported with:: + + bool pwm_is_atomic(struct pwm_device *pwm); + +If true, the PWM can be configured from atomic context with:: + + int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state); + This API controls both the PWM period/duty_cycle config and the enable/disable state. diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index fc92ba622e56..63174e207400 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -463,24 +463,15 @@ static void pwm_apply_debug(struct pwm_device *pwm, } /** - * pwm_apply_might_sleep() - atomically apply a new state to a PWM device + * pwm_apply_unchecked() - atomically apply a new state to a PWM device * @pwm: PWM device * @state: new state to apply */ -int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) +static int pwm_apply_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_might_sleep() 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; @@ -501,16 +492,64 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) pwm->state = *state; + return 0; +} + +/** + * pwm_apply_might_sleep() - 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_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) +{ + 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_might_sleep() 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->atomic) { + /* + * Catch any sleeping drivers when atomic is set. + */ + non_block_start(); + err = pwm_apply_unchecked(pwm, state); + non_block_end(); + } else { + err = pwm_apply_unchecked(pwm, state); + } + /* * only do this after pwm->state was applied as some * implementations of .get_state depend on this */ pwm_apply_debug(pwm, state); - return 0; + return err; } EXPORT_SYMBOL_GPL(pwm_apply_might_sleep); +/** + * pwm_apply_atomic() - apply a new state to a PWM device from atomic context + * Not all pwm devices support this function, check with pwm_is_atomic(). + * @pwm: PWM device + * @state: new state to apply + */ +int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state) +{ + WARN_ONCE(!pwm->chip->atomic, + "sleeping pwm driver used in atomic context"); + + return pwm_apply_unchecked(pwm, state); +} +EXPORT_SYMBOL_GPL(pwm_apply_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 4239f2c3e8b2..47ea92cd8c67 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 5c5c456948a4..f1fa1243e95a 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -286,6 +286,7 @@ struct pwm_ops { * @npwm: number of PWMs controlled by this chip * @of_xlate: request a PWM device given a device tree PWM specifier * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier + * @atomic: can the driver call pwm_apply_atomic in atomic context * @list: list node for internal use * @pwms: array of PWM devices allocated by the framework */ @@ -299,6 +300,7 @@ struct pwm_chip { struct pwm_device * (*of_xlate)(struct pwm_chip *chip, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; + bool atomic; /* only used internally by the PWM framework */ struct list_head list; @@ -308,6 +310,7 @@ struct pwm_chip { #if IS_ENABLED(CONFIG_PWM) /* PWM user APIs */ 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); /** @@ -378,6 +381,17 @@ static inline void pwm_disable(struct pwm_device *pwm) pwm_apply_might_sleep(pwm, &state); } +/** + * pwm_is_atomic() - is pwm_apply_atomic() supported? + * @pwm: PWM device + * + * Returns: true pwm_apply_atomic() can be called from atomic context. + */ +static inline bool pwm_is_atomic(struct pwm_device *pwm) +{ + return pwm->chip->atomic; +} + /* PWM provider APIs */ int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result, unsigned long timeout); @@ -406,16 +420,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_might_sleep(struct pwm_device *pwm, const struct pwm_state *state) { might_sleep(); - return -ENOTSUPP; + return -EOPNOTSUPP; +} + +static inline int pwm_apply_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 pwm devices require sleeping, for example if the pwm device is connected over i2c. However, many pwm devices could be used from atomic context, e.g. memmory mapped pwm. This is useful for, for example, the pwm-ir-tx driver which requires precise timing. Sleeping causes havoc with the generated IR signal. Since not all pmw devices can support atomic context, we also add a pwm_is_atomic() function to check if it is supported. Signed-off-by: Sean Young <sean@mess.org> --- Documentation/driver-api/pwm.rst | 9 +++++ drivers/pwm/core.c | 63 ++++++++++++++++++++++++++------ drivers/pwm/pwm-renesas-tpu.c | 1 - include/linux/pwm.h | 29 ++++++++++++++- 4 files changed, 87 insertions(+), 15 deletions(-)