diff mbox series

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

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

Commit Message

Uwe Kleine-König July 15, 2024, 11:16 a.m. UTC
Up to now the configuration of a PWM setting is described exclusively by
a struct pwm_state which contains information about period, duty_cycle,
polarity and if the PWM is enabled. (There is another member usage_power
which doesn't completely fit into pwm_state, I ignore it here for
simplicity.)

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

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

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

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

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

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

Comments

David Lechner July 15, 2024, 6:55 p.m. UTC | #1
On 7/15/24 6:16 AM, Uwe Kleine-König wrote:


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

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


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

Perhaps it would be helpful to take a hint from the IIO subsystem
and include the units of measurement in the field names here?
For example, period_length_ns or even just period_ns. This way,
the value is obvious even without reading the documentation.
Uwe Kleine-König July 15, 2024, 8:17 p.m. UTC | #2
Hello,

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

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

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

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

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

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

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

- Nuno Sá
diff mbox series

Patch

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