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

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

[Uwe Kleine-König]

Hello,

here comes v5 of the series to add support for duty offset in PWM
waveforms. With that (and using two PWMs of a single chip) the following 
waveform pair can be configured:

               ______         ______         ______         ______
   PWM #0  ___/      \_______/      \_______/      \_______/      \_______
                 __             __             __             __
   PWM #1  _____/  \___________/  \___________/  \___________/  \_________
              ^              ^              ^              ^

This is required for an adc driver by Trevor Gamblin[1]. The last patch
also adds a new userspace API using a character device per pwm_chip (if
the underlaying lowlevel driver support the new waveform callbacks).
Compared to the earlier revisions of this series it was moved to the
last patch because I don't intend to apply it during the next
development cycle. The reason that makes me hesitate is that the return
value convention by the .round_waveform_tohw() callback is unusual: It
returns either 0 or 1 or a negative error value. These return values are
passed to userspace as the return value of the added ioctl() calls and
so are not changable any more once they are considered part of the
userspace API. So for now the pwm internal convention stays unusual as
it was before, but can still be easily adapted if practise showed the
convention to be too bad to keep.

If you want to test this series, the current state is available at
https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/chardev
.

Changes since v4 which is available at
https://lore.kernel.org/linux-pwm/cover.1725635013.git.u.kleine-koenig@baylibre.com
:

 - As described above: New patch to reorder symbols in core.c and the
   character device patch is moved to the end.

 - PWM_IOCTL_REQUEST is now mandatory before using a pwm device via the
   pwmchip character device. Thanks to David for input here.
   The libpwm repo[2] is updated accordingly.

 - PWM_IOCTL_REQUEST and PWM_IOCTL_FREE calling convention changed.
   Before you had to do:

	someuint = 3;
	ioctl(pwmchipfd, PWM_IOCTL_REQUEST, &someuint);

   Now it's just:

	ioctl(pwmchipfd, PWM_IOCTL_REQUEST, 3);

 - There is a new patch that reorders functions in drivers/pwm/core.c.
   The motivation for that was a locking issue in the ioctl code where
   pwm_lock was taken twice on PWM_IOCTL_FREE. So a new variant of
   pwm_put() was introduced that relies on the caller to have grabbed
   the lock already. To not have to declare this new function, it had to
   be moved further up in core.c

 - Some debugging code removed. (huh, thanks to David for noticing.)

 - Additions to comments (also kernel doc) and commit logs for several
   patches to (hopefully) make things clearer.

 - Refactored how the input is validated for the PWM_IOCTL_SET*WF
   ioctls to remove code duplication. (IIRC this was feedback on an
   earlier revision. But I only remembered it and couldn't find it in my
   mailbox. I think it was Fabrice who wrote that, but I'm not entirely
   sure. Thanks to whoever it was.)

Unless something grave pops up, I intend to add this series (without the
last patch) to next after the merge window closes to give it some more
exposure and testing. I'm pretty sure the code still has to be fixed and
improved here and there, but I will do that in-tree then. Once I'm sure
it will go in, I'll create a tag for Jonathan to merge into his iio tree
to allow him to apply Trevor's adc driver.
@Jonathan: What's your desired timing? I'd target for around -rc3 time
to create that tag for you. Is that early enough for you?

Best regards
Uwe

[1] https://lore.kernel.org/linux-iio/20240909-ad7625_r1-v5-0-60a397768b25@baylibre.com
[2] https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git

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

 drivers/pwm/core.c           | 1144 +++++++++++++++++++++++++++++-----
 drivers/pwm/pwm-axi-pwmgen.c |  154 +++--
 drivers/pwm/pwm-stm32.c      |  612 +++++++++++-------
 include/linux/pwm.h          |   58 +-
 include/trace/events/pwm.h   |  134 +++-
 include/uapi/linux/pwm.h     |   32 +
 6 files changed, 1693 insertions(+), 441 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

base-commit: d242feaf81d63b25d8c1fb1a68738dc33966a376
-- 
2.45.2

[PATCH v5 1/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.

An additional change implemented here is that on driver remove the
.free() callback is called for each requested pwm_device. This is the
right time because later (e.g. when the consumer calls pwm_put()) the
free function is (maybe) not available any more.

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

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6e752e148b98..5a095eb46b54 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);
@@ -334,8 +365,18 @@ static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
 	if (!ops->capture)
 		return -ENOSYS;
 
+	/*
+	 * Holding the pwm_lock is probably not needed. If you use pwm_capture()
+	 * and you're interested to speed it up, please convince yourself it's
+	 * really not needed, test and then suggest a patch on the mailing list.
+	 */
 	guard(mutex)(&pwm_lock);
 
+	guard(pwmchip)(chip);
+
+	if (!chip->operational)
+		return -ENODEV;
+
 	return ops->capture(chip, pwm, result, timeout);
 }
 
@@ -368,6 +409,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 +445,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 +1071,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 +1177,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 +1195,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 +1205,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 +1227,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_warn(&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 +1617,17 @@ void pwm_put(struct pwm_device *pwm)
 
 	guard(mutex)(&pwm_lock);
 
-	if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+	/*
+	 * Trigger a warning if a consumer called pwm_put() twice.
+	 * If the chip isn't operational, PWMF_REQUESTED was already cleared in
+	 * pwmchip_remove(). So don't warn in this case.
+	 */
+	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..3ea73e075abe 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 the callbacks in .ops
+		 */
+		struct mutex nonatomic_lock;
+		spinlock_t atomic_lock;
+	};
 	struct pwm_device pwms[] __counted_by(npwm);
 };
 
-- 
2.45.2

[PATCH v5 2/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_ns
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_ns == 0 or duty_offset_ns == period_length_ns -
duty_length_ns).

A disabled PWM is modeled by .period_length_ns = 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_ns and
duty_length_ns. 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_ns" as suffix the semantic
should be more obvious to people unfamiliar with the pwm subsystem.
period is renamed to period_length_ns 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_ns. For that period pick the biggest
possible duty setting not bigger than wf->duty_length_ns. Third pick the
biggest possible offset not bigger than wf->duty_offset_ns. If the
requested period is too small for the hardware, it's expected that a
setting with the minimal period and duty_length_ns = duty_offset_ns = 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  | 234 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/pwm.h |  36 +++++++
 2 files changed, 249 insertions(+), 21 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 5a095eb46b54..e3e26aafa461 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_ns) {
+		if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns)
+			*state = (struct pwm_state){
+				.enabled = true,
+				.polarity = PWM_POLARITY_NORMAL,
+				.period = wf->period_length_ns,
+				.duty_cycle = wf->duty_length_ns,
+			};
+		else
+			*state = (struct pwm_state){
+				.enabled = true,
+				.polarity = PWM_POLARITY_INVERSED,
+				.period = wf->period_length_ns,
+				.duty_cycle = wf->period_length_ns - wf->duty_length_ns,
+			};
+	} 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_ns = state->period,
+				.duty_length_ns = state->duty_cycle,
+				.duty_offset_ns = 0,
+			};
+		else
+			*wf = (struct pwm_waveform){
+				.period_length_ns = state->period,
+				.duty_length_ns = state->period - state->duty_cycle,
+				.duty_offset_ns = state->duty_cycle,
+			};
+	} else {
+		*wf = (struct pwm_waveform){
+			.period_length_ns = 0,
+		};
+	}
+}
+
+static int pwm_check_rounding(const struct pwm_waveform *wf,
+			      const struct pwm_waveform *wf_rounded)
+{
+	if (!wf->period_length_ns)
+		return 0;
+
+	if (wf->period_length_ns < wf_rounded->period_length_ns)
+		return 1;
+
+	if (wf->duty_length_ns < wf_rounded->duty_length_ns)
+		return 1;
+
+	if (wf->duty_offset_ns < wf_rounded->duty_offset_ns)
+		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,69 @@ 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 need arises, until reported otherwise
+		 * let's assume that consumers don't care.
+		 */
+
+		err = __pwm_round_waveform_tohw(chip, pwm, &wf, &wfhw);
+		if (err) {
+			if (err > 0)
+				/*
+				 * This signals an invalid request, typically
+				 * the requested period (or duty_offset) is
+				 * smaller than possible with the hardware.
+				 */
+				return -EINVAL;
+
+			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_ns, wf.period_length_ns, wf.duty_offset_ns,
+					wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
+		}
+
+		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 +441,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
@@ -435,7 +619,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
@@ -445,11 +629,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;
 
@@ -1136,12 +1316,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 3ea73e075abe..d8cfe1c9b19d 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -49,6 +49,31 @@ enum {
 	PWMF_EXPORTED = 1,
 };
 
+/**
+ * struct pwm_waveform - description of a PWM waveform
+ * @period_length_ns: PWM period
+ * @duty_length_ns: PWM duty cycle
+ * @duty_offset_ns: 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_ns and so can represent offsets other than zero (with .polarity =
+ * PWM_POLARITY_NORMAL) and period - duty_cycle (.polarity =
+ * PWM_POLARITY_INVERSED).
+ *
+ * Note there is no explicit bool for enabled. A "disabled" PWM is represented
+ * by .period_length_ns = 0. Note further 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_ns;
+	u64 duty_length_ns;
+	u64 duty_offset_ns;
+};
+
 /*
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
@@ -259,6 +284,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.45.2

[PATCH v5 3/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  | 261 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/pwm.h |   6 +-
 2 files changed, 266 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index e3e26aafa461..1cefc6e9c10d 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_ns <= 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_ns > S64_MAX)
+		return false;
+
+	if (wf->duty_length_ns > wf->period_length_ns)
+		return false;
+
+	/*
+	 * .duty_offset_ns is supposed to be smaller than .period_length_ns, apart
+	 * from the corner case .duty_offset_ns == 0 && .period_length_ns == 0.
+	 */
+	if (wf->duty_offset_ns && wf->duty_offset_ns >= wf->period_length_ns)
+		return false;
+
+	return true;
+}
+
 static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
 {
 	if (wf->period_length_ns) {
@@ -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_ns > b->period_length_ns)
+		return 1;
+
+	if (a->period_length_ns < b->period_length_ns)
+		return -1;
+
+	if (a->duty_length_ns > b->duty_length_ns)
+		return 1;
+
+	if (a->duty_length_ns < b->duty_length_ns)
+		return -1;
+
+	if (a->duty_offset_ns > b->duty_offset_ns)
+		return 1;
+
+	if (a->duty_offset_ns < b->duty_offset_ns)
+		return -1;
+
+	return 0;
+}
+
 static int pwm_check_rounding(const struct pwm_waveform *wf,
 			      const struct pwm_waveform *wf_rounded)
 {
@@ -145,6 +192,220 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c
 
 #define WFHWSIZE 20
 
+/**
+ * pwm_round_waveform_might_sleep - Query hardware capabilities
+ * Cannot be used in atomic context.
+ * @pwm: PWM device
+ * @wf: waveform to round and output parameter
+ *
+ * Typically a given waveform cannot be implemented exactly by hardware, e.g.
+ * because hardware only supports coarse period resolution or no duty_offset.
+ * This function returns the actually implemented waveform if you pass wf to
+ * pwm_set_waveform_might_sleep now.
+ *
+ * Note however that the world doesn't stop turning when you call it, so when
+ * doing
+ *
+ * 	pwm_round_waveform_might_sleep(mypwm, &wf);
+ * 	pwm_set_waveform_might_sleep(mypwm, &wf, true);
+ *
+ * the latter might fail, e.g. because an input clock changed its rate between
+ * these two calls and the waveform determined by
+ * pwm_round_waveform_might_sleep() cannot be implemented any more.
+ *
+ * Returns 0 on success, 1 if there is no valid hardware configuration matching
+ * the input waveform under the PWM rounding rules or a negative errno.
+ */
+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;
+
+	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ret_tohw > 1)
+		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_tohw: requested %llu/%llu [+%llu], return value %d\n",
+			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, 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_fromhw > 0)
+		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_fromhw: requested %llu/%llu [+%llu], return value %d\n",
+			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, ret_tohw);
+
+	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_ns, wf_req.period_length_ns, wf_req.duty_offset_ns,
+			wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
+
+	return ret_tohw;
+}
+EXPORT_SYMBOL_GPL(pwm_round_waveform_might_sleep);
+
+/**
+ * pwm_get_waveform_might_sleep - Query hardware about current configuration
+ * Cannot be used in atomic context.
+ * @pwm: PWM device
+ * @wf: output parameter
+ *
+ * Stores the current configuration of the PWM in @wf. Note this is the
+ * equivalent of pwm_get_state_hw() (and not pwm_get_state()) for pwm_waveform.
+ */
+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);
+}
+EXPORT_SYMBOL_GPL(pwm_get_waveform_might_sleep);
+
+/* 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_ns) {
+		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_ns, wf->period_length_ns, wf->duty_offset_ns,
+				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
+
+		if (exact && pwmwfcmp(wf, &wf_rounded)) {
+			dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n",
+				wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
+				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
+
+			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_ns) {
+		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_ns, wf->period_length_ns, wf->duty_offset_ns,
+				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns,
+				wf_set.duty_length_ns, wf_set.period_length_ns, wf_set.duty_offset_ns);
+	}
+	return 0;
+}
+
+/**
+ * pwm_set_waveform_might_sleep - Apply a new waveform
+ * Cannot be used in atomic context.
+ * @pwm: PWM device
+ * @wf: The waveform to apply
+ * @exact: If true no rounding is allowed
+ *
+ * Typically a requested waveform cannot be implemented exactly, e.g. because
+ * you requested .period_length_ns = 100 ns, but the hardware can only set
+ * periods that are a multiple of 8.5 ns. With that hardware passing exact =
+ * true results in pwm_set_waveform_might_sleep() failing and returning 1. If
+ * exact = false you get a period of 93.5 ns (i.e. the biggest period not bigger
+ * than the requested value).
+ * Note that even with exact = true, some rounding by less than 1 is
+ * possible/needed. In the above example requesting .period_length_ns = 94 and
+ * exact = true, you get the hardware configured with period = 93.5 ns.
+ */
+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 d8cfe1c9b19d..c3d9ddeafa65 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -358,7 +358,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.45.2

[PATCH v5 4/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 1cefc6e9c10d..b688c72aba1f 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -164,30 +164,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..8ba898fd335c 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_ns)					\
+	__field(u64, wf ## _duty_length_ns)					\
+	__field(u64, wf ## _duty_offset_ns)					\
+
+#define fast_assign_pwmwf(wf)							\
+	__entry->wf ## _period_length_ns = wf->period_length_ns;		\
+	__entry->wf ## _duty_length_ns = wf->duty_length_ns;			\
+	__entry->wf ## _duty_offset_ns = wf->duty_offset_ns
+
+#define printk_pwmwf_format(wf)							\
+	"%lld/%lld [+%lld]"
+
+#define printk_pwmwf_formatargs(wf)						\
+	__entry->wf ## _duty_length_ns, __entry->wf ## _period_length_ns, __entry->wf ## _duty_offset_ns
+
+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.45.2

[PATCH v5 5/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.

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

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index b5477659ba18..39d184417c7c 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>
@@ -53,81 +54,142 @@ 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_ns == 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_ns, 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_ns, ddata->clk_rate_hz, NSEC_PER_SEC),
+						     U32_MAX);
+			wfhw->duty_offset_cnt = min_t(u64,
+						      mul_u64_u32_div(wf->duty_offset_ns, 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_ns, wf->period_length_ns, wf->duty_offset_ns,
+		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_ns = DIV64_U64_ROUND_UP((u64)wfhw->period_cnt * NSEC_PER_SEC,
+					ddata->clk_rate_hz);
+
+	wf->duty_length_ns = DIV64_U64_ROUND_UP((u64)wfhw->duty_cycle_cnt * NSEC_PER_SEC,
+					    ddata->clk_rate_hz);
+
+	wf->duty_offset_ns = 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.45.2

[PATCH v5 6/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 | 612 +++++++++++++++++++++++++---------------
 1 file changed, 391 insertions(+), 221 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index eb24054f9729..d2c1085aee74 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -51,6 +51,391 @@ 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_ns == 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 + 1);
+
+	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_ns, 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_ns, 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_ns, 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_ns, rate,
+				   (u64)NSEC_PER_SEC * (wfhw->psc + 1));
+	duty = min_t(u64, duty, wfhw->arr + 1);
+
+	if (wf->duty_length_ns && wf->duty_offset_ns &&
+	    wf->duty_length_ns + wf->duty_offset_ns >= wf->period_length_ns) {
+		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_ns, wf->period_length_ns, wf->duty_offset_ns,
+		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_ns = 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_ns =
+				stm32_pwm_mul_u64_u64_div_u64_roundup(((u64)wfhw->psc + 1) * (wfhw->arr + 1 - wfhw->ccr),
+								      NSEC_PER_SEC, rate);
+
+			wf->duty_offset_ns = ccr_ns;
+		} else {
+			wf->duty_length_ns = ccr_ns;
+			wf->duty_offset_ns = 0;
+		}
+
+		dev_dbg(&chip->dev, "pwm#%u: CCER: %08x, PSC: %08x, ARR: %08x, CCR: %08x @%lu -> %lld/%lld [+%lld]\n",
+			pwm->hwpwm, wfhw->ccer, wfhw->psc, wfhw->arr, wfhw->ccr, rate,
+			wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
+
+	} else {
+		*wf = (struct pwm_waveform){
+			.period_length_ns = 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 int 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 +693,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 + 1);
-
-	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.45.2

[PATCH v5 7/8] pwm: Reorder symbols in core.c

[Uwe Kleine-König]

This moves pwm_get() and friends above the functions handling
registration of pwmchips. The motivation is that character device
support needs pwm_get() and pwm_put() and so ideally is defined below
these and when a pwmchip is registered this registers the character
device. So the natural order is

	pwm_get() and friend
	pwm character device symbols
	pwm_chip functions

. The advantage of having these in their natural order is that static
functions don't need to be forward declared.

Note that the diff that git produces for this change some functions are
moved down instead. This is technically equivalent, but not how this
change was created.

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

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index b688c72aba1f..ed620e35db61 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -1615,132 +1615,6 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
 	return true;
 }
 
-/**
- * __pwmchip_add() - register a new PWM chip
- * @chip: the PWM chip to add
- * @owner: reference to the module providing the chip.
- *
- * Register a new PWM chip. @owner is supposed to be THIS_MODULE, use the
- * pwmchip_add wrapper to do this right.
- *
- * Returns: 0 on success or a negative error code on failure.
- */
-int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
-{
-	int ret;
-
-	if (!chip || !pwmchip_parent(chip) || !chip->ops || !chip->npwm)
-		return -EINVAL;
-
-	/*
-	 * a struct pwm_chip must be allocated using (devm_)pwmchip_alloc,
-	 * otherwise the embedded struct device might disappear too early
-	 * resulting in memory corruption.
-	 * Catch drivers that were not converted appropriately.
-	 */
-	if (!chip->uses_pwmchip_alloc)
-		return -EINVAL;
-
-	if (!pwm_ops_check(chip))
-		return -EINVAL;
-
-	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);
-	if (ret < 0)
-		return ret;
-
-	chip->id = ret;
-
-	dev_set_name(&chip->dev, "pwmchip%u", chip->id);
-
-	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;
-
-	return 0;
-
-err_device_add:
-	scoped_guard(pwmchip, chip)
-		chip->operational = false;
-
-	if (IS_ENABLED(CONFIG_OF))
-		of_pwmchip_remove(chip);
-
-	idr_remove(&pwm_chips, chip->id);
-
-	return ret;
-}
-EXPORT_SYMBOL_GPL(__pwmchip_add);
-
-/**
- * pwmchip_remove() - remove a PWM chip
- * @chip: the PWM chip to remove
- *
- * Removes a PWM chip.
- */
-void pwmchip_remove(struct pwm_chip *chip)
-{
-	pwmchip_sysfs_unexport(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_warn(&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);
-
-		idr_remove(&pwm_chips, chip->id);
-	}
-
-	device_del(&chip->dev);
-}
-EXPORT_SYMBOL_GPL(pwmchip_remove);
-
-static void devm_pwmchip_remove(void *data)
-{
-	struct pwm_chip *chip = data;
-
-	pwmchip_remove(chip);
-}
-
-int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner)
-{
-	int ret;
-
-	ret = __pwmchip_add(chip, owner);
-	if (ret)
-		return ret;
-
-	return devm_add_action_or_reset(dev, devm_pwmchip_remove, chip);
-}
-EXPORT_SYMBOL_GPL(__devm_pwmchip_add);
-
 static struct device_link *pwm_device_link_add(struct device *dev,
 					       struct pwm_device *pwm)
 {
@@ -1918,36 +1792,6 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode)
 static DEFINE_MUTEX(pwm_lookup_lock);
 static LIST_HEAD(pwm_lookup_list);
 
-/**
- * pwm_add_table() - register PWM device consumers
- * @table: array of consumers to register
- * @num: number of consumers in table
- */
-void pwm_add_table(struct pwm_lookup *table, size_t num)
-{
-	guard(mutex)(&pwm_lookup_lock);
-
-	while (num--) {
-		list_add_tail(&table->list, &pwm_lookup_list);
-		table++;
-	}
-}
-
-/**
- * pwm_remove_table() - unregister PWM device consumers
- * @table: array of consumers to unregister
- * @num: number of consumers in table
- */
-void pwm_remove_table(struct pwm_lookup *table, size_t num)
-{
-	guard(mutex)(&pwm_lookup_lock);
-
-	while (num--) {
-		list_del(&table->list);
-		table++;
-	}
-}
-
 /**
  * pwm_get() - look up and request a PWM device
  * @dev: device for PWM consumer
@@ -2174,6 +2018,162 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
 
+/**
+ * __pwmchip_add() - register a new PWM chip
+ * @chip: the PWM chip to add
+ * @owner: reference to the module providing the chip.
+ *
+ * Register a new PWM chip. @owner is supposed to be THIS_MODULE, use the
+ * pwmchip_add wrapper to do this right.
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
+{
+	int ret;
+
+	if (!chip || !pwmchip_parent(chip) || !chip->ops || !chip->npwm)
+		return -EINVAL;
+
+	/*
+	 * a struct pwm_chip must be allocated using (devm_)pwmchip_alloc,
+	 * otherwise the embedded struct device might disappear too early
+	 * resulting in memory corruption.
+	 * Catch drivers that were not converted appropriately.
+	 */
+	if (!chip->uses_pwmchip_alloc)
+		return -EINVAL;
+
+	if (!pwm_ops_check(chip))
+		return -EINVAL;
+
+	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);
+	if (ret < 0)
+		return ret;
+
+	chip->id = ret;
+
+	dev_set_name(&chip->dev, "pwmchip%u", chip->id);
+
+	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;
+
+	return 0;
+
+err_device_add:
+	scoped_guard(pwmchip, chip)
+		chip->operational = false;
+
+	if (IS_ENABLED(CONFIG_OF))
+		of_pwmchip_remove(chip);
+
+	idr_remove(&pwm_chips, chip->id);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__pwmchip_add);
+
+/**
+ * pwmchip_remove() - remove a PWM chip
+ * @chip: the PWM chip to remove
+ *
+ * Removes a PWM chip.
+ */
+void pwmchip_remove(struct pwm_chip *chip)
+{
+	pwmchip_sysfs_unexport(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_warn(&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);
+
+		idr_remove(&pwm_chips, chip->id);
+	}
+
+	device_del(&chip->dev);
+}
+EXPORT_SYMBOL_GPL(pwmchip_remove);
+
+static void devm_pwmchip_remove(void *data)
+{
+	struct pwm_chip *chip = data;
+
+	pwmchip_remove(chip);
+}
+
+int __devm_pwmchip_add(struct device *dev, struct pwm_chip *chip, struct module *owner)
+{
+	int ret;
+
+	ret = __pwmchip_add(chip, owner);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, devm_pwmchip_remove, chip);
+}
+EXPORT_SYMBOL_GPL(__devm_pwmchip_add);
+
+/**
+ * pwm_add_table() - register PWM device consumers
+ * @table: array of consumers to register
+ * @num: number of consumers in table
+ */
+void pwm_add_table(struct pwm_lookup *table, size_t num)
+{
+	guard(mutex)(&pwm_lookup_lock);
+
+	while (num--) {
+		list_add_tail(&table->list, &pwm_lookup_list);
+		table++;
+	}
+}
+
+/**
+ * pwm_remove_table() - unregister PWM device consumers
+ * @table: array of consumers to unregister
+ * @num: number of consumers in table
+ */
+void pwm_remove_table(struct pwm_lookup *table, size_t num)
+{
+	guard(mutex)(&pwm_lookup_lock);
+
+	while (num--) {
+		list_del(&table->list);
+		table++;
+	}
+}
+
 static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 {
 	unsigned int i;
-- 
2.45.2

[PATCH v5 8/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       | 291 +++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h      |   3 +
 include/uapi/linux/pwm.h |  32 +++++
 3 files changed, 311 insertions(+), 15 deletions(-)
 create mode 100644 include/uapi/linux/pwm.h

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index ed620e35db61..3c25e0ac682c 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>
 
@@ -1915,20 +1917,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 }
 EXPORT_SYMBOL_GPL(pwm_get);
 
-/**
- * pwm_put() - release a PWM device
- * @pwm: PWM device
- */
-void pwm_put(struct pwm_device *pwm)
+static void __pwm_put(struct pwm_device *pwm)
 {
-	struct pwm_chip *chip;
-
-	if (!pwm)
-		return;
-
-	chip = pwm->chip;
-
-	guard(mutex)(&pwm_lock);
+	struct pwm_chip *chip = pwm->chip;
 
 	/*
 	 * Trigger a warning if a consumer called pwm_put() twice.
@@ -1949,6 +1940,20 @@ void pwm_put(struct pwm_device *pwm)
 
 	module_put(chip->owner);
 }
+
+/**
+ * pwm_put() - release a PWM device
+ * @pwm: PWM device
+ */
+void pwm_put(struct pwm_device *pwm)
+{
+	if (!pwm)
+		return;
+
+	guard(mutex)(&pwm_lock);
+
+	__pwm_put(pwm);
+}
 EXPORT_SYMBOL_GPL(pwm_put);
 
 static void devm_pwm_release(void *pwm)
@@ -2018,6 +2023,249 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
 }
 EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
 
+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 struct pwm_device *pwm_cdev_get_requested_pwm(struct pwm_cdev_data *cdata,
+						     u32 hwpwm)
+{
+	struct pwm_chip *chip = cdata->chip;
+
+	if (hwpwm >= chip->npwm)
+		return ERR_PTR(-EINVAL);
+
+	if (cdata->pwm[hwpwm])
+		return cdata->pwm[hwpwm];
+
+	return ERR_PTR(-EINVAL);
+}
+
+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_REQUEST:
+		{
+			unsigned int hwpwm = arg;
+
+			return pwm_cdev_request(cdata, hwpwm);
+		}
+		break;
+
+	case PWM_IOCTL_FREE:
+		{
+			unsigned int hwpwm = arg;
+
+			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;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			wf = (struct pwm_waveform) {
+				.period_length_ns = cwf.period_length_ns,
+				.duty_length_ns = cwf.duty_length_ns,
+				.duty_offset_ns = cwf.duty_offset_ns,
+			};
+
+			ret = pwm_round_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf = (struct pwmchip_waveform) {
+				.hwpwm = cwf.hwpwm,
+				.period_length_ns = wf.period_length_ns,
+				.duty_length_ns = wf.duty_length_ns,
+				.duty_offset_ns = wf.duty_offset_ns,
+			};
+
+			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)
+				return -EINVAL;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			ret = pwm_get_waveform_might_sleep(pwm, &wf);
+			if (ret)
+				return ret;
+
+			cwf.period_length_ns = wf.period_length_ns;
+			cwf.duty_length_ns = wf.duty_length_ns;
+			cwf.duty_offset_ns = wf.duty_offset_ns;
+
+			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)
+				return -EINVAL;
+
+			wf = (struct pwm_waveform){
+				.period_length_ns = cwf.period_length_ns,
+				.duty_length_ns = cwf.duty_length_ns,
+				.duty_offset_ns = cwf.duty_offset_ns,
+			};
+
+			if (!pwm_wf_valid(&wf))
+				return -EINVAL;
+
+			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
+			if (IS_ERR(pwm))
+				return PTR_ERR(pwm);
+
+			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
@@ -2070,7 +2318,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;
 
@@ -2121,7 +2375,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);
 
@@ -2262,9 +2516,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 c3d9ddeafa65..f43dbfaef1a1 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>
@@ -304,6 +305,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
@@ -318,6 +320,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..0a9cdaa0fe37
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,32 @@
+/* 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 - Describe a PWM waveform for a pwm_chip's PWM channel
+ * @hwpwm: per-chip relative index of the PWM device
+ * @__pad: padding, must be zero
+ * @period_length_ns: duration of the repeating period
+ * @duty_length_ns: duration of the active part in each period
+ * @duty_offset_ns: offset of the rising edge from a period's start
+ */
+struct pwmchip_waveform {
+	__u32 hwpwm;
+	__u32 __pad;
+	__u64 period_length_ns;
+	__u64 duty_length_ns;
+	__u64 duty_offset_ns;
+};
+
+#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
+#define PWM_IOCTL_FREE		_IO(0x75, 2)
+#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.45.2

Re: [PATCH v5 2/8] pwm: New abstraction for PWM waveforms

[Trevor Gamblin]

On 2024-09-20 04:57, Uwe Kleine-König wrote:
> 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_ns
> 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_ns == 0 or duty_offset_ns == period_length_ns -
> duty_length_ns).
>
> A disabled PWM is modeled by .period_length_ns = 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_ns and
> duty_length_ns. 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_ns" as suffix the semantic
> should be more obvious to people unfamiliar with the pwm subsystem.
> period is renamed to period_length_ns 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_ns. For that period pick the biggest
> possible duty setting not bigger than wf->duty_length_ns. Third pick the
> biggest possible offset not bigger than wf->duty_offset_ns. If the
> requested period is too small for the hardware, it's expected that a
> setting with the minimal period and duty_length_ns = duty_offset_ns = 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>
Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/core.c  | 234 ++++++++++++++++++++++++++++++++++++++++----
>   include/linux/pwm.h |  36 +++++++
>   2 files changed, 249 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 5a095eb46b54..e3e26aafa461 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_ns) {
> +		if (wf->duty_length_ns + wf->duty_offset_ns < wf->period_length_ns)
> +			*state = (struct pwm_state){
> +				.enabled = true,
> +				.polarity = PWM_POLARITY_NORMAL,
> +				.period = wf->period_length_ns,
> +				.duty_cycle = wf->duty_length_ns,
> +			};
> +		else
> +			*state = (struct pwm_state){
> +				.enabled = true,
> +				.polarity = PWM_POLARITY_INVERSED,
> +				.period = wf->period_length_ns,
> +				.duty_cycle = wf->period_length_ns - wf->duty_length_ns,
> +			};
> +	} 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_ns = state->period,
> +				.duty_length_ns = state->duty_cycle,
> +				.duty_offset_ns = 0,
> +			};
> +		else
> +			*wf = (struct pwm_waveform){
> +				.period_length_ns = state->period,
> +				.duty_length_ns = state->period - state->duty_cycle,
> +				.duty_offset_ns = state->duty_cycle,
> +			};
> +	} else {
> +		*wf = (struct pwm_waveform){
> +			.period_length_ns = 0,
> +		};
> +	}
> +}
> +
> +static int pwm_check_rounding(const struct pwm_waveform *wf,
> +			      const struct pwm_waveform *wf_rounded)
> +{
> +	if (!wf->period_length_ns)
> +		return 0;
> +
> +	if (wf->period_length_ns < wf_rounded->period_length_ns)
> +		return 1;
> +
> +	if (wf->duty_length_ns < wf_rounded->duty_length_ns)
> +		return 1;
> +
> +	if (wf->duty_offset_ns < wf_rounded->duty_offset_ns)
> +		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,69 @@ 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 need arises, until reported otherwise
> +		 * let's assume that consumers don't care.
> +		 */
> +
> +		err = __pwm_round_waveform_tohw(chip, pwm, &wf, &wfhw);
> +		if (err) {
> +			if (err > 0)
> +				/*
> +				 * This signals an invalid request, typically
> +				 * the requested period (or duty_offset) is
> +				 * smaller than possible with the hardware.
> +				 */
> +				return -EINVAL;
> +
> +			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_ns, wf.period_length_ns, wf.duty_offset_ns,
> +					wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
> +		}
> +
> +		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 +441,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
> @@ -435,7 +619,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
> @@ -445,11 +629,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;
>   
> @@ -1136,12 +1316,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 3ea73e075abe..d8cfe1c9b19d 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -49,6 +49,31 @@ enum {
>   	PWMF_EXPORTED = 1,
>   };
>   
> +/**
> + * struct pwm_waveform - description of a PWM waveform
> + * @period_length_ns: PWM period
> + * @duty_length_ns: PWM duty cycle
> + * @duty_offset_ns: 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_ns and so can represent offsets other than zero (with .polarity =
> + * PWM_POLARITY_NORMAL) and period - duty_cycle (.polarity =
> + * PWM_POLARITY_INVERSED).
> + *
> + * Note there is no explicit bool for enabled. A "disabled" PWM is represented
> + * by .period_length_ns = 0. Note further 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_ns;
> +	u64 duty_length_ns;
> +	u64 duty_offset_ns;
> +};
> +
>   /*
>    * struct pwm_state - state of a PWM channel
>    * @period: PWM period (in nanoseconds)
> @@ -259,6 +284,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,

Re: [PATCH v5 3/8] pwm: Provide new consumer API functions for waveforms

[Trevor Gamblin]

On 2024-09-20 04:57, 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/core.c  | 261 ++++++++++++++++++++++++++++++++++++++++++++
>   include/linux/pwm.h |   6 +-
>   2 files changed, 266 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index e3e26aafa461..1cefc6e9c10d 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_ns <= 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_ns > S64_MAX)
> +		return false;
> +
> +	if (wf->duty_length_ns > wf->period_length_ns)
> +		return false;
> +
> +	/*
> +	 * .duty_offset_ns is supposed to be smaller than .period_length_ns, apart
> +	 * from the corner case .duty_offset_ns == 0 && .period_length_ns == 0.
> +	 */
> +	if (wf->duty_offset_ns && wf->duty_offset_ns >= wf->period_length_ns)
> +		return false;
> +
> +	return true;
> +}
> +
>   static void pwm_wf2state(const struct pwm_waveform *wf, struct pwm_state *state)
>   {
>   	if (wf->period_length_ns) {
> @@ -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_ns > b->period_length_ns)
> +		return 1;
> +
> +	if (a->period_length_ns < b->period_length_ns)
> +		return -1;
> +
> +	if (a->duty_length_ns > b->duty_length_ns)
> +		return 1;
> +
> +	if (a->duty_length_ns < b->duty_length_ns)
> +		return -1;
> +
> +	if (a->duty_offset_ns > b->duty_offset_ns)
> +		return 1;
> +
> +	if (a->duty_offset_ns < b->duty_offset_ns)
> +		return -1;
> +
> +	return 0;
> +}
> +
>   static int pwm_check_rounding(const struct pwm_waveform *wf,
>   			      const struct pwm_waveform *wf_rounded)
>   {
> @@ -145,6 +192,220 @@ static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, c
>   
>   #define WFHWSIZE 20
>   
> +/**
> + * pwm_round_waveform_might_sleep - Query hardware capabilities
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: waveform to round and output parameter
> + *
> + * Typically a given waveform cannot be implemented exactly by hardware, e.g.
> + * because hardware only supports coarse period resolution or no duty_offset.
> + * This function returns the actually implemented waveform if you pass wf to
> + * pwm_set_waveform_might_sleep now.
> + *
> + * Note however that the world doesn't stop turning when you call it, so when
> + * doing
> + *
> + * 	pwm_round_waveform_might_sleep(mypwm, &wf);
> + * 	pwm_set_waveform_might_sleep(mypwm, &wf, true);
> + *
> + * the latter might fail, e.g. because an input clock changed its rate between
> + * these two calls and the waveform determined by
> + * pwm_round_waveform_might_sleep() cannot be implemented any more.
> + *
> + * Returns 0 on success, 1 if there is no valid hardware configuration matching
> + * the input waveform under the PWM rounding rules or a negative errno.
> + */
> +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;
> +
> +	if (IS_ENABLED(CONFIG_PWM_DEBUG) && ret_tohw > 1)
> +		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_tohw: requested %llu/%llu [+%llu], return value %d\n",
> +			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, 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_fromhw > 0)
> +		dev_err(&chip->dev, "Unexpected return value from __pwm_round_waveform_fromhw: requested %llu/%llu [+%llu], return value %d\n",
> +			wf_req.duty_length_ns, wf_req.period_length_ns, wf_req.duty_offset_ns, ret_tohw);
> +
> +	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_ns, wf_req.period_length_ns, wf_req.duty_offset_ns,
> +			wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns);
> +
> +	return ret_tohw;
> +}
> +EXPORT_SYMBOL_GPL(pwm_round_waveform_might_sleep);
> +
> +/**
> + * pwm_get_waveform_might_sleep - Query hardware about current configuration
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: output parameter
> + *
> + * Stores the current configuration of the PWM in @wf. Note this is the
> + * equivalent of pwm_get_state_hw() (and not pwm_get_state()) for pwm_waveform.
> + */
> +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);
> +}
> +EXPORT_SYMBOL_GPL(pwm_get_waveform_might_sleep);
> +
> +/* 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_ns) {
> +		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_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
> +
> +		if (exact && pwmwfcmp(wf, &wf_rounded)) {
> +			dev_dbg(&chip->dev, "Requested no rounding, but %llu/%llu [+%llu] -> %llu/%llu [+%llu]\n",
> +				wf->duty_length_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);
> +
> +			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_ns) {
> +		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_ns, wf->period_length_ns, wf->duty_offset_ns,
> +				wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns,
> +				wf_set.duty_length_ns, wf_set.period_length_ns, wf_set.duty_offset_ns);
> +	}
> +	return 0;
> +}
> +
> +/**
> + * pwm_set_waveform_might_sleep - Apply a new waveform
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: The waveform to apply
> + * @exact: If true no rounding is allowed
> + *
> + * Typically a requested waveform cannot be implemented exactly, e.g. because
> + * you requested .period_length_ns = 100 ns, but the hardware can only set
> + * periods that are a multiple of 8.5 ns. With that hardware passing exact =
> + * true results in pwm_set_waveform_might_sleep() failing and returning 1. If
> + * exact = false you get a period of 93.5 ns (i.e. the biggest period not bigger
> + * than the requested value).
> + * Note that even with exact = true, some rounding by less than 1 is
> + * possible/needed. In the above example requesting .period_length_ns = 94 and
> + * exact = true, you get the hardware configured with period = 93.5 ns.
> + */
> +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 d8cfe1c9b19d..c3d9ddeafa65 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -358,7 +358,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);

Re: [PATCH v5 5/8] pwm: axi-pwmgen: Implementation of the waveform callbacks

[Trevor Gamblin]

On 2024-09-20 04:58, Uwe Kleine-König wrote:
> Convert the axi-pwmgen driver to use the new callbacks for hardware
> programming.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
Tested-by: Trevor Gamblin <tgamblin@baylibre.com>
> ---
>   drivers/pwm/pwm-axi-pwmgen.c | 154 ++++++++++++++++++++++++-----------
>   1 file changed, 108 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
> index b5477659ba18..39d184417c7c 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>
> @@ -53,81 +54,142 @@ 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_ns == 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_ns, 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_ns, ddata->clk_rate_hz, NSEC_PER_SEC),
> +						     U32_MAX);
> +			wfhw->duty_offset_cnt = min_t(u64,
> +						      mul_u64_u32_div(wf->duty_offset_ns, 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_ns, wf->period_length_ns, wf->duty_offset_ns,
> +		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_ns = DIV64_U64_ROUND_UP((u64)wfhw->period_cnt * NSEC_PER_SEC,
> +					ddata->clk_rate_hz);
> +
> +	wf->duty_length_ns = DIV64_U64_ROUND_UP((u64)wfhw->duty_cycle_cnt * NSEC_PER_SEC,
> +					    ddata->clk_rate_hz);
> +
> +	wf->duty_offset_ns = 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)

Re: [PATCH v5 2/8] pwm: New abstraction for PWM waveforms

[David Lechner]

On Fri, Sep 20, 2024 at 10:58 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> wrote:
>

...

>
> 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_ns. For that period pick the biggest
> possible duty setting not bigger than wf->duty_length_ns. Third pick the
> biggest possible offset not bigger than wf->duty_offset_ns. If the
> requested period is too small for the hardware, it's expected that a
> setting with the minimal period and duty_length_ns = duty_offset_ns = 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>
> ---

...

> +static int pwm_check_rounding(const struct pwm_waveform *wf,
> +                             const struct pwm_waveform *wf_rounded)
> +{
> +       if (!wf->period_length_ns)
> +               return 0;
> +
> +       if (wf->period_length_ns < wf_rounded->period_length_ns)
> +               return 1;
> +
> +       if (wf->duty_length_ns < wf_rounded->duty_length_ns)
> +               return 1;
> +
> +       if (wf->duty_offset_ns < wf_rounded->duty_offset_ns)
> +               return 1;
> +
> +       return 0;
> +}

It looks like this return value is being used as a bool, so maybe the
return type should be bool?

Re: [PATCH v5 3/8] pwm: Provide new consumer API functions for waveforms

[David Lechner]

On Fri, Sep 20, 2024 at 10:58 AM Uwe Kleine-König
<u.kleine-koenig@baylibre.com> 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.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---

...

> +/**
> + * pwm_set_waveform_might_sleep - Apply a new waveform
> + * Cannot be used in atomic context.
> + * @pwm: PWM device
> + * @wf: The waveform to apply
> + * @exact: If true no rounding is allowed
> + *
> + * Typically a requested waveform cannot be implemented exactly, e.g. because
> + * you requested .period_length_ns = 100 ns, but the hardware can only set
> + * periods that are a multiple of 8.5 ns. With that hardware passing exact =
> + * true results in pwm_set_waveform_might_sleep() failing and returning 1. If

I liked the previous suggestion to return -ERANGE instead of 1 here on failure.


> + * exact = false you get a period of 93.5 ns (i.e. the biggest period not bigger
> + * than the requested value).
> + * Note that even with exact = true, some rounding by less than 1 is
> + * possible/needed. In the above example requesting .period_length_ns = 94 and
> + * exact = true, you get the hardware configured with period = 93.5 ns.
> + */
> +int pwm_set_waveform_might_sleep(struct pwm_device *pwm,
> +                                const struct pwm_waveform *wf, bool exact)

Re: [PATCH v5 3/8] pwm: Provide new consumer API functions for waveforms

[Uwe Kleine-König]

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

Hello David,

On Thu, Sep 26, 2024 at 09:55:05AM +0200, David Lechner wrote:
> On Fri, Sep 20, 2024 at 10:58 AM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> 
> ...
> 
> > +/**
> > + * pwm_set_waveform_might_sleep - Apply a new waveform
> > + * Cannot be used in atomic context.
> > + * @pwm: PWM device
> > + * @wf: The waveform to apply
> > + * @exact: If true no rounding is allowed
> > + *
> > + * Typically a requested waveform cannot be implemented exactly, e.g. because
> > + * you requested .period_length_ns = 100 ns, but the hardware can only set
> > + * periods that are a multiple of 8.5 ns. With that hardware passing exact =
> > + * true results in pwm_set_waveform_might_sleep() failing and returning 1. If
> 
> I liked the previous suggestion to return -ERANGE instead of 1 here on failure.

I hear you. Still I haven't made up my mind and would like to keep it as
is for now to get forward with that series that is the base for Trevor's
adc driver. The return value convention isn't exposed to userspace (as I
won't apply the 8th patch for now), so it's not set in stone and can
easily be adapted accordingly. I hope to get a feeling what is right in
the course of working with this admittedly unusual return value
convention.

Best regards
Uwe

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

Re: [PATCH v5 2/8] pwm: New abstraction for PWM waveforms

[Uwe Kleine-König]

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

Hello David,

On Thu, Sep 26, 2024 at 09:54:56AM +0200, David Lechner wrote:
> On Fri, Sep 20, 2024 at 10:58 AM Uwe Kleine-König
> <u.kleine-koenig@baylibre.com> wrote:
> >
> 
> ...
> 
> >
> > 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_ns. For that period pick the biggest
> > possible duty setting not bigger than wf->duty_length_ns. Third pick the
> > biggest possible offset not bigger than wf->duty_offset_ns. If the
> > requested period is too small for the hardware, it's expected that a
> > setting with the minimal period and duty_length_ns = duty_offset_ns = 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>
> > ---
> 
> ...
> 
> > +static int pwm_check_rounding(const struct pwm_waveform *wf,
> > +                             const struct pwm_waveform *wf_rounded)
> > +{
> > +       if (!wf->period_length_ns)
> > +               return 0;
> > +
> > +       if (wf->period_length_ns < wf_rounded->period_length_ns)
> > +               return 1;
> > +
> > +       if (wf->duty_length_ns < wf_rounded->duty_length_ns)
> > +               return 1;
> > +
> > +       if (wf->duty_offset_ns < wf_rounded->duty_offset_ns)
> > +               return 1;
> > +
> > +       return 0;
> > +}
> 
> It looks like this return value is being used as a bool, so maybe the
> return type should be bool?

Good idea. I squashed the following diff in the commit:

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index e3e26aafa461..bbe7bfdb1549 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -95,22 +95,22 @@ static void pwm_state2wf(const struct pwm_state *state, struct pwm_waveform *wf)
 	}
 }
 
-static int pwm_check_rounding(const struct pwm_waveform *wf,
-			      const struct pwm_waveform *wf_rounded)
+static bool pwm_check_rounding(const struct pwm_waveform *wf,
+			       const struct pwm_waveform *wf_rounded)
 {
 	if (!wf->period_length_ns)
-		return 0;
+		return true;
 
 	if (wf->period_length_ns < wf_rounded->period_length_ns)
-		return 1;
+		return false;
 
 	if (wf->duty_length_ns < wf_rounded->duty_length_ns)
-		return 1;
+		return false;
 
 	if (wf->duty_offset_ns < wf_rounded->duty_offset_ns)
-		return 1;
+		return false;
 
-	return 0;
+	return true;
 }
 
 static int __pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *pwm,
@@ -348,7 +348,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
 			if (err)
 				return err;
 
-			if (pwm_check_rounding(&wf, &wf_rounded))
+			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_ns, wf.period_length_ns, wf.duty_offset_ns,
 					wf_rounded.duty_length_ns, wf_rounded.period_length_ns, wf_rounded.duty_offset_ns);

and fixed the two new occurences of pwm_check_rounding() in the patch
"Provide new consumer API functions for waveforms" to invert the result,
too.

I created a branch

	https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git pwm/duty_offset

that contains the updated result based on the pwm changes for 6.12-rc1
that are already in Linus' tree. When this survived being in next for a
week or so, I intend to tag it that Jonathan Cameron can pull it into
his iio tree and apply Trevor Gamblin's series for the ad7625 on top.

Best regards
Uwe

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

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

[Kent Gibson]

On Fri, Sep 20, 2024 at 10:58:04AM +0200, Uwe Kleine-König wrote:
> 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       | 291 +++++++++++++++++++++++++++++++++++++--
>  include/linux/pwm.h      |   3 +
>  include/uapi/linux/pwm.h |  32 +++++
>  3 files changed, 311 insertions(+), 15 deletions(-)
>  create mode 100644 include/uapi/linux/pwm.h
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index ed620e35db61..3c25e0ac682c 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>
>
> @@ -1915,20 +1917,9 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
>  }
>  EXPORT_SYMBOL_GPL(pwm_get);
>
> -/**
> - * pwm_put() - release a PWM device
> - * @pwm: PWM device
> - */
> -void pwm_put(struct pwm_device *pwm)
> +static void __pwm_put(struct pwm_device *pwm)
>  {
> -	struct pwm_chip *chip;
> -
> -	if (!pwm)
> -		return;
> -
> -	chip = pwm->chip;
> -
> -	guard(mutex)(&pwm_lock);
> +	struct pwm_chip *chip = pwm->chip;
>
>  	/*
>  	 * Trigger a warning if a consumer called pwm_put() twice.
> @@ -1949,6 +1940,20 @@ void pwm_put(struct pwm_device *pwm)
>
>  	module_put(chip->owner);
>  }
> +
> +/**
> + * pwm_put() - release a PWM device
> + * @pwm: PWM device
> + */
> +void pwm_put(struct pwm_device *pwm)
> +{
> +	if (!pwm)
> +		return;
> +
> +	guard(mutex)(&pwm_lock);
> +
> +	__pwm_put(pwm);
> +}
>  EXPORT_SYMBOL_GPL(pwm_put);
>
>  static void devm_pwm_release(void *pwm)
> @@ -2018,6 +2023,249 @@ struct pwm_device *devm_fwnode_pwm_get(struct device *dev,
>  }
>  EXPORT_SYMBOL_GPL(devm_fwnode_pwm_get);
>
> +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;
> +}
> +

Allow the user to specify the consumer label as part of the request rather
than hard coding it to "pwm-cdev"?

> +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 struct pwm_device *pwm_cdev_get_requested_pwm(struct pwm_cdev_data *cdata,
> +						     u32 hwpwm)
> +{
> +	struct pwm_chip *chip = cdata->chip;
> +
> +	if (hwpwm >= chip->npwm)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (cdata->pwm[hwpwm])
> +		return cdata->pwm[hwpwm];
> +
> +	return ERR_PTR(-EINVAL);
> +}
> +
> +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);
> +

Coarse grain locking of the whole of pwm for the duration, where some
calls my sleep, feels excessive. Is it really necessary for all of the
ioctls?

> +	if (!chip->operational)
> +		return -ENODEV;
> +
> +	switch (cmd) {
> +	case PWM_IOCTL_REQUEST:
> +		{
> +			unsigned int hwpwm = arg;
> +
> +			return pwm_cdev_request(cdata, hwpwm);
> +		}
> +		break;
> +
> +	case PWM_IOCTL_FREE:
> +		{
> +			unsigned int hwpwm = arg;
> +
> +			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;
> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			wf = (struct pwm_waveform) {
> +				.period_length_ns = cwf.period_length_ns,
> +				.duty_length_ns = cwf.duty_length_ns,
> +				.duty_offset_ns = cwf.duty_offset_ns,
> +			};
> +
> +			ret = pwm_round_waveform_might_sleep(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf = (struct pwmchip_waveform) {
> +				.hwpwm = cwf.hwpwm,
> +				.period_length_ns = wf.period_length_ns,
> +				.duty_length_ns = wf.duty_length_ns,
> +				.duty_offset_ns = wf.duty_offset_ns,
> +			};
> +
> +			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)
> +				return -EINVAL;
> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			ret = pwm_get_waveform_might_sleep(pwm, &wf);
> +			if (ret)
> +				return ret;
> +
> +			cwf.period_length_ns = wf.period_length_ns;
> +			cwf.duty_length_ns = wf.duty_length_ns;
> +			cwf.duty_offset_ns = wf.duty_offset_ns;
> +
> +			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)
> +				return -EINVAL;
> +
> +			wf = (struct pwm_waveform){
> +				.period_length_ns = cwf.period_length_ns,
> +				.duty_length_ns = cwf.duty_length_ns,
> +				.duty_offset_ns = cwf.duty_offset_ns,
> +			};
> +
> +			if (!pwm_wf_valid(&wf))
> +				return -EINVAL;
> +
> +			pwm = pwm_cdev_get_requested_pwm(cdata, cwf.hwpwm);
> +			if (IS_ERR(pwm))
> +				return PTR_ERR(pwm);
> +
> +			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
> @@ -2070,7 +2318,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;
>
> @@ -2121,7 +2375,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);
>
> @@ -2262,9 +2516,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 c3d9ddeafa65..f43dbfaef1a1 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>
> @@ -304,6 +305,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
> @@ -318,6 +320,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..0a9cdaa0fe37
> --- /dev/null
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,32 @@
> +/* 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 - Describe a PWM waveform for a pwm_chip's PWM channel
> + * @hwpwm: per-chip relative index of the PWM device
> + * @__pad: padding, must be zero
> + * @period_length_ns: duration of the repeating period
> + * @duty_length_ns: duration of the active part in each period
> + * @duty_offset_ns: offset of the rising edge from a period's start
> + */

While you have added some documentation, this is still lacking compared
to the corresponding in include/linux/pwm.h.  e.g. zeroing
period_length_ns to disable a PWM...

> +struct pwmchip_waveform {
> +	__u32 hwpwm;
> +	__u32 __pad;
> +	__u64 period_length_ns;
> +	__u64 duty_length_ns;
> +	__u64 duty_offset_ns;
> +};
> +
> +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> +#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)
> +

A brief description of the ioctls and their semantics would be handy,
either here or as full blown documentation in
linux/Documentation/userspace-api/pwm/...

PWMs are automatically released when the pwmchip file is closed?
And the state of the PWM line after release (or _FREE) is indeterminate?

Is it possible that the underlying PWM chip be removed while the user has
the pwmchip open and/or has pwm devices requested?

Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
npwms, module name.  For pwm devices the label, if the PWM is requested and
the consumer's label (similar to the GPIO chipinfo and lineinfo)?
Unless that information otherwise exposed to userspace?

Cheers,
Kent.

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

[Uwe Kleine-König]

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

Hello Kent,

On Sun, Sep 29, 2024 at 12:48:28PM +0800, Kent Gibson wrote:
> On Fri, Sep 20, 2024 at 10:58:04AM +0200, Uwe Kleine-König wrote:
> > +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;
> > +}
> > +
> 
> Allow the user to specify the consumer label as part of the request rather
> than hard coding it to "pwm-cdev"?

I considered using the process name, or pwm-cdev:$(pidof
userspace-program). And I'd like to have a possibility to specify names
in dt (that then could be used to lookup a PWM, too). I think these two
are better than having userspace provide a name. What do you think?

> > +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 struct pwm_device *pwm_cdev_get_requested_pwm(struct pwm_cdev_data *cdata,
> > +						     u32 hwpwm)
> > +{
> > +	struct pwm_chip *chip = cdata->chip;
> > +
> > +	if (hwpwm >= chip->npwm)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (cdata->pwm[hwpwm])
> > +		return cdata->pwm[hwpwm];
> > +
> > +	return ERR_PTR(-EINVAL);
> > +}
> > +
> > +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);
> > +
> 
> Coarse grain locking of the whole of pwm for the duration, where some
> calls my sleep, feels excessive. Is it really necessary for all of the
> ioctls?

This might be improvable a bit indeed. I think we won't come around
holding the pwmchip lock, but that would already be nice. I'll invest
some brain cycles here.
 
> > +/**
> > + * struct pwmchip_waveform - Describe a PWM waveform for a pwm_chip's PWM channel
> > + * @hwpwm: per-chip relative index of the PWM device
> > + * @__pad: padding, must be zero
> > + * @period_length_ns: duration of the repeating period
> > + * @duty_length_ns: duration of the active part in each period
> > + * @duty_offset_ns: offset of the rising edge from a period's start
> > + */
> 
> While you have added some documentation, this is still lacking compared
> to the corresponding in include/linux/pwm.h.  e.g. zeroing
> period_length_ns to disable a PWM...

Ack.

> > +struct pwmchip_waveform {
> > +	__u32 hwpwm;
> > +	__u32 __pad;
> > +	__u64 period_length_ns;
> > +	__u64 duty_length_ns;
> > +	__u64 duty_offset_ns;
> > +};
> > +
> > +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> > +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> > +#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)
> > +
> 
> A brief description of the ioctls and their semantics would be handy,
> either here or as full blown documentation in
> linux/Documentation/userspace-api/pwm/...

Ack.
 
> PWMs are automatically released when the pwmchip file is closed?
> And the state of the PWM line after release (or _FREE) is indeterminate?
> 
> Is it possible that the underlying PWM chip be removed while the user has
> the pwmchip open and/or has pwm devices requested?

I guess these questions are hints about what to describe in the docs to
be written and not actual questions. I fully agree that the
documentation front isn't optimal yet.

> Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
> npwms, module name.  For pwm devices the label, if the PWM is requested and
> the consumer's label (similar to the GPIO chipinfo and lineinfo)?
> Unless that information otherwise exposed to userspace?

Most of that is already in /sys/class/pwm. Duplicating that feels wrong.

Thanks a lot for your feedback
Uwe

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

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

[Kent Gibson]

On Mon, Sep 30, 2024 at 08:01:45AM +0200, Uwe Kleine-König wrote:
> Hello Kent,
>
> On Sun, Sep 29, 2024 at 12:48:28PM +0800, Kent Gibson wrote:
> > On Fri, Sep 20, 2024 at 10:58:04AM +0200, Uwe Kleine-König wrote:
> > > +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;
> > > +}
> > > +
> >
> > Allow the user to specify the consumer label as part of the request rather
> > than hard coding it to "pwm-cdev"?
>
> I considered using the process name, or pwm-cdev:$(pidof
> userspace-program). And I'd like to have a possibility to specify names
> in dt (that then could be used to lookup a PWM, too). I think these two
> are better than having userspace provide a name. What do you think?
>

Are we talking the PWM device or the consumer?  Or both?

Wrt the consumer, For GPIO we don't provide a useful default, though perhaps
we should. Instead we rely on the userspace library to provide one - if the
user hasn't.

I'd go with the pid option - that is more unique than the process name.

It could still be useful for the user to be able to provide a label,
especially if you are going to allow arbitrary labels via dt.  Though as
often as not the user doesn't know what name to provide and fills it with
something useless, so I don't think there is any clear winner here.

> > > +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 struct pwm_device *pwm_cdev_get_requested_pwm(struct pwm_cdev_data *cdata,
> > > +						     u32 hwpwm)
> > > +{
> > > +	struct pwm_chip *chip = cdata->chip;
> > > +
> > > +	if (hwpwm >= chip->npwm)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (cdata->pwm[hwpwm])
> > > +		return cdata->pwm[hwpwm];
> > > +
> > > +	return ERR_PTR(-EINVAL);
> > > +}
> > > +
> > > +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);
> > > +
> >
> > Coarse grain locking of the whole of pwm for the duration, where some
> > calls my sleep, feels excessive. Is it really necessary for all of the
> > ioctls?
>
> This might be improvable a bit indeed. I think we won't come around
> holding the pwmchip lock, but that would already be nice. I'll invest
> some brain cycles here.
>

That can always be a later refinement - better too much locking than
too little.

> > > +/**
> > > + * struct pwmchip_waveform - Describe a PWM waveform for a pwm_chip's PWM channel
> > > + * @hwpwm: per-chip relative index of the PWM device
> > > + * @__pad: padding, must be zero
> > > + * @period_length_ns: duration of the repeating period
> > > + * @duty_length_ns: duration of the active part in each period
> > > + * @duty_offset_ns: offset of the rising edge from a period's start
> > > + */
> >
> > While you have added some documentation, this is still lacking compared
> > to the corresponding in include/linux/pwm.h.  e.g. zeroing
> > period_length_ns to disable a PWM...
>
> Ack.
>
> > > +struct pwmchip_waveform {
> > > +	__u32 hwpwm;
> > > +	__u32 __pad;
> > > +	__u64 period_length_ns;
> > > +	__u64 duty_length_ns;
> > > +	__u64 duty_offset_ns;
> > > +};
> > > +
> > > +#define PWM_IOCTL_REQUEST	_IO(0x75, 1)
> > > +#define PWM_IOCTL_FREE		_IO(0x75, 2)
> > > +#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)
> > > +
> >
> > A brief description of the ioctls and their semantics would be handy,
> > either here or as full blown documentation in
> > linux/Documentation/userspace-api/pwm/...
>
> Ack.
>
> > PWMs are automatically released when the pwmchip file is closed?
> > And the state of the PWM line after release (or _FREE) is indeterminate?
> >
> > Is it possible that the underlying PWM chip be removed while the user has
> > the pwmchip open and/or has pwm devices requested?
>
> I guess these questions are hints about what to describe in the docs to
> be written and not actual questions. I fully agree that the
> documentation front isn't optimal yet.
>

Yup, and as per the locking, the documentation can always be a later
refinement - so long as your intent is clear now before the ioctls get
chiselled in stone.

> > Provide some ioctls to aid discoverability, e.g. for pwm chips exposing the
> > npwms, module name.  For pwm devices the label, if the PWM is requested and
> > the consumer's label (similar to the GPIO chipinfo and lineinfo)?
> > Unless that information otherwise exposed to userspace?
>
> Most of that is already in /sys/class/pwm. Duplicating that feels wrong.
>

Depends on whether you want the interface to be standalone or an
addition to the sysfs - and that is your call.

> Thanks a lot for your feedback

No problem, and again sorry for not getting around to it earlier.

Cheers,
Kent.

Re: [PATCH v5 6/8] pwm: stm32: Implementation of the waveform callbacks

[Kees Bakker]

Op 20-09-2024 om 10:58 schreef 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>
[...]
> +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 int 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);
Did you forget to assign to ret?
> +			if (ret)
> +				goto out;
> +
> [...]

Re: [PATCH v5 6/8] pwm: stm32: Implementation of the waveform callbacks

[Uwe Kleine-König]

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

Hello Kees,

On Tue, Oct 01, 2024 at 09:17:47PM +0200, Kees Bakker wrote:
> Op 20-09-2024 om 10:58 schreef Uwe Kleine-König:
> > +			regmap_read(priv->regmap, TIM_ARR, &arr);
> Did you forget to assign to ret?
> > +			if (ret)
> > +				goto out;
> > +
> > [...]

It seems so, yes. How did you find that one?

When I create a patch, is it ok if I add a Reported-by: for you?

Best reagrds
Uwe

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

Re: [PATCH v5 6/8] pwm: stm32: Implementation of the waveform callbacks

[Kees Bakker]

Op 02-10-2024 om 10:02 schreef Uwe Kleine-König:
> Hello Kees,
>
> On Tue, Oct 01, 2024 at 09:17:47PM +0200, Kees Bakker wrote:
>> Op 20-09-2024 om 10:58 schreef Uwe Kleine-König:
>>> +			regmap_read(priv->regmap, TIM_ARR, &arr);
>> Did you forget to assign to ret?
>>> +			if (ret)
>>> +				goto out;
>>> +
>>> [...]
> It seems so, yes. How did you find that one?
>
> When I create a patch, is it ok if I add a Reported-by: for you?
Fine by me. But I have to be honest here. It was reported by Coverity [1].
I'm subscribed to daily reports of linux-next scanning.
>
> Best reagrds
> Uwe
[1] https://scan.coverity.com/projects/linux-next-weekly-scan?tab=overview