diff mbox series

[v4,3/7] pwm: Provide new consumer API functions for waveforms

Message ID 857ae911c23a474e6de4a1ba0b224bc8d982d624.1725635013.git.u.kleine-koenig@baylibre.com
State Superseded
Headers show
Series pwm: New abstraction and userspace API | expand

Commit Message

Uwe Kleine-König Sept. 6, 2024, 3:42 p.m. UTC 
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(-)

Comments

David Lechner Sept. 6, 2024, 9:22 p.m. UTC | #1 
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)
> +{
Uwe Kleine-König Sept. 8, 2024, 12:01 p.m. UTC | #2 
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
diff mbox series

Patch

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);