[PATCH 0/2 v3] pwm: add support for duty_offset

[PATCH 0/2 v3] pwm: add support for duty_offset

[Trevor Gamblin]

This series extends the PWM subsystem to support the duty_offset feature
found on some PWM devices. It includes a patch to enable this feature
for the axi-pwmgen driver, which can also serve as an example of how to
implement it for other devices.

The series was tested on actual hardware using a Zedboard. An
oscilloscope was used to validate that the generated PWM signals matched
the requested ones. 

Note that in addition to the other patches in this series, the changes
to the axi-pwmgen driver rely on [1], which hasn't been picked up yet.

[1] https://lore.kernel.org/linux-pwm/20240424125850.4189116-1-tgamblin@baylibre.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

Link to v2: https://lore.kernel.org/linux-pwm/20240409174126.1296318-1-tgamblin@baylibre.com/

v2 changes:
* Address feedback for driver in v1:
  * Remove supports_offset flag in pwm_chip struct, and references to it
    in the axi-pwmgen driver patch
  * Drop pwm_config_full patch entirely
  * Don't return EINVAL when state->duty_offset + state->duty_cycle >
    state->period in __pwm_apply(), since this is valid as long as
    neither is greater than state->period on its own
  * Add a check to disallow setting the PWM signal as inverse and a
    nonzero duty_offset at the same time in __pwm_apply(), with a
    comment explaining why

Link to v1 (RFC): https://lore.kernel.org/linux-pwm/20240405003025.739603-1-tgamblin@baylibre.com/

Trevor Gamblin (2):
  pwm: add duty offset support
  pwm: axi-pwmgen: add duty offset support

 drivers/pwm/core.c           | 79 +++++++++++++++++++++++++++++++++---
 drivers/pwm/pwm-axi-pwmgen.c | 20 ++++++++-
 include/linux/pwm.h          | 15 +++++++
 include/trace/events/pwm.h   |  6 ++-
 4 files changed, 112 insertions(+), 8 deletions(-)

-- 
2.45.1

[PATCH 1/2 v3] pwm: add duty offset support

[Trevor Gamblin]

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@baylibre.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;
 		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,
+			 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.
+	 */
+	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)
@@ -246,10 +276,11 @@ int pwm_adjust_config(struct pwm_device *pwm)
 	 * been configured.
 	 *
 	 * In either case, we setup the new period and polarity, and assign a
-	 * duty cycle of 0.
+	 * duty cycle and offset of 0.
 	 */
 	if (!state.period) {
 		state.duty_cycle = 0;
+		state.duty_offset = 0;
 		state.period = pargs.period;
 		state.polarity = pargs.polarity;
 
@@ -555,6 +586,41 @@ static ssize_t duty_cycle_store(struct device *pwm_dev,
 	return ret ? : size;
 }
 
+static ssize_t duty_offset_show(struct device *pwm_dev,
+			       struct device_attribute *attr,
+			       char *buf)
+{
+	const struct pwm_device *pwm = pwm_from_dev(pwm_dev);
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return sysfs_emit(buf, "%llu\n", state.duty_offset);
+}
+
+static ssize_t duty_offset_store(struct device *pwm_dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	struct pwm_export *export = pwmexport_from_dev(pwm_dev);
+	struct pwm_device *pwm = export->pwm;
+	struct pwm_state state;
+	u64 val;
+	int ret;
+
+	ret = kstrtou64(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	mutex_lock(&export->lock);
+	pwm_get_state(pwm, &state);
+	state.duty_offset = val;
+	ret = pwm_apply_might_sleep(pwm, &state);
+	mutex_unlock(&export->lock);
+
+	return ret ? : size;
+}
+
 static ssize_t enable_show(struct device *pwm_dev,
 			   struct device_attribute *attr,
 			   char *buf)
@@ -669,6 +735,7 @@ static ssize_t capture_show(struct device *pwm_dev,
 
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
+static DEVICE_ATTR_RW(duty_offset);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
 static DEVICE_ATTR_RO(capture);
@@ -676,6 +743,7 @@ static DEVICE_ATTR_RO(capture);
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
 	&dev_attr_duty_cycle.attr,
+	&dev_attr_duty_offset.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
 	&dev_attr_capture.attr,
@@ -1639,6 +1707,7 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s)
 
 		seq_printf(s, " period: %llu ns", state.period);
 		seq_printf(s, " duty: %llu ns", state.duty_cycle);
+		seq_printf(s, " duty_offset: %llu ns", state.duty_offset);
 		seq_printf(s, " polarity: %s",
 			   state.polarity ? "inverse" : "normal");
 
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 60b92c2c75ef..03e3fc00d236 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -50,6 +50,7 @@ enum {
  * struct pwm_state - state of a PWM channel
  * @period: PWM period (in nanoseconds)
  * @duty_cycle: PWM duty cycle (in nanoseconds)
+ * @duty_offset: PWM duty offset (in nanoseconds)
  * @polarity: PWM polarity
  * @enabled: PWM enabled status
  * @usage_power: If set, the PWM driver is only required to maintain the power
@@ -60,6 +61,7 @@ enum {
 struct pwm_state {
 	u64 period;
 	u64 duty_cycle;
+	u64 duty_offset;
 	enum pwm_polarity polarity;
 	bool enabled;
 	bool usage_power;
@@ -129,6 +131,15 @@ static inline u64 pwm_get_duty_cycle(const struct pwm_device *pwm)
 	return state.duty_cycle;
 }
 
+static inline u64 pwm_get_duty_offset(const struct pwm_device *pwm)
+{
+	struct pwm_state state;
+
+	pwm_get_state(pwm, &state);
+
+	return state.duty_offset;
+}
+
 static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 {
 	struct pwm_state state;
@@ -160,6 +171,9 @@ static inline void pwm_get_args(const struct pwm_device *pwm,
  * ->duty_cycle value exceed the pwm_args->period one, which would trigger
  * an error if the user calls pwm_apply_might_sleep() without adjusting ->duty_cycle
  * first.
+ *
+ * ->duty_offset is likewise set to zero to avoid inconsistent default
+ *  states.
  */
 static inline void pwm_init_state(const struct pwm_device *pwm,
 				  struct pwm_state *state)
@@ -175,6 +189,7 @@ static inline void pwm_init_state(const struct pwm_device *pwm,
 	state->period = args.period;
 	state->polarity = args.polarity;
 	state->duty_cycle = 0;
+	state->duty_offset = 0;
 	state->usage_power = false;
 }
 
diff --git a/include/trace/events/pwm.h b/include/trace/events/pwm.h
index 12b35e4ff917..2d25ac5de816 100644
--- a/include/trace/events/pwm.h
+++ b/include/trace/events/pwm.h
@@ -18,6 +18,7 @@ DECLARE_EVENT_CLASS(pwm,
 		__field(struct pwm_device *, pwm)
 		__field(u64, period)
 		__field(u64, duty_cycle)
+		__field(u64, duty_offset)
 		__field(enum pwm_polarity, polarity)
 		__field(bool, enabled)
 		__field(int, err)
@@ -27,13 +28,14 @@ DECLARE_EVENT_CLASS(pwm,
 		__entry->pwm = pwm;
 		__entry->period = state->period;
 		__entry->duty_cycle = state->duty_cycle;
+		__entry->duty_offset = state->duty_offset;
 		__entry->polarity = state->polarity;
 		__entry->enabled = state->enabled;
 		__entry->err = err;
 	),
 
-	TP_printk("%p: period=%llu duty_cycle=%llu polarity=%d enabled=%d err=%d",
-		  __entry->pwm, __entry->period, __entry->duty_cycle,
+	TP_printk("%p: period=%llu duty_cycle=%llu duty_offset=%llu polarity=%d enabled=%d err=%d",
+		  __entry->pwm, __entry->period, __entry->duty_cycle, __entry->duty_offset,
 		  __entry->polarity, __entry->enabled, __entry->err)
 
 );
-- 
2.45.1

Re: [PATCH 1/2 v3] pwm: add duty offset support

[Uwe Kleine-König]

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

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@baylibre.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/ |

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

Re: [PATCH 1/2 v3] pwm: add duty offset support

[Trevor Gamblin]

On 2024-05-22 11:53 a.m., Uwe Kleine-König wrote:
> 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@baylibre.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/
Thanks for the catch.
>
>>   		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?
I like that. I'll clean it up.
>
>> +			 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.
Thanks for the clarification, I'll change this too.
>
>> +	 */
>> +	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.)
Makes sense. On a related note, will your pwm/chardev branch be merged soon?
>
> 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.)

Instead of "new", what about something like "raw", "base", "signal", or 
"waveform"? I think those (even abbreviated) might make it more clear 
what the purpose of the new struct is. "wstate" seems to be fairly 
unique to me - a quick grep caught other uses of the string only in:

- tools/testing/selftests/net/mptcp/mptcp_connect.c

- arch/sparc/include/asm/hibernate.h

- arch/sparc/kernel/asm-offsets.c

Thanks again for the feedback. I'll spend some time thinking about this 
and aim to come back with a v4 soon.

Trevor

>
> Best regards
> Uwe
>

Re: [PATCH 1/2 v3] pwm: add duty offset support

[Uwe Kleine-König]

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

On Wed, May 22, 2024 at 04:06:00PM -0400, Trevor Gamblin wrote:
> Makes sense. On a related note, will your pwm/chardev branch be merged soon?

My plan here is to first add core support for .duty_offset, and then
only expose those chips that implement the new .apply. The reasoning is
that I want to assert that there is a consistent rounding for all
new-style drivers and thus allow chardev users to rely on these rounding
rules.

Best regards
Uwe

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

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

[PATCH 2/2 v3] pwm: axi-pwmgen: add duty offset support

[Trevor Gamblin]

Enable duty_offset feature now that it is supported in the pwm
subsystem. Related macros and struct fields related to duty_offset are
renamed to be consistent.

Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
---
v3 changes:
* rebased on top of latest pwm/for-next

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/pwm-axi-pwmgen.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index e0bf90cc2ba3..9ae06b105d07 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -56,7 +56,7 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	struct axi_pwmgen_ddata *ddata = pwmchip_get_drvdata(chip);
 	unsigned int ch = pwm->hwpwm;
 	struct regmap *regmap = ddata->regmap;
-	u64 period_cnt, duty_cnt;
+	u64 period_cnt, duty_cnt, duty_offset_cnt;
 	int ret;
 
 	if (state->polarity != PWM_POLARITY_NORMAL)
@@ -81,6 +81,14 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), duty_cnt);
 		if (ret)
 			return ret;
+
+		duty_offset_cnt = mul_u64_u64_div_u64(state->duty_offset, ddata->clk_rate_hz, NSEC_PER_SEC);
+		if (duty_offset_cnt > UINT_MAX)
+			duty_offset_cnt = UINT_MAX;
+
+		ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(ch), duty_offset_cnt);
+		if (ret)
+			return ret;
 	} else {
 		ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(ch), 0);
 		if (ret)
@@ -89,6 +97,10 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(ch), 0);
 		if (ret)
 			return ret;
+
+		ret = regmap_write(regmap, AXI_PWMGEN_CHX_OFFSET(ch), 0);
+		if (ret)
+			return ret;
 	}
 
 	return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
@@ -117,6 +129,12 @@ static int axi_pwmgen_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	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), &cnt);
+	if (ret)
+		return ret;
+
+	state->duty_offset = DIV_ROUND_UP_ULL((u64)cnt * NSEC_PER_SEC, ddata->clk_rate_hz);
+
 	state->polarity = PWM_POLARITY_NORMAL;
 
 	return 0;
-- 
2.45.1

Re: [PATCH 0/2 v3] pwm: add support for duty_offset

[Uwe Kleine-König]

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

Hello Trevor,

On Tue, May 21, 2024 at 03:49:14PM -0400, Trevor Gamblin wrote:
> This series extends the PWM subsystem to support the duty_offset feature
> found on some PWM devices. It includes a patch to enable this feature
> for the axi-pwmgen driver, which can also serve as an example of how to
> implement it for other devices.
> 
> The series was tested on actual hardware using a Zedboard. An
> oscilloscope was used to validate that the generated PWM signals matched
> the requested ones. 
> 
> Note that in addition to the other patches in this series, the changes
> to the axi-pwmgen driver rely on [1], which hasn't been picked up yet.
> 
> [1] https://lore.kernel.org/linux-pwm/20240424125850.4189116-1-tgamblin@baylibre.com/

You can do yourself a favour and use git format-patch's --base option to
(additionally) tell this in a way that the autobuild bots understand.

Best regards
Uwe

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

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

Re: [PATCH 0/2 v3] pwm: add support for duty_offset

[Uwe Kleine-König]

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

Hello Trevor,

On Tue, May 21, 2024 at 03:49:14PM -0400, Trevor Gamblin wrote:
> This series extends the PWM subsystem to support the duty_offset feature
> found on some PWM devices. It includes a patch to enable this feature
> for the axi-pwmgen driver, which can also serve as an example of how to
> implement it for other devices.
> 
> The series was tested on actual hardware using a Zedboard. An
> oscilloscope was used to validate that the generated PWM signals matched
> the requested ones. 
> 
> Note that in addition to the other patches in this series, the changes
> to the axi-pwmgen driver rely on [1], which hasn't been picked up yet.
> 
> [1] https://lore.kernel.org/linux-pwm/20240424125850.4189116-1-tgamblin@baylibre.com/

I'm working on a more complete variant of that. It works in a prototype
stage, I hope to be able to send it out later today. I'll drop this
series from patchwork.

Best regards
Uwe

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