diff mbox series

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

Message ID 20240521194916.1897909-2-tgamblin@baylibre.com
State New
Headers show
Series pwm: add support for duty_offset | expand

Commit Message

Trevor Gamblin May 21, 2024, 7:49 p.m. UTC
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(-)

Comments

Uwe Kleine-König May 22, 2024, 3:53 p.m. UTC | #1
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
Trevor Gamblin May 22, 2024, 8:06 p.m. UTC | #2
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
>
Uwe Kleine-König May 23, 2024, 6:51 a.m. UTC | #3
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
diff mbox series

Patch

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