Re: [PATCH 1/2 v3] pwm: add duty offset support
From: Trevor Gamblin
Date: Wed May 22 2024 - 16:06:12 EST
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@xxxxxxxxxxxx>
---
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