[PATCH v2 0/8] pwm: New abstraction and userspace API

[PATCH v2 0/8] pwm: New abstraction and userspace API

[Uwe Kleine-König]

Hello,

here comes v2 of the series, (implicit) v1 can be found at
https://lore.kernel.org/linux-pwm/cover.1720435656.git.u.kleine-koenig@baylibre.com/.

It implements a new abstraction to model the output waveform of
a PWM. The main improvement is that it defines a duty_offset instead of
a polarity and so allows to model more wave forms. The motivation for
this is that we need a PWM channel to have an offset compared to another
channel from the same chip, that is something like that:

         __                 __                 __
        /  \_______________/  \_______________/  \_________
        ^    __            ^    __            ^    __
        ____/  \_______________/  \_______________/  \_____
        ^                  ^                  ^

Compared to the previous submission the following things changed:

 - rebase to current pwm/for-next

 - Some more simplifications and cleanups upfront

 - new commit to provide API functions for consumers
   These were added as needed in the respective commits (and not exported)
   before, but it should be easier to review this way.

 - Add padding to the userspace struct pwmchip_waveform

 - shifted the introduction of some helper functions to earlier commits to have
   less code that is introduced in one patch and then refactored in a later one.

 - stricter input validation

 - implement PWM_IOCTL_ROUNDWF ioctl

 - Commit #6 got a change log and sign-off (was #4 before) 🙄

I updated the pwm/chardev branch to hold this series.

I dropped Trevor's R-b tag for patch #5 (patch #3 before) because the
patch was modified in non-trivial ways. I hesitated to keep his T-b for
patch #7 (patch #5 before), because while the axi-pwmgen patch didn't
change, it builds on top of the previous patches. I kept it anyhow.

Thanks to Trevor and Nuno for feedback to the previous submission.

Best regards
Uwe

Uwe Kleine-König (8):
  pwm: Simplify pwm_capture()
  pwm: Add more locking
  pwm: New abstraction for PWM waveforms
  pwm: Provide new consumer API functions for waveforms
  pwm: Add support for pwmchip devices for faster and easier userspace
    access
  pwm: Add tracing for waveform callbacks
  pwm: axi-pwmgen: Implementation of the waveform callbacks
  pwm: stm32: Implementation of the waveform callbacks

 drivers/pwm/core.c           | 809 +++++++++++++++++++++++++++++++++--
 drivers/pwm/pwm-axi-pwmgen.c | 148 +++++--
 drivers/pwm/pwm-stm32.c      | 605 ++++++++++++++++----------
 include/linux/pwm.h          |  57 ++-
 include/trace/events/pwm.h   | 134 +++++-
 include/uapi/linux/pwm.h     |  25 ++
 6 files changed, 1470 insertions(+), 308 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h


base-commit: 240b129d597cb8a8880eb3a381ff10eb98ca0c07
prerequisite-patch-id: 0e21153cd012f41ba9db52357fd08219af53e26c
prerequisite-patch-id: b22c91bbc4e3412f8e7e1f796ed18570ae021c96
-- 
2.43.0

[PATCH v2 1/8] pwm: Simplify pwm_capture()

[Uwe Kleine-König]

When pwm_capture() is called, pwm is valid, so the checks for pwm and
pwm->chip->ops being NULL can be dropped.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 56d91c11f0d4..6e752e148b98 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -328,15 +328,15 @@ EXPORT_SYMBOL_GPL(pwm_adjust_config);
 static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 		       unsigned long timeout)
 {
-	if (!pwm || !pwm->chip->ops)
-		return -EINVAL;
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
 
-	if (!pwm->chip->ops->capture)
+	if (!ops->capture)
 		return -ENOSYS;
 
 	guard(mutex)(&pwm_lock);
 
-	return pwm->chip->ops->capture(pwm->chip, pwm, result, timeout);
+	return ops->capture(chip, pwm, result, timeout);
 }
 
 static struct pwm_chip *pwmchip_find_by_name(const char *name)
-- 
2.43.0

[PATCH v2 2/8] pwm: Add more locking

[Uwe Kleine-König]

This ensures that a pwm_chip that has no corresponding driver isn't used
and that a driver doesn't go away while a callback is still running.

In the presence of device links this isn't necessary yet (so this is no
fix) but for pwm character device support this is needed.

To not serialize all pwm_apply_state() calls, this introduces a per chip
lock. An additional complication is that for atomic chips a mutex cannot
be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be
held while calling an operation for a sleeping chip. So depending on the
chip being atomic or not a spinlock or a mutex is used.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c  | 95 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/pwm.h | 13 +++++++
 2 files changed, 100 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6e752e148b98..b97e2ea0691d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -31,6 +31,24 @@ static DEFINE_MUTEX(pwm_lock);
 
 static DEFINE_IDR(pwm_chips);
 
+static void pwmchip_lock(struct pwm_chip *chip)
+{
+	if (chip->atomic)
+		spin_lock(&chip->atomic_lock);
+	else
+		mutex_lock(&chip->nonatomic_lock);
+}
+
+static void pwmchip_unlock(struct pwm_chip *chip)
+{
+	if (chip->atomic)
+		spin_unlock(&chip->atomic_lock);
+	else
+		mutex_unlock(&chip->nonatomic_lock);
+}
+
+DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
+
 static void pwm_apply_debug(struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
@@ -220,6 +238,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	int err;
+	struct pwm_chip *chip = pwm->chip;
 
 	/*
 	 * Some lowlevel driver's implementations of .apply() make use of
@@ -230,7 +249,12 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
 	 */
 	might_sleep();
 
-	if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
 		/*
 		 * Catch any drivers that have been marked as atomic but
 		 * that will sleep anyway.
@@ -254,9 +278,16 @@ EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
  */
 int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 {
-	WARN_ONCE(!pwm->chip->atomic,
+	struct pwm_chip *chip = pwm->chip;
+
+	WARN_ONCE(!chip->atomic,
 		  "sleeping PWM driver used in atomic context\n");
 
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
 	return __pwm_apply(pwm, state);
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
@@ -336,6 +367,11 @@ static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 
 	guard(mutex)(&pwm_lock);
 
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
 	return ops->capture(chip, pwm, result, timeout);
 }
 
@@ -368,6 +404,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 	if (test_bit(PWMF_REQUESTED, &pwm->flags))
 		return -EBUSY;
 
+	/*
+	 * This function is called while holding pwm_lock. As .operational only
+	 * changes while holding this lock, checking it here without holding the
+	 * chip lock is fine.
+	 */
+	if (!chip->operational)
+		return -ENODEV;
+
 	if (!try_module_get(chip->owner))
 		return -ENODEV;
 
@@ -396,7 +440,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
-		err = ops->get_state(chip, pwm, &state);
+		scoped_guard(pwmchip, chip)
+			err = ops->get_state(chip, pwm, &state);
+
 		trace_pwm_get(pwm, &state, err);
 
 		if (!err)
@@ -1020,6 +1066,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
 
 	chip->npwm = npwm;
 	chip->uses_pwmchip_alloc = true;
+	chip->operational = false;
 
 	pwmchip_dev = &chip->dev;
 	device_initialize(pwmchip_dev);
@@ -1125,6 +1172,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 
 	chip->owner = owner;
 
+	if (chip->atomic)
+		spin_lock_init(&chip->atomic_lock);
+	else
+		mutex_init(&chip->nonatomic_lock);
+
 	guard(mutex)(&pwm_lock);
 
 	ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
@@ -1138,6 +1190,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_add(chip);
 
+	scoped_guard(pwmchip, chip)
+		chip->operational = true;
+
 	ret = device_add(&chip->dev);
 	if (ret)
 		goto err_device_add;
@@ -1145,6 +1200,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	return 0;
 
 err_device_add:
+	scoped_guard(pwmchip, chip)
+		chip->operational = false;
+
 	if (IS_ENABLED(CONFIG_OF))
 		of_pwmchip_remove(chip);
 
@@ -1164,11 +1222,27 @@ void pwmchip_remove(struct pwm_chip *chip)
 {
 	pwmchip_sysfs_unexport(chip);
 
-	if (IS_ENABLED(CONFIG_OF))
-		of_pwmchip_remove(chip);
+	scoped_guard(mutex, &pwm_lock) {
+		unsigned int i;
+
+		scoped_guard(pwmchip, chip)
+			chip->operational = false;
+
+		for (i = 0; i < chip->npwm; ++i) {
+			struct pwm_device *pwm = &chip->pwms[i];
+
+			if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+				dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
+				if (pwm->chip->ops->free)
+					pwm->chip->ops->free(pwm->chip, pwm);
+			}
+		}
+
+		if (IS_ENABLED(CONFIG_OF))
+			of_pwmchip_remove(chip);
 
-	scoped_guard(mutex, &pwm_lock)
 		idr_remove(&pwm_chips, chip->id);
+	}
 
 	device_del(&chip->dev);
 }
@@ -1538,12 +1612,17 @@ void pwm_put(struct pwm_device *pwm)
 
 	guard(mutex)(&pwm_lock);
 
-	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+	/*
+	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
+	 * don't warn in this case. This can only happen if a consumer called
+	 * pwm_put() twice.
+	 */
+	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
 		pr_warn("PWM device already freed\n");
 		return;
 	}
 
-	if (chip->ops->free)
+	if (chip->operational && chip->ops->free)
 		pwm->chip->ops->free(pwm->chip, pwm);
 
 	pwm->label = NULL;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 8acd60b53f58..464054a45e57 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -275,6 +275,9 @@ struct pwm_ops {
  * @of_xlate: request a PWM device given a device tree PWM specifier
  * @atomic: can the driver's ->apply() be called in atomic context
  * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
+ * @operational: signals if the chip can be used (or is already deregistered)
+ * @nonatomic_lock: mutex for nonatomic chips
+ * @atomic_lock: mutex for atomic chips
  * @pwms: array of PWM devices allocated by the framework
  */
 struct pwm_chip {
@@ -290,6 +293,16 @@ struct pwm_chip {
 
 	/* only used internally by the PWM framework */
 	bool uses_pwmchip_alloc;
+	bool operational;
+	union {
+		/*
+		 * depending on the chip being atomic or not either the mutex or
+		 * the spinlock is used. It protects .operational and
+		 * synchronizes calls to the .ops->apply and .ops->get_state()
+		 */
+		struct mutex nonatomic_lock;
+		struct spinlock atomic_lock;
+	};
 	struct pwm_device pwms[] __counted_by(npwm);
 };
 
-- 
2.43.0

Re: [PATCH v2 2/8] pwm: Add more locking

[David Lechner]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
> 
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
> 
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be
> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
>  drivers/pwm/core.c  | 95 +++++++++++++++++++++++++++++++++++++++++----
>  include/linux/pwm.h | 13 +++++++
>  2 files changed, 100 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 6e752e148b98..b97e2ea0691d 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c

...

> @@ -1138,6 +1190,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_add(chip);
>  
> +	scoped_guard(pwmchip, chip)
> +		chip->operational = true;

Strictly speaking, is the pwmchip lock actually needed here since nothing else
can access the chip until device_add() is called?

I guess it doesn't hurt to take it anyway though.

> +
>  	ret = device_add(&chip->dev);
>  	if (ret)
>  		goto err_device_add;
> @@ -1145,6 +1200,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
>  	return 0;
>  
>  err_device_add:
> +	scoped_guard(pwmchip, chip)
> +		chip->operational = false;
> +
>  	if (IS_ENABLED(CONFIG_OF))
>  		of_pwmchip_remove(chip);
>  
> @@ -1164,11 +1222,27 @@ void pwmchip_remove(struct pwm_chip *chip)
>  {
>  	pwmchip_sysfs_unexport(chip);
>  
> -	if (IS_ENABLED(CONFIG_OF))
> -		of_pwmchip_remove(chip);
> +	scoped_guard(mutex, &pwm_lock) {
> +		unsigned int i;
> +
> +		scoped_guard(pwmchip, chip)
> +			chip->operational = false;
> +
> +		for (i = 0; i < chip->npwm; ++i) {
> +			struct pwm_device *pwm = &chip->pwms[i];
> +
> +			if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +				dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);

Is it really so serious that dev_alert() is needed? dev_warn() or
dev_err() seems more appropriate IMHO.

> +				if (pwm->chip->ops->free)
> +					pwm->chip->ops->free(pwm->chip, pwm);
> +			}
> +		}
> +
> +		if (IS_ENABLED(CONFIG_OF))
> +			of_pwmchip_remove(chip);
>  
> -	scoped_guard(mutex, &pwm_lock)
>  		idr_remove(&pwm_chips, chip->id);
> +	}
>  
>  	device_del(&chip->dev);
>  }
> @@ -1538,12 +1612,17 @@ void pwm_put(struct pwm_device *pwm)
>  
>  	guard(mutex)(&pwm_lock);
>  
> -	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> +	/*
> +	 * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
> +	 * don't warn in this case. This can only happen if a consumer called
> +	 * pwm_put() twice.
> +	 */
> +	if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
>  		pr_warn("PWM device already freed\n");
>  		return;
>  	}
>  
> -	if (chip->ops->free)
> +	if (chip->operational && chip->ops->free)
>  		pwm->chip->ops->free(pwm->chip, pwm);
>  
>  	pwm->label = NULL;
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 8acd60b53f58..464054a45e57 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -275,6 +275,9 @@ struct pwm_ops {
>   * @of_xlate: request a PWM device given a device tree PWM specifier
>   * @atomic: can the driver's ->apply() be called in atomic context
>   * @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
> + * @operational: signals if the chip can be used (or is already deregistered)
> + * @nonatomic_lock: mutex for nonatomic chips
> + * @atomic_lock: mutex for atomic chips
>   * @pwms: array of PWM devices allocated by the framework
>   */
>  struct pwm_chip {
> @@ -290,6 +293,16 @@ struct pwm_chip {
>  
>  	/* only used internally by the PWM framework */
>  	bool uses_pwmchip_alloc;
> +	bool operational;
> +	union {
> +		/*
> +		 * depending on the chip being atomic or not either the mutex or
> +		 * the spinlock is used. It protects .operational and
> +		 * synchronizes calls to the .ops->apply and .ops->get_state()

nit: inconsistent use of (), and also synchronizes calls to .ops->capture()

> +		 */
> +		struct mutex nonatomic_lock;
> +		struct spinlock atomic_lock;
> +	};
>  	struct pwm_device pwms[] __counted_by(npwm);
>  };
>  

Re: [PATCH v2 2/8] pwm: Add more locking

[Uwe Kleine-König]

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

Hello David,

On Mon, Jul 15, 2024 at 11:56:05AM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > @@ -1138,6 +1190,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
> >  	if (IS_ENABLED(CONFIG_OF))
> >  		of_pwmchip_add(chip);
> >  
> > +	scoped_guard(pwmchip, chip)
> > +		chip->operational = true;
> 
> Strictly speaking, is the pwmchip lock actually needed here since nothing else
> can access the chip until device_add() is called?
> 
> I guess it doesn't hurt to take it anyway though.

It might not be technically necessary, but __pwmchip_add() is the
exception here and holding the lock consistently while accessing
chip->operational has some benefit, too. I prefer to keep it.

> > @@ -1164,11 +1222,27 @@ void pwmchip_remove(struct pwm_chip *chip)
> >  {
> >  	pwmchip_sysfs_unexport(chip);
> >  
> > -	if (IS_ENABLED(CONFIG_OF))
> > -		of_pwmchip_remove(chip);
> > +	scoped_guard(mutex, &pwm_lock) {
> > +		unsigned int i;
> > +
> > +		scoped_guard(pwmchip, chip)
> > +			chip->operational = false;
> > +
> > +		for (i = 0; i < chip->npwm; ++i) {
> > +			struct pwm_device *pwm = &chip->pwms[i];
> > +
> > +			if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
> > +				dev_alert(&chip->dev, "Freeing requested PWM #%u\n", i);
> 
> Is it really so serious that dev_alert() is needed? dev_warn() or
> dev_err() seems more appropriate IMHO.

I think I picked dev_alert back when this was indeed alarming and
leaving behind some dangling pointers. Today you're right, "action must
be taken immediately" doesn't apply any more. I'll go for dev_warn().

> > @@ -290,6 +293,16 @@ struct pwm_chip {
> >  
> >  	/* only used internally by the PWM framework */
> >  	bool uses_pwmchip_alloc;
> > +	bool operational;
> > +	union {
> > +		/*
> > +		 * depending on the chip being atomic or not either the mutex or
> > +		 * the spinlock is used. It protects .operational and
> > +		 * synchronizes calls to the .ops->apply and .ops->get_state()
> 
> nit: inconsistent use of (), and also synchronizes calls to .ops->capture()

ack. It also protects the new lowlevel driver callbacks, so will reword
to just speak about pwm_ops callbacks.

Thanks for your feedback, I really appreciate people looking in detail
before the new API is set in stone.

Best regards
Uwe

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

[PATCH v2 3/8] pwm: New abstraction for PWM waveforms

[Uwe Kleine-König]

Up to now the configuration of a PWM setting is described exclusively by
a struct pwm_state which contains information about period, duty_cycle,
polarity and if the PWM is enabled. (There is another member usage_power
which doesn't completely fit into pwm_state, I ignore it here for
simplicity.)

Instead of a polarity the new abstraction has a member duty_offset that
defines when the rising edge happens after the period start. This is
more general, as with a pwm_state the rising edge can only happen at the
period's start or such that the falling edge is at the end of the period
(i.e. duty_offset == 0 or duty_offset == period_length - duty_length).

A disabled PWM is modeled by .period_length = 0. In my eyes this is a
nice usage of that otherwise unusable setting, as it doesn't define
anything about the future which matches the fact that consumers should
consider the state of the output as undefined and it's just there to say
"No further requirements about the output, you can save some power.".

Further I renamed period and duty_cycle to period_length and
duty_length. In the past there was confusion from time to time about
duty_cycle being measured in nanoseconds because people expected a
percentage of period instead. With "length" as suffix the semantic
should be more obvious to people unfamiliar with the pwm subsystem.
period is renamed to period_length for consistency.

The API for consumers doesn't change yet, but lowlevel drivers can
implement callbacks that work with pwm_waveforms instead of pwm_states.
A new thing about these callbacks is that the calculation of hardware
settings needed to implement a certain waveform is separated from
actually writing these settings. The motivation for that is that this
allows a consumer to query the hardware capabilities without actually
modifying the hardware state.

The rounding rules that are expected to be implemented in the
round_waveform_tohw() are: First pick the biggest possible period not
bigger than wf->period_length. For that period pick the biggest possible
duty setting not bigger than wf->duty_length. Third pick the biggest
possible offset not bigger than wf->duty_offset. If the requested period
is too small for the hardware, it's expected that a setting with the
minimal period and duty_length = duty_offset = 0 is returned and this
fact is signaled by a return value of 1.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c  | 225 +++++++++++++++++++++++++++++++++++++++-----
 include/linux/pwm.h |  35 +++++++
 2 files changed, 239 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index b97e2ea0691d..1f52cabe0131 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -49,6 +49,102 @@ static void pwmchip_unlock(struct pwm_chip *chip)
 
 DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
 
+static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
+{
+	if (wf->period_length) {
+		if (wf->duty_length + wf->duty_offset < wf->period_length)
+			*state = (struct pwm_state){
+				.enabled = true,
+				.polarity = PWM_POLARITY_NORMAL,
+				.period = wf->period_length,
+				.duty_cycle = wf->duty_length,
+			};
+		else
+			*state = (struct pwm_state){
+				.enabled = true,
+				.polarity = PWM_POLARITY_INVERSED,
+				.period = wf->period_length,
+				.duty_cycle = wf->period_length - wf->duty_length,
+			};
+	} else {
+		*state = (struct pwm_state){
+			.enabled = false,
+		};
+	}
+}
+
+static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
+{
+	if (state->enabled) {
+		if (state->polarity == PWM_POLARITY_NORMAL)
+			*wf = (struct pwm_waveform){
+				.period_length = state->period,
+				.duty_length = state->duty_cycle,
+				.duty_offset = 0,
+			};
+		else
+			*wf = (struct pwm_waveform){
+				.period_length = state->period,
+				.duty_length = state->period - state->duty_cycle,
+				.duty_offset = state->duty_cycle,
+			};
+	} else {
+		*wf = (struct pwm_waveform){
+			.period_length = 0,
+		};
+	}
+}
+
+static int pwm_check_rounding(const struct pwm_waveform *wf,
+			      const struct pwm_waveform *wf_rounded)
+{
+	if (!wf->period_length)
+		return 0;
+
+	if (wf->period_length < wf_rounded->period_length)
+		return 1;
+
+	if (wf->duty_length < wf_rounded->duty_length)
+		return 1;
+
+	if (wf->duty_offset < wf_rounded->duty_offset)
+		return 1;
+
+	return 0;
+}
+
+static int __pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *pwm,
+				     const struct pwm_waveform *wf, void *wfhw)
+{
+	const struct pwm_ops *ops = chip->ops;
+
+	return ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+}
+
+static int __pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+				       const void *wfhw, struct pwm_waveform *wf)
+{
+	const struct pwm_ops *ops = chip->ops;
+
+	return ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+}
+
+static int __pwm_read_waveform(struct pwm_chip *chip, struct pwm_device *pwm, void *wfhw)
+{
+	const struct pwm_ops *ops = chip->ops;
+
+	return ops->read_waveform(chip, pwm, wfhw);
+}
+
+static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, const void *wfhw)
+{
+	const struct pwm_ops *ops = chip->ops;
+
+	return ops->write_waveform(chip, pwm, wfhw);
+}
+
+#define WFHWSIZE 20
+
 static void pwm_apply_debug(struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
@@ -182,6 +278,7 @@ static bool pwm_state_valid(const struct pwm_state *state)
 static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 {
 	struct pwm_chip *chip;
+	const struct pwm_ops *ops;
 	int err;
 
 	if (!pwm || !state)
@@ -205,6 +302,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 	}
 
 	chip = pwm->chip;
+	ops = chip->ops;
 
 	if (state->period == pwm->state.period &&
 	    state->duty_cycle == pwm->state.duty_cycle &&
@@ -213,18 +311,60 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 	    state->usage_power == pwm->state.usage_power)
 		return 0;
 
-	err = chip->ops->apply(chip, pwm, state);
-	trace_pwm_apply(pwm, state, err);
-	if (err)
-		return err;
+	if (ops->write_waveform) {
+		struct pwm_waveform wf;
+		char wfhw[WFHWSIZE];
 
-	pwm->state = *state;
+		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
 
-	/*
-	 * only do this after pwm->state was applied as some
-	 * implementations of .get_state depend on this
-	 */
-	pwm_apply_debug(pwm, state);
+		pwm_state2wf(state, &wf);
+
+		/*
+		 * The rounding is wrong here for states with inverted
+		 * polarity. While .apply() rounds down duty_cycle (which
+		 * represents the time from the start of the period to the inner
+		 * edge), .round_waveform_tohw() rounds down the time the PWM is
+		 * high. Can be fixed if the ned arises, until reported
+		 * otherwise let's assume that consumers don't care.
+		 */
+
+		err = __pwm_round_waveform_tohw(chip, pwm, &wf, &wfhw);
+		if (err)
+			return err;
+
+		if (IS_ENABLED(CONFIG_PWM_DEBUG)) {
+			struct pwm_waveform wf_rounded;
+
+			err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
+			if (err)
+				return err;
+
+			if (pwm_check_rounding(&wf, &wf_rounded))
+				dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
+					wf.duty_length, wf.period_length, wf.duty_offset,
+					wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
+		}
+
+		err = __pwm_write_waveform(chip, pwm, &wfhw);
+		if (err)
+			return err;
+
+		pwm->state = *state;
+
+	} else {
+		err = ops->apply(chip, pwm, state);
+		trace_pwm_apply(pwm, state, err);
+		if (err)
+			return err;
+
+		pwm->state = *state;
+
+		/*
+		 * only do this after pwm->state was applied as some
+		 * implementations of .get_state depend on this
+		 */
+		pwm_apply_debug(pwm, state);
+	}
 
 	return 0;
 }
@@ -292,6 +432,41 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
 }
 EXPORT_SYMBOL_GPL(pwm_apply_atomic);
 
+static int pwm_get_state_hw(struct pwm_device *pwm, struct pwm_state *state)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	int ret = -EOPNOTSUPP;
+
+	if (ops->read_waveform) {
+		char wfhw[WFHWSIZE];
+		struct pwm_waveform wf;
+
+		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
+
+		scoped_guard(pwmchip, chip) {
+
+			ret = __pwm_read_waveform(chip, pwm, &wfhw);
+			if (ret)
+				return ret;
+
+			ret = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf);
+			if (ret)
+				return ret;
+		}
+
+		pwm_wf2state(&wf, state);
+
+	} else if (ops->get_state) {
+		scoped_guard(pwmchip, chip)
+			ret = ops->get_state(chip, pwm, state);
+
+		trace_pwm_get(pwm, state, ret);
+	}
+
+	return ret;
+}
+
 /**
  * pwm_adjust_config() - adjust the current PWM config to the PWM arguments
  * @pwm: PWM device
@@ -430,7 +605,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		}
 	}
 
-	if (ops->get_state) {
+	if (ops->read_waveform || ops->get_state) {
 		/*
 		 * Zero-initialize state because most drivers are unaware of
 		 * .usage_power. The other members of state are supposed to be
@@ -440,11 +615,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
 		 */
 		struct pwm_state state = { 0, };
 
-		scoped_guard(pwmchip, chip)
-			err = ops->get_state(chip, pwm, &state);
-
-		trace_pwm_get(pwm, &state, err);
-
+		err = pwm_get_state_hw(pwm, &state);
 		if (!err)
 			pwm->state = state;
 
@@ -1131,12 +1302,24 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
 {
 	const struct pwm_ops *ops = chip->ops;
 
-	if (!ops->apply)
-		return false;
+	if (ops->write_waveform) {
+		if (!ops->round_waveform_tohw ||
+		    !ops->round_waveform_fromhw ||
+		    !ops->write_waveform)
+			return false;
 
-	if (IS_ENABLED(CONFIG_PWM_DEBUG) && !ops->get_state)
-		dev_warn(pwmchip_parent(chip),
-			 "Please implement the .get_state() callback\n");
+		if (WFHWSIZE < ops->sizeof_wfhw) {
+			dev_warn(pwmchip_parent(chip), "WFHWSIZE < %zu\n", ops->sizeof_wfhw);
+			return false;
+		}
+	} else {
+		if (!ops->apply)
+			return false;
+
+		if (IS_ENABLED(CONFIG_PWM_DEBUG) && !ops->get_state)
+			dev_warn(pwmchip_parent(chip),
+				 "Please implement the .get_state() callback\n");
+	}
 
 	return true;
 }
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 464054a45e57..2a1f1f25a56c 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -49,6 +49,30 @@ enum {
 	PWMF_EXPORTED = 1,
 };
 
+/*
+ * struct pwm_waveform - description of a PWM waveform
+ * @period_length: PWM period
+ * @duty_length: PWM duty cycle
+ * @duty_offset: offset of the rising edge from the period's start
+ *
+ * This is a representation of a PWM waveform alternative to struct pwm_state
+ * below. It's more expressive than struct pwm_state as it contains a
+ * duty_offset and so can represent offsets other than $period - $duty_cycle
+ * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
+ * explicit bool for enabled. A "disabled" PWM is represented by .period = 0.
+ *
+ * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
+ * hardware's capabilities it might drive the active or inactive level, go
+ * high-z or even continue to toggle.
+ *
+ * The unit for all three members is nanoseconds.
+ */
+struct pwm_waveform {
+	u64 period_length;
+	u64 duty_length;
+	u64 duty_offset;
+};
+
 /*
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
@@ -259,6 +283,17 @@ struct pwm_ops {
 	void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);
 	int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
 		       struct pwm_capture *result, unsigned long timeout);
+
+	size_t sizeof_wfhw;
+	int (*round_waveform_tohw)(struct pwm_chip *chip, struct pwm_device *pwm,
+				   const struct pwm_waveform *wf, void *wfhw);
+	int (*round_waveform_fromhw)(struct pwm_chip *chip, struct pwm_device *pwm,
+				     const void *wfhw, struct pwm_waveform *wf);
+	int (*read_waveform)(struct pwm_chip *chip, struct pwm_device *pwm,
+			    void *wfhw);
+	int (*write_waveform)(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const void *wfhw);
+
 	int (*apply)(struct pwm_chip *chip, struct pwm_device *pwm,
 		     const struct pwm_state *state);
 	int (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.43.0

Re: [PATCH v2 3/8] pwm: New abstraction for PWM waveforms

[David Lechner]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:


> @@ -213,18 +311,60 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>  	    state->usage_power == pwm->state.usage_power)
>  		return 0;
>  
> -	err = chip->ops->apply(chip, pwm, state);
> -	trace_pwm_apply(pwm, state, err);
> -	if (err)
> -		return err;
> +	if (ops->write_waveform) {
> +		struct pwm_waveform wf;
> +		char wfhw[WFHWSIZE];
>  
> -	pwm->state = *state;
> +		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);

Since this is already validated in pwm_ops_check(), do we really need the
BUG_ON() check?


> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 464054a45e57..2a1f1f25a56c 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -49,6 +49,30 @@ enum {
>  	PWMF_EXPORTED = 1,
>  };
>  
> +/*
> + * struct pwm_waveform - description of a PWM waveform
> + * @period_length: PWM period
> + * @duty_length: PWM duty cycle
> + * @duty_offset: offset of the rising edge from the period's start
> + *
> + * This is a representation of a PWM waveform alternative to struct pwm_state
> + * below. It's more expressive than struct pwm_state as it contains a
> + * duty_offset and so can represent offsets other than $period - $duty_cycle
> + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
> + * explicit bool for enabled. A "disabled" PWM is represented by .period = 0.
> + *
> + * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
> + * hardware's capabilities it might drive the active or inactive level, go
> + * high-z or even continue to toggle.
> + *
> + * The unit for all three members is nanoseconds.
> + */
> +struct pwm_waveform {
> +	u64 period_length;
> +	u64 duty_length;
> +	u64 duty_offset;
> +};

Perhaps it would be helpful to take a hint from the IIO subsystem
and include the units of measurement in the field names here?
For example, period_length_ns or even just period_ns. This way,
the value is obvious even without reading the documentation.

Re: [PATCH v2 3/8] pwm: New abstraction for PWM waveforms

[Uwe Kleine-König]

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

Hello,

On Mon, Jul 15, 2024 at 01:55:43PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > @@ -213,18 +311,60 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
> >  	    state->usage_power == pwm->state.usage_power)
> >  		return 0;
> >  
> > -	err = chip->ops->apply(chip, pwm, state);
> > -	trace_pwm_apply(pwm, state, err);
> > -	if (err)
> > -		return err;
> > +	if (ops->write_waveform) {
> > +		struct pwm_waveform wf;
> > +		char wfhw[WFHWSIZE];
> >  
> > -	pwm->state = *state;
> > +		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> 
> Since this is already validated in pwm_ops_check(), do we really need the
> BUG_ON() check?

It indeed should not happen, and I'm glad you seem to agree it's safe.
The motivation to still keep it is that if (now or after some changes
in the future) I missed a code path, it's IMHO better when the kernel
dies on a BUG_ON (which indicates the error condition) than via some
stack corruption some time later.

> > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > index 464054a45e57..2a1f1f25a56c 100644
> > --- a/include/linux/pwm.h
> > +++ b/include/linux/pwm.h
> > @@ -49,6 +49,30 @@ enum {
> >  	PWMF_EXPORTED = 1,
> >  };
> >  
> > +/*
> > + * struct pwm_waveform - description of a PWM waveform
> > + * @period_length: PWM period
> > + * @duty_length: PWM duty cycle
> > + * @duty_offset: offset of the rising edge from the period's start
> > + *
> > + * This is a representation of a PWM waveform alternative to struct pwm_state
> > + * below. It's more expressive than struct pwm_state as it contains a
> > + * duty_offset and so can represent offsets other than $period - $duty_cycle
> > + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
> > + * explicit bool for enabled. A "disabled" PWM is represented by .period = 0.
> > + *
> > + * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
> > + * hardware's capabilities it might drive the active or inactive level, go
> > + * high-z or even continue to toggle.
> > + *
> > + * The unit for all three members is nanoseconds.
> > + */
> > +struct pwm_waveform {
> > +	u64 period_length;
> > +	u64 duty_length;
> > +	u64 duty_offset;
> > +};
> 
> Perhaps it would be helpful to take a hint from the IIO subsystem
> and include the units of measurement in the field names here?
> For example, period_length_ns or even just period_ns. This way,
> the value is obvious even without reading the documentation.

Good idea. For duty_length the "length" part is more important than for
period_length. And indeed I wasn't sure if I should rename period at
all. Being a fan of consistency I prefer to keep "length" also for
period (length). But I like the _ns suffix and will rework accordingly.

Best regards
Uwe

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

Re: [PATCH v2 3/8] pwm: New abstraction for PWM waveforms

[Nuno Sá]

On Mon, 2024-07-15 at 22:17 +0200, Uwe Kleine-König wrote:
> Hello,
> 
> On Mon, Jul 15, 2024 at 01:55:43PM -0500, David Lechner wrote:
> > On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > > @@ -213,18 +311,60 @@ static int __pwm_apply(struct pwm_device *pwm, const
> > > struct pwm_state *state)
> > >  	    state->usage_power == pwm->state.usage_power)
> > >  		return 0;
> > >  
> > > -	err = chip->ops->apply(chip, pwm, state);
> > > -	trace_pwm_apply(pwm, state, err);
> > > -	if (err)
> > > -		return err;
> > > +	if (ops->write_waveform) {
> > > +		struct pwm_waveform wf;
> > > +		char wfhw[WFHWSIZE];
> > >  
> > > -	pwm->state = *state;
> > > +		BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
> > 
> > Since this is already validated in pwm_ops_check(), do we really need the
> > BUG_ON() check?
> 
> It indeed should not happen, and I'm glad you seem to agree it's safe.
> The motivation to still keep it is that if (now or after some changes
> in the future) I missed a code path, it's IMHO better when the kernel
> dies on a BUG_ON (which indicates the error condition) than via some
> stack corruption some time later.
> 
> > > diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> > > index 464054a45e57..2a1f1f25a56c 100644
> > > --- a/include/linux/pwm.h
> > > +++ b/include/linux/pwm.h
> > > @@ -49,6 +49,30 @@ enum {
> > >  	PWMF_EXPORTED = 1,
> > >  };
> > >  
> > > +/*
> > > + * struct pwm_waveform - description of a PWM waveform
> > > + * @period_length: PWM period
> > > + * @duty_length: PWM duty cycle
> > > + * @duty_offset: offset of the rising edge from the period's start
> > > + *
> > > + * This is a representation of a PWM waveform alternative to struct
> > > pwm_state
> > > + * below. It's more expressive than struct pwm_state as it contains a
> > > + * duty_offset and so can represent offsets other than $period -
> > > $duty_cycle
> > > + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is
> > > no
> > > + * explicit bool for enabled. A "disabled" PWM is represented by .period
> > > = 0.
> > > + *
> > > + * Note that the behaviour of a "disabled" PWM is undefined. Depending on
> > > the
> > > + * hardware's capabilities it might drive the active or inactive level,
> > > go
> > > + * high-z or even continue to toggle.
> > > + *
> > > + * The unit for all three members is nanoseconds.
> > > + */
> > > +struct pwm_waveform {
> > > +	u64 period_length;
> > > +	u64 duty_length;
> > > +	u64 duty_offset;
> > > +};
> > 
> > Perhaps it would be helpful to take a hint from the IIO subsystem
> > and include the units of measurement in the field names here?
> > For example, period_length_ns or even just period_ns. This way,
> > the value is obvious even without reading the documentation.
> 
> Good idea. For duty_length the "length" part is more important than for
> period_length. And indeed I wasn't sure if I should rename period at
> all. Being a fan of consistency I prefer to keep "length" also for
> period (length). But I like the _ns suffix and will rework accordingly.
> 

Just as a side note. BUG_ON() usage is highly discouraged [1]... Even WARN_ON()
- which I don't agree much FWIW - is also being discouraged because of
panic_on_warn. But it actually seems that WARN* is fine but should really be
used with care.

[1]: https://docs.kernel.org/process/coding-style.html#use-warn-rather-than-bug

- Nuno Sá

[PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[Uwe Kleine-König]

Provide API functions for consumers to work with waveforms.

Note that one relevant difference between pwm_get_state() and
pwm_get_waveform*() is that the latter yields the actually configured
hardware state, while the former yields the last state passed to
pwm_apply*() and so doesn't account for hardware specific rounding.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c  | 201 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h |   6 +-
 2 files changed, 206 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1f52cabe0131..4dcb7ec4223f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -49,6 +49,30 @@ static void pwmchip_unlock(struct pwm_chip *chip)
 
 DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
 
+static bool pwm_wf_valid(const struct pwm_waveform *wf)
+{
+	/*
+	 * For now restrict waveforms to period_length <= S64_MAX to provide
+	 * some space for future extensions. One possibility is to simplify
+	 * representing waveforms with inverted polarity using negative values
+	 * somehow.
+	 */
+	if (wf->period_length > S64_MAX)
+		return false;
+
+	if (wf->duty_length > wf->period_length)
+		return false;
+
+	/*
+	 * .duty_offset is supposed to be smaller than .period_length, apart
+	 * from the corner case .duty_offset = 0 + .period = 0.
+	 */
+	if (wf->duty_offset && wf->duty_offset >= wf->period_length)
+		return false;
+
+	return true;
+}
+
 static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
 {
 	if (wf->period_length) {
@@ -95,6 +119,29 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
 	}
 }
 
+static int pwmwfcmp(const struct pwm_waveform *a, const struct pwm_waveform *b)
+{
+	if (a->period_length > b->period_length)
+		return 1;
+
+	if (a->period_length < b->period_length)
+		return -1;
+
+	if (a->duty_length > b->duty_length)
+		return 1;
+
+	if (a->duty_length < b->duty_length)
+		return -1;
+
+	if (a->duty_offset > b->duty_offset)
+		return 1;
+
+	if (a->duty_offset < b->duty_offset)
+		return -1;
+
+	return 0;
+}
+
 static int pwm_check_rounding(const struct pwm_waveform *wf,
 			      const struct pwm_waveform *wf_rounded)
 {
@@ -145,6 +192,160 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c
 
 #define WFHWSIZE 20
 
+int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	struct pwm_waveform wf_req = *wf;
+	char wfhw[WFHWSIZE];
+	int ret_tohw, ret_fromhw;
+
+	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
+
+	if (!pwm_wf_valid(wf))
+		return -EINVAL;
+
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	ret_tohw = __pwm_round_waveform_tohw(chip, pwm, wf, wfhw);
+	if (ret_tohw < 0)
+		return ret_tohw;
+
+	ret_fromhw = __pwm_round_waveform_fromhw(chip, pwm, wfhw, wf);
+	if (ret_fromhw < 0)
+		return ret_fromhw;
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) &&
+	    ret_tohw == 0 && pwm_check_rounding(&wf_req, wf))
+		dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
+			wf_req.duty_length, wf_req.period_length, wf_req.duty_offset,
+			wf->duty_length, wf->period_length, wf->duty_offset);
+
+	return ret_tohw;
+}
+
+int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	char wfhw[WFHWSIZE];
+	int err;
+
+	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
+
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	err = __pwm_read_waveform(chip, pwm, &wfhw);
+	if (err)
+		return err;
+
+	return __pwm_round_waveform_fromhw(chip, pwm, &wfhw, wf);
+}
+
+/* Called with the pwmchip lock held */
+static int __pwm_set_waveform(struct pwm_device *pwm,
+			      const struct pwm_waveform *wf,
+			      bool exact)
+{
+	struct pwm_chip *chip = pwm->chip;
+	const struct pwm_ops *ops = chip->ops;
+	char wfhw[WFHWSIZE];
+	struct pwm_waveform wf_rounded;
+	int err;
+
+	BUG_ON(WFHWSIZE < ops->sizeof_wfhw);
+
+	if (!pwm_wf_valid(wf))
+		return -EINVAL;
+
+	err = __pwm_round_waveform_tohw(chip, pwm, wf, &wfhw);
+	if (err)
+		return err;
+
+	if ((IS_ENABLED(CONFIG_PWM_DEBUG) || exact) && wf->period_length) {
+		err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_rounded);
+		if (err)
+			return err;
+
+		if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm_check_rounding(wf, &wf_rounded))
+			dev_err(&chip->dev, "Wrong rounding: requested %llu/%llu [+%llu], result %llu/%llu [+%llu]\n",
+				wf->duty_length, wf->period_length, wf->duty_offset,
+				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
+
+		if (exact && pwmwfcmp(wf, &wf_rounded)) {
+			dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n",
+				wf->duty_length, wf->period_length, wf->duty_offset,
+				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset);
+
+			return 1;
+		}
+	}
+
+	err = __pwm_write_waveform(chip, pwm, &wfhw);
+	if (err)
+		return err;
+
+	/* update .state */
+	pwm_wf2state(wf, &pwm->state);
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ops->read_waveform && wf->period_length) {
+		struct pwm_waveform wf_set;
+
+		err = __pwm_read_waveform(chip, pwm, &wfhw);
+		if (err)
+			/* maybe ignore? */
+			return err;
+
+		err = __pwm_round_waveform_fromhw(chip, pwm, &wfhw, &wf_set);
+		if (err)
+			/* maybe ignore? */
+			return err;
+
+		if (pwmwfcmp(&wf_set, &wf_rounded) != 0)
+			dev_err(&chip->dev,
+				"Unexpected setting: requested %llu/%llu [+%llu], expected %llu/%llu [+%llu], set %llu/%llu [+%llu]\n",
+				wf->duty_length, wf->period_length, wf->duty_offset,
+				wf_rounded.duty_length, wf_rounded.period_length, wf_rounded.duty_offset,
+				wf_set.duty_length, wf_set.period_length, wf_set.duty_offset);
+	}
+	return 0;
+}
+
+int pwm_set_waveform_might_sleep(struct pwm_device *pwm,
+				 const struct pwm_waveform *wf, bool exact)
+{
+	struct pwm_chip *chip = pwm->chip;
+	int err;
+
+	might_sleep();
+
+	guard(pwmchip)(chip);
+
+        if (!chip->operational)
+                return -ENODEV;
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
+		/*
+		 * Catch any drivers that have been marked as atomic but
+		 * that will sleep anyway.
+		 */
+		non_block_start();
+		err = __pwm_set_waveform(pwm, wf, exact);
+		non_block_end();
+	} else {
+		err = __pwm_set_waveform(pwm, wf, exact);
+	}
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_set_waveform_might_sleep);
+
 static void pwm_apply_debug(struct pwm_device *pwm,
 			    const struct pwm_state *state)
 {
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2a1f1f25a56c..2d1f36a84f1e 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -357,7 +357,11 @@ static inline void pwmchip_set_drvdata(struct pwm_chip *chip, void *data)
 }
 
 #if IS_ENABLED(CONFIG_PWM)
-/* PWM user APIs */
+
+/* PWM consumer APIs */
+int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
+int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
+int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
 int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
 int pwm_adjust_config(struct pwm_device *pwm);
-- 
2.43.0

Re: [PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[David Lechner]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> Provide API functions for consumers to work with waveforms.
> 
> Note that one relevant difference between pwm_get_state() and
> pwm_get_waveform*() is that the latter yields the actually configured
> hardware state, while the former yields the last state passed to
> pwm_apply*() and so doesn't account for hardware specific rounding.
> 

...

> +
> +/* PWM consumer APIs */
> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
>  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
>  int pwm_adjust_config(struct pwm_device *pwm);


It seems like there could be a potential race condition between rounding
and setting a PWM.

Consider two PWM devices that share the same clock and the driver can
set the clock rate to accommodate a wider range of periods or get a
more accurate duty length.

Thread 1				Thread 2
--------				--------
PWM consumer A calls round_waveform()
a few times, e.g. to round up or round
closest. Clock is not exclusive so
rounding assumes the rate can be
changed to get the best rate.
					PWM consumer B call set_waveform().
					clk_set_rate_exclusive() is called
					on the clock so the rate can no
					longer be changed and the rate is
					not the one PWM consumer A selected
					in the rounding operation.
PWM consumer A calls set_waveform().
This will either fail or will
not get the same results that
was returned by round_waveform().

---

If it wasn't for the userspace cdev interface, I would suggest
to drop pwm_round_waveform_might_sleep() and pass an optional
function pointer to pwm_set_waveform_might_sleep() instead of
the `bool exact` argument so that the full operation of rounding
and setting could be performed with the pwmchip lock held without
consumer drivers having to worry about getting it right. Consumer
drivers would then just have to implement functions to suit their
needs, or there could even be standard ones like round_closest().

But that still might not fix it in all cases e.g. if two pwmchips
share the same clock as opposed to one pwmchip with two outputs.
If this is possible, then then "pwm: Add more locking" patch
might already cause some problems with race conditions in the
existing PWM apply() ops. Although, I suppose this kind of
clock coordination between pwm chips (and potentially other
devices) should be handled outside of the PWM subsystem.

Re: [PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[Uwe Kleine-König]

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

Hello David,

On Mon, Jul 15, 2024 at 05:23:45PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> > Provide API functions for consumers to work with waveforms.
> > 
> > Note that one relevant difference between pwm_get_state() and
> > pwm_get_waveform*() is that the latter yields the actually configured
> > hardware state, while the former yields the last state passed to
> > pwm_apply*() and so doesn't account for hardware specific rounding.
> > 
> 
> ...
> 
> > +
> > +/* PWM consumer APIs */
> > +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> > +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> > +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
> >  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
> >  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
> >  int pwm_adjust_config(struct pwm_device *pwm);
> 
> 
> It seems like there could be a potential race condition between rounding
> and setting a PWM.
> 
> Consider two PWM devices that share the same clock and the driver can
> set the clock rate to accommodate a wider range of periods or get a
> more accurate duty length.
> 
> Thread 1				Thread 2
> --------				--------
> PWM consumer A calls round_waveform()
> a few times, e.g. to round up or round
> closest. Clock is not exclusive so
> rounding assumes the rate can be
> changed to get the best rate.
> 					PWM consumer B call set_waveform().
> 					clk_set_rate_exclusive() is called
> 					on the clock so the rate can no
> 					longer be changed and the rate is
> 					not the one PWM consumer A selected
> 					in the rounding operation.
> PWM consumer A calls set_waveform().
> This will either fail or will
> not get the same results that
> was returned by round_waveform().

The "exact" parameter has the purpose to make this fail. While
implementing the idea I wondered if I should drop the parameter and
make .set_waveform() imply exact=true.

Two more thoughts about this: First, I think the most usual use cases
are rounding up or maybe rounding closest (instead of rounding down as
done by default). It's easy to implement a helper function in the pwm
core that holds the chip lock and does the necessary function calls to
determine the rounded setting needed.

The second thought is: Even when holding the chip lock, another clk
consumer can theoretically change the flexibility of a participating clk
while the right settings are determined for a given pwm consumer. Also
if I use clk_round_rate() to determine the resulting rate of a parent
clock, it's not sure that I can set this rate because again the
situation might have changed since I called clk_round_rate() or because
another consumer refuses my request to change the rate.

So as soon as you consider changing an upstream clock to reach a certain
PWM setting, this all degrades to a racy best effort quest.

> If it wasn't for the userspace cdev interface, I would suggest
> to drop pwm_round_waveform_might_sleep() and pass an optional
> function pointer to pwm_set_waveform_might_sleep() instead of
> the `bool exact` argument so that the full operation of rounding
> and setting could be performed with the pwmchip lock held without
> consumer drivers having to worry about getting it right. Consumer
> drivers would then just have to implement functions to suit their
> needs, or there could even be standard ones like round_closest().
> 
> But that still might not fix it in all cases e.g. if two pwmchips
> share the same clock as opposed to one pwmchip with two outputs.
> If this is possible, then then "pwm: Add more locking" patch
> might already cause some problems with race conditions in the
> existing PWM apply() ops. Although, I suppose this kind of
> clock coordination between pwm chips (and potentially other
> devices) should be handled outside of the PWM subsystem.

I would expect that the "Add more locking" patch doesn't enable more
races than are possible without it? I don't understand the problems you
think of here.

Best regards
Uwe

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

Re: [PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[David Lechner]

On 7/16/24 2:06 AM, Uwe Kleine-König wrote:
> Hello David,
> 
> On Mon, Jul 15, 2024 at 05:23:45PM -0500, David Lechner wrote:
>> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
>>> Provide API functions for consumers to work with waveforms.
>>>
>>> Note that one relevant difference between pwm_get_state() and
>>> pwm_get_waveform*() is that the latter yields the actually configured
>>> hardware state, while the former yields the last state passed to
>>> pwm_apply*() and so doesn't account for hardware specific rounding.
>>>
>>
>> ...
>>
>>> +
>>> +/* PWM consumer APIs */
>>> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
>>> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
>>> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
>>>  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
>>>  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
>>>  int pwm_adjust_config(struct pwm_device *pwm);
>>
>>
>> It seems like there could be a potential race condition between rounding
>> and setting a PWM.
>>
>> Consider two PWM devices that share the same clock and the driver can
>> set the clock rate to accommodate a wider range of periods or get a
>> more accurate duty length.
>>
>> Thread 1				Thread 2
>> --------				--------
>> PWM consumer A calls round_waveform()
>> a few times, e.g. to round up or round
>> closest. Clock is not exclusive so
>> rounding assumes the rate can be
>> changed to get the best rate.
>> 					PWM consumer B call set_waveform().
>> 					clk_set_rate_exclusive() is called
>> 					on the clock so the rate can no
>> 					longer be changed and the rate is
>> 					not the one PWM consumer A selected
>> 					in the rounding operation.
>> PWM consumer A calls set_waveform().
>> This will either fail or will
>> not get the same results that
>> was returned by round_waveform().
> 
> The "exact" parameter has the purpose to make this fail. While
> implementing the idea I wondered if I should drop the parameter and
> make .set_waveform() imply exact=true.

Would consumers then be expected to implement a retry loop to
handle the error when an exact=true call failed because the same
rounding was no longer possible?

> 
> Two more thoughts about this: First, I think the most usual use cases
> are rounding up or maybe rounding closest (instead of rounding down as
> done by default). It's easy to implement a helper function in the pwm
> core that holds the chip lock and does the necessary function calls to
> determine the rounded setting needed.

Would these same functions also get ioctls for the cdev interface?

> 
> The second thought is: Even when holding the chip lock, another clk
> consumer can theoretically change the flexibility of a participating clk
> while the right settings are determined for a given pwm consumer. Also
> if I use clk_round_rate() to determine the resulting rate of a parent
> clock, it's not sure that I can set this rate because again the
> situation might have changed since I called clk_round_rate() or because
> another consumer refuses my request to change the rate.
> 
> So as soon as you consider changing an upstream clock to reach a certain
> PWM setting, this all degrades to a racy best effort quest.
> 
>> If it wasn't for the userspace cdev interface, I would suggest
>> to drop pwm_round_waveform_might_sleep() and pass an optional
>> function pointer to pwm_set_waveform_might_sleep() instead of
>> the `bool exact` argument so that the full operation of rounding
>> and setting could be performed with the pwmchip lock held without
>> consumer drivers having to worry about getting it right. Consumer
>> drivers would then just have to implement functions to suit their
>> needs, or there could even be standard ones like round_closest().
>>
>> But that still might not fix it in all cases e.g. if two pwmchips
>> share the same clock as opposed to one pwmchip with two outputs.
>> If this is possible, then then "pwm: Add more locking" patch
>> might already cause some problems with race conditions in the
>> existing PWM apply() ops. Although, I suppose this kind of
>> clock coordination between pwm chips (and potentially other
>> devices) should be handled outside of the PWM subsystem.
> 
> I would expect that the "Add more locking" patch doesn't enable more
> races than are possible without it? I don't understand the problems you
> think of here.
> 

When we consider other non-PWM clock consumers you are right, it
is not any different that before. I think you understood me fully.
I agree that there aren't any serious issues here any more than
the current state of things.

Re: [PATCH v2 4/8] pwm: Provide new consumer API functions for waveforms

[Uwe Kleine-König]

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

Hello David,

On Tue, Jul 16, 2024 at 09:28:32AM -0500, David Lechner wrote:
> On 7/16/24 2:06 AM, Uwe Kleine-König wrote:
> > On Mon, Jul 15, 2024 at 05:23:45PM -0500, David Lechner wrote:
> >> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> >>> Provide API functions for consumers to work with waveforms.
> >>>
> >>> Note that one relevant difference between pwm_get_state() and
> >>> pwm_get_waveform*() is that the latter yields the actually configured
> >>> hardware state, while the former yields the last state passed to
> >>> pwm_apply*() and so doesn't account for hardware specific rounding.
> >>>
> >>
> >> ...
> >>
> >>> +
> >>> +/* PWM consumer APIs */
> >>> +int pwm_round_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> >>> +int pwm_get_waveform_might_sleep(struct pwm_device *pwm, struct pwm_waveform *wf);
> >>> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm, const struct pwm_waveform *wf, bool exact);
> >>>  int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state);
> >>>  int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
> >>>  int pwm_adjust_config(struct pwm_device *pwm);
> >>
> >>
> >> It seems like there could be a potential race condition between rounding
> >> and setting a PWM.
> >>
> >> Consider two PWM devices that share the same clock and the driver can
> >> set the clock rate to accommodate a wider range of periods or get a
> >> more accurate duty length.
> >>
> >> Thread 1				Thread 2
> >> --------				--------
> >> PWM consumer A calls round_waveform()
> >> a few times, e.g. to round up or round
> >> closest. Clock is not exclusive so
> >> rounding assumes the rate can be
> >> changed to get the best rate.
> >> 					PWM consumer B call set_waveform().
> >> 					clk_set_rate_exclusive() is called
> >> 					on the clock so the rate can no
> >> 					longer be changed and the rate is
> >> 					not the one PWM consumer A selected
> >> 					in the rounding operation.
> >> PWM consumer A calls set_waveform().
> >> This will either fail or will
> >> not get the same results that
> >> was returned by round_waveform().
> > 
> > The "exact" parameter has the purpose to make this fail. While
> > implementing the idea I wondered if I should drop the parameter and
> > make .set_waveform() imply exact=true.
> 
> Would consumers then be expected to implement a retry loop to
> handle the error when an exact=true call failed because the same
> rounding was no longer possible?

That's the obvious consequence, yes.

> > Two more thoughts about this: First, I think the most usual use cases
> > are rounding up or maybe rounding closest (instead of rounding down as
> > done by default). It's easy to implement a helper function in the pwm
> > core that holds the chip lock and does the necessary function calls to
> > determine the rounded setting needed.
> 
> Would these same functions also get ioctls for the cdev interface?

Not sure. They should be available to libpwm users. But if libpwm
delegates to the kernel via an ioctl or does the necessary math itself
is a detail that I didn't think about yet.
 
> > The second thought is: Even when holding the chip lock, another clk
> > consumer can theoretically change the flexibility of a participating clk
> > while the right settings are determined for a given pwm consumer. Also
> > if I use clk_round_rate() to determine the resulting rate of a parent
> > clock, it's not sure that I can set this rate because again the
> > situation might have changed since I called clk_round_rate() or because
> > another consumer refuses my request to change the rate.
> > 
> > So as soon as you consider changing an upstream clock to reach a certain
> > PWM setting, this all degrades to a racy best effort quest.
> > 
> >> If it wasn't for the userspace cdev interface, I would suggest
> >> to drop pwm_round_waveform_might_sleep() and pass an optional
> >> function pointer to pwm_set_waveform_might_sleep() instead of
> >> the `bool exact` argument so that the full operation of rounding
> >> and setting could be performed with the pwmchip lock held without
> >> consumer drivers having to worry about getting it right. Consumer
> >> drivers would then just have to implement functions to suit their
> >> needs, or there could even be standard ones like round_closest().
> >>
> >> But that still might not fix it in all cases e.g. if two pwmchips
> >> share the same clock as opposed to one pwmchip with two outputs.
> >> If this is possible, then then "pwm: Add more locking" patch
> >> might already cause some problems with race conditions in the
> >> existing PWM apply() ops. Although, I suppose this kind of
> >> clock coordination between pwm chips (and potentially other
> >> devices) should be handled outside of the PWM subsystem.
> > 
> > I would expect that the "Add more locking" patch doesn't enable more
> > races than are possible without it? I don't understand the problems you
> > think of here.
> > 
> 
> When we consider other non-PWM clock consumers you are right, it
> is not any different that before. I think you understood me fully.
> I agree that there aren't any serious issues here any more than
> the current state of things.

Ack.

Best regards
Uwe

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

[PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

With this change each pwmchip defining the new-style waveform callbacks
can be accessed from userspace via a character device. Compared to the
sysfs-API this is faster (on a stm32mp157 applying a new configuration
takes approx 25% only) and allows to pass the whole configuration in a
single ioctl allowing atomic application.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c       | 270 ++++++++++++++++++++++++++++++++++++++-
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  25 ++++
 3 files changed, 296 insertions(+), 2 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 4dcb7ec4223f..918915563606 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -23,6 +23,8 @@
 
 #include <dt-bindings/pwm/pwm.h>
 
+#include <uapi/linux/pwm.h>
+
 #define CREATE_TRACE_POINTS
 #include <trace/events/pwm.h>
 
@@ -1525,6 +1527,257 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
 	return true;
 }
 
+struct pwm_cdev_data {
+	struct pwm_chip *chip;
+	struct pwm_device *pwm[];
+};
+
+static int pwm_cdev_open(struct inode *inode, struct file *file)
+{
+	struct pwm_chip *chip = container_of(inode->i_cdev, struct pwm_chip, cdev);
+	struct pwm_cdev_data *cdata;
+
+	guard(mutex)(&pwm_lock);
+
+	if (!chip->operational)
+		return -ENXIO;
+
+	cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
+	if (!cdata)
+		return -ENOMEM;
+
+	cdata->chip = chip;
+
+	file->private_data = cdata;
+
+	return nonseekable_open(inode, file);
+}
+
+static int pwm_cdev_release(struct inode *inode, struct file *file)
+{
+	struct pwm_cdev_data *cdata = file->private_data;
+	unsigned int i;
+
+	for (i = 0; i < cdata->chip->npwm; ++i) {
+		if (cdata->pwm[i])
+			pwm_put(cdata->pwm[i]);
+	}
+	kfree(cdata);
+
+	return 0;
+}
+
+static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (!cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = &chip->pwms[hwpwm];
+		int ret;
+
+		ret = pwm_device_request(pwm, "pwm-cdev");
+		if (ret < 0)
+			return ret;
+
+		cdata->pwm[hwpwm] = pwm;
+	}
+
+	return 0;
+}
+
+static int pwm_cdev_free(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return -EINVAL;
+
+	if (cdata->pwm[hwpwm]) {
+		struct pwm_device *pwm = cdata->pwm[hwpwm];
+
+		pwm_put(pwm);
+
+		cdata->pwm[hwpwm] = NULL;
+	}
+
+	return 0;
+}
+
+static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret = 0;
+	struct pwm_cdev_data *cdata = file->private_data;
+	struct pwm_chip *chip = cdata->chip;
+
+	guard(mutex)(&pwm_lock);
+
+	if (!chip->operational)
+		return -ENODEV;
+
+	switch (cmd) {
+	case PWM_IOCTL_GET_NUM_PWMS:
+		return chip->npwm;
+
+	case PWM_IOCTL_REQUEST:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				return ret;
+
+			return pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm;
+
+			ret = get_user(hwpwm, (unsigned int __user *)arg);
+			if (ret)
+				return ret;
+
+			return pwm_cdev_free(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_ROUNDWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+
+			if (cwf.__pad != 0)
+				return -EINVAL;
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			wf = (struct pwm_waveform) {
+				.period_length = cwf.period_length,
+				.duty_length = cwf.duty_length,
+				.duty_offset = cwf.duty_offset,
+			};
+
+			ret = pwm_round_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf = (struct pwmchip_waveform) {
+				.hwpwm = cwf.hwpwm,
+				.period_length = wf.period_length,
+				.duty_length = wf.duty_length,
+				.duty_offset = wf.duty_offset,
+			};
+
+			return copy_to_user((struct pwmchip_waveform __user *)arg,
+					    &cwf, sizeof(cwf));
+		}
+		break;
+
+	case PWM_IOCTL_GETWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+
+			if (cwf.__pad != 0) {
+				pr_warn("huh\n");
+				return -EINVAL;
+			}
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			ret = pwm_get_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf.period_length = wf.period_length;
+			cwf.duty_length = wf.duty_length;
+			cwf.duty_offset = wf.duty_offset;
+
+			return copy_to_user((struct pwmchip_waveform __user *)arg,
+					    &cwf, sizeof(cwf));
+		}
+		break;
+
+	case PWM_IOCTL_SETROUNDEDWF:
+	case PWM_IOCTL_SETEXACTWF:
+		{
+			struct pwmchip_waveform cwf;
+			struct pwm_waveform wf;
+			struct pwm_device *pwm;
+
+			ret = copy_from_user(&cwf,
+					     (struct pwmchip_waveform __user *)arg,
+					     sizeof(cwf));
+			if (ret)
+				return -EFAULT;
+			if (cwf.__pad != 0) {
+				pr_warn("huh\n");
+				return -EINVAL;
+			}
+
+			if (cwf.period_length > 0 &&
+			    (cwf.duty_length > cwf.period_length ||
+			     cwf.duty_offset >= cwf.period_length))
+				return -EINVAL;
+
+			ret = pwm_cdev_request(cdata, cwf.hwpwm);
+			if (ret)
+				return ret;
+
+			pwm = cdata->pwm[cwf.hwpwm];
+
+			wf = (struct pwm_waveform){
+				.period_length = cwf.period_length,
+				.duty_length = cwf.duty_length,
+				.duty_offset = cwf.duty_offset,
+			};
+
+			return pwm_set_waveform_might_sleep(pwm, &wf,
+							    cmd == PWM_IOCTL_SETEXACTWF);
+		}
+		break;
+
+	default:
+		return -ENOTTY;
+	}
+}
+
+static const struct file_operations pwm_cdev_fileops = {
+	.open = pwm_cdev_open,
+	.release = pwm_cdev_release,
+	.owner = THIS_MODULE,
+	.llseek = no_llseek,
+	.unlocked_ioctl = pwm_cdev_ioctl,
+};
+
+static dev_t pwm_devt;
+
 /**
  * __pwmchip_add() - register a new PWM chip
  * @chip: the PWM chip to add
@@ -1577,7 +1830,13 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
 	scoped_guard(pwmchip, chip)
 		chip->operational = true;
 
-	ret = device_add(&chip->dev);
+	if (chip->id < 256 && chip->ops->write_waveform)
+		chip->dev.devt = MKDEV(MAJOR(pwm_devt), chip->id);
+
+	cdev_init(&chip->cdev, &pwm_cdev_fileops);
+	chip->cdev.owner = owner;
+
+	ret = cdev_device_add(&chip->cdev, &chip->dev);
 	if (ret)
 		goto err_device_add;
 
@@ -1628,7 +1887,7 @@ void pwmchip_remove(struct pwm_chip *chip)
 		idr_remove(&pwm_chips, chip->id);
 	}
 
-	device_del(&chip->dev);
+	cdev_device_del(&chip->cdev, &chip->dev);
 }
 EXPORT_SYMBOL_GPL(pwmchip_remove);
 
@@ -2172,9 +2431,16 @@ static int __init pwm_init(void)
 {
 	int ret;
 
+	ret = alloc_chrdev_region(&pwm_devt, 0, 256, "pwm");
+	if (ret) {
+		pr_warn("Failed to initialize chrdev region for PWM usage\n");
+		return ret;
+	}
+
 	ret = class_register(&pwm_class);
 	if (ret) {
 		pr_err("Failed to initialize PWM class (%pe)\n", ERR_PTR(ret));
+		unregister_chrdev_region(pwm_devt, 256);
 		return ret;
 	}
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 2d1f36a84f1e..fd100c27f109 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -2,6 +2,7 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/cdev.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/module.h>
@@ -303,6 +304,7 @@ struct pwm_ops {
 /**
  * struct pwm_chip - abstract a PWM controller
  * @dev: device providing the PWMs
+ * @cdev: &struct cdev for this device
  * @ops: callbacks for this PWM controller
  * @owner: module providing this chip
  * @id: unique number of this PWM chip
@@ -317,6 +319,7 @@ struct pwm_ops {
  */
 struct pwm_chip {
 	struct device dev;
+	struct cdev cdev;
 	const struct pwm_ops *ops;
 	struct module *owner;
 	unsigned int id;
diff --git a/include/uapi/linux/pwm.h b/include/uapi/linux/pwm.h
new file mode 100644
index 000000000000..5038dca7c84c
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0-only WITH Linux-syscall-note */
+
+#ifndef _UAPI_PWM_H_
+#define _UAPI_PWM_H_
+
+#include <linux/ioctl.h>
+#include <linux/types.h>
+
+struct pwmchip_waveform {
+	unsigned int hwpwm;
+	unsigned int __pad; /* padding, must be zero */
+	__u64 period_length;
+	__u64 duty_length;
+	__u64 duty_offset;
+};
+
+#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
+#define PWM_IOCTL_REQUEST	_IOW(0x75, 1, unsigned int)
+#define PWM_IOCTL_FREE		_IOW(0x75, 2, unsigned int)
+#define PWM_IOCTL_ROUNDWF	_IOWR(0x75, 3, struct pwmchip_waveform)
+#define PWM_IOCTL_GETWF		_IOWR(0x75, 4, struct pwmchip_waveform)
+#define PWM_IOCTL_SETROUNDEDWF	_IOW(0x75, 5, struct pwmchip_waveform)
+#define PWM_IOCTL_SETEXACTWF	_IOW(0x75, 6, struct pwmchip_waveform)
+
+#endif /* _UAPI_PWM_H_ */
-- 
2.43.0

Re: [PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[David Lechner]

On 7/15/24 6:16 AM, Uwe Kleine-König wrote:

> +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)

What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
from sysfs, and it doesn't seem like there would be any performance consideration
for using an ioctl to get the same info.

Re: [PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

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

On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> 
> > +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
> 
> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
> from sysfs, and it doesn't seem like there would be any performance consideration
> for using an ioctl to get the same info.

This is provide all relevant information without looking at sysfs. Of
course "relevant" is somehow fuzzy. While it's nice not to have to look
at sysfs for basic operation---libpwm currently doesn't explore
there---you're right, the information duplication is somewhat ugly.

I'll think about it.

Thanks for your feedback,
Uwe

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

Re: [PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

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

On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
> 
> > +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
> 
> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
> from sysfs, and it doesn't seem like there would be any performance consideration
> for using an ioctl to get the same info.

Thinking about that (and sending out a v3 that didn't change anything
here), I think it would be sensible to drop this. Maybe in favour of an
ioctl that gets the chip id. This way a consumer could find the
respective device directory below /sys/class/pwm without parsing the
chip id from the filename (assuming a sane udev configuration).

Would that make sense to you, too?

Best regards
Uwe

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

Re: [PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[David Lechner]

On 7/30/24 5:26 AM, Uwe Kleine-König wrote:
> On Mon, Jul 15, 2024 at 02:37:57PM -0500, David Lechner wrote:
>> On 7/15/24 6:16 AM, Uwe Kleine-König wrote:
>>
>>> +#define PWM_IOCTL_GET_NUM_PWMS	_IO(0x75, 0)
>>
>> What is the use case for PWM_IOCTL_GET_NUM_PWMS? This info is already available
>> from sysfs, and it doesn't seem like there would be any performance consideration
>> for using an ioctl to get the same info.
> 
> Thinking about that (and sending out a v3 that didn't change anything
> here), I think it would be sensible to drop this. Maybe in favour of an
> ioctl that gets the chip id. This way a consumer could find the
> respective device directory below /sys/class/pwm without parsing the
> chip id from the filename (assuming a sane udev configuration).
> 
> Would that make sense to you, too?

How do we expect users to find the "right" PWM to use in the first
place? If libpwm is going to use libudev to enumerate all PWM devices
to find a match then we will already be able to get both the /sys/ and
/dev/ paths for the device from libudev.

And wouldn't the file name in both cases be "pwmchipX" (e.g.
/dev/pwm/pwmchip0 and /sys/class/pwm/pwmchip0) so we wouldn't need
to scrape the number out of the name if we wanted to do the matching
that way?

So I'm not convinced yet that having an IOCTL to get the device ID
it is especially useful.

Re: [PATCH v2 5/8] pwm: Add support for pwmchip devices for faster and easier userspace access

[Uwe Kleine-König]

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

Hello David,

On Tue, Jul 30, 2024 at 01:41:32PM -0500, David Lechner wrote:
> > This way a consumer could find the
> > respective device directory below /sys/class/pwm without parsing the
> > chip id from the filename (assuming a sane udev configuration).
> > 
> > Would that make sense to you, too?
> 
> How do we expect users to find the "right" PWM to use in the first
> place? If libpwm is going to use libudev to enumerate all PWM devices
> to find a match then we will already be able to get both the /sys/ and
> /dev/ paths for the device from libudev.

I guess I have to take a deeper look into libudev. Until then just
dropping the ioctl for getting the number of pwm lines seems sensible.
Adding something later should be easy, when and if we see that an ioctl
for getting the chip number would be helpful.
 
> And wouldn't the file name in both cases be "pwmchipX" (e.g.
> /dev/pwm/pwmchip0 and /sys/class/pwm/pwmchip0) so we wouldn't need
> to scrape the number out of the name if we wanted to do the matching
> that way?

My train of thought was independent to libpwm. It was just: What if I
have a file descriptor for a pwm device (provided by some HAL lib maybe)
and I want to find out the /sys dir for it? But maybe that's not
something that will occur in practise. And if there is no such ioctl,
I just need more info than a plain file descriptor.
 
> So I'm not convinced yet that having an IOCTL to get the device ID
> it is especially useful.

Thanks for your input.

Best regards
Uwe

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

[PATCH v2 6/8] pwm: Add tracing for waveform callbacks

[Uwe Kleine-König]

This adds trace events for the recently introduced waveform callbacks.
With the introduction of some helper macros consistency among the
different events is ensured.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/core.c         |  24 +++++--
 include/trace/events/pwm.h | 134 ++++++++++++++++++++++++++++++++++---
 2 files changed, 146 insertions(+), 12 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 918915563606..21fca27bb8a3 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -166,30 +166,46 @@ static int __pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *p
 				     const struct pwm_waveform *wf, void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+	ret = ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+	trace_pwm_round_waveform_tohw(pwm, wf, wfhw, ret);
+
+	return ret;
 }
 
 static int __pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
 				       const void *wfhw, struct pwm_waveform *wf)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+	ret = ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+	trace_pwm_round_waveform_fromhw(pwm, wfhw, wf, ret);
+
+	return ret;
 }
 
 static int __pwm_read_waveform(struct pwm_chip *chip, struct pwm_device *pwm, void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->read_waveform(chip, pwm, wfhw);
+	ret = ops->read_waveform(chip, pwm, wfhw);
+	trace_pwm_read_waveform(pwm, wfhw, ret);
+
+	return ret;
 }
 
 static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, const void *wfhw)
 {
 	const struct pwm_ops *ops = chip->ops;
+	int ret;
 
-	return ops->write_waveform(chip, pwm, wfhw);
+	ret = ops->write_waveform(chip, pwm, wfhw);
+	trace_pwm_write_waveform(pwm, wfhw, ret);
+
+	return ret;
 }
 
 #define WFHWSIZE 20
diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
index 8022701c446d..3b5b20d2aff0 100644
--- a/include/trace/events/pwm.h
+++ b/include/trace/events/pwm.h
@@ -8,15 +8,135 @@
 #include <linux/pwm.h>
 #include <linux/tracepoint.h>
 
+#define TP_PROTO_pwm(args...)							\
+	TP_PROTO(struct pwm_device *pwm, args)
+
+#define TP_ARGS_pwm(args...)							\
+	TP_ARGS(pwm, args)
+
+#define TP_STRUCT__entry_pwm(args...)						\
+	TP_STRUCT__entry(							\
+		__field(unsigned int, chipid)					\
+		__field(unsigned int, hwpwm)					\
+		args)
+
+#define TP_fast_assign_pwm(args...)						\
+	TP_fast_assign(								\
+		__entry->chipid = pwm->chip->id;				\
+		__entry->hwpwm = pwm->hwpwm;					\
+		args)
+
+#define TP_printk_pwm(fmt, args...)						\
+	TP_printk("pwmchip%u.%u: " fmt, __entry->chipid, __entry->hwpwm, args)
+
+#define __field_pwmwf(wf)							\
+	__field(u64, wf ## _period_length)					\
+	__field(u64, wf ## _duty_length)					\
+	__field(u64, wf ## _duty_offset)					\
+
+#define fast_assign_pwmwf(wf)							\
+	__entry->wf ## _period_length = wf->period_length;			\
+	__entry->wf ## _duty_length = wf->duty_length;				\
+	__entry->wf ## _duty_offset = wf->duty_offset
+
+#define printk_pwmwf_format(wf)							\
+	"%lld/%lld [+%lld]"
+
+#define printk_pwmwf_formatargs(wf)						\
+	__entry->wf ## _duty_length, __entry->wf ## _period_length, __entry->wf ## _duty_offset
+
+TRACE_EVENT(pwm_round_waveform_tohw,
+
+	TP_PROTO_pwm(const struct pwm_waveform *wf, void *wfhw, int err),
+
+	TP_ARGS_pwm(wf, wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field_pwmwf(wf)
+		__field(void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		fast_assign_pwmwf(wf);
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm(printk_pwmwf_format(wf) " > %p err=%d",
+		printk_pwmwf_formatargs(wf), __entry->wfhw, __entry->err)
+);
+
+TRACE_EVENT(pwm_round_waveform_fromhw,
+
+	TP_PROTO_pwm(const void *wfhw, struct pwm_waveform *wf, int err),
+
+	TP_ARGS_pwm(wfhw, wf, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(const void *, wfhw)
+		__field_pwmwf(wf)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		fast_assign_pwmwf(wf);
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p > " printk_pwmwf_format(wf) " err=%d",
+		__entry->wfhw, printk_pwmwf_formatargs(wf), __entry->err)
+);
+
+TRACE_EVENT(pwm_read_waveform,
+
+	TP_PROTO_pwm(void *wfhw, int err),
+
+	TP_ARGS_pwm(wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p err=%d",
+		__entry->wfhw, __entry->err)
+);
+
+TRACE_EVENT(pwm_write_waveform,
+
+	TP_PROTO_pwm(const void *wfhw, int err),
+
+	TP_ARGS_pwm(wfhw, err),
+
+	TP_STRUCT__entry_pwm(
+		__field(const void *, wfhw)
+		__field(int, err)
+	),
+
+	TP_fast_assign_pwm(
+		__entry->wfhw = wfhw;
+		__entry->err = err;
+	),
+
+	TP_printk_pwm("%p err=%d",
+		__entry->wfhw, __entry->err)
+);
+
+
 DECLARE_EVENT_CLASS(pwm,
 
 	TP_PROTO(struct pwm_device *pwm, const struct pwm_state *state, int err),
 
 	TP_ARGS(pwm, state, err),
 
-	TP_STRUCT__entry(
-		__field(unsigned int, chipid)
-		__field(unsigned int, hwpwm)
+	TP_STRUCT__entry_pwm(
 		__field(u64, period)
 		__field(u64, duty_cycle)
 		__field(enum pwm_polarity, polarity)
@@ -24,9 +144,7 @@ DECLARE_EVENT_CLASS(pwm,
 		__field(int, err)
 	),
 
-	TP_fast_assign(
-		__entry->chipid = pwm->chip->id;
-		__entry->hwpwm = pwm->hwpwm;
+	TP_fast_assign_pwm(
 		__entry->period = state->period;
 		__entry->duty_cycle = state->duty_cycle;
 		__entry->polarity = state->polarity;
@@ -34,8 +152,8 @@ DECLARE_EVENT_CLASS(pwm,
 		__entry->err = err;
 	),
 
-	TP_printk("pwmchip%u.%u: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d",
-		  __entry->chipid, __entry->hwpwm, __entry->period, __entry->duty_cycle,
+	TP_printk_pwm("period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d",
+		  __entry->period, __entry->duty_cycle,
 		  __entry->polarity, __entry->enabled, __entry->err)
 
 );
-- 
2.43.0

[PATCH v2 7/8] pwm: axi-pwmgen: Implementation of the waveform callbacks

[Uwe Kleine-König]

Convert the axi-pwmgen driver to use the new callbacks for hardware
programming.

Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-axi-pwmgen.c | 148 ++++++++++++++++++++++++-----------
 1 file changed, 102 insertions(+), 46 deletions(-)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index 3ad60edf20a5..5b76ba31b77f 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -23,6 +23,7 @@
 #include <linux/err.h>
 #include <linux/fpga/adi-axi-common.h>
 #include <linux/io.h>
+#include <linux/minmax.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
@@ -54,81 +55,136 @@ static const struct regmap_config axi_pwmgen_regmap_config = {
 	.max_register = 0xFC,
 };
 
-static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			    const struct pwm_state *state)
+/* This represents a hardware configuration for one channel */
+struct axi_pwmgen_waveform {
+	u32 period_cnt;
+	u32 duty_cycle_cnt;
+	u32 duty_offset_cnt;
+};
+
+static int axi_pwmgen_round_waveform_tohw(struct pwm_chip *chip,
+					  struct pwm_device *pwm,
+					  const struct pwm_waveform *wf,
+					  void *_wfhw)
 {
+	struct axi_pwmgen_waveform *wfhw = _wfhw;
+	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
+
+	if (wf->period_length == 0) {
+		*wfhw = (struct axi_pwmgen_waveform){
+			.period_cnt = 0,
+			.duty_cycle_cnt = 0,
+			.duty_offset_cnt = 0,
+		};
+	} else {
+		/* With ddata->clk_rate_hz < NSEC_PER_SEC this won't overflow. */
+		wfhw->period_cnt = min_t(u64, mul_u64_u32_div(wf->period_length, ddata->clk_rate_hz, NSEC_PER_SEC), U32_MAX);
+
+		if (wfhw->period_cnt == 0) {
+			/*
+			 * The specified period is too short for the hardware.
+			 * Let's round .duty_cycle down to 0 to get a (somewhat)
+			 * valid result.
+			 */
+			wfhw->period_cnt = 1;
+			wfhw->duty_cycle_cnt = 0;
+			wfhw->duty_offset_cnt = 0;
+		} else {
+			wfhw->duty_cycle_cnt = min_t(u64, mul_u64_u32_div(wf->duty_length, ddata->clk_rate_hz, NSEC_PER_SEC), U32_MAX);
+			wfhw->duty_offset_cnt = min_t(u64, mul_u64_u32_div(wf->duty_offset, ddata->clk_rate_hz, NSEC_PER_SEC), U32_MAX);
+		}
+	}
+
+	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> PERIOD: %08x, DUTY: %08x, OFFSET: %08x\n",
+		pwm->hwpwm, wf->duty_length, wf->period_length, wf->duty_offset,
+                ddata->clk_rate_hz, wfhw->period_cnt, wfhw->duty_cycle_cnt, wfhw->duty_offset_cnt);
+
+	return 0;
+}
+
+static int axi_pwmgen_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
+					     const void *_wfhw, struct pwm_waveform *wf)
+{
+	const struct axi_pwmgen_waveform *wfhw = _wfhw;
+	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
+
+	wf->period_length = DIV64_U64_ROUND_UP((u64)wfhw->period_cnt * NSEC_PER_SEC,
+					ddata->clk_rate_hz);
+
+	wf->duty_length = DIV64_U64_ROUND_UP((u64)wfhw->duty_cycle_cnt * NSEC_PER_SEC,
+					    ddata->clk_rate_hz);
+
+	wf->duty_offset = DIV64_U64_ROUND_UP((u64)wfhw->duty_offset_cnt * NSEC_PER_SEC,
+					     ddata->clk_rate_hz);
+
+	return 0;
+}
+
+static int axi_pwmgen_write_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     const void *_wfhw)
+{
+	const struct axi_pwmgen_waveform *wfhw = _wfhw;
 	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
-	unsigned int ch = pwm->hwpwm;
 	struct regmap *regmap = ddata->regmap;
-	u64 period_cnt, duty_cnt;
+	unsigned int ch = pwm->hwpwm;
 	int ret;
 
-	if (state->polarity != PWM_POLARITY_NORMAL)
-		return -EINVAL;
+	ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), wfhw->period_cnt);
+	if (ret)
+		return ret;
 
-	if (state->enabled) {
-		period_cnt = mul_u64_u64_div_u64(state->period, ddata->clk_rate_hz, NSEC_PER_SEC);
-		if (period_cnt > UINT_MAX)
-			period_cnt = UINT_MAX;
+	ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), wfhw->duty_cycle_cnt);
+	if (ret)
+		return ret;
 
-		if (period_cnt == 0)
-			return -EINVAL;
-
-		ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), period_cnt);
-		if (ret)
-			return ret;
-
-		duty_cnt = mul_u64_u64_div_u64(state->duty_cycle, ddata->clk_rate_hz, NSEC_PER_SEC);
-		if (duty_cnt > UINT_MAX)
-			duty_cnt = UINT_MAX;
-
-		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
-		if (ret)
-			return ret;
-	} else {
-		ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), 0);
-		if (ret)
-			return ret;
-
-		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), 0);
-		if (ret)
-			return ret;
-	}
+	ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(ch), wfhw->duty_offset_cnt);
+	if (ret)
+		return ret;
 
 	return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
 }
 
-static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
-				struct pwm_state *state)
+static int axi_pwmgen_read_waveform(struct pwm_chip *chip,
+				    struct pwm_device *pwm,
+				    void *_wfhw)
 {
+	struct axi_pwmgen_waveform *wfhw = _wfhw;
 	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
 	struct regmap *regmap = ddata->regmap;
 	unsigned int ch = pwm->hwpwm;
-	u32 cnt;
 	int ret;
 
-	ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &cnt);
+	ret = regmap_read(regmap, AXI_PWMGEN_CHX_PERIOD(ch), &wfhw->period_cnt);
 	if (ret)
 		return ret;
 
-	state->enabled = cnt != 0;
-
-	state->period = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, ddata->clk_rate_hz);
-
-	ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &cnt);
+	ret = regmap_read(regmap, AXI_PWMGEN_CHX_DUTY(ch), &wfhw->duty_cycle_cnt);
 	if (ret)
 		return ret;
 
-	state->duty_cycle = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, ddata->clk_rate_hz);
+	ret = regmap_read(regmap, AXI_PWMGEN_CHX_OFFSET(ch), &wfhw->duty_offset_cnt);
+	if (ret)
+		return ret;
 
-	state->polarity = PWM_POLARITY_NORMAL;
+	if (wfhw->duty_cycle_cnt > wfhw->period_cnt)
+		wfhw->duty_cycle_cnt = wfhw->period_cnt;
+
+	/* XXX: is this the actual behaviour of the hardware? */
+	if (wfhw->duty_offset_cnt >= wfhw->period_cnt) {
+		wfhw->duty_cycle_cnt = 0;
+		wfhw->duty_offset_cnt = 0;
+	}
 
 	return 0;
 }
 
 static const struct pwm_ops axi_pwmgen_pwm_ops = {
-	.apply = axi_pwmgen_apply,
-	.get_state = axi_pwmgen_get_state,
+	.sizeof_wfhw = sizeof(struct axi_pwmgen_waveform),
+	.round_waveform_tohw = axi_pwmgen_round_waveform_tohw,
+	.round_waveform_fromhw = axi_pwmgen_round_waveform_fromhw,
+	.read_waveform = axi_pwmgen_read_waveform,
+	.write_waveform = axi_pwmgen_write_waveform,
 };
 
 static int axi_pwmgen_setup(struct regmap *regmap, struct device *dev)
-- 
2.43.0

[PATCH v2 8/8] pwm: stm32: Implementation of the waveform callbacks

[Uwe Kleine-König]

Convert the stm32 pwm driver to use the new callbacks for hardware
programming.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
 drivers/pwm/pwm-stm32.c | 605 +++++++++++++++++++++++++---------------
 1 file changed, 384 insertions(+), 221 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index fd754a99cf2e..4dedfabce63b 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -51,6 +51,384 @@ static u32 active_channels(struct stm32_pwm *dev)
 	return ccer & TIM_CCER_CCXE;
 }
 
+struct stm32_pwm_waveform {
+	u32 ccer;
+	u32 psc;
+	u32 arr;
+	u32 ccr;
+};
+
+static int stm32_pwm_round_waveform_tohw(struct pwm_chip *chip,
+					 struct pwm_device *pwm,
+					 const struct pwm_waveform *wf,
+					 void *_wfhw)
+{
+	struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	unsigned long rate;
+	u64 ccr, duty;
+	int ret;
+
+	if (wf->period_length == 0) {
+		*wfhw = (struct stm32_pwm_waveform){
+			.ccer = 0,
+		};
+
+		return 0;
+	}
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	wfhw->ccer = TIM_CCER_CCxE(ch + 1);
+	if (priv->have_complementary_output)
+		wfhw->ccer = TIM_CCER_CCxNE(ch);
+
+	rate = clk_get_rate(priv->clk);
+
+	if (active_channels(priv) & ~(1 << ch * 4)) {
+		u64 arr;
+
+		/*
+		 * Other channels are already enabled, so the configured PSC and
+		 * ARR must be used for this channel, too.
+		 */
+		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
+		if (ret)
+			goto out;
+
+		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
+		if (ret)
+			goto out;
+
+		/*
+		 * calculate the best value for ARR for the given PSC, refuse if
+		 * the resulting period gets bigger than the requested one.
+		 */
+		arr = mul_u64_u64_div_u64(wf->period_length, rate,
+					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+		if (arr <= wfhw->arr) {
+			/*
+			 * requested period is small than the currently
+			 * configured and unchangable period, report back the smallest
+			 * possible period, i.e. the current state; Initialize
+			 * ccr to anything valid.
+			 */
+			wfhw->ccr = 0;
+			ret = 1;
+			goto out;
+		}
+
+	} else {
+		/*
+		 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
+		 * the calculations here won't overflow.
+		 * First we need to find the minimal value for prescaler such that
+		 *
+		 *        period_ns * clkrate
+		 *   ------------------------------ < max_arr + 1
+		 *   NSEC_PER_SEC * (prescaler + 1)
+		 *
+		 * This equation is equivalent to
+		 *
+		 *        period_ns * clkrate
+		 *   ---------------------------- < prescaler + 1
+		 *   NSEC_PER_SEC * (max_arr + 1)
+		 *
+		 * Using integer division and knowing that the right hand side is
+		 * integer, this is further equivalent to
+		 *
+		 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
+		 */
+		u64 psc = mul_u64_u64_div_u64(wf->period_length, rate,
+					      (u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
+		u64 arr;
+
+		wfhw->psc = min_t(u64, psc, MAX_TIM_PSC);
+
+		arr = mul_u64_u64_div_u64(wf->period_length, rate,
+					  (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+		if (!arr) {
+			/*
+			 * requested period is too small, report back the smallest
+			 * possible period, i.e. ARR = 0. The only valid CCR
+			 * value is then zero, too.
+			 */
+			wfhw->arr = 0;
+			wfhw->ccr = 0;
+			ret = 1;
+			goto out;
+		}
+
+		/*
+		 * ARR is limited intentionally to values less than
+		 * priv->max_arr to allow 100% duty cycle.
+		 */
+		wfhw->arr = min_t(u64, arr, priv->max_arr) - 1;
+	}
+
+	duty = mul_u64_u64_div_u64(wf->duty_length, rate,
+				   (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+	duty = min_t(u64, duty, wfhw->arr + 1);
+
+	if (wf->duty_length && wf->duty_offset &&
+	    wf->duty_length + wf->duty_offset >= wf->period_length) {
+		wfhw->ccer |= TIM_CCER_CCxP(ch + 1);
+		if (priv->have_complementary_output)
+			wfhw->ccer |= TIM_CCER_CCxNP(ch + 1);
+
+		ccr = wfhw->arr + 1 - duty;
+	} else {
+		ccr = duty;
+	}
+
+	wfhw->ccr = min_t(u64, ccr, wfhw->arr + 1);
+
+	dev_dbg(&chip->dev, "pwm#%u: %lld/%lld [+%lld] @%lu -> CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x\n",
+		pwm->hwpwm, wf->duty_length, wf->period_length, wf->duty_offset,
+		rate, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr);
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
+/*
+ * This should be moved to lib/math/div64.c. Currently there are some changes
+ * pending to mul_u64_u64_div_u64. Uwe will care for that when the dust settles.
+ */
+static u64 stm32_pwm_mul_u64_u64_div_u64_roundup(u64 a, u64 b, u64 c)
+{
+	u64 res = mul_u64_u64_div_u64(a, b, c);
+	/* Those multiplications might overflow but it doesn't matter */
+	u64 rem = a * b - c * res;
+
+	if (rem)
+		res += 1;
+
+	return res;
+}
+
+static int stm32_pwm_round_waveform_fromhw(struct pwm_chip *chip,
+					   struct pwm_device *pwm,
+					   const void *_wfhw,
+					   struct pwm_waveform *wf)
+{
+	const struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		unsigned long rate = clk_get_rate(priv->clk);
+		u64 ccr_ns;
+
+		/* The result doesn't overflow for rate >= 15259 */
+		wf->period_length = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1), NSEC_PER_SEC, rate);
+
+		ccr_ns = stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * wfhw->ccr, NSEC_PER_SEC, rate);
+
+		if (wfhw->ccer & TIM_CCER_CCxP(ch + 1)) {
+			wf->duty_length =
+				stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
+								      NSEC_PER_SEC, rate);
+
+			wf->duty_offset = ccr_ns;
+		} else {
+			wf->duty_length = ccr_ns;
+			wf->duty_offset = 0;
+		}
+	} else {
+		*wf = (struct pwm_waveform){
+			.period_length = 0,
+		};
+	}
+
+	return 0;
+}
+
+static int stm32_pwm_read_waveform(struct pwm_chip *chip,
+				     struct pwm_device *pwm,
+				     void *_wfhw)
+{
+	struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(priv->regmap, TIM_CCER, &wfhw->ccer);
+	if (ret)
+		goto out;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		ret = regmap_read(priv->regmap, TIM_PSC, &wfhw->psc);
+		if (ret)
+			goto out;
+
+		ret = regmap_read(priv->regmap, TIM_ARR, &wfhw->arr);
+		if (ret)
+			goto out;
+
+		if (wfhw->arr == U32_MAX)
+			wfhw->arr -= 1;
+
+		ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &wfhw->ccr);
+		if (ret)
+			goto out;
+
+		if (wfhw->ccr > wfhw->arr + 1)
+			wfhw->ccr = wfhw->arr + 1;
+	}
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
+static int stm32_pwm_write_waveform(struct pwm_chip *chip,
+				      struct pwm_device *pwm,
+				      const void *_wfhw)
+{
+	const struct stm32_pwm_waveform *wfhw = _wfhw;
+	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
+	unsigned int ch = pwm->hwpwm;
+	int ret;
+
+	ret = clk_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	if (wfhw->ccer & TIM_CCER_CCxE(ch + 1)) {
+		u32 ccer, mask;
+		unsigned shift;
+		u32 ccmr;
+
+		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+		if (ret)
+			goto out;
+
+		/* If there are other channels enabled, don't update PSC and ARR */
+		if (ccer & ~TIM_CCER_CCxE(ch + 1) & TIM_CCER_CCXE) {
+			u32 psc, arr;
+
+			ret = regmap_read(priv->regmap, TIM_PSC, &psc);
+			if (ret)
+				goto out;
+
+			if (psc != wfhw->psc) {
+				ret = -EBUSY;
+				goto out;
+			}
+
+			regmap_read(priv->regmap, TIM_ARR, &arr);
+			if (ret)
+				goto out;
+
+			if (arr != wfhw->arr) {
+				ret = -EBUSY;
+				goto out;
+			}
+		} else {
+			ret = regmap_write(priv->regmap, TIM_PSC, wfhw->psc);
+			if (ret)
+				goto out;
+
+			ret = regmap_write(priv->regmap, TIM_ARR, wfhw->arr);
+			if (ret)
+				goto out;
+
+			ret = regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
+			if (ret)
+				goto out;
+
+		}
+
+		/* set polarity */
+		mask = TIM_CCER_CCxP(ch + 1) | TIM_CCER_CCxNP(ch + 1);
+		ret = regmap_update_bits(priv->regmap, TIM_CCER, mask, wfhw->ccer);
+		if (ret)
+			goto out;
+
+		ret = regmap_write(priv->regmap, TIM_CCRx(ch + 1), wfhw->ccr);
+		if (ret)
+			goto out;
+
+		/* Configure output mode */
+		shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
+		ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
+		mask = CCMR_CHANNEL_MASK << shift;
+
+		if (ch < 2)
+			ret = regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
+		else
+			ret = regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
+		if (ret)
+			goto out;
+
+		ret = regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
+		if (ret)
+			goto out;
+
+		if (!(ccer & TIM_CCER_CCxE(ch + 1))) {
+			mask = TIM_CCER_CCxE(ch + 1) | TIM_CCER_CCxNE(ch + 1);
+
+			ret = clk_enable(priv->clk);
+			if (ret)
+				goto out;
+
+			ccer = (ccer & ~mask) | (wfhw->ccer & mask);
+			regmap_write(priv->regmap, TIM_CCER, ccer);
+
+			/* Make sure that registers are updated */
+			regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
+
+			/* Enable controller */
+			regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
+		}
+
+	} else {
+		/* disable channel */
+		u32 mask, ccer;
+
+		mask = TIM_CCER_CCxE(ch + 1);
+		if (priv->have_complementary_output)
+			mask |= TIM_CCER_CCxNE(ch + 1);
+
+		ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
+		if (ret)
+			goto out;
+
+		if (ccer & mask) {
+			ccer = ccer & ~mask;
+
+			ret = regmap_write(priv->regmap, TIM_CCER, ccer);
+			if (ret)
+				goto out;
+
+			if (!(ccer & TIM_CCER_CCXE)) {
+				/* When all channels are disabled, we can disable the controller */
+				ret = regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
+				if (ret)
+					goto out;
+			}
+
+			clk_disable(priv->clk);
+		}
+	}
+
+out:
+	clk_disable(priv->clk);
+
+	return ret;
+}
+
 #define TIM_CCER_CC12P (TIM_CCER_CC1P | TIM_CCER_CC2P)
 #define TIM_CCER_CC12E (TIM_CCER_CC1E | TIM_CCER_CC2E)
 #define TIM_CCER_CC34P (TIM_CCER_CC3P | TIM_CCER_CC4P)
@@ -308,228 +686,13 @@ static int stm32_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
 	return ret;
 }
 
-static int stm32_pwm_config(struct stm32_pwm *priv, unsigned int ch,
-			    u64 duty_ns, u64 period_ns)
-{
-	unsigned long long prd, dty;
-	unsigned long long prescaler;
-	u32 ccmr, mask, shift;
-
-	/*
-	 * .probe() asserted that clk_get_rate() is not bigger than 1 GHz, so
-	 * the calculations here won't overflow.
-	 * First we need to find the minimal value for prescaler such that
-	 *
-	 *        period_ns * clkrate
-	 *   ------------------------------ < max_arr + 1
-	 *   NSEC_PER_SEC * (prescaler + 1)
-	 *
-	 * This equation is equivalent to
-	 *
-	 *        period_ns * clkrate
-	 *   ---------------------------- < prescaler + 1
-	 *   NSEC_PER_SEC * (max_arr + 1)
-	 *
-	 * Using integer division and knowing that the right hand side is
-	 * integer, this is further equivalent to
-	 *
-	 *   (period_ns * clkrate) // (NSEC_PER_SEC * (max_arr + 1)) ≤ prescaler
-	 */
-
-	prescaler = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
-					(u64)NSEC_PER_SEC * ((u64)priv->max_arr + 1));
-	if (prescaler > MAX_TIM_PSC)
-		return -EINVAL;
-
-	prd = mul_u64_u64_div_u64(period_ns, clk_get_rate(priv->clk),
-				  (u64)NSEC_PER_SEC * (prescaler + 1));
-	if (!prd)
-		return -EINVAL;
-
-	/*
-	 * All channels share the same prescaler and counter so when two
-	 * channels are active at the same time we can't change them
-	 */
-	if (active_channels(priv) & ~(1 << ch * 4)) {
-		u32 psc, arr;
-
-		regmap_read(priv->regmap, TIM_PSC, &psc);
-		regmap_read(priv->regmap, TIM_ARR, &arr);
-
-		if ((psc != prescaler) || (arr != prd - 1))
-			return -EBUSY;
-	}
-
-	regmap_write(priv->regmap, TIM_PSC, prescaler);
-	regmap_write(priv->regmap, TIM_ARR, prd - 1);
-	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE);
-
-	/* Calculate the duty cycles */
-	dty = mul_u64_u64_div_u64(duty_ns, clk_get_rate(priv->clk),
-				  (u64)NSEC_PER_SEC * (prescaler + 1));
-
-	regmap_write(priv->regmap, TIM_CCRx(ch + 1), dty);
-
-	/* Configure output mode */
-	shift = (ch & 0x1) * CCMR_CHANNEL_SHIFT;
-	ccmr = (TIM_CCMR_PE | TIM_CCMR_M1) << shift;
-	mask = CCMR_CHANNEL_MASK << shift;
-
-	if (ch < 2)
-		regmap_update_bits(priv->regmap, TIM_CCMR1, mask, ccmr);
-	else
-		regmap_update_bits(priv->regmap, TIM_CCMR2, mask, ccmr);
-
-	regmap_set_bits(priv->regmap, TIM_BDTR, TIM_BDTR_MOE);
-
-	return 0;
-}
-
-static int stm32_pwm_set_polarity(struct stm32_pwm *priv, unsigned int ch,
-				  enum pwm_polarity polarity)
-{
-	u32 mask;
-
-	mask = TIM_CCER_CCxP(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNP(ch + 1);
-
-	regmap_update_bits(priv->regmap, TIM_CCER, mask,
-			   polarity == PWM_POLARITY_NORMAL ? 0 : mask);
-
-	return 0;
-}
-
-static int stm32_pwm_enable(struct stm32_pwm *priv, unsigned int ch)
-{
-	u32 mask;
-	int ret;
-
-	ret = clk_enable(priv->clk);
-	if (ret)
-		return ret;
-
-	/* Enable channel */
-	mask = TIM_CCER_CCxE(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNE(ch);
-
-	regmap_set_bits(priv->regmap, TIM_CCER, mask);
-
-	/* Make sure that registers are updated */
-	regmap_set_bits(priv->regmap, TIM_EGR, TIM_EGR_UG);
-
-	/* Enable controller */
-	regmap_set_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
-
-	return 0;
-}
-
-static void stm32_pwm_disable(struct stm32_pwm *priv, unsigned int ch)
-{
-	u32 mask;
-
-	/* Disable channel */
-	mask = TIM_CCER_CCxE(ch + 1);
-	if (priv->have_complementary_output)
-		mask |= TIM_CCER_CCxNE(ch + 1);
-
-	regmap_clear_bits(priv->regmap, TIM_CCER, mask);
-
-	/* When all channels are disabled, we can disable the controller */
-	if (!active_channels(priv))
-		regmap_clear_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN);
-
-	clk_disable(priv->clk);
-}
-
-static int stm32_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
-			   const struct pwm_state *state)
-{
-	bool enabled;
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ret;
-
-	enabled = pwm->state.enabled;
-
-	if (!state->enabled) {
-		if (enabled)
-			stm32_pwm_disable(priv, pwm->hwpwm);
-		return 0;
-	}
-
-	if (state->polarity != pwm->state.polarity)
-		stm32_pwm_set_polarity(priv, pwm->hwpwm, state->polarity);
-
-	ret = stm32_pwm_config(priv, pwm->hwpwm,
-			       state->duty_cycle, state->period);
-	if (ret)
-		return ret;
-
-	if (!enabled && state->enabled)
-		ret = stm32_pwm_enable(priv, pwm->hwpwm);
-
-	return ret;
-}
-
-static int stm32_pwm_apply_locked(struct pwm_chip *chip, struct pwm_device *pwm,
-				  const struct pwm_state *state)
-{
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ret;
-
-	/* protect common prescaler for all active channels */
-	mutex_lock(&priv->lock);
-	ret = stm32_pwm_apply(chip, pwm, state);
-	mutex_unlock(&priv->lock);
-
-	return ret;
-}
-
-static int stm32_pwm_get_state(struct pwm_chip *chip,
-			       struct pwm_device *pwm, struct pwm_state *state)
-{
-	struct stm32_pwm *priv = to_stm32_pwm_dev(chip);
-	int ch = pwm->hwpwm;
-	unsigned long rate;
-	u32 ccer, psc, arr, ccr;
-	u64 dty, prd;
-	int ret;
-
-	mutex_lock(&priv->lock);
-
-	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
-	if (ret)
-		goto out;
-
-	state->enabled = ccer & TIM_CCER_CCxE(ch + 1);
-	state->polarity = (ccer & TIM_CCER_CCxP(ch + 1)) ?
-			  PWM_POLARITY_INVERSED : PWM_POLARITY_NORMAL;
-	ret = regmap_read(priv->regmap, TIM_PSC, &psc);
-	if (ret)
-		goto out;
-	ret = regmap_read(priv->regmap, TIM_ARR, &arr);
-	if (ret)
-		goto out;
-	ret = regmap_read(priv->regmap, TIM_CCRx(ch + 1), &ccr);
-	if (ret)
-		goto out;
-
-	rate = clk_get_rate(priv->clk);
-
-	prd = (u64)NSEC_PER_SEC * (psc + 1) * (arr + 1);
-	state->period = DIV_ROUND_UP_ULL(prd, rate);
-	dty = (u64)NSEC_PER_SEC * (psc + 1) * ccr;
-	state->duty_cycle = DIV_ROUND_UP_ULL(dty, rate);
-
-out:
-	mutex_unlock(&priv->lock);
-	return ret;
-}
-
 static const struct pwm_ops stm32pwm_ops = {
-	.apply = stm32_pwm_apply_locked,
-	.get_state = stm32_pwm_get_state,
+	.sizeof_wfhw = sizeof(struct stm32_pwm_waveform),
+	.round_waveform_tohw = stm32_pwm_round_waveform_tohw,
+	.round_waveform_fromhw = stm32_pwm_round_waveform_fromhw,
+	.read_waveform = stm32_pwm_read_waveform,
+	.write_waveform = stm32_pwm_write_waveform,
+
 	.capture = IS_ENABLED(CONFIG_DMA_ENGINE) ? stm32_pwm_capture : NULL,
 };
 
-- 
2.43.0