On Mon, Dec 11, 2023 at 08:24:53AM +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. memory 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 PWM devices can support atomic context, we also add a
> pwm_might_sleep() function to check if is not supported.
>
> Signed-off-by: Sean Young <sean@xxxxxxxx>
> ---
> Documentation/driver-api/pwm.rst | 9 +++++
> MAINTAINERS | 2 +-
> drivers/pwm/core.c | 64 ++++++++++++++++++++++++++------
> drivers/pwm/pwm-renesas-tpu.c | 1 -
> include/linux/pwm.h | 29 ++++++++++++++-
> 5 files changed, 89 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> index f1d8197c8c430..3c28ccc4b6113 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -46,6 +46,15 @@ After being requested, a PWM has to be configured using::
> This API controls both the PWM period/duty_cycle config and the
> enable/disable state.
>
> +PWM devices can be used from atomic context, if the PWM does not sleep. You
> +can check if this the case with::
> +
> + bool pwm_might_sleep(struct pwm_device *pwm);
> +
> +If false, the PWM can also be configured from atomic context with::
> +
> + int pwm_apply_atomic(struct pwm_device *pwm, struct pwm_state *state);
> +
> As a consumer, don't rely on the output's state for a disabled PWM. If it's
> easily possible, drivers are supposed to emit the inactive state, but some
> drivers cannot. If you rely on getting the inactive state, use .duty_cycle=0,
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c2a9e0b5594e7..b55ac220b923d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -17585,7 +17585,7 @@ F: drivers/video/backlight/pwm_bl.c
> F: include/dt-bindings/pwm/
> F: include/linux/pwm.h
> F: include/linux/pwm_backlight.h
> -K: pwm_(config|apply_might_sleep|ops)
> +K: pwm_(config|apply_might_sleep|apply_atomic|ops)
>
> PXA GPIO DRIVER
> M: Robert Jarzmik <robert.jarzmik@xxxxxxx>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c2d78136625d5..5fd35cda5786b 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -433,24 +433,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;
> @@ -471,16 +462,65 @@ 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 drivers that have been marked as atomic but
> + * that will sleep anyway.
> + */
> + 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);
I think this is broken. Currently pwm_apply_state_debug() is only called
if chip->ops->apply() succeeds.
> - 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_might_sleep().
> + * @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 ce92db1f85113..28265fdfc92a9 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>
Unrelated change
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index b64b8a82415c4..495af3627939c 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -285,6 +285,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's ->apply() be called in atomic context
> * @pwms: array of PWM devices allocated by the framework
> */
> struct pwm_chip {
> @@ -297,6 +298,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 pwm_device *pwms;
> @@ -305,6 +307,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);
>
> /**
> @@ -375,6 +378,17 @@ static inline void pwm_disable(struct pwm_device *pwm)
> pwm_apply_might_sleep(pwm, &state);
> }
>
> +/**
> + * pwm_might_sleep() - is pwm_apply_atomic() supported?
> + * @pwm: PWM device
> + *
> + * Returns: false if pwm_apply_atomic() can be called from atomic context.
> + */
> +static inline bool pwm_might_sleep(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);
> @@ -403,16 +417,27 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
> struct fwnode_handle *fwnode,
> const char *con_id);
> #else
> +static inline bool pwm_might_sleep(struct pwm_device *pwm)
> +{
> + return true;
> +}
> +
> 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;
I'd do s/-ENOTSUPP/-EOPNOTSUPP/ in a separate change.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
