Message-ID: <73y7ovftjv35gw3sjeu3jisg7feplhyebmcnldqvszuofqnn7q@eh4lyicuhfmq>
Date: Wed, 22 May 2024 17:53:53 +0200
From: Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To: Trevor Gamblin <tgamblin@...libre.com>
Cc: linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	michael.hennerich@...log.com, nuno.sa@...log.com, dlechner@...libre.com
Subject: Re: [PATCH 1/2 v3] pwm: add duty offset support

Hello Trevor,

On Tue, May 21, 2024 at 03:49:15PM -0400, Trevor Gamblin wrote:
> Some PWM chips support a "phase" or "duty_offset" feature. This patch
> continues adding support for configuring this property in the PWM
> subsystem.
> 
> Functions duty_offset_show(), duty_offset_store(), and
> pwm_get_duty_offset() are added to match what exists for duty_cycle.
> 
> Add a check to disallow applying a state with both inversed polarity and
> a nonzero duty_offset.
> 
> Also add duty_offset to TP_printk in include/trace/events/pwm.h so that
> it is reported with other properties when using the event tracing pipe
> for debug.
> 
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
> ---
> v3 changes:
> * rebased on top of latest pwm/for-next
> * removed changes related to cdev to match current pwm tree
> * fixed minor whitespace issue caught by checkpatch
> 
> v2 changes:
> * Address feedback for driver in v1:
>   * Remove line setting supports_offset flag in pwm_chip, since that has
>     been removed from the struct in core.c.
> 
> ---
>  drivers/pwm/core.c         | 79 +++++++++++++++++++++++++++++++++++---
>  include/linux/pwm.h        | 15 ++++++++
>  include/trace/events/pwm.h |  6 ++-
>  3 files changed, 93 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 18574857641e..2ebfc7f3de8a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -62,6 +62,7 @@ static void pwm_apply_debug(struct pwm_device *pwm,
>  	 */
>  	if (s1.enabled && s1.polarity != state->polarity) {
>  		s2.polarity = state->polarity;
> +		s2.duty_offset = s1.duty_cycle;

s/duty_cycle/duty_offset/

>  		s2.duty_cycle = s1.period - s1.duty_cycle;
>  		s2.period = s1.period;
>  		s2.enabled = s1.enabled;
> @@ -103,6 +104,23 @@ static void pwm_apply_debug(struct pwm_device *pwm,
>  			 state->duty_cycle, state->period,
>  			 s2.duty_cycle, s2.period);
>  
> +	if (state->enabled &&
> +	    last->polarity == state->polarity &&
> +	    last->period == s2.period &&
> +	    last->duty_offset > s2.duty_offset &&
> +	    last->duty_offset <= state->duty_offset)
> +		dev_warn(pwmchip_parent(chip),
> +			 ".apply didn't pick the best available duty offset (requested: %llu/%llu, applied: %llu/%llu, possible: %llu/%llu)\n",
> +			 state->duty_offset, state->period,

Does it make sense to emit $duty_offset/$period here? Establishing a
consistent way to write this would be nice. Something like:

	$duty_cycle/$period [+$duty_offset]

maybe?

> +			 s2.duty_offset, s2.period,
> +			 last->duty_offset, last->period);
> +
> +	if (state->enabled && state->duty_offset < s2.duty_offset)
> +		dev_warn(pwmchip_parent(chip),
> +			 ".apply is supposed to round down duty_offset (requested: %llu/%llu, applied: %llu/%llu)\n",
> +			 state->duty_offset, state->period,
> +			 s2.duty_offset, s2.period);
> +
>  	if (!state->enabled && s2.enabled && s2.duty_cycle > 0)
>  		dev_warn(pwmchip_parent(chip),
>  			 "requested disabled, but yielded enabled with duty > 0\n");
> @@ -126,12 +144,13 @@ static void pwm_apply_debug(struct pwm_device *pwm,
>  	if (s1.enabled != last->enabled ||
>  	    s1.polarity != last->polarity ||
>  	    (s1.enabled && s1.period != last->period) ||
> +	    (s1.enabled && s1.duty_offset != last->duty_offset) ||
>  	    (s1.enabled && s1.duty_cycle != last->duty_cycle)) {
>  		dev_err(pwmchip_parent(chip),
> -			".apply is not idempotent (ena=%d pol=%d %llu/%llu) -> (ena=%d pol=%d %llu/%llu)\n",
> +			".apply is not idempotent (ena=%d pol=%d %llu/%llu/%llu) -> (ena=%d pol=%d %llu/%llu/%llu)\n",
>  			s1.enabled, s1.polarity, s1.duty_cycle, s1.period,
> -			last->enabled, last->polarity, last->duty_cycle,
> -			last->period);
> +			s1.duty_offset, last->enabled, last->polarity,
> +			last->duty_cycle, last->period, last->duty_offset);
>  	}
>  }
>  
> @@ -146,13 +165,24 @@ static int __pwm_apply(struct pwm_device *pwm, const struct pwm_state *state)
>  	int err;
>  
>  	if (!pwm || !state || !state->period ||
> -	    state->duty_cycle > state->period)
> +	    state->duty_cycle > state->period ||
> +	    state->duty_offset > state->period)
>  		return -EINVAL;
>  
>  	chip = pwm->chip;
>  
> +	/*
> +	 * There is no need to set duty_offset with inverse polarity,
> +	 * since signals with duty_offset values greater than 0.5 *
> +	 * period can equivalently be represented by an inverted signal
> +	 * without offset.

This isn't exact. The equation is:

	state_with_offset.period = inverted_state.period
	state_with_offset.duty_cycle = inverted_state.period - inverted_state.duty_cycle
	state_with_offset.duty_offset = inverted_state.duty_cycle

And with duty_offset you can express more wave-forms than with
inversion.

> +	 */
> +	if (state->polarity == PWM_POLARITY_INVERSED && state->duty_offset)
> +		return -EINVAL;
> +
>  	if (state->period == pwm->state.period &&
>  	    state->duty_cycle == pwm->state.duty_cycle &&
> +	    state->duty_offset == pwm->state.duty_offset &&
>  	    state->polarity == pwm->state.polarity &&
>  	    state->enabled == pwm->state.enabled &&
>  	    state->usage_power == pwm->state.usage_power)

While I like the added expressiveness of having .duty_offset, I think we
shouldn't let the low-level drivers face both .duty_offset and
.polarity.

I suggest to add a new callback similar to .apply that gets passed a
variant of pwm_state that only has

	u64 period
	u64 duty_cycle
	u64 duty_offset

period = 0 then signals disable. Implementers are then supposed to first
round down period to a possible period (> 0), then duty_cycle to a
possible duty_cycle for the picked period and then duty_offset to a
possible duty_offset with the picked period and duty_cycle.

Then there is a single code location that handles the translation
between state with polarity and state with duty_offset in the core,
instead of case switching in each lowlevel driver. And I wouldn't
add .duty_offset to the sysfs interface, to lure people into using the
character device support which has several advantages over the sysfs
API. (One reasonable justification is that we cannot remove polarity
there, and we also cannot report polarity = normal + some duty_offset
without possibly breaking assumptions in sysfs users.)

What is missing in my plan is a nice name for the new struct pwm_state
and the .apply() (and matching .get_state()) callback. Maybe pwm_nstate,
.apply_nstate() and .get_nstate()? n for "new", which however won't age
nicely. Maybe it also makes sense to add a .round_nstate() in the same
go. We'd have to touch all drivers anyhow and implementing a rounding
callback (that has similar semantics to clk_round_rate() for clocks,
i.e. it reports what would be configured for a given (n)state) isn't
much more work. With rounding in place we could also request that
.apply_nstate() only succeeds if rounding down decrements the values by
less than 1, which gives still more control. (The more lax variant can
then be implemented by first passing an nstate to .round_nstate and then
.apply_nstate that one.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists