[PATCH v3 0/3] Improve pwm-ir-tx precision

[PATCH v3 0/3] Improve pwm-ir-tx precision

[Sean Young]

The pwm-ir-tx driver has to turn the pwm signal on and off, and suffers
from delays as this is done in process context. Make this work in atomic
context.

changes:

v3:
 - Renamed pwm_apply_state() to pwm_apply_cansleep()
 - Moved pwm atomic to struct pwm_chip
 - Fixed pwm docs
 - Other minor code review comments

v2:
 - Simplified the pwm ir tx based on Ivaylo Dimitrov suggestions
 - Renamed pwm_can_sleep() to pwm_is_atomic
 - Fix sleeping issue in pwm-bcm2835 (clk_get_rate() can sleep)
 - use non_block_start() in pwm to check atomic-ness

Sean Young (3):
  pwm: make it possible to apply pwm changes in atomic context
  pwm: bcm2835: allow pwm driver to be used in atomic context
  media: pwm-ir-tx: trigger edges from hrtimer interrupt context

 Documentation/driver-api/pwm.rst              | 16 +++-
 .../gpu/drm/i915/display/intel_backlight.c    |  6 +-
 drivers/gpu/drm/solomon/ssd130x.c             |  2 +-
 drivers/hwmon/pwm-fan.c                       |  8 +-
 drivers/input/misc/da7280.c                   |  4 +-
 drivers/input/misc/pwm-beeper.c               |  4 +-
 drivers/input/misc/pwm-vibra.c                |  8 +-
 drivers/leds/leds-pwm.c                       |  2 +-
 drivers/leds/rgb/leds-pwm-multicolor.c        |  4 +-
 drivers/media/rc/pwm-ir-tx.c                  | 83 +++++++++++++++++--
 drivers/platform/x86/lenovo-yogabook.c        |  2 +-
 drivers/pwm/core.c                            | 75 +++++++++++++----
 drivers/pwm/pwm-bcm2835.c                     | 30 ++++---
 drivers/pwm/pwm-renesas-tpu.c                 |  1 -
 drivers/pwm/pwm-twl-led.c                     |  2 +-
 drivers/pwm/pwm-vt8500.c                      |  2 +-
 drivers/pwm/sysfs.c                           | 10 +--
 drivers/regulator/pwm-regulator.c             |  4 +-
 drivers/video/backlight/lm3630a_bl.c          |  2 +-
 drivers/video/backlight/lp855x_bl.c           |  2 +-
 drivers/video/backlight/pwm_bl.c              |  6 +-
 drivers/video/fbdev/ssd1307fb.c               |  2 +-
 include/linux/pwm.h                           | 57 +++++++++----
 23 files changed, 243 insertions(+), 89 deletions(-)

-- 
2.42.0

[PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

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>
---
 Documentation/driver-api/pwm.rst              | 16 +++-
 .../gpu/drm/i915/display/intel_backlight.c    |  6 +-
 drivers/gpu/drm/solomon/ssd130x.c             |  2 +-
 drivers/hwmon/pwm-fan.c                       |  8 +-
 drivers/input/misc/da7280.c                   |  4 +-
 drivers/input/misc/pwm-beeper.c               |  4 +-
 drivers/input/misc/pwm-vibra.c                |  8 +-
 drivers/leds/leds-pwm.c                       |  2 +-
 drivers/leds/rgb/leds-pwm-multicolor.c        |  4 +-
 drivers/media/rc/pwm-ir-tx.c                  |  4 +-
 drivers/platform/x86/lenovo-yogabook.c        |  2 +-
 drivers/pwm/core.c                            | 75 ++++++++++++++-----
 drivers/pwm/pwm-renesas-tpu.c                 |  1 -
 drivers/pwm/pwm-twl-led.c                     |  2 +-
 drivers/pwm/pwm-vt8500.c                      |  2 +-
 drivers/pwm/sysfs.c                           | 10 +--
 drivers/regulator/pwm-regulator.c             |  4 +-
 drivers/video/backlight/lm3630a_bl.c          |  2 +-
 drivers/video/backlight/lp855x_bl.c           |  2 +-
 drivers/video/backlight/pwm_bl.c              |  6 +-
 drivers/video/fbdev/ssd1307fb.c               |  2 +-
 include/linux/pwm.h                           | 57 ++++++++++----
 22 files changed, 147 insertions(+), 76 deletions(-)

diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
index 3fdc95f7a1d15..a2fb5f8f6e1f8 100644
--- a/Documentation/driver-api/pwm.rst
+++ b/Documentation/driver-api/pwm.rst
@@ -41,7 +41,15 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist.
 
 After being requested, a PWM has to be configured using::
 
-	int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
+	int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
+
+If the PWM support atomic mode, which can be determined with::
+
+        bool pwm_is_atomic(struct pwm_device *pwm);
+
+Then the PWM can be configured with::
+
+	int pwm_apply(struct pwm_device *pwm, struct pwm_state *state);
 
 This API controls both the PWM period/duty_cycle config and the
 enable/disable state.
@@ -57,13 +65,13 @@ If supported by the driver, the signal can be optimized, for example to improve
 EMI by phase shifting the individual channels of a chip.
 
 The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
-around pwm_apply_state() and should not be used if the user wants to change
+around pwm_apply_cansleep() and should not be used if the user wants to change
 several parameter at once. For example, if you see pwm_config() and
 pwm_{enable,disable}() calls in the same function, this probably means you
-should switch to pwm_apply_state().
+should switch to pwm_apply_cansleep().
 
 The PWM user API also allows one to query the PWM state that was passed to the
-last invocation of pwm_apply_state() using pwm_get_state(). Note this is
+last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
 different to what the driver has actually implemented if the request cannot be
 satisfied exactly with the hardware in use. There is currently no way for
 consumers to get the actually implemented settings.
diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
index 2e8f17c045222..cf516190cde8f 100644
--- a/drivers/gpu/drm/i915/display/intel_backlight.c
+++ b/drivers/gpu/drm/i915/display/intel_backlight.c
@@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
 	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
 
 	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
-	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
 }
 
 static void
@@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
 	intel_backlight_set_pwm_level(old_conn_state, level);
 
 	panel->backlight.pwm_state.enabled = false;
-	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
 }
 
 void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
@@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
 
 	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
 	panel->backlight.pwm_state.enabled = true;
-	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
+	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
 }
 
 static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 5a80b228d18ca..5045966d43039 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -267,7 +267,7 @@ static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
 
 	pwm_init_state(ssd130x->pwm, &pwmstate);
 	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
-	pwm_apply_state(ssd130x->pwm, &pwmstate);
+	pwm_apply_cansleep(ssd130x->pwm, &pwmstate);
 
 	/* Enable the PWM */
 	pwm_enable(ssd130x->pwm);
diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
index 6e4516c2ab894..f68deb1f236b7 100644
--- a/drivers/hwmon/pwm-fan.c
+++ b/drivers/hwmon/pwm-fan.c
@@ -151,7 +151,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
 	}
 
 	state->enabled = true;
-	ret = pwm_apply_state(ctx->pwm, state);
+	ret = pwm_apply_cansleep(ctx->pwm, state);
 	if (ret) {
 		dev_err(ctx->dev, "failed to enable PWM\n");
 		goto disable_regulator;
@@ -181,7 +181,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
 
 	state->enabled = false;
 	state->duty_cycle = 0;
-	ret = pwm_apply_state(ctx->pwm, state);
+	ret = pwm_apply_cansleep(ctx->pwm, state);
 	if (ret) {
 		dev_err(ctx->dev, "failed to disable PWM\n");
 		return ret;
@@ -207,7 +207,7 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
 
 		period = state->period;
 		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
-		ret = pwm_apply_state(ctx->pwm, state);
+		ret = pwm_apply_cansleep(ctx->pwm, state);
 		if (ret)
 			return ret;
 		ret = pwm_fan_power_on(ctx);
@@ -278,7 +278,7 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
 						    state,
 						    &enable_regulator);
 
-			pwm_apply_state(ctx->pwm, state);
+			pwm_apply_cansleep(ctx->pwm, state);
 			pwm_fan_switch_power(ctx, enable_regulator);
 			pwm_fan_update_state(ctx, 0);
 		}
diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
index ce82548916bbc..f10be2cdba803 100644
--- a/drivers/input/misc/da7280.c
+++ b/drivers/input/misc/da7280.c
@@ -352,7 +352,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
 		state.duty_cycle = period_mag_multi;
 	}
 
-	error = pwm_apply_state(haptics->pwm_dev, &state);
+	error = pwm_apply_cansleep(haptics->pwm_dev, &state);
 	if (error)
 		dev_err(haptics->dev, "Failed to apply pwm state: %d\n", error);
 
@@ -1175,7 +1175,7 @@ static int da7280_probe(struct i2c_client *client)
 		/* Sync up PWM state and ensure it is off. */
 		pwm_init_state(haptics->pwm_dev, &state);
 		state.enabled = false;
-		error = pwm_apply_state(haptics->pwm_dev, &state);
+		error = pwm_apply_cansleep(haptics->pwm_dev, &state);
 		if (error) {
 			dev_err(dev, "Failed to apply PWM state: %d\n", error);
 			return error;
diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
index 1e731d8397c6f..1d6c4fb5f0caf 100644
--- a/drivers/input/misc/pwm-beeper.c
+++ b/drivers/input/misc/pwm-beeper.c
@@ -39,7 +39,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
 	state.period = period;
 	pwm_set_relative_duty_cycle(&state, 50, 100);
 
-	error = pwm_apply_state(beeper->pwm, &state);
+	error = pwm_apply_cansleep(beeper->pwm, &state);
 	if (error)
 		return error;
 
@@ -138,7 +138,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
 	/* Sync up PWM state and ensure it is off. */
 	pwm_init_state(beeper->pwm, &state);
 	state.enabled = false;
-	error = pwm_apply_state(beeper->pwm, &state);
+	error = pwm_apply_cansleep(beeper->pwm, &state);
 	if (error) {
 		dev_err(dev, "failed to apply initial PWM state: %d\n",
 			error);
diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
index acac79c488aa1..6552ce712d8dc 100644
--- a/drivers/input/misc/pwm-vibra.c
+++ b/drivers/input/misc/pwm-vibra.c
@@ -56,7 +56,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
 	pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
 	state.enabled = true;
 
-	err = pwm_apply_state(vibrator->pwm, &state);
+	err = pwm_apply_cansleep(vibrator->pwm, &state);
 	if (err) {
 		dev_err(pdev, "failed to apply pwm state: %d\n", err);
 		return err;
@@ -67,7 +67,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
 		state.duty_cycle = vibrator->direction_duty_cycle;
 		state.enabled = true;
 
-		err = pwm_apply_state(vibrator->pwm_dir, &state);
+		err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
 		if (err) {
 			dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
 			pwm_disable(vibrator->pwm);
@@ -160,7 +160,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
 	/* Sync up PWM state and ensure it is off. */
 	pwm_init_state(vibrator->pwm, &state);
 	state.enabled = false;
-	err = pwm_apply_state(vibrator->pwm, &state);
+	err = pwm_apply_cansleep(vibrator->pwm, &state);
 	if (err) {
 		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
 			err);
@@ -174,7 +174,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
 		/* Sync up PWM state and ensure it is off. */
 		pwm_init_state(vibrator->pwm_dir, &state);
 		state.enabled = false;
-		err = pwm_apply_state(vibrator->pwm_dir, &state);
+		err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
 		if (err) {
 			dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
 				err);
diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
index 419b710984ab6..e1fe1fd8f189a 100644
--- a/drivers/leds/leds-pwm.c
+++ b/drivers/leds/leds-pwm.c
@@ -54,7 +54,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
 
 	led_dat->pwmstate.duty_cycle = duty;
 	led_dat->pwmstate.enabled = duty > 0;
-	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
+	return pwm_apply_cansleep(led_dat->pwm, &led_dat->pwmstate);
 }
 
 __attribute__((nonnull))
diff --git a/drivers/leds/rgb/leds-pwm-multicolor.c b/drivers/leds/rgb/leds-pwm-multicolor.c
index 46cd062b8b24c..8114adcdad9bb 100644
--- a/drivers/leds/rgb/leds-pwm-multicolor.c
+++ b/drivers/leds/rgb/leds-pwm-multicolor.c
@@ -51,8 +51,8 @@ static int led_pwm_mc_set(struct led_classdev *cdev,
 
 		priv->leds[i].state.duty_cycle = duty;
 		priv->leds[i].state.enabled = duty > 0;
-		ret = pwm_apply_state(priv->leds[i].pwm,
-				      &priv->leds[i].state);
+		ret = pwm_apply_cansleep(priv->leds[i].pwm,
+					 &priv->leds[i].state);
 		if (ret)
 			break;
 	}
diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index c5f37c03af9c9..ccb86890adcea 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 
 	for (i = 0; i < count; i++) {
 		state.enabled = !(i % 2);
-		pwm_apply_state(pwm, &state);
+		pwm_apply_cansleep(pwm, &state);
 
 		edge = ktime_add_us(edge, txbuf[i]);
 		delta = ktime_us_delta(edge, ktime_get());
@@ -77,7 +77,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 	}
 
 	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	pwm_apply_cansleep(pwm, &state);
 
 	return count;
 }
diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo-yogabook.c
index b8d0239192cbf..cbc285f77c2bd 100644
--- a/drivers/platform/x86/lenovo-yogabook.c
+++ b/drivers/platform/x86/lenovo-yogabook.c
@@ -435,7 +435,7 @@ static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level)
 		.enabled = level,
 	};
 
-	pwm_apply_state(data->kbd_bl_pwm, &state);
+	pwm_apply_cansleep(data->kbd_bl_pwm, &state);
 	gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0);
 	return 0;
 }
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index dc66e3405bf50..99896a59a25aa 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -382,8 +382,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 }
 EXPORT_SYMBOL_GPL(pwm_request_from_chip);
 
-static void pwm_apply_state_debug(struct pwm_device *pwm,
-				  const struct pwm_state *state)
+static void pwm_apply_cansleep_debug(struct pwm_device *pwm,
+				     const struct pwm_state *state)
 {
 	struct pwm_state *last = &pwm->last;
 	struct pwm_chip *chip = pwm->chip;
@@ -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_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_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;
@@ -527,15 +518,63 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
 
 	pwm->state = *state;
 
+	return 0;
+}
+
+/**
+ * pwm_apply_cansleep() - 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_cansleep(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_cansleep() 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_state_debug(pwm, state);
+	pwm_apply_cansleep_debug(pwm, state);
 
-	return 0;
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_apply_cansleep);
+
+/**
+ * pwm_apply() - 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(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_state);
+EXPORT_SYMBOL_GPL(pwm_apply);
 
 /**
  * pwm_capture() - capture and report a PWM signal
@@ -593,7 +632,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
 		state.period = pargs.period;
 		state.polarity = pargs.polarity;
 
-		return pwm_apply_state(pwm, &state);
+		return pwm_apply_cansleep(pwm, &state);
 	}
 
 	/*
@@ -616,7 +655,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
 		state.duty_cycle = state.period - state.duty_cycle;
 	}
 
-	return pwm_apply_state(pwm, &state);
+	return pwm_apply_cansleep(pwm, &state);
 }
 EXPORT_SYMBOL_GPL(pwm_adjust_config);
 
diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
index d7311614c846d..96797a33d8c62 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/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
index 8fb84b4418538..a1fc2fa0d03e0 100644
--- a/drivers/pwm/pwm-twl-led.c
+++ b/drivers/pwm/pwm-twl-led.c
@@ -172,7 +172,7 @@ static int twl4030_pwmled_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * We cannot skip calling ->config even if state->period ==
 	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
 	 * because we might have exited early in the last call to
-	 * pwm_apply_state because of !state->enabled and so the two values in
+	 * pwm_apply_cansleep because of !state->enabled and so the two values in
 	 * pwm->state might not be configured in hardware.
 	 */
 	ret = twl4030_pwmled_config(pwm->chip, pwm,
diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
index 6d46db51daacc..3a815dfbf31ce 100644
--- a/drivers/pwm/pwm-vt8500.c
+++ b/drivers/pwm/pwm-vt8500.c
@@ -206,7 +206,7 @@ static int vt8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * We cannot skip calling ->config even if state->period ==
 	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
 	 * because we might have exited early in the last call to
-	 * pwm_apply_state because of !state->enabled and so the two values in
+	 * pwm_apply_cansleep because of !state->enabled and so the two values in
 	 * pwm->state might not be configured in hardware.
 	 */
 	err = vt8500_pwm_config(pwm->chip, pwm, state->duty_cycle, state->period);
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 8d1254761e4dd..eca9cad3be765 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -62,7 +62,7 @@ static ssize_t period_store(struct device *child,
 	mutex_lock(&export->lock);
 	pwm_get_state(pwm, &state);
 	state.period = val;
-	ret = pwm_apply_state(pwm, &state);
+	ret = pwm_apply_cansleep(pwm, &state);
 	mutex_unlock(&export->lock);
 
 	return ret ? : size;
@@ -97,7 +97,7 @@ static ssize_t duty_cycle_store(struct device *child,
 	mutex_lock(&export->lock);
 	pwm_get_state(pwm, &state);
 	state.duty_cycle = val;
-	ret = pwm_apply_state(pwm, &state);
+	ret = pwm_apply_cansleep(pwm, &state);
 	mutex_unlock(&export->lock);
 
 	return ret ? : size;
@@ -144,7 +144,7 @@ static ssize_t enable_store(struct device *child,
 		goto unlock;
 	}
 
-	ret = pwm_apply_state(pwm, &state);
+	ret = pwm_apply_cansleep(pwm, &state);
 
 unlock:
 	mutex_unlock(&export->lock);
@@ -194,7 +194,7 @@ static ssize_t polarity_store(struct device *child,
 	mutex_lock(&export->lock);
 	pwm_get_state(pwm, &state);
 	state.polarity = polarity;
-	ret = pwm_apply_state(pwm, &state);
+	ret = pwm_apply_cansleep(pwm, &state);
 	mutex_unlock(&export->lock);
 
 	return ret ? : size;
@@ -401,7 +401,7 @@ static int pwm_class_apply_state(struct pwm_export *export,
 				 struct pwm_device *pwm,
 				 struct pwm_state *state)
 {
-	int ret = pwm_apply_state(pwm, state);
+	int ret = pwm_apply_cansleep(pwm, state);
 
 	/* release lock taken in pwm_class_get_state */
 	mutex_unlock(&export->lock);
diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index 2aff6db748e2c..c19d37a479d43 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -90,7 +90,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
 	pwm_set_relative_duty_cycle(&pstate,
 			drvdata->duty_cycle_table[selector].dutycycle, 100);
 
-	ret = pwm_apply_state(drvdata->pwm, &pstate);
+	ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
@@ -216,7 +216,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
 
 	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
 
-	ret = pwm_apply_state(drvdata->pwm, &pstate);
+	ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
 	if (ret) {
 		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
 		return ret;
diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
index 8fcb62be597b8..5cb702989ef61 100644
--- a/drivers/video/backlight/lm3630a_bl.c
+++ b/drivers/video/backlight/lm3630a_bl.c
@@ -180,7 +180,7 @@ static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
 
 	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
 
-	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
+	return pwm_apply_cansleep(pchip->pwmd, &pchip->pwmd_state);
 }
 
 /* update and get brightness */
diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
index da1f124db69c0..b7edbaaa169a4 100644
--- a/drivers/video/backlight/lp855x_bl.c
+++ b/drivers/video/backlight/lp855x_bl.c
@@ -234,7 +234,7 @@ static int lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
 	state.duty_cycle = div_u64(br * state.period, max_br);
 	state.enabled = state.duty_cycle;
 
-	return pwm_apply_state(lp->pwm, &state);
+	return pwm_apply_cansleep(lp->pwm, &state);
 }
 
 static int lp855x_bl_update_status(struct backlight_device *bl)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index a51fbab963680..f2568aaae4769 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -103,7 +103,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		pwm_get_state(pb->pwm, &state);
 		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
 		state.enabled = true;
-		pwm_apply_state(pb->pwm, &state);
+		pwm_apply_cansleep(pb->pwm, &state);
 
 		pwm_backlight_power_on(pb);
 	} else {
@@ -120,7 +120,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
 		 * inactive output.
 		 */
 		state.enabled = !pb->power_supply && !pb->enable_gpio;
-		pwm_apply_state(pb->pwm, &state);
+		pwm_apply_cansleep(pb->pwm, &state);
 	}
 
 	if (pb->notify_after)
@@ -528,7 +528,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
 	if (!state.period && (data->pwm_period_ns > 0))
 		state.period = data->pwm_period_ns;
 
-	ret = pwm_apply_state(pb->pwm, &state);
+	ret = pwm_apply_cansleep(pb->pwm, &state);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
 			ret);
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 5ae48e36fccb4..e5cca01af55f3 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -347,7 +347,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
 
 		pwm_init_state(par->pwm, &pwmstate);
 		pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
-		pwm_apply_state(par->pwm, &pwmstate);
+		pwm_apply_cansleep(par->pwm, &pwmstate);
 
 		/* Enable the PWM */
 		pwm_enable(par->pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d2f9f690a9c14..373b5a4fe27dc 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -95,8 +95,8 @@ struct pwm_device {
  * @state: state to fill with the current PWM state
  *
  * The returned PWM state represents the state that was applied by a previous call to
- * pwm_apply_state(). Drivers may have to slightly tweak that state before programming it to
- * hardware. If pwm_apply_state() was never called, this returns either the current hardware
+ * pwm_apply_cansleep(). Drivers may have to slightly tweak that state before programming it to
+ * hardware. If pwm_apply_cansleep() was never called, this returns either the current hardware
  * state (if supported) or the default settings.
  */
 static inline void pwm_get_state(const struct pwm_device *pwm,
@@ -160,20 +160,20 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
 }
 
 /**
- * pwm_init_state() - prepare a new state to be applied with pwm_apply_state()
+ * pwm_init_state() - prepare a new state to be applied with pwm_apply_cansleep()
  * @pwm: PWM device
  * @state: state to fill with the prepared PWM state
  *
  * This functions prepares a state that can later be tweaked and applied
- * to the PWM device with pwm_apply_state(). This is a convenient function
+ * to the PWM device with pwm_apply_cansleep(). This is a convenient function
  * that first retrieves the current PWM state and the replaces the period
  * and polarity fields with the reference values defined in pwm->args.
  * Once the function returns, you can adjust the ->enabled and ->duty_cycle
- * fields according to your needs before calling pwm_apply_state().
+ * fields according to your needs before calling pwm_apply_cansleep().
  *
  * ->duty_cycle is initially set to zero to avoid cases where the current
  * ->duty_cycle value exceed the pwm_args->period one, which would trigger
- * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle
+ * an error if the user calls pwm_apply_cansleep() without adjusting ->duty_cycle
  * first.
  */
 static inline void pwm_init_state(const struct pwm_device *pwm,
@@ -229,7 +229,7 @@ pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
  *
  * pwm_init_state(pwm, &state);
  * pwm_set_relative_duty_cycle(&state, 50, 100);
- * pwm_apply_state(pwm, &state);
+ * pwm_apply_cansleep(pwm, &state);
  *
  * This functions returns -EINVAL if @duty_cycle and/or @scale are
  * inconsistent (@scale == 0 or @duty_cycle > @scale).
@@ -289,6 +289,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 execute pwm_apply_cansleep in atomic context
  * @list: list node for internal use
  * @pwms: array of PWM devices allocated by the framework
  */
@@ -301,6 +302,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;
@@ -309,7 +311,8 @@ 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_cansleep(struct pwm_device *pwm, const struct pwm_state *state);
+int pwm_apply(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
 
 /**
@@ -337,7 +340,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
 
 	state.duty_cycle = duty_ns;
 	state.period = period_ns;
-	return pwm_apply_state(pwm, &state);
+	return pwm_apply_cansleep(pwm, &state);
 }
 
 /**
@@ -358,7 +361,7 @@ static inline int pwm_enable(struct pwm_device *pwm)
 		return 0;
 
 	state.enabled = true;
-	return pwm_apply_state(pwm, &state);
+	return pwm_apply_cansleep(pwm, &state);
 }
 
 /**
@@ -377,7 +380,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
 		return;
 
 	state.enabled = false;
-	pwm_apply_state(pwm, &state);
+	pwm_apply_cansleep(pwm, &state);
+}
+
+/**
+ * pwm_is_atomic() - is pwm_apply() supported?
+ * @pwm: PWM device
+ *
+ * Returns: true pwm_apply() can be called from atomic context.
+ */
+static inline bool pwm_is_atomic(struct pwm_device *pwm)
+{
+	return pwm->chip->atomic;
 }
 
 /* PWM provider APIs */
@@ -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 int pwm_apply_state(struct pwm_device *pwm,
-				  const struct pwm_state *state)
+static inline bool pwm_is_atomic(struct pwm_device *pwm)
+{
+	return false;
+}
+
+static inline int pwm_apply_cansleep(struct pwm_device *pwm,
+				     const struct pwm_state *state)
 {
 	might_sleep();
-	return -ENOTSUPP;
+	return -EOPNOTSUPP;
+}
+
+static inline int pwm_apply(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,
@@ -536,7 +561,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
 	state.period = pwm->args.period;
 	state.usage_power = false;
 
-	pwm_apply_state(pwm, &state);
+	pwm_apply_cansleep(pwm, &state);
 }
 
 struct pwm_lookup {
-- 
2.42.0

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Hans de Goede]

Hi Sean,

On 10/17/23 11:17, Sean Young wrote:
> 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>

I have no objection to this patch by itself, but it seems a bit
of unnecessary churn to change all current callers of pwm_apply_state()
to a new API.

Why not just keep pwm_apply_state() as is and introduce a new
pwm_apply_state_atomic() for callers which want to apply state
in a case where sleeping is not allowed ?

Regards,

Hans



> ---
>  Documentation/driver-api/pwm.rst              | 16 +++-
>  .../gpu/drm/i915/display/intel_backlight.c    |  6 +-
>  drivers/gpu/drm/solomon/ssd130x.c             |  2 +-
>  drivers/hwmon/pwm-fan.c                       |  8 +-
>  drivers/input/misc/da7280.c                   |  4 +-
>  drivers/input/misc/pwm-beeper.c               |  4 +-
>  drivers/input/misc/pwm-vibra.c                |  8 +-
>  drivers/leds/leds-pwm.c                       |  2 +-
>  drivers/leds/rgb/leds-pwm-multicolor.c        |  4 +-
>  drivers/media/rc/pwm-ir-tx.c                  |  4 +-
>  drivers/platform/x86/lenovo-yogabook.c        |  2 +-
>  drivers/pwm/core.c                            | 75 ++++++++++++++-----
>  drivers/pwm/pwm-renesas-tpu.c                 |  1 -
>  drivers/pwm/pwm-twl-led.c                     |  2 +-
>  drivers/pwm/pwm-vt8500.c                      |  2 +-
>  drivers/pwm/sysfs.c                           | 10 +--
>  drivers/regulator/pwm-regulator.c             |  4 +-
>  drivers/video/backlight/lm3630a_bl.c          |  2 +-
>  drivers/video/backlight/lp855x_bl.c           |  2 +-
>  drivers/video/backlight/pwm_bl.c              |  6 +-
>  drivers/video/fbdev/ssd1307fb.c               |  2 +-
>  include/linux/pwm.h                           | 57 ++++++++++----
>  22 files changed, 147 insertions(+), 76 deletions(-)
> 
> diff --git a/Documentation/driver-api/pwm.rst b/Documentation/driver-api/pwm.rst
> index 3fdc95f7a1d15..a2fb5f8f6e1f8 100644
> --- a/Documentation/driver-api/pwm.rst
> +++ b/Documentation/driver-api/pwm.rst
> @@ -41,7 +41,15 @@ the getter, devm_pwm_get() and devm_fwnode_pwm_get(), also exist.
>  
>  After being requested, a PWM has to be configured using::
>  
> -	int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state);
> +	int pwm_apply_cansleep(struct pwm_device *pwm, struct pwm_state *state);
> +
> +If the PWM support atomic mode, which can be determined with::
> +
> +        bool pwm_is_atomic(struct pwm_device *pwm);
> +
> +Then the PWM can be configured with::
> +
> +	int pwm_apply(struct pwm_device *pwm, struct pwm_state *state);
>  
>  This API controls both the PWM period/duty_cycle config and the
>  enable/disable state.
> @@ -57,13 +65,13 @@ If supported by the driver, the signal can be optimized, for example to improve
>  EMI by phase shifting the individual channels of a chip.
>  
>  The pwm_config(), pwm_enable() and pwm_disable() functions are just wrappers
> -around pwm_apply_state() and should not be used if the user wants to change
> +around pwm_apply_cansleep() and should not be used if the user wants to change
>  several parameter at once. For example, if you see pwm_config() and
>  pwm_{enable,disable}() calls in the same function, this probably means you
> -should switch to pwm_apply_state().
> +should switch to pwm_apply_cansleep().
>  
>  The PWM user API also allows one to query the PWM state that was passed to the
> -last invocation of pwm_apply_state() using pwm_get_state(). Note this is
> +last invocation of pwm_apply_cansleep() using pwm_get_state(). Note this is
>  different to what the driver has actually implemented if the request cannot be
>  satisfied exactly with the hardware in use. There is currently no way for
>  consumers to get the actually implemented settings.
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..cf516190cde8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
>  	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
>  
>  	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  static void
> @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
>  	intel_backlight_set_pwm_level(old_conn_state, level);
>  
>  	panel->backlight.pwm_state.enabled = false;
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
> @@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
>  
>  	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
>  	panel->backlight.pwm_state.enabled = true;
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,
> diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
> index 5a80b228d18ca..5045966d43039 100644
> --- a/drivers/gpu/drm/solomon/ssd130x.c
> +++ b/drivers/gpu/drm/solomon/ssd130x.c
> @@ -267,7 +267,7 @@ static int ssd130x_pwm_enable(struct ssd130x_device *ssd130x)
>  
>  	pwm_init_state(ssd130x->pwm, &pwmstate);
>  	pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
> -	pwm_apply_state(ssd130x->pwm, &pwmstate);
> +	pwm_apply_cansleep(ssd130x->pwm, &pwmstate);
>  
>  	/* Enable the PWM */
>  	pwm_enable(ssd130x->pwm);
> diff --git a/drivers/hwmon/pwm-fan.c b/drivers/hwmon/pwm-fan.c
> index 6e4516c2ab894..f68deb1f236b7 100644
> --- a/drivers/hwmon/pwm-fan.c
> +++ b/drivers/hwmon/pwm-fan.c
> @@ -151,7 +151,7 @@ static int pwm_fan_power_on(struct pwm_fan_ctx *ctx)
>  	}
>  
>  	state->enabled = true;
> -	ret = pwm_apply_state(ctx->pwm, state);
> +	ret = pwm_apply_cansleep(ctx->pwm, state);
>  	if (ret) {
>  		dev_err(ctx->dev, "failed to enable PWM\n");
>  		goto disable_regulator;
> @@ -181,7 +181,7 @@ static int pwm_fan_power_off(struct pwm_fan_ctx *ctx)
>  
>  	state->enabled = false;
>  	state->duty_cycle = 0;
> -	ret = pwm_apply_state(ctx->pwm, state);
> +	ret = pwm_apply_cansleep(ctx->pwm, state);
>  	if (ret) {
>  		dev_err(ctx->dev, "failed to disable PWM\n");
>  		return ret;
> @@ -207,7 +207,7 @@ static int  __set_pwm(struct pwm_fan_ctx *ctx, unsigned long pwm)
>  
>  		period = state->period;
>  		state->duty_cycle = DIV_ROUND_UP(pwm * (period - 1), MAX_PWM);
> -		ret = pwm_apply_state(ctx->pwm, state);
> +		ret = pwm_apply_cansleep(ctx->pwm, state);
>  		if (ret)
>  			return ret;
>  		ret = pwm_fan_power_on(ctx);
> @@ -278,7 +278,7 @@ static int pwm_fan_update_enable(struct pwm_fan_ctx *ctx, long val)
>  						    state,
>  						    &enable_regulator);
>  
> -			pwm_apply_state(ctx->pwm, state);
> +			pwm_apply_cansleep(ctx->pwm, state);
>  			pwm_fan_switch_power(ctx, enable_regulator);
>  			pwm_fan_update_state(ctx, 0);
>  		}
> diff --git a/drivers/input/misc/da7280.c b/drivers/input/misc/da7280.c
> index ce82548916bbc..f10be2cdba803 100644
> --- a/drivers/input/misc/da7280.c
> +++ b/drivers/input/misc/da7280.c
> @@ -352,7 +352,7 @@ static int da7280_haptic_set_pwm(struct da7280_haptic *haptics, bool enabled)
>  		state.duty_cycle = period_mag_multi;
>  	}
>  
> -	error = pwm_apply_state(haptics->pwm_dev, &state);
> +	error = pwm_apply_cansleep(haptics->pwm_dev, &state);
>  	if (error)
>  		dev_err(haptics->dev, "Failed to apply pwm state: %d\n", error);
>  
> @@ -1175,7 +1175,7 @@ static int da7280_probe(struct i2c_client *client)
>  		/* Sync up PWM state and ensure it is off. */
>  		pwm_init_state(haptics->pwm_dev, &state);
>  		state.enabled = false;
> -		error = pwm_apply_state(haptics->pwm_dev, &state);
> +		error = pwm_apply_cansleep(haptics->pwm_dev, &state);
>  		if (error) {
>  			dev_err(dev, "Failed to apply PWM state: %d\n", error);
>  			return error;
> diff --git a/drivers/input/misc/pwm-beeper.c b/drivers/input/misc/pwm-beeper.c
> index 1e731d8397c6f..1d6c4fb5f0caf 100644
> --- a/drivers/input/misc/pwm-beeper.c
> +++ b/drivers/input/misc/pwm-beeper.c
> @@ -39,7 +39,7 @@ static int pwm_beeper_on(struct pwm_beeper *beeper, unsigned long period)
>  	state.period = period;
>  	pwm_set_relative_duty_cycle(&state, 50, 100);
>  
> -	error = pwm_apply_state(beeper->pwm, &state);
> +	error = pwm_apply_cansleep(beeper->pwm, &state);
>  	if (error)
>  		return error;
>  
> @@ -138,7 +138,7 @@ static int pwm_beeper_probe(struct platform_device *pdev)
>  	/* Sync up PWM state and ensure it is off. */
>  	pwm_init_state(beeper->pwm, &state);
>  	state.enabled = false;
> -	error = pwm_apply_state(beeper->pwm, &state);
> +	error = pwm_apply_cansleep(beeper->pwm, &state);
>  	if (error) {
>  		dev_err(dev, "failed to apply initial PWM state: %d\n",
>  			error);
> diff --git a/drivers/input/misc/pwm-vibra.c b/drivers/input/misc/pwm-vibra.c
> index acac79c488aa1..6552ce712d8dc 100644
> --- a/drivers/input/misc/pwm-vibra.c
> +++ b/drivers/input/misc/pwm-vibra.c
> @@ -56,7 +56,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>  	pwm_set_relative_duty_cycle(&state, vibrator->level, 0xffff);
>  	state.enabled = true;
>  
> -	err = pwm_apply_state(vibrator->pwm, &state);
> +	err = pwm_apply_cansleep(vibrator->pwm, &state);
>  	if (err) {
>  		dev_err(pdev, "failed to apply pwm state: %d\n", err);
>  		return err;
> @@ -67,7 +67,7 @@ static int pwm_vibrator_start(struct pwm_vibrator *vibrator)
>  		state.duty_cycle = vibrator->direction_duty_cycle;
>  		state.enabled = true;
>  
> -		err = pwm_apply_state(vibrator->pwm_dir, &state);
> +		err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
>  		if (err) {
>  			dev_err(pdev, "failed to apply dir-pwm state: %d\n", err);
>  			pwm_disable(vibrator->pwm);
> @@ -160,7 +160,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
>  	/* Sync up PWM state and ensure it is off. */
>  	pwm_init_state(vibrator->pwm, &state);
>  	state.enabled = false;
> -	err = pwm_apply_state(vibrator->pwm, &state);
> +	err = pwm_apply_cansleep(vibrator->pwm, &state);
>  	if (err) {
>  		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
>  			err);
> @@ -174,7 +174,7 @@ static int pwm_vibrator_probe(struct platform_device *pdev)
>  		/* Sync up PWM state and ensure it is off. */
>  		pwm_init_state(vibrator->pwm_dir, &state);
>  		state.enabled = false;
> -		err = pwm_apply_state(vibrator->pwm_dir, &state);
> +		err = pwm_apply_cansleep(vibrator->pwm_dir, &state);
>  		if (err) {
>  			dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
>  				err);
> diff --git a/drivers/leds/leds-pwm.c b/drivers/leds/leds-pwm.c
> index 419b710984ab6..e1fe1fd8f189a 100644
> --- a/drivers/leds/leds-pwm.c
> +++ b/drivers/leds/leds-pwm.c
> @@ -54,7 +54,7 @@ static int led_pwm_set(struct led_classdev *led_cdev,
>  
>  	led_dat->pwmstate.duty_cycle = duty;
>  	led_dat->pwmstate.enabled = duty > 0;
> -	return pwm_apply_state(led_dat->pwm, &led_dat->pwmstate);
> +	return pwm_apply_cansleep(led_dat->pwm, &led_dat->pwmstate);
>  }
>  
>  __attribute__((nonnull))
> diff --git a/drivers/leds/rgb/leds-pwm-multicolor.c b/drivers/leds/rgb/leds-pwm-multicolor.c
> index 46cd062b8b24c..8114adcdad9bb 100644
> --- a/drivers/leds/rgb/leds-pwm-multicolor.c
> +++ b/drivers/leds/rgb/leds-pwm-multicolor.c
> @@ -51,8 +51,8 @@ static int led_pwm_mc_set(struct led_classdev *cdev,
>  
>  		priv->leds[i].state.duty_cycle = duty;
>  		priv->leds[i].state.enabled = duty > 0;
> -		ret = pwm_apply_state(priv->leds[i].pwm,
> -				      &priv->leds[i].state);
> +		ret = pwm_apply_cansleep(priv->leds[i].pwm,
> +					 &priv->leds[i].state);
>  		if (ret)
>  			break;
>  	}
> diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
> index c5f37c03af9c9..ccb86890adcea 100644
> --- a/drivers/media/rc/pwm-ir-tx.c
> +++ b/drivers/media/rc/pwm-ir-tx.c
> @@ -68,7 +68,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>  
>  	for (i = 0; i < count; i++) {
>  		state.enabled = !(i % 2);
> -		pwm_apply_state(pwm, &state);
> +		pwm_apply_cansleep(pwm, &state);
>  
>  		edge = ktime_add_us(edge, txbuf[i]);
>  		delta = ktime_us_delta(edge, ktime_get());
> @@ -77,7 +77,7 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
>  	}
>  
>  	state.enabled = false;
> -	pwm_apply_state(pwm, &state);
> +	pwm_apply_cansleep(pwm, &state);
>  
>  	return count;
>  }
> diff --git a/drivers/platform/x86/lenovo-yogabook.c b/drivers/platform/x86/lenovo-yogabook.c
> index b8d0239192cbf..cbc285f77c2bd 100644
> --- a/drivers/platform/x86/lenovo-yogabook.c
> +++ b/drivers/platform/x86/lenovo-yogabook.c
> @@ -435,7 +435,7 @@ static int yogabook_pdev_set_kbd_backlight(struct yogabook_data *data, u8 level)
>  		.enabled = level,
>  	};
>  
> -	pwm_apply_state(data->kbd_bl_pwm, &state);
> +	pwm_apply_cansleep(data->kbd_bl_pwm, &state);
>  	gpiod_set_value(data->kbd_bl_led_enable, level ? 1 : 0);
>  	return 0;
>  }
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index dc66e3405bf50..99896a59a25aa 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -382,8 +382,8 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
>  }
>  EXPORT_SYMBOL_GPL(pwm_request_from_chip);
>  
> -static void pwm_apply_state_debug(struct pwm_device *pwm,
> -				  const struct pwm_state *state)
> +static void pwm_apply_cansleep_debug(struct pwm_device *pwm,
> +				     const struct pwm_state *state)
>  {
>  	struct pwm_state *last = &pwm->last;
>  	struct pwm_chip *chip = pwm->chip;
> @@ -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_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_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;
> @@ -527,15 +518,63 @@ int pwm_apply_state(struct pwm_device *pwm, const struct pwm_state *state)
>  
>  	pwm->state = *state;
>  
> +	return 0;
> +}
> +
> +/**
> + * pwm_apply_cansleep() - 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_cansleep(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_cansleep() 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_state_debug(pwm, state);
> +	pwm_apply_cansleep_debug(pwm, state);
>  
> -	return 0;
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(pwm_apply_cansleep);
> +
> +/**
> + * pwm_apply() - 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(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_state);
> +EXPORT_SYMBOL_GPL(pwm_apply);
>  
>  /**
>   * pwm_capture() - capture and report a PWM signal
> @@ -593,7 +632,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
>  		state.period = pargs.period;
>  		state.polarity = pargs.polarity;
>  
> -		return pwm_apply_state(pwm, &state);
> +		return pwm_apply_cansleep(pwm, &state);
>  	}
>  
>  	/*
> @@ -616,7 +655,7 @@ int pwm_adjust_config(struct pwm_device *pwm)
>  		state.duty_cycle = state.period - state.duty_cycle;
>  	}
>  
> -	return pwm_apply_state(pwm, &state);
> +	return pwm_apply_cansleep(pwm, &state);
>  }
>  EXPORT_SYMBOL_GPL(pwm_adjust_config);
>  
> diff --git a/drivers/pwm/pwm-renesas-tpu.c b/drivers/pwm/pwm-renesas-tpu.c
> index d7311614c846d..96797a33d8c62 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/drivers/pwm/pwm-twl-led.c b/drivers/pwm/pwm-twl-led.c
> index 8fb84b4418538..a1fc2fa0d03e0 100644
> --- a/drivers/pwm/pwm-twl-led.c
> +++ b/drivers/pwm/pwm-twl-led.c
> @@ -172,7 +172,7 @@ static int twl4030_pwmled_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * We cannot skip calling ->config even if state->period ==
>  	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
>  	 * because we might have exited early in the last call to
> -	 * pwm_apply_state because of !state->enabled and so the two values in
> +	 * pwm_apply_cansleep because of !state->enabled and so the two values in
>  	 * pwm->state might not be configured in hardware.
>  	 */
>  	ret = twl4030_pwmled_config(pwm->chip, pwm,
> diff --git a/drivers/pwm/pwm-vt8500.c b/drivers/pwm/pwm-vt8500.c
> index 6d46db51daacc..3a815dfbf31ce 100644
> --- a/drivers/pwm/pwm-vt8500.c
> +++ b/drivers/pwm/pwm-vt8500.c
> @@ -206,7 +206,7 @@ static int vt8500_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  	 * We cannot skip calling ->config even if state->period ==
>  	 * pwm->state.period && state->duty_cycle == pwm->state.duty_cycle
>  	 * because we might have exited early in the last call to
> -	 * pwm_apply_state because of !state->enabled and so the two values in
> +	 * pwm_apply_cansleep because of !state->enabled and so the two values in
>  	 * pwm->state might not be configured in hardware.
>  	 */
>  	err = vt8500_pwm_config(pwm->chip, pwm, state->duty_cycle, state->period);
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 8d1254761e4dd..eca9cad3be765 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -62,7 +62,7 @@ static ssize_t period_store(struct device *child,
>  	mutex_lock(&export->lock);
>  	pwm_get_state(pwm, &state);
>  	state.period = val;
> -	ret = pwm_apply_state(pwm, &state);
> +	ret = pwm_apply_cansleep(pwm, &state);
>  	mutex_unlock(&export->lock);
>  
>  	return ret ? : size;
> @@ -97,7 +97,7 @@ static ssize_t duty_cycle_store(struct device *child,
>  	mutex_lock(&export->lock);
>  	pwm_get_state(pwm, &state);
>  	state.duty_cycle = val;
> -	ret = pwm_apply_state(pwm, &state);
> +	ret = pwm_apply_cansleep(pwm, &state);
>  	mutex_unlock(&export->lock);
>  
>  	return ret ? : size;
> @@ -144,7 +144,7 @@ static ssize_t enable_store(struct device *child,
>  		goto unlock;
>  	}
>  
> -	ret = pwm_apply_state(pwm, &state);
> +	ret = pwm_apply_cansleep(pwm, &state);
>  
>  unlock:
>  	mutex_unlock(&export->lock);
> @@ -194,7 +194,7 @@ static ssize_t polarity_store(struct device *child,
>  	mutex_lock(&export->lock);
>  	pwm_get_state(pwm, &state);
>  	state.polarity = polarity;
> -	ret = pwm_apply_state(pwm, &state);
> +	ret = pwm_apply_cansleep(pwm, &state);
>  	mutex_unlock(&export->lock);
>  
>  	return ret ? : size;
> @@ -401,7 +401,7 @@ static int pwm_class_apply_state(struct pwm_export *export,
>  				 struct pwm_device *pwm,
>  				 struct pwm_state *state)
>  {
> -	int ret = pwm_apply_state(pwm, state);
> +	int ret = pwm_apply_cansleep(pwm, state);
>  
>  	/* release lock taken in pwm_class_get_state */
>  	mutex_unlock(&export->lock);
> diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
> index 2aff6db748e2c..c19d37a479d43 100644
> --- a/drivers/regulator/pwm-regulator.c
> +++ b/drivers/regulator/pwm-regulator.c
> @@ -90,7 +90,7 @@ static int pwm_regulator_set_voltage_sel(struct regulator_dev *rdev,
>  	pwm_set_relative_duty_cycle(&pstate,
>  			drvdata->duty_cycle_table[selector].dutycycle, 100);
>  
> -	ret = pwm_apply_state(drvdata->pwm, &pstate);
> +	ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
> @@ -216,7 +216,7 @@ static int pwm_regulator_set_voltage(struct regulator_dev *rdev,
>  
>  	pwm_set_relative_duty_cycle(&pstate, dutycycle, duty_unit);
>  
> -	ret = pwm_apply_state(drvdata->pwm, &pstate);
> +	ret = pwm_apply_cansleep(drvdata->pwm, &pstate);
>  	if (ret) {
>  		dev_err(&rdev->dev, "Failed to configure PWM: %d\n", ret);
>  		return ret;
> diff --git a/drivers/video/backlight/lm3630a_bl.c b/drivers/video/backlight/lm3630a_bl.c
> index 8fcb62be597b8..5cb702989ef61 100644
> --- a/drivers/video/backlight/lm3630a_bl.c
> +++ b/drivers/video/backlight/lm3630a_bl.c
> @@ -180,7 +180,7 @@ static int lm3630a_pwm_ctrl(struct lm3630a_chip *pchip, int br, int br_max)
>  
>  	pchip->pwmd_state.enabled = pchip->pwmd_state.duty_cycle ? true : false;
>  
> -	return pwm_apply_state(pchip->pwmd, &pchip->pwmd_state);
> +	return pwm_apply_cansleep(pchip->pwmd, &pchip->pwmd_state);
>  }
>  
>  /* update and get brightness */
> diff --git a/drivers/video/backlight/lp855x_bl.c b/drivers/video/backlight/lp855x_bl.c
> index da1f124db69c0..b7edbaaa169a4 100644
> --- a/drivers/video/backlight/lp855x_bl.c
> +++ b/drivers/video/backlight/lp855x_bl.c
> @@ -234,7 +234,7 @@ static int lp855x_pwm_ctrl(struct lp855x *lp, int br, int max_br)
>  	state.duty_cycle = div_u64(br * state.period, max_br);
>  	state.enabled = state.duty_cycle;
>  
> -	return pwm_apply_state(lp->pwm, &state);
> +	return pwm_apply_cansleep(lp->pwm, &state);
>  }
>  
>  static int lp855x_bl_update_status(struct backlight_device *bl)
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index a51fbab963680..f2568aaae4769 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
> @@ -103,7 +103,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>  		pwm_get_state(pb->pwm, &state);
>  		state.duty_cycle = compute_duty_cycle(pb, brightness, &state);
>  		state.enabled = true;
> -		pwm_apply_state(pb->pwm, &state);
> +		pwm_apply_cansleep(pb->pwm, &state);
>  
>  		pwm_backlight_power_on(pb);
>  	} else {
> @@ -120,7 +120,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
>  		 * inactive output.
>  		 */
>  		state.enabled = !pb->power_supply && !pb->enable_gpio;
> -		pwm_apply_state(pb->pwm, &state);
> +		pwm_apply_cansleep(pb->pwm, &state);
>  	}
>  
>  	if (pb->notify_after)
> @@ -528,7 +528,7 @@ static int pwm_backlight_probe(struct platform_device *pdev)
>  	if (!state.period && (data->pwm_period_ns > 0))
>  		state.period = data->pwm_period_ns;
>  
> -	ret = pwm_apply_state(pb->pwm, &state);
> +	ret = pwm_apply_cansleep(pb->pwm, &state);
>  	if (ret) {
>  		dev_err(&pdev->dev, "failed to apply initial PWM state: %d\n",
>  			ret);
> diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
> index 5ae48e36fccb4..e5cca01af55f3 100644
> --- a/drivers/video/fbdev/ssd1307fb.c
> +++ b/drivers/video/fbdev/ssd1307fb.c
> @@ -347,7 +347,7 @@ static int ssd1307fb_init(struct ssd1307fb_par *par)
>  
>  		pwm_init_state(par->pwm, &pwmstate);
>  		pwm_set_relative_duty_cycle(&pwmstate, 50, 100);
> -		pwm_apply_state(par->pwm, &pwmstate);
> +		pwm_apply_cansleep(par->pwm, &pwmstate);
>  
>  		/* Enable the PWM */
>  		pwm_enable(par->pwm);
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index d2f9f690a9c14..373b5a4fe27dc 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -95,8 +95,8 @@ struct pwm_device {
>   * @state: state to fill with the current PWM state
>   *
>   * The returned PWM state represents the state that was applied by a previous call to
> - * pwm_apply_state(). Drivers may have to slightly tweak that state before programming it to
> - * hardware. If pwm_apply_state() was never called, this returns either the current hardware
> + * pwm_apply_cansleep(). Drivers may have to slightly tweak that state before programming it to
> + * hardware. If pwm_apply_cansleep() was never called, this returns either the current hardware
>   * state (if supported) or the default settings.
>   */
>  static inline void pwm_get_state(const struct pwm_device *pwm,
> @@ -160,20 +160,20 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
>  }
>  
>  /**
> - * pwm_init_state() - prepare a new state to be applied with pwm_apply_state()
> + * pwm_init_state() - prepare a new state to be applied with pwm_apply_cansleep()
>   * @pwm: PWM device
>   * @state: state to fill with the prepared PWM state
>   *
>   * This functions prepares a state that can later be tweaked and applied
> - * to the PWM device with pwm_apply_state(). This is a convenient function
> + * to the PWM device with pwm_apply_cansleep(). This is a convenient function
>   * that first retrieves the current PWM state and the replaces the period
>   * and polarity fields with the reference values defined in pwm->args.
>   * Once the function returns, you can adjust the ->enabled and ->duty_cycle
> - * fields according to your needs before calling pwm_apply_state().
> + * fields according to your needs before calling pwm_apply_cansleep().
>   *
>   * ->duty_cycle is initially set to zero to avoid cases where the current
>   * ->duty_cycle value exceed the pwm_args->period one, which would trigger
> - * an error if the user calls pwm_apply_state() without adjusting ->duty_cycle
> + * an error if the user calls pwm_apply_cansleep() without adjusting ->duty_cycle
>   * first.
>   */
>  static inline void pwm_init_state(const struct pwm_device *pwm,
> @@ -229,7 +229,7 @@ pwm_get_relative_duty_cycle(const struct pwm_state *state, unsigned int scale)
>   *
>   * pwm_init_state(pwm, &state);
>   * pwm_set_relative_duty_cycle(&state, 50, 100);
> - * pwm_apply_state(pwm, &state);
> + * pwm_apply_cansleep(pwm, &state);
>   *
>   * This functions returns -EINVAL if @duty_cycle and/or @scale are
>   * inconsistent (@scale == 0 or @duty_cycle > @scale).
> @@ -289,6 +289,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 execute pwm_apply_cansleep in atomic context
>   * @list: list node for internal use
>   * @pwms: array of PWM devices allocated by the framework
>   */
> @@ -301,6 +302,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;
> @@ -309,7 +311,8 @@ 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_cansleep(struct pwm_device *pwm, const struct pwm_state *state);
> +int pwm_apply(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_adjust_config(struct pwm_device *pwm);
>  
>  /**
> @@ -337,7 +340,7 @@ static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
>  
>  	state.duty_cycle = duty_ns;
>  	state.period = period_ns;
> -	return pwm_apply_state(pwm, &state);
> +	return pwm_apply_cansleep(pwm, &state);
>  }
>  
>  /**
> @@ -358,7 +361,7 @@ static inline int pwm_enable(struct pwm_device *pwm)
>  		return 0;
>  
>  	state.enabled = true;
> -	return pwm_apply_state(pwm, &state);
> +	return pwm_apply_cansleep(pwm, &state);
>  }
>  
>  /**
> @@ -377,7 +380,18 @@ static inline void pwm_disable(struct pwm_device *pwm)
>  		return;
>  
>  	state.enabled = false;
> -	pwm_apply_state(pwm, &state);
> +	pwm_apply_cansleep(pwm, &state);
> +}
> +
> +/**
> + * pwm_is_atomic() - is pwm_apply() supported?
> + * @pwm: PWM device
> + *
> + * Returns: true pwm_apply() can be called from atomic context.
> + */
> +static inline bool pwm_is_atomic(struct pwm_device *pwm)
> +{
> +	return pwm->chip->atomic;
>  }
>  
>  /* PWM provider APIs */
> @@ -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 int pwm_apply_state(struct pwm_device *pwm,
> -				  const struct pwm_state *state)
> +static inline bool pwm_is_atomic(struct pwm_device *pwm)
> +{
> +	return false;
> +}
> +
> +static inline int pwm_apply_cansleep(struct pwm_device *pwm,
> +				     const struct pwm_state *state)
>  {
>  	might_sleep();
> -	return -ENOTSUPP;
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline int pwm_apply(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,
> @@ -536,7 +561,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
>  	state.period = pwm->args.period;
>  	state.usage_power = false;
>  
> -	pwm_apply_state(pwm, &state);
> +	pwm_apply_cansleep(pwm, &state);
>  }
>  
>  struct pwm_lookup {

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2548 bytes --]

On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> Hi Sean,
> 
> On 10/17/23 11:17, Sean Young wrote:
> > 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>
> 
> I have no objection to this patch by itself, but it seems a bit
> of unnecessary churn to change all current callers of pwm_apply_state()
> to a new API.

The idea is to improve the semantic of the function name, see
https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
for more context. I think it's very subjective if you consider this
churn or not. While it's nice to have every caller converted in a single
step, I'd go for

	#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)

, keep that macro for a while and convert all users step by step. This
way we don't needlessly break oot code and the changes to convert to the
new API can go via their usual trees without time pressure.

> Why not just keep pwm_apply_state() as is and introduce a new
> pwm_apply_state_atomic() for callers which want to apply state
> in a case where sleeping is not allowed ?

There is a big spontaneous growth of function name patterns. I didn't
claim having done a complete research, but not using a suffix for the
fast variant and _cansleep for the sleeping one at least aligns to how
the gpio subsystem names things.

Grepping a bit more:

 - regmap has: regmap_might_sleep() and struct regmap::can_sleep
   The actual API doesn't have different functions to differentiate
   sleeping and non-sleeping calls. (Though there is
   regmap_read_poll_timeout_atomic().)

 - kmap() + kmap_atomic()
 - set_pte() + set_pte_atomic()

 - There is scmi_is_transport_atomic() and scmi_handle::is_transport_atomic()
   (Is this also about sleeping?)

 - There are quite a lot more symbols ending in _atomic and in
   _cansleep, but several of them don't exists to differentiate a slow
   and a fast procedure.  (e.g. drm_mode_atomic)

Not entirely sure what to read out of that.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Hans de Goede]

Hi Uwe,

On 10/19/23 12:51, Uwe Kleine-König wrote:
> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
>> Hi Sean,
>>
>> On 10/17/23 11:17, Sean Young wrote:
>>> 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>
>>
>> I have no objection to this patch by itself, but it seems a bit
>> of unnecessary churn to change all current callers of pwm_apply_state()
>> to a new API.
> 
> The idea is to improve the semantic of the function name, see
> https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
> for more context.

Hmm, so the argument here is that the GPIO API has this, but GPIOs
generally speaking can be set atomically, so there not being able
to set it atomically is special.

OTOH we have many many many other kernel functions which may sleep
and we don't all postfix them with _can_sleep.

And for PWM controllers pwm_apply_state is IMHO sorta expected to
sleep. Many of these are attached over I2C so things will sleep,
others have a handshake to wait for the current dutycycle to
end before you can apply a second change on top of an earlier
change during the current dutycycle which often also involves
sleeping.

So the natural/expeected thing for pwm_apply_state() is to sleep
and thus it does not need a postfix for this IMHO.

> I think it's very subjective if you consider this
> churn or not.

I consider it churn because I don't think adding a postfix
for what is the default/expected behavior is a good idea
(with GPIOs not sleeping is the expected behavior).

I agree that this is very subjective and very much goes
into the territory of bikeshedding. So please consider
the above my 2 cents on this and lets leave it at that.

> While it's nice to have every caller converted in a single
> step, I'd go for
> 
> 	#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
> 
> , keep that macro for a while and convert all users step by step. This
> way we don't needlessly break oot code and the changes to convert to the
> new API can go via their usual trees without time pressure.

I don't think there are enough users of pwm_apply_state() to warrant
such an exercise.

So if people want to move ahead with the _can_sleep postfix addition
(still not a fan) here is my acked-by for the drivers/platform/x86
changes, for merging this through the PWM tree in a single commit:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

Hi Hans,

On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> On 10/19/23 12:51, Uwe Kleine-König wrote:
> > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> >> On 10/17/23 11:17, Sean Young wrote:
> >>> 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>
> >>
> >> I have no objection to this patch by itself, but it seems a bit
> >> of unnecessary churn to change all current callers of pwm_apply_state()
> >> to a new API.
> > 
> > The idea is to improve the semantic of the function name, see
> > https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
> > for more context.
> 
> Hmm, so the argument here is that the GPIO API has this, but GPIOs
> generally speaking can be set atomically, so there not being able
> to set it atomically is special.
> 
> OTOH we have many many many other kernel functions which may sleep
> and we don't all postfix them with _can_sleep.
> 
> And for PWM controllers pwm_apply_state is IMHO sorta expected to
> sleep. Many of these are attached over I2C so things will sleep,
> others have a handshake to wait for the current dutycycle to
> end before you can apply a second change on top of an earlier
> change during the current dutycycle which often also involves
> sleeping.
> 
> So the natural/expeected thing for pwm_apply_state() is to sleep
> and thus it does not need a postfix for this IMHO.

Most pwm drivers look like they can be made to work in atomic context,
I think. Like you say this is not the case for all of them. Whatever
we choose to be the default for pwm_apply_state(), we should have a
clear function name for the alternative. This is essentially why
pam_apply_cansleep() was picked.

The alternative to pwm_apply_cansleep() is to have a function name
which implies it can be used from atomic context. However, 
pwm_apply_atomic() is not great because the "atomic" could be
confused with the PWM atomic API, not the kernel process/atomic
context.

So what should the non-sleeping function be called then? 
 - pwm_apply_cannotsleep() 
 - pwm_apply_nosleep()
 - pwm_apply_nonsleeping()
 - pwm_apply_atomic_context()

> > I think it's very subjective if you consider this
> > churn or not.
> 
> I consider it churn because I don't think adding a postfix
> for what is the default/expected behavior is a good idea
> (with GPIOs not sleeping is the expected behavior).
> 
> I agree that this is very subjective and very much goes
> into the territory of bikeshedding. So please consider
> the above my 2 cents on this and lets leave it at that.

You have a valid point. Let's focus on having descriptive function names.

> > While it's nice to have every caller converted in a single
> > step, I'd go for
> > 
> > 	#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
> > 
> > , keep that macro for a while and convert all users step by step. This
> > way we don't needlessly break oot code and the changes to convert to the
> > new API can go via their usual trees without time pressure.
> 
> I don't think there are enough users of pwm_apply_state() to warrant
> such an exercise.
> 
> So if people want to move ahead with the _can_sleep postfix addition
> (still not a fan) here is my acked-by for the drivers/platform/x86
> changes, for merging this through the PWM tree in a single commit:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Thanks,

Sean

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Hans de Goede]

Hi Sean,

On 10/22/23 12:46, Sean Young wrote:
> Hi Hans,
> 
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
>> On 10/19/23 12:51, Uwe Kleine-König wrote:
>>> On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
>>>> On 10/17/23 11:17, Sean Young wrote:
>>>>> 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>
>>>>
>>>> I have no objection to this patch by itself, but it seems a bit
>>>> of unnecessary churn to change all current callers of pwm_apply_state()
>>>> to a new API.
>>>
>>> The idea is to improve the semantic of the function name, see
>>> https://lore.kernel.org/linux-pwm/20231013180449.mcdmklbsz2rlymzz@pengutronix.de
>>> for more context.
>>
>> Hmm, so the argument here is that the GPIO API has this, but GPIOs
>> generally speaking can be set atomically, so there not being able
>> to set it atomically is special.
>>
>> OTOH we have many many many other kernel functions which may sleep
>> and we don't all postfix them with _can_sleep.
>>
>> And for PWM controllers pwm_apply_state is IMHO sorta expected to
>> sleep. Many of these are attached over I2C so things will sleep,
>> others have a handshake to wait for the current dutycycle to
>> end before you can apply a second change on top of an earlier
>> change during the current dutycycle which often also involves
>> sleeping.
>>
>> So the natural/expeected thing for pwm_apply_state() is to sleep
>> and thus it does not need a postfix for this IMHO.
> 
> Most pwm drivers look like they can be made to work in atomic context,
> I think. Like you say this is not the case for all of them. Whatever
> we choose to be the default for pwm_apply_state(), we should have a
> clear function name for the alternative. This is essentially why
> pam_apply_cansleep() was picked.
> 
> The alternative to pwm_apply_cansleep() is to have a function name
> which implies it can be used from atomic context. However, 
> pwm_apply_atomic() is not great because the "atomic" could be
> confused with the PWM atomic API, not the kernel process/atomic
> context.

Well pwm_apply_state() is the atomic PWM interface right?

So to me pwm_apply_state_atomic() would be clearly about
running atomically.

> So what should the non-sleeping function be called then? 
>  - pwm_apply_cannotsleep() 
>  - pwm_apply_nosleep()
>  - pwm_apply_nonsleeping()
>  - pwm_apply_atomic_context()

I would just go with:

pwm_apply_state_atomic()

but if this is disliked by others then lets just rename

pwm_apply_state() to pwm_apply_state_cansleep() as
is done in this patch and use plain pwm_apply_state()
for the new atomic-context version.

Regards,

Hans



> 
>>> I think it's very subjective if you consider this
>>> churn or not.
>>
>> I consider it churn because I don't think adding a postfix
>> for what is the default/expected behavior is a good idea
>> (with GPIOs not sleeping is the expected behavior).
>>
>> I agree that this is very subjective and very much goes
>> into the territory of bikeshedding. So please consider
>> the above my 2 cents on this and lets leave it at that.
> 
> You have a valid point. Let's focus on having descriptive function names.
> 
>>> While it's nice to have every caller converted in a single
>>> step, I'd go for
>>>
>>> 	#define pwm_apply_state(pwm, state) pwm_apply_cansleep(pwm, state)
>>>
>>> , keep that macro for a while and convert all users step by step. This
>>> way we don't needlessly break oot code and the changes to convert to the
>>> new API can go via their usual trees without time pressure.
>>
>> I don't think there are enough users of pwm_apply_state() to warrant
>> such an exercise.
>>
>> So if people want to move ahead with the _can_sleep postfix addition
>> (still not a fan) here is my acked-by for the drivers/platform/x86
>> changes, for merging this through the PWM tree in a single commit:
>>
>> Acked-by: Hans de Goede <hdegoede@redhat.com>
> 
> Thanks,
> 
> Sean
> 

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Daniel Thompson]

On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> Hi Hans,
>
> On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > >> On 10/17/23 11:17, Sean Young wrote:
> > > I think it's very subjective if you consider this
> > > churn or not.
> >
> > I consider it churn because I don't think adding a postfix
> > for what is the default/expected behavior is a good idea
> > (with GPIOs not sleeping is the expected behavior).
> >
> > I agree that this is very subjective and very much goes
> > into the territory of bikeshedding. So please consider
> > the above my 2 cents on this and lets leave it at that.
>
> You have a valid point. Let's focus on having descriptive function names.

For a couple of days I've been trying to resist the bikeshedding (esp.
given the changes to backlight are tiny) so I'll try to keep it as
brief as I can:

1. I dislike the do_it() and do_it_cansleep() pairing. It is
   difficult to detect when a client driver calls do_it() by mistake.
   In fact a latent bug of this nature can only be detected by runtime
   testing with the small number of PWMs that do not support
   configuration from an atomic context.

   In contrast do_it() and do_it_atomic()[*] means that although
   incorrectly calling do_it() from an atomic context can be pretty
   catastrophic it is also trivially detected (with any PWM driver)
   simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.

   No objections (beyond churn) to fully spelt out pairings such as
   do_it_cansleep() and do_it_atomic()[*]!


2. If there is an API rename can we make sure the patch contains no
   other changes (e.g. don't introduce any new API in the same patch).
   Seperating renames makes the patches easier to review!
   It makes each one smaller and easier to review!


Daniel.


[*] or do_it_nosleep()... etc.

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Sean Young]

On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote:
> On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > > >> On 10/17/23 11:17, Sean Young wrote:
> > > > I think it's very subjective if you consider this
> > > > churn or not.
> > >
> > > I consider it churn because I don't think adding a postfix
> > > for what is the default/expected behavior is a good idea
> > > (with GPIOs not sleeping is the expected behavior).
> > >
> > > I agree that this is very subjective and very much goes
> > > into the territory of bikeshedding. So please consider
> > > the above my 2 cents on this and lets leave it at that.
> >
> > You have a valid point. Let's focus on having descriptive function names.
> 
> For a couple of days I've been trying to resist the bikeshedding (esp.
> given the changes to backlight are tiny) so I'll try to keep it as
> brief as I can:
> 
> 1. I dislike the do_it() and do_it_cansleep() pairing. It is
>    difficult to detect when a client driver calls do_it() by mistake.
>    In fact a latent bug of this nature can only be detected by runtime
>    testing with the small number of PWMs that do not support
>    configuration from an atomic context.
> 
>    In contrast do_it() and do_it_atomic()[*] means that although
>    incorrectly calling do_it() from an atomic context can be pretty
>    catastrophic it is also trivially detected (with any PWM driver)
>    simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.
> 
>    No objections (beyond churn) to fully spelt out pairings such as
>    do_it_cansleep() and do_it_atomic()[*]!

I must say I do like the look of this. Uwe, how do you feel about:
pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about
pwm_apply_atomic in the past, however I think this this the best 
option I've seen so far.

> 2. If there is an API rename can we make sure the patch contains no
>    other changes (e.g. don't introduce any new API in the same patch).
>    Seperating renames makes the patches easier to review!
>    It makes each one smaller and easier to review!

Yes, this should have been separated out. Will fix for next version.

Thanks,

Sean

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 3932 bytes --]

Hello,

On Wed, Oct 25, 2023 at 10:53:27AM +0100, Sean Young wrote:
> On Mon, Oct 23, 2023 at 02:34:17PM +0100, Daniel Thompson wrote:
> > On Sun, Oct 22, 2023 at 11:46:22AM +0100, Sean Young wrote:
> > > On Sat, Oct 21, 2023 at 11:08:22AM +0200, Hans de Goede wrote:
> > > > On 10/19/23 12:51, Uwe Kleine-König wrote:
> > > > > On Wed, Oct 18, 2023 at 03:57:48PM +0200, Hans de Goede wrote:
> > > > >> On 10/17/23 11:17, Sean Young wrote:
> > > > > I think it's very subjective if you consider this
> > > > > churn or not.
> > > >
> > > > I consider it churn because I don't think adding a postfix
> > > > for what is the default/expected behavior is a good idea
> > > > (with GPIOs not sleeping is the expected behavior).
> > > >
> > > > I agree that this is very subjective and very much goes
> > > > into the territory of bikeshedding. So please consider
> > > > the above my 2 cents on this and lets leave it at that.
> > >
> > > You have a valid point. Let's focus on having descriptive function names.
> > 
> > For a couple of days I've been trying to resist the bikeshedding (esp.
> > given the changes to backlight are tiny) so I'll try to keep it as
> > brief as I can:
> > 
> > 1. I dislike the do_it() and do_it_cansleep() pairing. It is
> >    difficult to detect when a client driver calls do_it() by mistake.
> >    In fact a latent bug of this nature can only be detected by runtime
> >    testing with the small number of PWMs that do not support
> >    configuration from an atomic context.
> > 
> >    In contrast do_it() and do_it_atomic()[*] means that although
> >    incorrectly calling do_it() from an atomic context can be pretty
> >    catastrophic it is also trivially detected (with any PWM driver)
> >    simply by running with CONFIG_DEBUG_ATOMIC_SLEEP.

Wrongly calling the atomic variant (no matter how it's named) in a
context where sleeping is possible is only a minor issue. Being faster
than necessary is hardly a problem, so it only hurts by not being an
preemption point with PREEMPT_VOLUNTARY which might not even be relevant
because we're near to a system call anyhow.

For me the naming is only very loosely related to the possible bugs. I
think calling the wrong function happens mainly because the driver author
isn't aware in which context the call happens and not because of wrong
assumptions about the sleepiness of a certain function call.
If you consider this an argument however, do_it + do_it_cansleep is
better than do_it_atomic + do_it as wrongly assuming do_it would sleep
is less bad than wrongly assuming do_it wouldn't sleep. (The latter is
catched by CONFIG_DEBUG_ATOMIC_SLEEP, but only if it's enabled.)

Having said that while my subjective preference ordering is (with first
= best):

	do_it + do_it_cansleep
	do_it_atomic + do_it_cansleep
	do_it_atomic + do_it

wi(th a _might_sleep or _mightsleep suffix ranging below _cansleep)
I wouldn't get sleepless nights when I get overruled here
(uwe_cansleep :-).

> >    No objections (beyond churn) to fully spelt out pairings such as
> >    do_it_cansleep() and do_it_atomic()[*]!
> 
> I must say I do like the look of this. Uwe, how do you feel about:
> pwm_apply_cansleep() and pwm_apply_atomic()? I know we've talked about
> pwm_apply_atomic in the past, however I think this this the best 
> option I've seen so far.
> 
> > 2. If there is an API rename can we make sure the patch contains no
> >    other changes (e.g. don't introduce any new API in the same patch).
> >    Seperating renames makes the patches easier to review!
> >    It makes each one smaller and easier to review!
> 
> Yes, this should have been separated out. Will fix for next version.

+1

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[kernel test robot]

Hi Sean,

kernel test robot noticed the following build errors:

[auto build test ERROR on media-tree/master]
[also build test ERROR on linus/master v6.6-rc6]
[cannot apply to thierry-reding-pwm/for-next dtor-input/next dtor-input/for-linus next-20231020]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sean-Young/pwm-make-it-possible-to-apply-pwm-changes-in-atomic-context/20231017-173221
base:   git://linuxtv.org/media_tree.git master
patch link:    https://lore.kernel.org/r/a7fcd19938d5422abc59c968ff7b3d5c275577ed.1697534024.git.sean%40mess.org
patch subject: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context
config: openrisc-allmodconfig (https://download.01.org/0day-ci/archive/20231022/202310220706.HH9RMDHJ-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231022/202310220706.HH9RMDHJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310220706.HH9RMDHJ-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/media/rc/ir-rx51.c: In function 'ir_rx51_on':
>> drivers/media/rc/ir-rx51.c:37:9: error: implicit declaration of function 'pwm_apply_state'; did you mean 'pwm_apply_args'? [-Werror=implicit-function-declaration]
      37 |         pwm_apply_state(ir_rx51->pwm, &ir_rx51->state);
         |         ^~~~~~~~~~~~~~~
         |         pwm_apply_args
   cc1: some warnings being treated as errors


vim +37 drivers/media/rc/ir-rx51.c

c332e8472d7db6 Timo Kokkonen 2012-08-10  33  
a92def1becf33e Sean Young    2016-12-19  34  static inline void ir_rx51_on(struct ir_rx51 *ir_rx51)
c332e8472d7db6 Timo Kokkonen 2012-08-10  35  {
99076cd117c4cb Maíra Canal   2021-11-04  36  	ir_rx51->state.enabled = true;
99076cd117c4cb Maíra Canal   2021-11-04 @37  	pwm_apply_state(ir_rx51->pwm, &ir_rx51->state);
c332e8472d7db6 Timo Kokkonen 2012-08-10  38  }
c332e8472d7db6 Timo Kokkonen 2012-08-10  39  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v3 1/3] pwm: make it possible to apply pwm changes in atomic context

[Jani Nikula]

On Tue, 17 Oct 2023, Sean Young <sean@mess.org> wrote:
> diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c b/drivers/gpu/drm/i915/display/intel_backlight.c
> index 2e8f17c045222..cf516190cde8f 100644
> --- a/drivers/gpu/drm/i915/display/intel_backlight.c
> +++ b/drivers/gpu/drm/i915/display/intel_backlight.c
> @@ -274,7 +274,7 @@ static void ext_pwm_set_backlight(const struct drm_connector_state *conn_state,
>  	struct intel_panel *panel = &to_intel_connector(conn_state->connector)->panel;
>  
>  	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  static void
> @@ -427,7 +427,7 @@ static void ext_pwm_disable_backlight(const struct drm_connector_state *old_conn
>  	intel_backlight_set_pwm_level(old_conn_state, level);
>  
>  	panel->backlight.pwm_state.enabled = false;
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  void intel_backlight_disable(const struct drm_connector_state *old_conn_state)
> @@ -749,7 +749,7 @@ static void ext_pwm_enable_backlight(const struct intel_crtc_state *crtc_state,
>  
>  	pwm_set_relative_duty_cycle(&panel->backlight.pwm_state, level, 100);
>  	panel->backlight.pwm_state.enabled = true;
> -	pwm_apply_state(panel->backlight.pwm, &panel->backlight.pwm_state);
> +	pwm_apply_cansleep(panel->backlight.pwm, &panel->backlight.pwm_state);
>  }
>  
>  static void __intel_backlight_enable(const struct intel_crtc_state *crtc_state,

The i915 parts are

Acked-by: Jani Nikula <jani.nikula@intel.com>

for merging via whichever tree you find most convenient, and with
whatever naming you end up with.

-- 
Jani Nikula, Intel

[PATCH v3 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Sean Young]

clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
an rpi, simply fetch it once.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/pwm/pwm-bcm2835.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/pwm/pwm-bcm2835.c b/drivers/pwm/pwm-bcm2835.c
index bdfc2a5ec0d69..bd636ac1c5074 100644
--- a/drivers/pwm/pwm-bcm2835.c
+++ b/drivers/pwm/pwm-bcm2835.c
@@ -28,6 +28,7 @@ struct bcm2835_pwm {
 	struct device *dev;
 	void __iomem *base;
 	struct clk *clk;
+	unsigned long rate;
 };
 
 static inline struct bcm2835_pwm *to_bcm2835_pwm(struct pwm_chip *chip)
@@ -63,17 +64,11 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 
 	struct bcm2835_pwm *pc = to_bcm2835_pwm(chip);
-	unsigned long rate = clk_get_rate(pc->clk);
 	unsigned long long period_cycles;
 	u64 max_period;
 
 	u32 val;
 
-	if (!rate) {
-		dev_err(pc->dev, "failed to get clock rate\n");
-		return -EINVAL;
-	}
-
 	/*
 	 * period_cycles must be a 32 bit value, so period * rate / NSEC_PER_SEC
 	 * must be <= U32_MAX. As U32_MAX * NSEC_PER_SEC < U64_MAX the
@@ -88,13 +83,13 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	 * <=> period < ((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate
 	 * <=> period <= ceil((U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC/2) / rate) - 1
 	 */
-	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, rate) - 1;
+	max_period = DIV_ROUND_UP_ULL((u64)U32_MAX * NSEC_PER_SEC + NSEC_PER_SEC / 2, pc->rate) - 1;
 
 	if (state->period > max_period)
 		return -EINVAL;
 
 	/* set period */
-	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * rate, NSEC_PER_SEC);
+	period_cycles = DIV_ROUND_CLOSEST_ULL(state->period * pc->rate, NSEC_PER_SEC);
 
 	/* don't accept a period that is too small */
 	if (period_cycles < PERIOD_MIN)
@@ -103,7 +98,7 @@ static int bcm2835_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	writel(period_cycles, pc->base + PERIOD(pwm->hwpwm));
 
 	/* set duty cycle */
-	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * rate, NSEC_PER_SEC);
+	val = DIV_ROUND_CLOSEST_ULL(state->duty_cycle * pc->rate, NSEC_PER_SEC);
 	writel(val, pc->base + DUTY(pwm->hwpwm));
 
 	/* set polarity */
@@ -156,18 +151,32 @@ static int bcm2835_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	ret = clk_rate_exclusive_get(pc->clk);
+	if (ret)
+		goto add_fail;
+
+	pc->rate = clk_get_rate(pc->clk);
+	if (!pc->rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		ret = -EINVAL;
+		goto rate_fail;
+	}
+
 	pc->chip.dev = &pdev->dev;
 	pc->chip.ops = &bcm2835_pwm_ops;
+	pc->chip.atomic = true;
 	pc->chip.npwm = 2;
 
 	platform_set_drvdata(pdev, pc);
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0)
-		goto add_fail;
+		goto rate_fail;
 
 	return 0;
 
+rate_fail:
+	clk_rate_exclusive_put(pc->clk);
 add_fail:
 	clk_disable_unprepare(pc->clk);
 	return ret;
@@ -179,6 +188,7 @@ static void bcm2835_pwm_remove(struct platform_device *pdev)
 
 	pwmchip_remove(&pc->chip);
 
+	clk_rate_exclusive_put(pc->clk);
 	clk_disable_unprepare(pc->clk);
 }
 
-- 
2.42.0

Re: [PATCH v3 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Stefan Wahren]

Hi Sean,

Am 17.10.23 um 11:17 schrieb Sean Young:
> clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> an rpi, simply fetch it once.
the driver changes looks good to me, but could you please adapt the
commit message to your recent changes.

Best regards
>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>

Re: [PATCH v3 2/3] pwm: bcm2835: allow pwm driver to be used in atomic context

[Sean Young]

On Wed, Oct 18, 2023 at 05:00:23PM +0200, Stefan Wahren wrote:
> Hi Sean,
> 
> Am 17.10.23 um 11:17 schrieb Sean Young:
> > clk_get_rate() may do a mutex lock. Since the clock rate cannot change on
> > an rpi, simply fetch it once.
> the driver changes looks good to me, but could you please adapt the
> commit message to your recent changes.

Yes, the commit message needs fixing. Thanks for pointing that out.


Sean

[PATCH v3 3/3] media: pwm-ir-tx: trigger edges from hrtimer interrupt context

[Sean Young]

This makes the driver much more precise.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/pwm-ir-tx.c | 79 ++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/media/rc/pwm-ir-tx.c b/drivers/media/rc/pwm-ir-tx.c
index ccb86890adcea..4f9844e6d3fa2 100644
--- a/drivers/media/rc/pwm-ir-tx.c
+++ b/drivers/media/rc/pwm-ir-tx.c
@@ -10,6 +10,8 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/completion.h>
 #include <media/rc-core.h>
 
 #define DRIVER_NAME	"pwm-ir-tx"
@@ -17,8 +19,14 @@
 
 struct pwm_ir {
 	struct pwm_device *pwm;
-	unsigned int carrier;
-	unsigned int duty_cycle;
+	struct hrtimer timer;
+	struct completion tx_done;
+	struct pwm_state *state;
+	u32 carrier;
+	u32 duty_cycle;
+	uint *txbuf;
+	uint txbuf_len;
+	uint txbuf_index;
 };
 
 static const struct of_device_id pwm_ir_of_match[] = {
@@ -82,6 +90,62 @@ static int pwm_ir_tx(struct rc_dev *dev, unsigned int *txbuf,
 	return count;
 }
 
+static int pwm_ir_tx_atomic(struct rc_dev *dev, unsigned int *txbuf,
+			    unsigned int count)
+{
+	struct pwm_ir *pwm_ir = dev->priv;
+	struct pwm_device *pwm = pwm_ir->pwm;
+	struct pwm_state state;
+
+	pwm_init_state(pwm, &state);
+
+	state.period = DIV_ROUND_CLOSEST(NSEC_PER_SEC, pwm_ir->carrier);
+	pwm_set_relative_duty_cycle(&state, pwm_ir->duty_cycle, 100);
+
+	pwm_ir->txbuf = txbuf;
+	pwm_ir->txbuf_len = count;
+	pwm_ir->txbuf_index = 0;
+	pwm_ir->state = &state;
+
+	hrtimer_start(&pwm_ir->timer, 0, HRTIMER_MODE_REL);
+
+	wait_for_completion(&pwm_ir->tx_done);
+
+	return count;
+}
+
+static enum hrtimer_restart pwm_ir_timer(struct hrtimer *timer)
+{
+	struct pwm_ir *pwm_ir = container_of(timer, struct pwm_ir, timer);
+	ktime_t now;
+
+	/*
+	 * If we happen to hit an odd latency spike, loop through the
+	 * pulses until we catch up.
+	 */
+	do {
+		u64 ns;
+
+		pwm_ir->state->enabled = !(pwm_ir->txbuf_index % 2);
+		pwm_apply(pwm_ir->pwm, pwm_ir->state);
+
+		if (pwm_ir->txbuf_index >= pwm_ir->txbuf_len) {
+			complete(&pwm_ir->tx_done);
+
+			return HRTIMER_NORESTART;
+		}
+
+		ns = US_TO_NS(pwm_ir->txbuf[pwm_ir->txbuf_index]);
+		hrtimer_add_expires_ns(timer, ns);
+
+		pwm_ir->txbuf_index++;
+
+		now = timer->base->get_time();
+	} while (hrtimer_get_expires_tv64(timer) < now);
+
+	return HRTIMER_RESTART;
+}
+
 static int pwm_ir_probe(struct platform_device *pdev)
 {
 	struct pwm_ir *pwm_ir;
@@ -103,10 +167,19 @@ static int pwm_ir_probe(struct platform_device *pdev)
 	if (!rcdev)
 		return -ENOMEM;
 
+	if (pwm_is_atomic(pwm_ir->pwm)) {
+		init_completion(&pwm_ir->tx_done);
+		hrtimer_init(&pwm_ir->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		pwm_ir->timer.function = pwm_ir_timer;
+		rcdev->tx_ir = pwm_ir_tx_atomic;
+	} else {
+		dev_info(&pdev->dev, "tx will not be accurate as pwm device does not support atomic mode");
+		rcdev->tx_ir = pwm_ir_tx;
+	}
+
 	rcdev->priv = pwm_ir;
 	rcdev->driver_name = DRIVER_NAME;
 	rcdev->device_name = DEVICE_NAME;
-	rcdev->tx_ir = pwm_ir_tx;
 	rcdev->s_tx_duty_cycle = pwm_ir_set_duty_cycle;
 	rcdev->s_tx_carrier = pwm_ir_set_carrier;
 
-- 
2.42.0