* [PATCH v4 1/7] pwm: Add more locking
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
@ 2024-09-06 15:42 ` Uwe Kleine-König
2024-09-06 19:54 ` David Lechner
2024-09-06 15:42 ` [PATCH v4 2/7] pwm: New abstraction for PWM waveforms Uwe Kleine-König
` (7 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:42 UTC (permalink / raw)
To: linux-pwm; +Cc: Kent Gibson, David Lechner
This ensures that a pwm_chip that has no corresponding driver isn't used
and that a driver doesn't go away while a callback is still running.
In the presence of device links this isn't necessary yet (so this is no
fix) but for pwm character device support this is needed.
To not serialize all pwm_apply_state() calls, this introduces a per chip
lock. An additional complication is that for atomic chips a mutex cannot
be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be
held while calling an operation for a sleeping chip. So depending on the
chip being atomic or not a spinlock or a mutex is used.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
---
drivers/pwm/core.c | 95 +++++++++++++++++++++++++++++++++++++++++----
include/linux/pwm.h | 13 +++++++
2 files changed, 100 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 6e752e148b98..9752eb446879 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -31,6 +31,24 @@ static DEFINE_MUTEX(pwm_lock);
static DEFINE_IDR(pwm_chips);
+static void pwmchip_lock(struct pwm_chip *chip)
+{
+ if (chip->atomic)
+ spin_lock(&chip->atomic_lock);
+ else
+ mutex_lock(&chip->nonatomic_lock);
+}
+
+static void pwmchip_unlock(struct pwm_chip *chip)
+{
+ if (chip->atomic)
+ spin_unlock(&chip->atomic_lock);
+ else
+ mutex_unlock(&chip->nonatomic_lock);
+}
+
+DEFINE_GUARD(pwmchip, struct pwm_chip *, pwmchip_lock(_T), pwmchip_unlock(_T))
+
static void pwm_apply_debug(struct pwm_device *pwm,
const struct pwm_state *state)
{
@@ -220,6 +238,7 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
{
int err;
+ struct pwm_chip *chip = pwm->chip;
/*
* Some lowlevel driver's implementations of .apply() make use of
@@ -230,7 +249,12 @@ int pwm_apply_might_sleep(struct pwm_device *pwm, const struct pwm_state *state)
*/
might_sleep();
- if (IS_ENABLED(CONFIG_PWM_DEBUG) && pwm->chip->atomic) {
+ guard(pwmchip)(chip);
+
+ if (!chip->operational)
+ return -ENODEV;
+
+ if (IS_ENABLED(CONFIG_PWM_DEBUG) && chip->atomic) {
/*
* Catch any drivers that have been marked as atomic but
* that will sleep anyway.
@@ -254,9 +278,16 @@ EXPORT_SYMBOL_GPL(pwm_apply_might_sleep);
*/
int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state)
{
- WARN_ONCE(!pwm->chip->atomic,
+ struct pwm_chip *chip = pwm->chip;
+
+ WARN_ONCE(!chip->atomic,
"sleeping PWM driver used in atomic context\n");
+ guard(pwmchip)(chip);
+
+ if (!chip->operational)
+ return -ENODEV;
+
return __pwm_apply(pwm, state);
}
EXPORT_SYMBOL_GPL(pwm_apply_atomic);
@@ -336,6 +367,11 @@ static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
guard(mutex)(&pwm_lock);
+ guard(pwmchip)(chip);
+
+ if (!chip->operational)
+ return -ENODEV;
+
return ops->capture(chip, pwm, result, timeout);
}
@@ -368,6 +404,14 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
if (test_bit(PWMF_REQUESTED, &pwm->flags))
return -EBUSY;
+ /*
+ * This function is called while holding pwm_lock. As .operational only
+ * changes while holding this lock, checking it here without holding the
+ * chip lock is fine.
+ */
+ if (!chip->operational)
+ return -ENODEV;
+
if (!try_module_get(chip->owner))
return -ENODEV;
@@ -396,7 +440,9 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
*/
struct pwm_state state = { 0, };
- err = ops->get_state(chip, pwm, &state);
+ scoped_guard(pwmchip, chip)
+ err = ops->get_state(chip, pwm, &state);
+
trace_pwm_get(pwm, &state, err);
if (!err)
@@ -1020,6 +1066,7 @@ struct pwm_chip *pwmchip_alloc(struct device *parent, unsigned int npwm, size_t
chip->npwm = npwm;
chip->uses_pwmchip_alloc = true;
+ chip->operational = false;
pwmchip_dev = &chip->dev;
device_initialize(pwmchip_dev);
@@ -1125,6 +1172,11 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
chip->owner = owner;
+ if (chip->atomic)
+ spin_lock_init(&chip->atomic_lock);
+ else
+ mutex_init(&chip->nonatomic_lock);
+
guard(mutex)(&pwm_lock);
ret = idr_alloc(&pwm_chips, chip, 0, 0, GFP_KERNEL);
@@ -1138,6 +1190,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
if (IS_ENABLED(CONFIG_OF))
of_pwmchip_add(chip);
+ scoped_guard(pwmchip, chip)
+ chip->operational = true;
+
ret = device_add(&chip->dev);
if (ret)
goto err_device_add;
@@ -1145,6 +1200,9 @@ int __pwmchip_add(struct pwm_chip *chip, struct module *owner)
return 0;
err_device_add:
+ scoped_guard(pwmchip, chip)
+ chip->operational = false;
+
if (IS_ENABLED(CONFIG_OF))
of_pwmchip_remove(chip);
@@ -1164,11 +1222,27 @@ void pwmchip_remove(struct pwm_chip *chip)
{
pwmchip_sysfs_unexport(chip);
- if (IS_ENABLED(CONFIG_OF))
- of_pwmchip_remove(chip);
+ scoped_guard(mutex, &pwm_lock) {
+ unsigned int i;
+
+ scoped_guard(pwmchip, chip)
+ chip->operational = false;
+
+ for (i = 0; i < chip->npwm; ++i) {
+ struct pwm_device *pwm = &chip->pwms[i];
+
+ if (test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+ dev_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 +1612,17 @@ void pwm_put(struct pwm_device *pwm)
guard(mutex)(&pwm_lock);
- if (!test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
+ /*
+ * If the chip isn't operational, PWMF_REQUESTED was already cleared. So
+ * don't warn in this case. This can only happen if a consumer called
+ * pwm_put() twice.
+ */
+ if (chip->operational && !test_and_clear_bit(PWMF_REQUESTED, &pwm->flags)) {
pr_warn("PWM device already freed\n");
return;
}
- if (chip->ops->free)
+ if (chip->operational && chip->ops->free)
pwm->chip->ops->free(pwm->chip, pwm);
pwm->label = NULL;
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 8acd60b53f58..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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 1/7] pwm: Add more locking
2024-09-06 15:42 ` [PATCH v4 1/7] pwm: Add more locking Uwe Kleine-König
@ 2024-09-06 19:54 ` David Lechner
2024-09-17 16:01 ` Uwe Kleine-König
0 siblings, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-09-06 19:54 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm; +Cc: Kent Gibson
On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> This ensures that a pwm_chip that has no corresponding driver isn't used
> and that a driver doesn't go away while a callback is still running.
>
> In the presence of device links this isn't necessary yet (so this is no
> fix) but for pwm character device support this is needed.
>
> To not serialize all pwm_apply_state() calls, this introduces a per chip
> lock. An additional complication is that for atomic chips a mutex cannot
> be used (as pwm_apply_atomic() must not sleep) and a spinlock cannot be
> held while calling an operation for a sleeping chip. So depending on the
> chip being atomic or not a spinlock or a mutex is used.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
...
> @@ -336,6 +367,11 @@ static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
>
> guard(mutex)(&pwm_lock);
Do we still need to hold pwm_lock here? Maybe we can remove it?
Or add a comment about why it is still necessary even though we
also hold pwmchip lock.
>
> + guard(pwmchip)(chip);
> +
> + if (!chip->operational)
> + return -ENODEV;
> +
> return ops->capture(chip, pwm, result, timeout);
> }
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 1/7] pwm: Add more locking
2024-09-06 19:54 ` David Lechner
@ 2024-09-17 16:01 ` Uwe Kleine-König
0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-17 16:01 UTC (permalink / raw)
To: David Lechner; +Cc: linux-pwm, Kent Gibson
[-- Attachment #1: Type: text/plain, Size: 1387 bytes --]
Hello David,
On Fri, Sep 06, 2024 at 02:54:29PM -0500, David Lechner wrote:
> On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> > @@ -336,6 +367,11 @@ static int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
> >
> > guard(mutex)(&pwm_lock);
>
> Do we still need to hold pwm_lock here? Maybe we can remove it?
> Or add a comment about why it is still necessary even though we
> also hold pwmchip lock.
Probably we don't. And even if it were required from a theoretic POV,
it's most probably still not needed because nobody actually uses
pwm_capture(). So I suggest to add a comment like:
/*
* 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.
*/
Until someone speaks up and tells that pwm_capture is still useful, I'm
happy if I don't have to spend brain cycles about it.
I also still consider
https://lore.kernel.org/linux-pwm/20220523174502.987113-3-u.kleine-koenig@pengutronix.de
a good idea to make it more likely that someone who still uses capture
support shows up on the list.
> > + guard(pwmchip)(chip);
> > +
> > + if (!chip->operational)
> > + return -ENODEV;
> > +
> > return ops->capture(chip, pwm, result, timeout);
> > }
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 2/7] pwm: New abstraction for PWM waveforms
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
2024-09-06 15:42 ` [PATCH v4 1/7] pwm: Add more locking Uwe Kleine-König
@ 2024-09-06 15:42 ` Uwe Kleine-König
2024-09-06 17:50 ` Trevor Gamblin
2024-09-06 20:33 ` David Lechner
2024-09-06 15:42 ` [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms Uwe Kleine-König
` (6 subsequent siblings)
8 siblings, 2 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:42 UTC (permalink / raw)
To: linux-pwm; +Cc: Fabrice Gasnier, Kent Gibson, David Lechner
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 9752eb446879..a5aec732e2a4 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
@@ -430,7 +614,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
}
}
- if (ops->get_state) {
+ if (ops->read_waveform || ops->get_state) {
/*
* Zero-initialize state because most drivers are unaware of
* .usage_power. The other members of state are supposed to be
@@ -440,11 +624,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
*/
struct pwm_state state = { 0, };
- scoped_guard(pwmchip, chip)
- err = ops->get_state(chip, pwm, &state);
-
- trace_pwm_get(pwm, &state, err);
-
+ err = pwm_get_state_hw(pwm, &state);
if (!err)
pwm->state = state;
@@ -1131,12 +1311,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..6a26a5210dab 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 $period - $duty_cycle
+ * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
+ * explicit bool for enabled. A "disabled" PWM is represented by
+ * .period_length_ns = 0.
+ *
+ * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
+ * hardware's capabilities it might drive the active or inactive level, go
+ * high-z or even continue to toggle.
+ *
+ * The unit for all three members is nanoseconds.
+ */
+struct pwm_waveform {
+ u64 period_length_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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 2/7] pwm: New abstraction for PWM waveforms
2024-09-06 15:42 ` [PATCH v4 2/7] pwm: New abstraction for PWM waveforms Uwe Kleine-König
@ 2024-09-06 17:50 ` Trevor Gamblin
2024-09-06 20:33 ` David Lechner
1 sibling, 0 replies; 24+ messages in thread
From: Trevor Gamblin @ 2024-09-06 17:50 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm
Cc: Fabrice Gasnier, Kent Gibson, David Lechner
On 2024-09-06 11:42, 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>
Reviewed-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 9752eb446879..a5aec732e2a4 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
> @@ -430,7 +614,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> }
> }
>
> - if (ops->get_state) {
> + if (ops->read_waveform || ops->get_state) {
> /*
> * Zero-initialize state because most drivers are unaware of
> * .usage_power. The other members of state are supposed to be
> @@ -440,11 +624,7 @@ static int pwm_device_request(struct pwm_device *pwm, const char *label)
> */
> struct pwm_state state = { 0, };
>
> - scoped_guard(pwmchip, chip)
> - err = ops->get_state(chip, pwm, &state);
> -
> - trace_pwm_get(pwm, &state, err);
> -
> + err = pwm_get_state_hw(pwm, &state);
> if (!err)
> pwm->state = state;
>
> @@ -1131,12 +1311,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..6a26a5210dab 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 $period - $duty_cycle
> + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
> + * explicit bool for enabled. A "disabled" PWM is represented by
> + * .period_length_ns = 0.
> + *
> + * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
> + * hardware's capabilities it might drive the active or inactive level, go
> + * high-z or even continue to toggle.
> + *
> + * The unit for all three members is nanoseconds.
> + */
> +struct pwm_waveform {
> + u64 period_length_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,
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 2/7] pwm: New abstraction for PWM waveforms
2024-09-06 15:42 ` [PATCH v4 2/7] pwm: New abstraction for PWM waveforms Uwe Kleine-König
2024-09-06 17:50 ` Trevor Gamblin
@ 2024-09-06 20:33 ` David Lechner
1 sibling, 0 replies; 24+ messages in thread
From: David Lechner @ 2024-09-06 20:33 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm; +Cc: Fabrice Gasnier, Kent Gibson
On 9/6/24 10:42 AM, 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.)
>
...
> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 3ea73e075abe..6a26a5210dab 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -49,6 +49,31 @@ enum {
> PWMF_EXPORTED = 1,
> };
>
> +/*
Should this be /** for kernel-doc?
> + * 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 $period - $duty_cycle
According to [1], "$" is generally used for environment variables. It
suggests that &struct_name.member should be preferred. I don't have any
strong opinions on what is best, but $ jumped out at me as unusual.
[1]: https://docs.kernel.org/doc-guide/kernel-doc.html
> + * which is done using .polarity = PWM_POLARITY_INVERSED. Note there is no
> + * explicit bool for enabled. A "disabled" PWM is represented by
> + * .period_length_ns = 0.
> + *
> + * Note that the behaviour of a "disabled" PWM is undefined. Depending on the
> + * hardware's capabilities it might drive the active or inactive level, go
> + * high-z or even continue to toggle.
> + *
> + * The unit for all three members is nanoseconds.
> + */
> +struct pwm_waveform {
> + u64 period_length_ns;
> + u64 duty_length_ns;
> + u64 duty_offset_ns;
> +};
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
2024-09-06 15:42 ` [PATCH v4 1/7] pwm: Add more locking Uwe Kleine-König
2024-09-06 15:42 ` [PATCH v4 2/7] pwm: New abstraction for PWM waveforms Uwe Kleine-König
@ 2024-09-06 15:42 ` Uwe Kleine-König
2024-09-06 21:22 ` David Lechner
2024-09-06 15:43 ` [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access Uwe Kleine-König
` (5 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:42 UTC (permalink / raw)
To: linux-pwm; +Cc: Kent Gibson, David Lechner
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 a5aec732e2a4..c7f39f9f4bcf 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 6a26a5210dab..40cef0bc0de7 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms
2024-09-06 15:42 ` [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms Uwe Kleine-König
@ 2024-09-06 21:22 ` David Lechner
2024-09-08 12:01 ` Uwe Kleine-König
0 siblings, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-09-06 21:22 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm; +Cc: Kent Gibson
On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> Provide API functions for consumers to work with waveforms.
>
> Note that one relevant difference between pwm_get_state() and
> pwm_get_waveform*() is that the latter yields the actually configured
> hardware state, while the former yields the last state passed to
> pwm_apply*() and so doesn't account for hardware specific rounding.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> ---
One thing I am wondering about is how to implement rounding up instead
of down. For example, I we need a >= 10 ns duty cycle.
Here is my attempt...
#define MIN_DUTY_NS 10
int some_func(struct pwm_device *pwm)
{
struct pwm_waveform wf = {
.period_length_ns = 400,
};
u64 trial_ns = MIN_DUTY_NS;
int ret;
do {
wf.duty_length_ns = trial_ns;
ret = pwm_round_waveform_might_sleep(pwm, &wf);
if (ret < 0)
return ret;
/*
* ret == 1 could be either because duty or period
* is not attainable. In any case, we have to wait
* until the last trial to rule out earlier trials
* failing because of too small duty since we try
* again with larger duty. Maybe this check isn't
* needed though since pwm_round_waveform_might_sleep()
* should fail when trial_ns > wf.period_length_ns?
*/
if (ret == 1 && trial_ns == wf.period_length_ns)
return -ERANGE;
trial_ns++;
} while (wf.duty_length_ns < MIN_DUTY_NS);
return pwm_set_waveform_might_sleep(pwm, &wf, true);
}
1. This seems like it would waste time trying each 1 ns increment
compared to being able to tell the low level driver which way
we want to round.
2. Even with this, we could end up with an actual period of 9.5 ns
which is < 10 ns but have to way to know since the returned value
will be 10. Probably not likely that 0.5 ns is going to cause
something to malfunction, but you never know.
3. Handling ret == 1 seems kind of messy since it could be caused
by multiple different problems.
Maybe we could consider including a rounding direction (up/down/closest)
for each of the waveform parameters and pass that along to low level
driver to avoid much of this? Or at least have these parameters for
the high-level pwm_round_waveform_might_sleep() so each consumer doesn't
have to try to figure out how to do the rounding right?
> @@ -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
It would be helpful to spell out in the description below that @wf will
be modified upon non-error return and what the modified values will
actually be. (Or refer to the other functions where the values are already
documented to avoid duplication.)
> + *
> + * 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)
> +{
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms
2024-09-06 21:22 ` David Lechner
@ 2024-09-08 12:01 ` Uwe Kleine-König
0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-08 12:01 UTC (permalink / raw)
To: David Lechner; +Cc: linux-pwm, Kent Gibson
[-- Attachment #1: Type: text/plain, Size: 4909 bytes --]
Hello David,
On Fri, Sep 06, 2024 at 04:22:42PM -0500, David Lechner wrote:
> On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> > Provide API functions for consumers to work with waveforms.
> >
> > Note that one relevant difference between pwm_get_state() and
> > pwm_get_waveform*() is that the latter yields the actually configured
> > hardware state, while the former yields the last state passed to
> > pwm_apply*() and so doesn't account for hardware specific rounding.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@baylibre.com>
> > ---
>
> One thing I am wondering about is how to implement rounding up instead
> of down. For example, I we need a >= 10 ns duty cycle.
>
> Here is my attempt...
>
> #define MIN_DUTY_NS 10
>
> int some_func(struct pwm_device *pwm)
> {
> struct pwm_waveform wf = {
> .period_length_ns = 400,
> };
> u64 trial_ns = MIN_DUTY_NS;
> int ret;
>
> do {
> wf.duty_length_ns = trial_ns;
>
> ret = pwm_round_waveform_might_sleep(pwm, &wf);
> if (ret < 0)
> return ret;
>
> /*
> * ret == 1 could be either because duty or period
> * is not attainable. In any case, we have to wait
> * until the last trial to rule out earlier trials
> * failing because of too small duty since we try
> * again with larger duty. Maybe this check isn't
> * needed though since pwm_round_waveform_might_sleep()
> * should fail when trial_ns > wf.period_length_ns?
> */
> if (ret == 1 && trial_ns == wf.period_length_ns)
> return -ERANGE;
>
> trial_ns++;
> } while (wf.duty_length_ns < MIN_DUTY_NS);
>
> return pwm_set_waveform_might_sleep(pwm, &wf, true);
> }
>
>
> 1. This seems like it would waste time trying each 1 ns increment
> compared to being able to tell the low level driver which way
> we want to round.
if you do
struct pwm_waveform wf = {
.period_length_ns = 400,
.duty_length_ns = 10;
};
ret = pwm_round_waveform_might_sleep(pwm, &wf);
and the hardware can do handle .period_length_ns = 400 (i.e. supports a
period less or equal to 400 ns), then wf.duty_length_ns holds a value
that can be implemented in combination with the returned
.period_length_ns.
This bit lacks documentation, but the two converted drivers have this
implemented.
And then the rounding up could be implemented more effectively:
If 10 doesn't work, duplicate the value to test in each step (i.e try:
10, 20, 40, 80 ...) and if say 80 is the first value that could be done
-- with say .duty_length_ns rounded down to 67 --, try 66 to check if
something else between 40 and 67 works. (You need to repeat that if 66
can be rounded down.) This is more complicated that your approach, but
should involve less calls to pwm_round_waveform_might_sleep() than your
approach for any actually existing hardware.)
> 2. Even with this, we could end up with an actual period of 9.5 ns
> which is < 10 ns but have to way to know since the returned value
> will be 10. Probably not likely that 0.5 ns is going to cause
> something to malfunction, but you never know.
The only thing that helps here is to increase the precision (i.e. make
the unit picosecond instead of nanoseconds). That's something I don't
want to do without deep thoughts and knowing exactly that the increased
precision is actually used.
The alternative to shift semantics and declare that .period_length_ns =
10 means [10,11) only shifts the problem (and makes the per driver
implementations more complicated).
> 3. Handling ret == 1 seems kind of messy since it could be caused
> by multiple different problems.
>
> Maybe we could consider including a rounding direction (up/down/closest)
> for each of the waveform parameters and pass that along to low level
> driver to avoid much of this? Or at least have these parameters for
> the high-level pwm_round_waveform_might_sleep() so each consumer doesn't
> have to try to figure out how to do the rounding right?
One thing that I want to prevent for sure is that each lowlevel driver
has to implement all possible rounding wishes individually. The
requirements are already complicated enough as is.
> > @@ -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
>
> It would be helpful to spell out in the description below that @wf will
> be modified upon non-error return and what the modified values will
> actually be. (Or refer to the other functions where the values are already
> documented to avoid duplication.)
ack, the rounding behaviour needs documentation.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (2 preceding siblings ...)
2024-09-06 15:42 ` [PATCH v4 3/7] pwm: Provide new consumer API functions for waveforms Uwe Kleine-König
@ 2024-09-06 15:43 ` Uwe Kleine-König
2024-09-06 22:26 ` David Lechner
2024-09-07 1:58 ` Kent Gibson
2024-09-06 15:43 ` [PATCH v4 5/7] pwm: Add tracing for waveform callbacks Uwe Kleine-König
` (4 subsequent siblings)
8 siblings, 2 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:43 UTC (permalink / raw)
To: linux-pwm; +Cc: Kent Gibson, David Lechner
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 | 267 ++++++++++++++++++++++++++++++++++++++-
include/linux/pwm.h | 3 +
include/uapi/linux/pwm.h | 24 ++++
3 files changed, 292 insertions(+), 2 deletions(-)
create mode 100644 include/uapi/linux/pwm.h
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index c7f39f9f4bcf..16615a4673ef 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>
@@ -1594,6 +1596,254 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
return true;
}
+struct pwm_cdev_data {
+ struct pwm_chip *chip;
+ struct pwm_device *pwm[];
+};
+
+static int pwm_cdev_open(struct inode *inode, struct file *file)
+{
+ struct pwm_chip *chip = container_of(inode->i_cdev, struct pwm_chip, cdev);
+ struct pwm_cdev_data *cdata;
+
+ guard(mutex)(&pwm_lock);
+
+ if (!chip->operational)
+ return -ENXIO;
+
+ cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
+ if (!cdata)
+ return -ENOMEM;
+
+ cdata->chip = chip;
+
+ file->private_data = cdata;
+
+ return nonseekable_open(inode, file);
+}
+
+static int pwm_cdev_release(struct inode *inode, struct file *file)
+{
+ struct pwm_cdev_data *cdata = file->private_data;
+ unsigned int i;
+
+ for (i = 0; i < cdata->chip->npwm; ++i) {
+ if (cdata->pwm[i])
+ pwm_put(cdata->pwm[i]);
+ }
+ kfree(cdata);
+
+ return 0;
+}
+
+static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+ struct pwm_chip *chip = cdata->chip;
+
+ if (hwpwm >= chip->npwm)
+ return -EINVAL;
+
+ if (!cdata->pwm[hwpwm]) {
+ struct pwm_device *pwm = &chip->pwms[hwpwm];
+ int ret;
+
+ ret = pwm_device_request(pwm, "pwm-cdev");
+ if (ret < 0)
+ return ret;
+
+ cdata->pwm[hwpwm] = pwm;
+ }
+
+ return 0;
+}
+
+static int pwm_cdev_free(struct pwm_cdev_data *cdata, unsigned int hwpwm)
+{
+ struct pwm_chip *chip = cdata->chip;
+
+ if (hwpwm >= chip->npwm)
+ return -EINVAL;
+
+ if (cdata->pwm[hwpwm]) {
+ struct pwm_device *pwm = cdata->pwm[hwpwm];
+
+ pwm_put(pwm);
+
+ cdata->pwm[hwpwm] = NULL;
+ }
+
+ return 0;
+}
+
+static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret = 0;
+ struct pwm_cdev_data *cdata = file->private_data;
+ struct pwm_chip *chip = cdata->chip;
+
+ guard(mutex)(&pwm_lock);
+
+ if (!chip->operational)
+ return -ENODEV;
+
+ switch (cmd) {
+ case PWM_IOCTL_REQUEST:
+ {
+ unsigned int hwpwm;
+
+ ret = get_user(hwpwm, (unsigned int __user *)arg);
+ if (ret)
+ return ret;
+
+ return pwm_cdev_request(cdata, hwpwm);
+ }
+ break;
+
+ case PWM_IOCTL_FREE:
+ {
+ unsigned int hwpwm;
+
+ ret = get_user(hwpwm, (unsigned int __user *)arg);
+ if (ret)
+ return ret;
+
+ return pwm_cdev_free(cdata, hwpwm);
+ }
+ break;
+
+ case PWM_IOCTL_ROUNDWF:
+ {
+ struct pwmchip_waveform cwf;
+ struct pwm_waveform wf;
+ struct pwm_device *pwm;
+
+ ret = copy_from_user(&cwf,
+ (struct pwmchip_waveform __user *)arg,
+ sizeof(cwf));
+ if (ret)
+ return -EFAULT;
+
+ if (cwf.__pad != 0)
+ return -EINVAL;
+
+ ret = pwm_cdev_request(cdata, cwf.hwpwm);
+ if (ret)
+ return ret;
+
+ pwm = cdata->pwm[cwf.hwpwm];
+
+ wf = (struct pwm_waveform) {
+ .period_length_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) {
+ pr_warn("huh\n");
+ return -EINVAL;
+ }
+
+ ret = pwm_cdev_request(cdata, cwf.hwpwm);
+ if (ret)
+ return ret;
+
+ pwm = cdata->pwm[cwf.hwpwm];
+
+ ret = pwm_get_waveform_might_sleep(pwm, &wf);
+ if (ret)
+ return ret;
+
+ cwf.period_length_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) {
+ pr_warn("huh\n");
+ return -EINVAL;
+ }
+
+ if (cwf.period_length_ns > 0 &&
+ (cwf.duty_length_ns > cwf.period_length_ns ||
+ cwf.duty_offset_ns >= cwf.period_length_ns))
+ return -EINVAL;
+
+ ret = pwm_cdev_request(cdata, cwf.hwpwm);
+ if (ret)
+ return ret;
+
+ pwm = cdata->pwm[cwf.hwpwm];
+
+ 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,
+ };
+
+ 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
@@ -1646,7 +1896,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;
@@ -1697,7 +1953,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);
@@ -2241,9 +2497,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 40cef0bc0de7..b12ca9531e32 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..ed127a80afbf
--- /dev/null
+++ b/include/uapi/linux/pwm.h
@@ -0,0 +1,24 @@
+/* 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 {
+ __u32 hwpwm;
+ __u32 __pad; /* padding, must be zero */
+ __u64 period_length_ns;
+ __u64 duty_length_ns;
+ __u64 duty_offset_ns;
+};
+
+#define PWM_IOCTL_REQUEST _IOW(0x75, 1, unsigned int)
+#define PWM_IOCTL_FREE _IOW(0x75, 2, unsigned int)
+#define PWM_IOCTL_ROUNDWF _IOWR(0x75, 3, struct pwmchip_waveform)
+#define PWM_IOCTL_GETWF _IOWR(0x75, 4, struct pwmchip_waveform)
+#define PWM_IOCTL_SETROUNDEDWF _IOW(0x75, 5, struct pwmchip_waveform)
+#define PWM_IOCTL_SETEXACTWF _IOW(0x75, 6, struct pwmchip_waveform)
+
+#endif /* _UAPI_PWM_H_ */
--
2.45.2
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-06 15:43 ` [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access Uwe Kleine-König
@ 2024-09-06 22:26 ` David Lechner
2024-09-08 14:59 ` Uwe Kleine-König
2024-09-07 1:58 ` Kent Gibson
1 sibling, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-09-06 22:26 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm; +Cc: Kent Gibson
On 9/6/24 10:43 AM, 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 | 267 ++++++++++++++++++++++++++++++++++++++-
> include/linux/pwm.h | 3 +
> include/uapi/linux/pwm.h | 24 ++++
> 3 files changed, 292 insertions(+), 2 deletions(-)
> create mode 100644 include/uapi/linux/pwm.h
>
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index c7f39f9f4bcf..16615a4673ef 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>
>
> @@ -1594,6 +1596,254 @@ static bool pwm_ops_check(const struct pwm_chip *chip)
> return true;
> }
>
> +struct pwm_cdev_data {
> + struct pwm_chip *chip;
> + struct pwm_device *pwm[];
> +};
> +
> +static int pwm_cdev_open(struct inode *inode, struct file *file)
> +{
> + struct pwm_chip *chip = container_of(inode->i_cdev, struct pwm_chip, cdev);
> + struct pwm_cdev_data *cdata;
> +
> + guard(mutex)(&pwm_lock);
> +
> + if (!chip->operational)
> + return -ENXIO;
> +
> + cdata = kzalloc(struct_size(cdata, pwm, chip->npwm), GFP_KERNEL);
> + if (!cdata)
> + return -ENOMEM;
> +
> + cdata->chip = chip;
> +
> + file->private_data = cdata;
> +
> + return nonseekable_open(inode, file);
> +}
> +
> +static int pwm_cdev_release(struct inode *inode, struct file *file)
> +{
> + struct pwm_cdev_data *cdata = file->private_data;
> + unsigned int i;
> +
> + for (i = 0; i < cdata->chip->npwm; ++i) {
> + if (cdata->pwm[i])
> + pwm_put(cdata->pwm[i]);
> + }
> + kfree(cdata);
> +
> + return 0;
> +}
> +
> +static int pwm_cdev_request(struct pwm_cdev_data *cdata, unsigned int hwpwm)
> +{
> + struct pwm_chip *chip = cdata->chip;
> +
> + if (hwpwm >= chip->npwm)
> + return -EINVAL;
> +
> + if (!cdata->pwm[hwpwm]) {
> + struct pwm_device *pwm = &chip->pwms[hwpwm];
> + int ret;
> +
> + ret = pwm_device_request(pwm, "pwm-cdev");
> + if (ret < 0)
> + return ret;
> +
> + cdata->pwm[hwpwm] = pwm;
> + }
> +
> + return 0;
> +}
> +
> +static int pwm_cdev_free(struct pwm_cdev_data *cdata, unsigned int hwpwm)
> +{
> + struct pwm_chip *chip = cdata->chip;
> +
> + if (hwpwm >= chip->npwm)
> + return -EINVAL;
> +
> + if (cdata->pwm[hwpwm]) {
> + struct pwm_device *pwm = cdata->pwm[hwpwm];
> +
> + pwm_put(pwm);
> +
> + cdata->pwm[hwpwm] = NULL;
> + }
> +
> + return 0;
> +}
Maybe a potential problem here with multiple requests.
Suppose an applications does:
// main thread
fd = open("/dev/pwm0", ...);
// start some threads
// thread A
ioctl(fd, PWM_IOCTL_REQUEST, 0);
// in kernel, pwm_device_request() is called and
// cdata->pwm[0] is assigned
// does some stuff - OK
// thread B
ioctl(fd, PWM_IOCTL_REQUEST, 0);
// succeeds since cdata->pwm[0] is assigned
// does some stuff - messes up thread A
// does some stuff - messes up thread B
// cleans up after itself
ioctl(fd, PWM_IOCTL_FREE, 0);
// pwm_put() is called and
// cdata->pwm[0] is set to NULL
// does some stuff - kernel has to call pwm_device_request()
// again, which may fail? e.g. if sysfs stole it in the meantime
// cleans up after itself
ioctl(fd, PWM_IOCTL_FREE, 0);
Maybe we should be more strict and only allow one requester at a time?
Or is it a documentation problem that this isn't the intended way to
use these ioctls?
Or maybe we just don't need the REQUEST and FREE ioctls?
To me, the most intuitive is that REQUEST would gain exclusive
access until FREE is called and other ioctls should fail if
you don't have exclusive access.
> +
> +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;
> +
> + ret = get_user(hwpwm, (unsigned int __user *)arg);
> + if (ret)
> + return ret;
> +
> + return pwm_cdev_request(cdata, hwpwm);
> + }
> + break;
> +
> + case PWM_IOCTL_FREE:
> + {
> + unsigned int hwpwm;
> +
> + ret = get_user(hwpwm, (unsigned int __user *)arg);
> + if (ret)
> + return ret;
> +
> + return pwm_cdev_free(cdata, hwpwm);
> + }
> + break;
> +
> + case PWM_IOCTL_ROUNDWF:
> + {
> + struct pwmchip_waveform cwf;
> + struct pwm_waveform wf;
> + struct pwm_device *pwm;
> +
> + ret = copy_from_user(&cwf,
> + (struct pwmchip_waveform __user *)arg,
> + sizeof(cwf));
> + if (ret)
> + return -EFAULT;
> +
> + if (cwf.__pad != 0)
> + return -EINVAL;
> +
> + ret = pwm_cdev_request(cdata, cwf.hwpwm);
> + if (ret)
> + return ret;
> +
> + pwm = cdata->pwm[cwf.hwpwm];
> +
> + wf = (struct pwm_waveform) {
> + .period_length_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) {
> + pr_warn("huh\n");
This is what I will say when I see this in the kernel log. :-p
> + return -EINVAL;
> + }
> +
> + ret = pwm_cdev_request(cdata, cwf.hwpwm);
> + if (ret)
> + return ret;
> +
> + pwm = cdata->pwm[cwf.hwpwm];
> +
> + ret = pwm_get_waveform_might_sleep(pwm, &wf);
> + if (ret)
> + return ret;
> +
> + cwf.period_length_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;
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-06 22:26 ` David Lechner
@ 2024-09-08 14:59 ` Uwe Kleine-König
2024-09-09 20:53 ` David Lechner
0 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-08 14:59 UTC (permalink / raw)
To: David Lechner; +Cc: linux-pwm, Kent Gibson
[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]
Hello David,
On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
> Maybe a potential problem here with multiple requests.
>
> Suppose an applications does:
>
> // main thread
> fd = open("/dev/pwm0", ...);
>
> // start some threads
>
> // thread A
>
> ioctl(fd, PWM_IOCTL_REQUEST, 0);
> // in kernel, pwm_device_request() is called and
> // cdata->pwm[0] is assigned
>
> // does some stuff - OK
>
> // thread B
>
> ioctl(fd, PWM_IOCTL_REQUEST, 0);
> // succeeds since cdata->pwm[0] is assigned
>
> // does some stuff - messes up thread A
>
> // does some stuff - messes up thread B
If two threads share a single fd for a given pwmchip char device, it's
in the responsibility of the application to not shoot in its own foot.
There are similar problems if two threads write to the same fd
corresponding to an ordinary file or directory. I think behaving
differently here isn't a good idea.
> // cleans up after itself
> ioctl(fd, PWM_IOCTL_FREE, 0);
> // pwm_put() is called and
> // cdata->pwm[0] is set to NULL
>
> // does some stuff - kernel has to call pwm_device_request()
> // again, which may fail? e.g. if sysfs stole it in the meantime
>
> // cleans up after itself
> ioctl(fd, PWM_IOCTL_FREE, 0);
>
> Maybe we should be more strict and only allow one requester at a time?
From the POV of the kernel code, there is only one requestor, identified
by the opened file. Handling that in a different way isn't a good idea I
think.
> Or maybe we just don't need the REQUEST and FREE ioctls?
The idea of the REQUEST ioctl is that an application can make sure it
can access all PWMs it needs before starting to change the
configuration.
> > + if (cwf.__pad != 0) {
> > + pr_warn("huh\n");
>
> This is what I will say when I see this in the kernel log. :-p
So the message seems to be chosen correctly :-)
Of course will drop that, thanks for finding that.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-08 14:59 ` Uwe Kleine-König
@ 2024-09-09 20:53 ` David Lechner
2024-09-17 17:10 ` Uwe Kleine-König
0 siblings, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-09-09 20:53 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: linux-pwm, Kent Gibson
On 9/8/24 9:59 AM, Uwe Kleine-König wrote:
> Hello David,
>
> On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
>> Maybe a potential problem here with multiple requests.
>>
>> Suppose an applications does:
>>
>> // main thread
>> fd = open("/dev/pwm0", ...);
>>
>> // start some threads
>>
>> // thread A
>>
>> ioctl(fd, PWM_IOCTL_REQUEST, 0);
>> // in kernel, pwm_device_request() is called and
>> // cdata->pwm[0] is assigned
>>
>> // does some stuff - OK
>>
>> // thread B
>>
>> ioctl(fd, PWM_IOCTL_REQUEST, 0);
>> // succeeds since cdata->pwm[0] is assigned
>>
>> // does some stuff - messes up thread A
>>
>> // does some stuff - messes up thread B
>
> If two threads share a single fd for a given pwmchip char device, it's
> in the responsibility of the application to not shoot in its own foot.
> There are similar problems if two threads write to the same fd
> corresponding to an ordinary file or directory. I think behaving
> differently here isn't a good idea.
Yes, applications should absolutely not be doing what I did in this
bad example. :-) So, that is why it would make more sense to me if a
second call of the REQUEST ioctl using the same fd would return an
error instead of succeeding without actually doing anything.
>
>> // cleans up after itself
>> ioctl(fd, PWM_IOCTL_FREE, 0);
>> // pwm_put() is called and
>> // cdata->pwm[0] is set to NULL
>>
>> // does some stuff - kernel has to call pwm_device_request()
>> // again, which may fail? e.g. if sysfs stole it in the meantime
>>
>> // cleans up after itself
>> ioctl(fd, PWM_IOCTL_FREE, 0);
>>
>> Maybe we should be more strict and only allow one requester at a time?
>
> From the POV of the kernel code, there is only one requestor, identified
> by the opened file. Handling that in a different way isn't a good idea I
> think.
>
>> Or maybe we just don't need the REQUEST and FREE ioctls?
>
> The idea of the REQUEST ioctl is that an application can make sure it
> can access all PWMs it needs before starting to change the
> configuration.
>
Ah, I had not considered that case.
But if it is required in some cases, I feel like it would be better
to just make it required in all cases. Otherwise, it feels like there
are too many ways to do the same thing if all of the other GET/SET
ioctls implicitly call the equivalent of REQUEST if it was not
explicitly called. It is one less decision to make for me as a user
if there is only one "right" way to use this interface.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-09 20:53 ` David Lechner
@ 2024-09-17 17:10 ` Uwe Kleine-König
2024-09-18 9:21 ` Uwe Kleine-König
0 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-17 17:10 UTC (permalink / raw)
To: David Lechner; +Cc: linux-pwm, Kent Gibson
[-- Attachment #1: Type: text/plain, Size: 3813 bytes --]
Hello David,
On Mon, Sep 09, 2024 at 03:53:11PM -0500, David Lechner wrote:
> On 9/8/24 9:59 AM, Uwe Kleine-König wrote:
> > On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
> >> Maybe a potential problem here with multiple requests.
> >>
> >> Suppose an applications does:
> >>
> >> // main thread
> >> fd = open("/dev/pwm0", ...);
> >>
> >> // start some threads
> >>
> >> // thread A
> >>
> >> ioctl(fd, PWM_IOCTL_REQUEST, 0);
> >> // in kernel, pwm_device_request() is called and
> >> // cdata->pwm[0] is assigned
> >>
> >> // does some stuff - OK
> >>
> >> // thread B
> >>
> >> ioctl(fd, PWM_IOCTL_REQUEST, 0);
> >> // succeeds since cdata->pwm[0] is assigned
> >>
> >> // does some stuff - messes up thread A
> >>
> >> // does some stuff - messes up thread B
> >
> > If two threads share a single fd for a given pwmchip char device, it's
> > in the responsibility of the application to not shoot in its own foot.
> > There are similar problems if two threads write to the same fd
> > corresponding to an ordinary file or directory. I think behaving
> > differently here isn't a good idea.
>
> Yes, applications should absolutely not be doing what I did in this
> bad example. :-) So, that is why it would make more sense to me if a
> second call of the REQUEST ioctl using the same fd would return an
> error instead of succeeding without actually doing anything.
Yeah, I understand what you mean. So the semantic isn't "take control
over the PWM line" but "ensure control over the PWM line". This also is
simpler to handle: Iff the request ioctl fails, you don't have control
over the PWM line. That's (I think) what an application cares about. It
wants to *have* control and doesn't care what is needed to reach that
state. And the request routine returning an error if *taking* control
fails because it was already taken before is an annoyance that makes
error handling more complicated.
But I agree and admit this is all very subjective.
> >> // cleans up after itself
> >> ioctl(fd, PWM_IOCTL_FREE, 0);
> >> // pwm_put() is called and
> >> // cdata->pwm[0] is set to NULL
> >>
> >> // does some stuff - kernel has to call pwm_device_request()
> >> // again, which may fail? e.g. if sysfs stole it in the meantime
> >>
> >> // cleans up after itself
> >> ioctl(fd, PWM_IOCTL_FREE, 0);
> >>
> >> Maybe we should be more strict and only allow one requester at a time?
> >
> > From the POV of the kernel code, there is only one requestor, identified
> > by the opened file. Handling that in a different way isn't a good idea I
> > think.
> >
> >> Or maybe we just don't need the REQUEST and FREE ioctls?
> >
> > The idea of the REQUEST ioctl is that an application can make sure it
> > can access all PWMs it needs before starting to change the
> > configuration.
> >
> Ah, I had not considered that case.
>
> But if it is required in some cases, I feel like it would be better
> to just make it required in all cases. Otherwise, it feels like there
> are too many ways to do the same thing if all of the other GET/SET
> ioctls implicitly call the equivalent of REQUEST if it was not
> explicitly called. It is one less decision to make for me as a user
> if there is only one "right" way to use this interface.
I'd hope you as a user would use libpwm or at least use it as a template
for your code. :-)
And to argue honestly: I see an advantage in keeping usage simple and so
I think it's nice that if an application doesn't care about requesting a
PWM line, it doesn't need to and the kernel just cares enough about
these details automatically to still make the operation safe.
But again, this is subjective and somewhat expected that tastes differ.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-17 17:10 ` Uwe Kleine-König
@ 2024-09-18 9:21 ` Uwe Kleine-König
0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-18 9:21 UTC (permalink / raw)
To: David Lechner; +Cc: linux-pwm, Kent Gibson
On Tue, Sep 17, 2024 at 07:10:54PM +0200, Uwe Kleine-König wrote:
> Hello David,
>
> On Mon, Sep 09, 2024 at 03:53:11PM -0500, David Lechner wrote:
> > On 9/8/24 9:59 AM, Uwe Kleine-König wrote:
> > > On Fri, Sep 06, 2024 at 05:26:46PM -0500, David Lechner wrote:
> > > > Or maybe we just don't need the REQUEST and FREE ioctls?
> > >
> > > The idea of the REQUEST ioctl is that an application can make sure it
> > > can access all PWMs it needs before starting to change the
> > > configuration.
> >
> > Ah, I had not considered that case.
> >
> > But if it is required in some cases, I feel like it would be better
> > to just make it required in all cases. Otherwise, it feels like there
> > are too many ways to do the same thing if all of the other GET/SET
> > ioctls implicitly call the equivalent of REQUEST if it was not
> > explicitly called. It is one less decision to make for me as a user
> > if there is only one "right" way to use this interface.
>
> I'd hope you as a user would use libpwm or at least use it as a template
> for your code. :-)
>
> And to argue honestly: I see an advantage in keeping usage simple and so
> I think it's nice that if an application doesn't care about requesting a
> PWM line, it doesn't need to and the kernel just cares enough about
> these details automatically to still make the operation safe.
>
> But again, this is subjective and somewhat expected that tastes differ.
While working on v5 of this series (and still having your concern in
mind) I found a good argument to make the REQUEST ioctl explicitly
necessary: In v4, if a call to say SETEXACTWF fails after the pwm_device
was auto-requested the error of pwm_set_waveform_might_sleep() was just
forwarded to userspace without releasing the pwm_device. That semantic
is ugly: While the ioctl failed, still something was changed (i.e. the
pwm_device was requested). The obvious fix is to auto-free the
pwm_device again if pwm_set_waveform_might_sleep() failed. But the
semantic then is still not so nice IMHO. Then SETEXACTWF can fail for
two different sets of reasons: a) requesting failed, and b) programming
the hardware failed. A userspace program has to act differently on these
two error classes, so it cannot be unaware of the whole request
complication. This defeats the idea to make the API simple. From the
userspace program's point of view it then is sensible to explicitly
request the pwm_device to have a simple means to distinguish the two
error classes.
That together with your reasoning that TMTOWTDI isn't usually good for
kernel interfaces convinces me that making the REQUEST ioctl obligatory
is the right way to move forward. So that's what I will do in v5.
Thanks for your input
Uwe
PS: I forgot my crypto token at home, so this mail is unsigned.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access
2024-09-06 15:43 ` [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access Uwe Kleine-König
2024-09-06 22:26 ` David Lechner
@ 2024-09-07 1:58 ` Kent Gibson
1 sibling, 0 replies; 24+ messages in thread
From: Kent Gibson @ 2024-09-07 1:58 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: linux-pwm, David Lechner
On Fri, Sep 06, 2024 at 05:43:00PM +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>
> ---
> +++ b/include/uapi/linux/pwm.h
> @@ -0,0 +1,24 @@
> +/* 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 {
> + __u32 hwpwm;
> + __u32 __pad; /* padding, must be zero */
> + __u64 period_length_ns;
> + __u64 duty_length_ns;
> + __u64 duty_offset_ns;
> +};
> +
Some documentation?
> +#define PWM_IOCTL_REQUEST _IOW(0x75, 1, unsigned int)
> +#define PWM_IOCTL_FREE _IOW(0x75, 2, unsigned int)
> +#define PWM_IOCTL_ROUNDWF _IOWR(0x75, 3, struct pwmchip_waveform)
> +#define PWM_IOCTL_GETWF _IOWR(0x75, 4, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETROUNDEDWF _IOW(0x75, 5, struct pwmchip_waveform)
> +#define PWM_IOCTL_SETEXACTWF _IOW(0x75, 6, struct pwmchip_waveform)
> +
> +#endif /* _UAPI_PWM_H_ */
> --
> 2.45.2
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 5/7] pwm: Add tracing for waveform callbacks
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (3 preceding siblings ...)
2024-09-06 15:43 ` [PATCH v4 4/7] pwm: Add support for pwmchip devices for faster and easier userspace access Uwe Kleine-König
@ 2024-09-06 15:43 ` Uwe Kleine-König
2024-09-06 15:43 ` [PATCH v4 6/7] pwm: axi-pwmgen: Implementation of the " Uwe Kleine-König
` (3 subsequent siblings)
8 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:43 UTC (permalink / raw)
To: linux-pwm
Cc: Steven Rostedt, Masami Hiramatsu, Mathieu Desnoyers,
linux-trace-kernel, Kent Gibson, David Lechner
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 16615a4673ef..7bc536b98d18 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -166,30 +166,46 @@ static int __pwm_round_waveform_tohw(struct pwm_chip *chip, struct pwm_device *p
const struct pwm_waveform *wf, void *wfhw)
{
const struct pwm_ops *ops = chip->ops;
+ int ret;
- return ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+ ret = ops->round_waveform_tohw(chip, pwm, wf, wfhw);
+ trace_pwm_round_waveform_tohw(pwm, wf, wfhw, ret);
+
+ return ret;
}
static int __pwm_round_waveform_fromhw(struct pwm_chip *chip, struct pwm_device *pwm,
const void *wfhw, struct pwm_waveform *wf)
{
const struct pwm_ops *ops = chip->ops;
+ int ret;
- return ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+ ret = ops->round_waveform_fromhw(chip, pwm, wfhw, wf);
+ trace_pwm_round_waveform_fromhw(pwm, wfhw, wf, ret);
+
+ return ret;
}
static int __pwm_read_waveform(struct pwm_chip *chip, struct pwm_device *pwm, void *wfhw)
{
const struct pwm_ops *ops = chip->ops;
+ int ret;
- return ops->read_waveform(chip, pwm, wfhw);
+ ret = ops->read_waveform(chip, pwm, wfhw);
+ trace_pwm_read_waveform(pwm, wfhw, ret);
+
+ return ret;
}
static int __pwm_write_waveform(struct pwm_chip *chip, struct pwm_device *pwm, const void *wfhw)
{
const struct pwm_ops *ops = chip->ops;
+ int ret;
- return ops->write_waveform(chip, pwm, wfhw);
+ ret = ops->write_waveform(chip, pwm, wfhw);
+ trace_pwm_write_waveform(pwm, wfhw, ret);
+
+ return ret;
}
#define WFHWSIZE 20
diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
index 8022701c446d..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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [PATCH v4 6/7] pwm: axi-pwmgen: Implementation of the waveform callbacks
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (4 preceding siblings ...)
2024-09-06 15:43 ` [PATCH v4 5/7] pwm: Add tracing for waveform callbacks Uwe Kleine-König
@ 2024-09-06 15:43 ` Uwe Kleine-König
2024-09-06 17:44 ` Trevor Gamblin
2024-09-06 15:43 ` [PATCH v4 7/7] pwm: stm32: " Uwe Kleine-König
` (2 subsequent siblings)
8 siblings, 1 reply; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:43 UTC (permalink / raw)
To: linux-pwm
Cc: Michael Hennerich, Trevor Gamblin, Nuno Sá, Kent Gibson,
David Lechner
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 3ad60edf20a5..14c3274b551b 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -23,6 +23,7 @@
#include <linux/err.h>
#include <linux/fpga/adi-axi-common.h>
#include <linux/io.h>
+#include <linux/minmax.h>
#include <linux/module.h>
#include <linux/platform_device.h>
#include <linux/pwm.h>
@@ -54,81 +55,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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 6/7] pwm: axi-pwmgen: Implementation of the waveform callbacks
2024-09-06 15:43 ` [PATCH v4 6/7] pwm: axi-pwmgen: Implementation of the " Uwe Kleine-König
@ 2024-09-06 17:44 ` Trevor Gamblin
0 siblings, 0 replies; 24+ messages in thread
From: Trevor Gamblin @ 2024-09-06 17:44 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm
Cc: Michael Hennerich, Nuno Sá, Kent Gibson, David Lechner
On 2024-09-06 11:43, 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 3ad60edf20a5..14c3274b551b 100644
> --- a/drivers/pwm/pwm-axi-pwmgen.c
> +++ b/drivers/pwm/pwm-axi-pwmgen.c
> @@ -23,6 +23,7 @@
> #include <linux/err.h>
> #include <linux/fpga/adi-axi-common.h>
> #include <linux/io.h>
> +#include <linux/minmax.h>
> #include <linux/module.h>
> #include <linux/platform_device.h>
> #include <linux/pwm.h>
> @@ -54,81 +55,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)
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH v4 7/7] pwm: stm32: Implementation of the waveform callbacks
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (5 preceding siblings ...)
2024-09-06 15:43 ` [PATCH v4 6/7] pwm: axi-pwmgen: Implementation of the " Uwe Kleine-König
@ 2024-09-06 15:43 ` Uwe Kleine-König
2024-09-06 18:08 ` [PATCH v4 0/7] pwm: New abstraction and userspace API Trevor Gamblin
2024-09-06 19:06 ` David Lechner
8 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-06 15:43 UTC (permalink / raw)
To: linux-pwm
Cc: Fabrice Gasnier, Maxime Coquelin, Alexandre Torgue, linux-stm32,
Kent Gibson, David Lechner
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 f85eb41cb084..166f080706da 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
^ permalink raw reply related [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/7] pwm: New abstraction and userspace API
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (6 preceding siblings ...)
2024-09-06 15:43 ` [PATCH v4 7/7] pwm: stm32: " Uwe Kleine-König
@ 2024-09-06 18:08 ` Trevor Gamblin
2024-09-06 19:06 ` David Lechner
8 siblings, 0 replies; 24+ messages in thread
From: Trevor Gamblin @ 2024-09-06 18:08 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm
Cc: Fabrice Gasnier, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, linux-trace-kernel, Michael Hennerich,
Nuno Sá, Maxime Coquelin, Alexandre Torgue, linux-stm32,
Kent Gibson, David Lechner
On 2024-09-06 11:42, Uwe Kleine-König wrote:
> Hello,
>
> here comes v4 of the series to add support for duty offset in PWM
> waveforms. For a single PWM output there is no gain, but if you have a
> chip with two (or more) outputs and both operate with the same period,
> you can generate an output like:
>
> ______ ______ ______ ______
> PWM #0 ___/ \_______/ \_______/ \_______/ \_______
> __ __ __ __
> PWM #1 _____/ \___________/ \___________/ \___________/ \_________
> ^ ^ ^ ^
>
> The opportunity for a new abstraction is/should be used to also improve
> precision of the API functions, which implies that the rules for
> lowlevel drivers are more strict for the new callbacks.
>
> Changes since v3, which is available from
> https://lore.kernel.org/linux-pwm/cover.1722261050.git.u.kleine-koenig@baylibre.com/
> include:
>
> - Drop PWM_IOCTL_GET_NUM_PWMS ioctl (patch #4), suggestion by David
> Lechner
>
> - Define members of userspace API struct using __u32 instead of
> unsigned int; thanks to Kent Gibson for the suggestion (patch #4)
>
> - Ensure that pwm_apply_state_{atomic,might_sleep}() don't return 1
> Noticed by Fabrice Gasnier
>
> - Rebased to current pwm/for-next.
> (fixing a missing +1 noticed by Fabrice)
>
> - Dropped Tested-by: from Trevor
> While the axi-pwmgen driver didn't change, there were considerable
> changes in the core. So I dropped it.
>
> - add some missing EXPORT_SYMBOL_GPL for the new API functions
>
> - Add kernel doc comments for the new API functions
>
> - debug message in stm32_pwm_round_waveform_fromhw (another suggestion
> by Fabrice)
>
> I also updated the branch pwm/chardev in
> https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/linux.git to
> match this v4.
>
> I'm aware of two issues in this series:
>
> - Triggers a lockdep warning in the pwm-meson driver. This affects the
> new pwm locking vs the clk framework's prepare lock. I think this is
> a false positive and to fix it, only changes in the clk framework are
> necessary.
>
> - The functions pwm_set_waveform_might_sleep() and
> pwm_round_waveform_might_sleep() have an unusual return value
> convention: They return 0 on success, 1 if the requested waveform
> cannot be implemented without breaking the rounding rules, or a
> negative errno if an error occurs.
> Fabrice rightfully pointed out this to be surprised by this and
> suggested to use at least a define for it.
>
> I couldn't find a decision that I'm entirely happy with here. My
> conflicts are:
>
> - I want a constant that now and in the future only means "cannot be
> done due to the rounding rules in the pwm framework". So the
> options are:
> * Introduce a new ESOMETHING and return -ESOMETHING
> I think that's hard to motivate and also myself doubt this
> would be sensible. As below, the question for a good name is
> unresolved.
> * return 1
> This is what was done in the earlier revisions and also here.
>
> - When keeping the return 1 semantics (and also for a new
> ESOMETHING):
> I agree that a name instead of a plain 1 would be nice, but I
> couldn't come up with a name I liked. Given that this can be
> introduced later without breaking anything, I don't consider that
> very urgent.
> My candidates were PWM_REQUIRES_BROKEN_ROUNDING,
> PWM_REQUIRES_FORBIDDEN_ROUNDING and PWM_ERR_ROUNDING.
> These are too long or/and imprecise.
> If you have a good idea, please tell.
PWM_ERR_ROUNDING doesn't seem too bad.
What about something like PWM_REQ_INVALID, PWM_WF_INVALID, or
PWM_WF_REQ_INVALID?
>
> Still I'd like to get that forward and (unless a new and relevant issue
> pops up until then) intend to put it into next after the next merge
> window. Nevertheless I'm open for suggestions to further improve this
> series.
>
> Best regards
> Uwe
>
> Uwe Kleine-König (7):
> pwm: Add more locking
> pwm: New abstraction for PWM waveforms
> pwm: Provide new consumer API functions for waveforms
> pwm: Add support for pwmchip devices for faster and easier userspace
> access
> pwm: Add tracing for waveform callbacks
> pwm: axi-pwmgen: Implementation of the waveform callbacks
> pwm: stm32: Implementation of the waveform callbacks
>
> drivers/pwm/core.c | 867 +++++++++++++++++++++++++++++++++--
> 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 | 24 +
> 6 files changed, 1545 insertions(+), 304 deletions(-)
> create mode 100644 include/uapi/linux/pwm.h
>
> base-commit: cf6631b07b907d4c644ca42f7cc234e7149290a2
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/7] pwm: New abstraction and userspace API
2024-09-06 15:42 [PATCH v4 0/7] pwm: New abstraction and userspace API Uwe Kleine-König
` (7 preceding siblings ...)
2024-09-06 18:08 ` [PATCH v4 0/7] pwm: New abstraction and userspace API Trevor Gamblin
@ 2024-09-06 19:06 ` David Lechner
2024-09-08 11:32 ` Uwe Kleine-König
8 siblings, 1 reply; 24+ messages in thread
From: David Lechner @ 2024-09-06 19:06 UTC (permalink / raw)
To: Uwe Kleine-König, linux-pwm
Cc: Fabrice Gasnier, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, linux-trace-kernel, Michael Hennerich,
Trevor Gamblin, Nuno Sá, Maxime Coquelin, Alexandre Torgue,
linux-stm32, Kent Gibson
On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> Hello,
>
> here comes v4 of the series to add support for duty offset in PWM
> waveforms. For a single PWM output there is no gain, but if you have a
> chip with two (or more) outputs and both operate with the same period,
> you can generate an output like:
>
> ______ ______ ______ ______
> PWM #0 ___/ \_______/ \_______/ \_______/ \_______
> __ __ __ __
> PWM #1 _____/ \___________/ \___________/ \___________/ \_________
> ^ ^ ^ ^
>
While working on an ADC driver that uses these new waveform APIs, we came
across a case where we wanted wf->duty_offset_ns >= wf->period_length_ns,
which is currently not allowed. [1]
______ ______ ______ ______
PWM #0 ___/ \_______/ \_______/ \_______/ \_______
__ __ __
PWM #1 __________________/ \___________/ \___________/ \___________
^ ^ ^
We worked around it by setting:
wf->duty_offset_ns = DESIRED_NS % wf->period_length_ns
Having PWM #1 trigger too early just causes the first sample data
read to be invalid data.
But even if we allowed wf->duty_offset_ns >= wf->period_length_ns,
this offset wouldn't matter because there currently isn't a way to
enable two PWM outputs at exactly the same time.
In the ADC application we work around both of these shortcomings by not
enabling the DMA that is triggered by PWM #1 until after both PWMs are
enabled. However, there may be similar applications in the future that
also need such an offset and synchronized enable that might not be so
easy to work around, so something to keep in mind.
[1]: https://lore.kernel.org/linux-iio/20240904-ad7625_r1-v4-2-78bc7dfb2b35@baylibre.com/
>
> - The functions pwm_set_waveform_might_sleep() and
> pwm_round_waveform_might_sleep() have an unusual return value
> convention: They return 0 on success, 1 if the requested waveform
> cannot be implemented without breaking the rounding rules, or a
> negative errno if an error occurs.
> Fabrice rightfully pointed out this to be surprised by this and
> suggested to use at least a define for it.
>
> I couldn't find a decision that I'm entirely happy with here. My
> conflicts are:
>
> - I want a constant that now and in the future only means "cannot be
> done due to the rounding rules in the pwm framework". So the
> options are:
> * Introduce a new ESOMETHING and return -ESOMETHING
> I think that's hard to motivate and also myself doubt this
> would be sensible. As below, the question for a good name is
> unresolved.
> * return 1
> This is what was done in the earlier revisions and also here.
>
> - When keeping the return 1 semantics (and also for a new
> ESOMETHING):
> I agree that a name instead of a plain 1 would be nice, but I
> couldn't come up with a name I liked. Given that this can be
> introduced later without breaking anything, I don't consider that
> very urgent.
> My candidates were PWM_REQUIRES_BROKEN_ROUNDING,
> PWM_REQUIRES_FORBIDDEN_ROUNDING and PWM_ERR_ROUNDING.
> These are too long or/and imprecise.
> If you have a good idea, please tell.
To avoid using the return value for status flags, we could introduce
an optional output parameter. Consumers where best effort is good
enough can just pass NULL and consumers that care can pass an unsigned
int to receive the status flag. This could even be a bitmap of multiple
flags if it would be useful to know which rule(s) could not be met.
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH v4 0/7] pwm: New abstraction and userspace API
2024-09-06 19:06 ` David Lechner
@ 2024-09-08 11:32 ` Uwe Kleine-König
0 siblings, 0 replies; 24+ messages in thread
From: Uwe Kleine-König @ 2024-09-08 11:32 UTC (permalink / raw)
To: David Lechner
Cc: linux-pwm, Fabrice Gasnier, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, linux-trace-kernel, Michael Hennerich,
Trevor Gamblin, Nuno Sá, Maxime Coquelin, Alexandre Torgue,
linux-stm32, Kent Gibson
[-- Attachment #1: Type: text/plain, Size: 5808 bytes --]
Hello David,
On Fri, Sep 06, 2024 at 02:06:18PM -0500, David Lechner wrote:
> On 9/6/24 10:42 AM, Uwe Kleine-König wrote:
> > Hello,
> >
> > here comes v4 of the series to add support for duty offset in PWM
> > waveforms. For a single PWM output there is no gain, but if you have a
> > chip with two (or more) outputs and both operate with the same period,
> > you can generate an output like:
> >
> > ______ ______ ______ ______
> > PWM #0 ___/ \_______/ \_______/ \_______/ \_______
> > __ __ __ __
> > PWM #1 _____/ \___________/ \___________/ \___________/ \_________
> > ^ ^ ^ ^
> >
>
> While working on an ADC driver that uses these new waveform APIs, we came
> across a case where we wanted wf->duty_offset_ns >= wf->period_length_ns,
> which is currently not allowed. [1]
>
> ______ ______ ______ ______
> PWM #0 ___/ \_______/ \_______/ \_______/ \_______
> __ __ __
> PWM #1 __________________/ \___________/ \___________/ \___________
> ^ ^ ^
I restricted to waveforms with .duty_offset_ns < .period_length_ns
because the signal you want is only periodic once PWM #1 begins to
toggle. Given that the pwm subsystem is about periodic signals and has a
wide range of behaviours at the moments the configuration is changed, I
think it's little sensible today to consider reliably implementing
offsets bigger than the period length.
Does your hardware really behave like that?
> We worked around it by setting:
>
> wf->duty_offset_ns = DESIRED_NS % wf->period_length_ns
>
> Having PWM #1 trigger too early just causes the first sample data
> read to be invalid data.
>
> But even if we allowed wf->duty_offset_ns >= wf->period_length_ns,
> this offset wouldn't matter because there currently isn't a way to
> enable two PWM outputs at exactly the same time.
Yup, that's another challenge. Up to now it's even special knowledge
about the used pwm chip that with configuring two pwms with the same
period, the period starts are synced and you don't get:
______ ______ ______ ______
PWM #0 _____/ \_______/ \_______/ \_______/ \_______
^ ^ ^ ^
__ __ __ __
PWM #1 ___/ \___________/ \___________/ \___________/ \___________
^ ^ ^ ^
> > - The functions pwm_set_waveform_might_sleep() and
> > pwm_round_waveform_might_sleep() have an unusual return value
> > convention: They return 0 on success, 1 if the requested waveform
> > cannot be implemented without breaking the rounding rules, or a
> > negative errno if an error occurs.
> > Fabrice rightfully pointed out this to be surprised by this and
> > suggested to use at least a define for it.
> >
> > I couldn't find a decision that I'm entirely happy with here. My
> > conflicts are:
> >
> > - I want a constant that now and in the future only means "cannot be
> > done due to the rounding rules in the pwm framework". So the
> > options are:
> > * Introduce a new ESOMETHING and return -ESOMETHING
> > I think that's hard to motivate and also myself doubt this
> > would be sensible. As below, the question for a good name is
> > unresolved.
> > * return 1
> > This is what was done in the earlier revisions and also here.
> >
> > - When keeping the return 1 semantics (and also for a new
> > ESOMETHING):
> > I agree that a name instead of a plain 1 would be nice, but I
> > couldn't come up with a name I liked. Given that this can be
> > introduced later without breaking anything, I don't consider that
> > very urgent.
> > My candidates were PWM_REQUIRES_BROKEN_ROUNDING,
> > PWM_REQUIRES_FORBIDDEN_ROUNDING and PWM_ERR_ROUNDING.
> > These are too long or/and imprecise.
> > If you have a good idea, please tell.
>
> To avoid using the return value for status flags, we could introduce
> an optional output parameter. Consumers where best effort is good
> enough can just pass NULL and consumers that care can pass an unsigned
> int to receive the status flag. This could even be a bitmap of multiple
> flags if it would be useful to know which rule(s) could not be met.
Which rule couldn't be met is obvious when you look at the resulting
waveform because the lowlevel driver is supposed to give you the
smallest possible value for the relevant parameter if rounding down
doesn't work.
So if you request
.period_length_ns = 3000
.duty_length_ns = 2
.duty_offset_ns = 10
and your hardware can do 3000 ns period but the smallest duty_length is
10, it is supposed to write
.period_length_ns = 3000
.duty_length_ns = 10
.duty_offset_ns = 10
in the waveform parameter and return 1.
My intuitive reaction is that (another) output parameter is worse than
the return value semantics I came up with. After some more thought I
wonder if the wish to have something PWM specific is the problem, and
just picking one of the available error constants (ERANGE?) is the nice
way out. Alternatively return 0 in this case and let the caller work out
themselves that not all values were rounded down?!
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 24+ messages in thread