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. It also contains a patch adding a new
pwm_config_full() function mirroring the behavior of pwm_config() but
with duty_offset included, to help maintain compatibility for drivers
that don't support the feature.
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. The libpwm [1] tool was also used for testing the
char device functionality.
The series is marked RFC as there are some outstanding questions about
implementation:
1. In drivers/pwm/core.c, __pwm_apply() was modified to check that the
sum of state->duty_offset + state->duty_cycle does not exceed
state->period, but in the character device section these values are
being checked separately. Is this intentional? What is the intended
behavior?
2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
duty_offset together?
3. Are there other places that would need duty_offset handling which
have been missed?
Note that in addition to the other patches in this series, the changes
to the axi-pwmgen driver rely on [2] and [3], which haven't been picked
up yet.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/ukleinek/libpwm.git/
[2] https://lore.kernel.org/linux-pwm/[email protected]/
[3] https://lore.kernel.org/linux-pwm/[email protected]/
Trevor Gamblin (3):
pwm: add duty offset support
pwm: axi-pwmgen: add duty offset support
pwm: add pwm_config_full to pwm.h
drivers/pwm/core.c | 75 +++++++++++++++++++++++++++++++++---
drivers/pwm/pwm-axi-pwmgen.c | 35 +++++++++++++----
include/linux/pwm.h | 52 ++++++++++++++++++++++---
include/trace/events/pwm.h | 6 ++-
4 files changed, 147 insertions(+), 21 deletions(-)
--
2.44.0
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 <[email protected]>
---
drivers/pwm/pwm-axi-pwmgen.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/pwm-axi-pwmgen.c b/drivers/pwm/pwm-axi-pwmgen.c
index 539625c404ac..84ecb12e1e21 100644
--- a/drivers/pwm/pwm-axi-pwmgen.c
+++ b/drivers/pwm/pwm-axi-pwmgen.c
@@ -6,9 +6,9 @@
* Copyright 2024 Baylibre SAS
*
* Limitations:
- * - The writes to registers for period and duty are shadowed until
- * LOAD_CONFIG is written to AXI_PWMGEN_REG_CONFIG at the end of the
- * current period.
+ * - The writes to registers for period, duty, and duty_offset are
+ * shadowed until LOAD_CONFIG is written to AXI_PWMGEN_REG_CONFIG at
+ * the end of the current period.
* - Writing LOAD_CONFIG also has the effect of re-synchronizing all
* enabled channels, which could cause glitching on other channels. It
* is therefore expected that channels are assigned harmonic periods
@@ -34,7 +34,7 @@
#define AXI_PWMGEN_REG_NPWM 0x14
#define AXI_PWMGEN_CHX_PERIOD(v, ch) ((v)->period_base + (v)->ch_step * (ch))
#define AXI_PWMGEN_CHX_DUTY(v, ch) ((v)->duty_base + (v)->ch_step * (ch))
-#define AXI_PWMGEN_CHX_OFFSET(v, ch) ((v)->offset_base + (v)->ch_step * (ch))
+#define AXI_PWMGEN_CHX_DUTY_OFFSET(v, ch) ((v)->duty_offset_base + (v)->ch_step * (ch))
#define AXI_PWMGEN_REG_CORE_MAGIC_VAL 0x601A3471 /* Identification number to test during setup */
#define AXI_PWMGEN_LOAD_CONFIG BIT(1)
#define AXI_PWMGEN_RESET BIT(0)
@@ -42,7 +42,7 @@
struct axi_pwm_variant {
u8 period_base;
u8 duty_base;
- u8 offset_base;
+ u8 duty_offset_base;
u8 major_version;
u8 ch_step;
};
@@ -62,7 +62,7 @@ static const struct regmap_config axi_pwmgen_regmap_config = {
static const struct axi_pwm_variant pwmgen_1_00_variant = {
.period_base = 0x40,
.duty_base = 0x44,
- .offset_base = 0x48,
+ .duty_offset_base = 0x48,
.major_version = 1,
.ch_step = 12,
};
@@ -70,7 +70,7 @@ static const struct axi_pwm_variant pwmgen_1_00_variant = {
static const struct axi_pwm_variant pwmgen_2_00_variant = {
.period_base = 0x40,
.duty_base = 0x80,
- .offset_base = 0xC0,
+ .duty_offset_base = 0xC0,
.major_version = 2,
.ch_step = 4,
};
@@ -83,7 +83,7 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
unsigned int ch = pwm->hwpwm;
struct regmap *regmap = ddata->regmap;
const struct axi_pwm_variant *variant = ddata->variant;
- u64 period_cnt, duty_cnt;
+ u64 period_cnt, duty_cnt, duty_offset_cnt;
int ret;
if (state->polarity != PWM_POLARITY_NORMAL)
@@ -108,6 +108,14 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(variant, 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_DUTY_OFFSET(variant, ch), duty_offset_cnt);
+ if (ret)
+ return ret;
} else {
ret = regmap_write(regmap, AXI_PWMGEN_CHX_PERIOD(variant, ch), 0);
if (ret)
@@ -116,6 +124,10 @@ static int axi_pwmgen_apply(struct pwm_chip *chip, struct pwm_device *pwm,
ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY(variant, ch), 0);
if (ret)
return ret;
+
+ ret = regmap_write(regmap, AXI_PWMGEN_CHX_DUTY_OFFSET(variant, ch), 0);
+ if (ret)
+ return ret;
}
return regmap_write(regmap, AXI_PWMGEN_REG_CONFIG, AXI_PWMGEN_LOAD_CONFIG);
@@ -145,6 +157,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_DUTY_OFFSET(variant, 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;
@@ -254,6 +272,7 @@ static int axi_pwmgen_probe(struct platform_device *pdev)
chip->ops = &axi_pwmgen_pwm_ops;
chip->atomic = true;
+ chip->supports_offset = true;
return devm_pwmchip_add(dev, chip);
}
--
2.44.0
Add a function that performs the old pwm_config operations while also
handling duty_offset. Change pwm_config to use pwm_config_full with the
duty_offset_ns argument set to 0.
Signed-off-by: Trevor Gamblin <[email protected]>
---
include/linux/pwm.h | 35 ++++++++++++++++++++++++++++++-----
1 file changed, 30 insertions(+), 5 deletions(-)
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index e0e5960f91ba..eb018f11a48f 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -347,33 +347,51 @@ int pwm_apply_atomic(struct pwm_device *pwm, const struct pwm_state *state);
int pwm_adjust_config(struct pwm_device *pwm);
/**
- * pwm_config() - change a PWM device configuration
+ * pwm_config_full() - change a PWM device configuration, including duty
+ * offset
* @pwm: PWM device
* @duty_ns: "on" time (in nanoseconds)
+ * @duty_offset_ns: offset (in nanoseconds) of "on" pulse
* @period_ns: duration (in nanoseconds) of one cycle
*
* Returns: 0 on success or a negative error code on failure.
*/
-static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
- int period_ns)
+static inline int pwm_config_full(struct pwm_device *pwm, int duty_ns,
+ int duty_offset_ns, int period_ns)
{
struct pwm_state state;
if (!pwm)
return -EINVAL;
- if (duty_ns < 0 || period_ns < 0)
+ if (duty_ns < 0 || period_ns < 0 || duty_offset_ns < 0)
return -EINVAL;
pwm_get_state(pwm, &state);
- if (state.duty_cycle == duty_ns && state.period == period_ns)
+ if (state.duty_cycle == duty_ns && state.period == period_ns &&
+ state.duty_offset == duty_offset_ns)
return 0;
state.duty_cycle = duty_ns;
+ state.duty_offset = duty_offset_ns;
state.period = period_ns;
return pwm_apply_might_sleep(pwm, &state);
}
+/**
+ * pwm_config() - change a PWM device configuration
+ * @pwm: PWM device
+ * @duty_ns: "on" time (in nanoseconds)
+ * @period_ns: duration (in nanoseconds) of one cycle
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
+ int period_ns)
+{
+ return pwm_config_full(pwm, duty_ns, 0, period_ns);
+}
+
/**
* pwm_enable() - start a PWM output toggling
* @pwm: PWM device
@@ -480,6 +498,13 @@ static inline int pwm_adjust_config(struct pwm_device *pwm)
return -EOPNOTSUPP;
}
+static inline int pwm_config_full(struct pwm_device *pwm, int duty_ns,
+ int duty_offset_ns, int period_ns)
+{
+ might_sleep();
+ return -EINVAL;
+}
+
static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
int period_ns)
{
--
2.44.0
Some PWM chips support a "phase" or "duty_offset" feature. This patch
continues adding support for configuring this property in the PWM
subsystem.
The pwm_chip struct gains a new supports_offset flag, which can be set
for compatible parts. This is checked in __pwm_apply, where attempts to
set duty_offset result in an EOPNOTSUPP if the flag is not set.
Functions duty_offset_show(), duty_offset_store(), and
pwm_get_duty_offset() are added to match what exists for duty_cycle.
Handle duty_offset in the new pwmchip char device logic.
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 <[email protected]>
---
drivers/pwm/core.c | 75 +++++++++++++++++++++++++++++++++++---
include/linux/pwm.h | 17 +++++++++
include/trace/events/pwm.h | 6 ++-
3 files changed, 90 insertions(+), 8 deletions(-)
diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 2745941a008b..0e05518feb21 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -80,6 +80,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;
@@ -121,6 +122,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");
@@ -144,12 +162,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);
}
}
@@ -164,13 +183,17 @@ 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_offset + state->duty_cycle > state->period)
return -EINVAL;
chip = pwm->chip;
+ if (!chip->supports_offset && state->duty_offset)
+ return -EOPNOTSUPP;
+
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)
@@ -292,10 +315,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;
@@ -617,6 +641,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)
@@ -731,6 +790,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);
@@ -738,6 +798,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,
@@ -1290,7 +1351,7 @@ static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long ar
if (state.enabled) {
cstate.period = state.period;
if (state.polarity == PWM_POLARITY_NORMAL) {
- cstate.duty_offset = 0;
+ cstate.duty_offset = state.duty_offset;
cstate.duty_cycle = state.duty_cycle;
} else {
cstate.duty_offset = state.duty_cycle;
@@ -1356,6 +1417,7 @@ static long pwm_cdev_ioctl(struct file *file, unsigned int cmd, unsigned long ar
state.period = cstate.period;
state.polarity = PWM_POLARITY_NORMAL;
state.duty_cycle = cstate.duty_cycle;
+ state.duty_offset = cstate.duty_offset;
} else {
state.enabled = false;
}
@@ -1991,6 +2053,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 a58db7011807..e0e5960f91ba 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -51,6 +51,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
@@ -61,6 +62,7 @@ enum {
struct pwm_state {
u64 period;
u64 duty_cycle;
+ u64 duty_offset;
enum pwm_polarity polarity;
bool enabled;
bool usage_power;
@@ -130,6 +132,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;
@@ -161,6 +172,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)
@@ -176,6 +190,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;
}
@@ -275,6 +290,7 @@ struct pwm_ops {
* @npwm: number of PWMs controlled by this chip
* @of_xlate: request a PWM device given a device tree PWM specifier
* @atomic: can the driver's ->apply() be called in atomic context
+ * @supports_offset: does the driver support duty cycle offset
* @uses_pwmchip_alloc: signals if pwmchip_allow was used to allocate this chip
* @operational: signals if the chip can be used (or is already deregistered)
* @nonatomic_lock: mutex for nonatomic chips
@@ -292,6 +308,7 @@ struct pwm_chip {
struct pwm_device * (*of_xlate)(struct pwm_chip *chip,
const struct of_phandle_args *args);
bool atomic;
+ bool supports_offset;
/* only used internally by the PWM framework */
bool uses_pwmchip_alloc;
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.44.0
Hello Trevor,
In general I really like your effort to generalize the pwm framework.
Thanks a lot!
On Thu, Apr 04, 2024 at 08:30:22PM -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. It also contains a patch adding a new
> pwm_config_full() function mirroring the behavior of pwm_config() but
Please don't. pwm_config() is a function I want to get rid of in the
long term. Consumers that want to make use of it should use
pwm_apply_*().
> with duty_offset included, to help maintain compatibility for drivers
> that don't support the feature.
>
> 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. The libpwm [1] tool was also used for testing the
> char device functionality.
>
> The series is marked RFC as there are some outstanding questions about
> implementation:
>
> 1. In drivers/pwm/core.c, __pwm_apply() was modified to check that the
> sum of state->duty_offset + state->duty_cycle does not exceed
> state->period, but in the character device section these values are
> being checked separately. Is this intentional? What is the intended
> behavior?
state->duty_offset + state->duty_cycle doesn't necessarily need to be
less or equal to state->period. Consider this waveform, where ^ marks
the start of a period.
___ _________ __...
\_____/ \_____/
^ ^
This one has duty_offset = 9 and duty_cycle = 10 while
period = 16 < 10 + 9.
> 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
> duty_offset together?
While there is no technical need for that, a configuration with both
PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes,
it should be disallowed. When I created the cdev API I even considered
dropping .polarity for lowlevel drivers and convert them all to
.duty_offset.
Having said that I don't like the addition of .supports_offset to
struct pwm_chip, which only signals a new incomplete evolution of the
pwm framework. Better adapt all drivers and then assume all of them
support it.
> 3. Are there other places that would need duty_offset handling which
> have been missed?
I'm happy you adapted the tracing stuff. I didn't look closely, but I
don't think something important was missed.
> Note that in addition to the other patches in this series, the changes
> to the axi-pwmgen driver rely on [2] and [3], which haven't been picked
> up yet.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On 2024-04-05 08:23, Uwe Kleine-König wrote:
> Hello Trevor,
>
> In general I really like your effort to generalize the pwm framework.
> Thanks a lot!
Thanks! I'm glad that it (mostly) fits.
>
> On Thu, Apr 04, 2024 at 08:30:22PM -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. It also contains a patch adding a new
>> pwm_config_full() function mirroring the behavior of pwm_config() but
> Please don't. pwm_config() is a function I want to get rid of in the
> long term. Consumers that want to make use of it should use
> pwm_apply_*().
Okay, I'll drop this patch.
>
>> with duty_offset included, to help maintain compatibility for drivers
>> that don't support the feature.
>>
>> 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. The libpwm [1] tool was also used for testing the
>> char device functionality.
>>
>> The series is marked RFC as there are some outstanding questions about
>> implementation:
>>
>> 1. In drivers/pwm/core.c, __pwm_apply() was modified to check that the
>> sum of state->duty_offset + state->duty_cycle does not exceed
>> state->period, but in the character device section these values are
>> being checked separately. Is this intentional? What is the intended
>> behavior?
> state->duty_offset + state->duty_cycle doesn't necessarily need to be
> less or equal to state->period. Consider this waveform, where ^ marks
> the start of a period.
>
>
> ___ _________ __...
> \_____/ \_____/
> ^ ^
>
> This one has duty_offset = 9 and duty_cycle = 10 while
> period = 16 < 10 + 9.
Alright, I'll make a change here.
>
>> 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
>> duty_offset together?
> While there is no technical need for that, a configuration with both
> PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes,
> it should be disallowed. When I created the cdev API I even considered
> dropping .polarity for lowlevel drivers and convert them all to
> .duty_offset.
Will do.
>
> Having said that I don't like the addition of .supports_offset to
> struct pwm_chip, which only signals a new incomplete evolution of the
> pwm framework. Better adapt all drivers and then assume all of them
> support it.
Can you clarify what you mean here - is the intent to put basic handling
of duty_offset (even if that means simply setting it to 0) in each driver?
>
>> 3. Are there other places that would need duty_offset handling which
>> have been missed?
> I'm happy you adapted the tracing stuff. I didn't look closely, but I
> don't think something important was missed.
>
>> Note that in addition to the other patches in this series, the changes
>> to the axi-pwmgen driver rely on [2] and [3], which haven't been picked
>> up yet.
> Best regards
> Uwe
>
Hello Trevor,
On Fri, Apr 05, 2024 at 10:19:20AM -0400, Trevor Gamblin wrote:
> On 2024-04-05 08:23, Uwe Kleine-K?nig wrote:
> > Having said that I don't like the addition of .supports_offset to
> > struct pwm_chip, which only signals a new incomplete evolution of the
> > pwm framework. Better adapt all drivers and then assume all of them
> > support it.
> Can you clarify what you mean here - is the intent to put basic handling of
> duty_offset (even if that means simply setting it to 0) in each driver?
Well, it's a bit more complicated than setting it to 0. It involves
translating a setting with inverted polarity to one using .duty_offset
and make all drivers implement that accordingly.
For drivers that support both polarities the logic in .apply should be:
if (.duty_offset >= .period - .duty_cycle)
... set inverted polarity
else
... set normal polarity
I'm usure how to do the transformation in reviewable chunks. Maybe the
easiest option is a new .apply callback that honors .duty_offset?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Apr 5, 2024 at 1:23 AM Uwe Kleine-König
<[email protected]> wrote:
> On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote:
..
> > 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
> > duty_offset together?
>
> While there is no technical need for that, a configuration with both
> PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes,
> it should be disallowed. When I created the cdev API I even considered
> dropping .polarity for lowlevel drivers and convert them all to
> .duty_offset.
>
> Having said that I don't like the addition of .supports_offset to
> struct pwm_chip, which only signals a new incomplete evolution of the
> pwm framework. Better adapt all drivers and then assume all of them
> support it.
But not all chips can fully support this feature. I envisioned this
flag as something consumer drivers would check when they require a
chip capable of providing a phase offset between two PWM output
channels. This way, the consumer driver could fail to probe if the PWM
chip is not up to the task.
For example the consumer driver might require two coordinated signals like this:
_ _
PWM0 | |_________________| |_________________
___ ___
PWM1 _____| |_______________| |__________
PWM0: duty_offset = 0, duty_cycle = 1, period = 10
PWM1: duty_offset = 2, duty_cycle = 2, period = 10
Do we need to do additional work to support cases like this? Or should
we just let it fail silently and let it generate incorrect signals if
someone attempts to use an unsupported hardware configuration?
Hello David,
On Fri, Apr 05, 2024 at 12:03:56PM -0500, David Lechner wrote:
> On Fri, Apr 5, 2024 at 1:23 AM Uwe Kleine-König
> <[email protected]> wrote:
> > On Thu, Apr 04, 2024 at 08:30:22PM -0400, Trevor Gamblin wrote:
>
> ...
>
> > > 2. Should __pwm_apply() explicitly disallow PWM_POLARITY_INVERSED and
> > > duty_offset together?
> >
> > While there is no technical need for that, a configuration with both
> > PWM_POLARITY_INVERSED and duty_offset > 0 is irritating. So I'd say yes,
> > it should be disallowed. When I created the cdev API I even considered
> > dropping .polarity for lowlevel drivers and convert them all to
> > .duty_offset.
> >
> > Having said that I don't like the addition of .supports_offset to
> > struct pwm_chip, which only signals a new incomplete evolution of the
> > pwm framework. Better adapt all drivers and then assume all of them
> > support it.
>
> But not all chips can fully support this feature. I envisioned this
> flag as something consumer drivers would check when they require a
> chip capable of providing a phase offset between two PWM output
> channels. This way, the consumer driver could fail to probe if the PWM
> chip is not up to the task.
>
> For example the consumer driver might require two coordinated signals like this:
> _ _
> PWM0 | |_________________| |_________________
> ___ ___
> PWM1 _____| |_______________| |__________
>
> PWM0: duty_offset = 0, duty_cycle = 1, period = 10
> PWM1: duty_offset = 2, duty_cycle = 2, period = 10
>
> Do we need to do additional work to support cases like this? Or should
> we just let it fail silently and let it generate incorrect signals if
> someone attempts to use an unsupported hardware configuration?
My vision here is that you can do the following:
state.duty_offset = 0;
state.duty_cycle = 1;
state.period = 10;
ret = pwm_round_state(pwm, &state);
if (!is_good_enough(state))
goto err;
This way pwm_apply_* could just continue to work as it does now (i.e.
implement the biggest period not bigger than requested. For that
implement the biggest duty_offset not bigger than requested and for
these values of period and duty_offset implement the biggest duty_cycle
not bigger than requested.)
This has the advantage that the lowlevel driver doesn't need to judge if
the setting it implements is good enough.
To get there, quite some work has to be invested.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |