* [PATCH 0/2] extend PWM framework to support PWM dead-times
@ 2017-05-09 12:15 Claudiu Beznea
2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Claudiu Beznea @ 2017-05-09 12:15 UTC (permalink / raw)
To: linux-arm-kernel
Hi all,
Please give feedback on these patches which extends the PWM
framework in order to support multiple PWM signal types.
Since I didn't receive any inputs on RFC series I'm resending it as
normal patch series.
The current patch series add the following PWM signal types:
- PWM complementary signals
- PWM push-pull signal
Definition of PWM complementary mode:
For a PWM controllers with more than one outputs per PWM channel,
e.g. with 2 outputs per PWM channels, the PWM complementary signals
have opposite levels, same duration and same starting times,
as in the following diagram:
__ __ __ __
PWMH __| |__| |__| |__| |__
__ __ __ __ __
PWML |__| |__| |__| |__|
<--T-->
Where T is the signal period.
Definition of PWM push-pull mode:
For PWM controllers with more than one outputs per PWM channel,
e.g. with 2 outputs per PWM channel, the PWM push-pull signals
have same levels, same duration and are delayed until the begining
of the next period, as in the following diagram:
__ __
PWMH __| |________| |________
__ __
PWML ________| |________| |__
<--T-->
Where T is the signal period.
These output signals could be configured by setting PWM mode
(a new input in sysfs has been added in order to support this
operation).
root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l
-r--r--r-- 1 root root 4096 Feb 9 10:54 capture
-rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle
-rw-r--r-- 1 root root 4096 Feb 9 10:54 enable
-rw-r--r-- 1 root root 4096 Feb 9 10:54 mode
-rw-r--r-- 1 root root 4096 Feb 9 10:54 period
-rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity
drwxr-xr-x 2 root root 0 Feb 9 10:54 power
-rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent
The PWM push-pull mode could be usefull in applications like
half bridge converters.
This series add support for PWM modes on atmel SAMA5D2 SoC.
Thank you,
Claudiu Beznea
Claudiu Beznea (2):
drivers: pwm: core: implement pwm mode
drivers: pwm: pwm-atmel: add support for pwm modes
Documentation/pwm.txt | 24 ++++++++--
drivers/pwm/core.c | 13 +++++-
drivers/pwm/pwm-atmel.c | 103 +++++++++++++++++++++++++++++-------------
drivers/pwm/sysfs.c | 52 +++++++++++++++++++++
include/dt-bindings/pwm/pwm.h | 1 +
include/linux/pwm.h | 37 ++++++++++++++-
6 files changed, 192 insertions(+), 38 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] drivers: pwm: core: implement pwm mode 2017-05-09 12:15 [PATCH 0/2] extend PWM framework to support PWM dead-times Claudiu Beznea @ 2017-05-09 12:15 ` Claudiu Beznea 2017-05-27 22:11 ` Andy Shevchenko 2017-07-19 12:13 ` m18063 2017-05-09 12:15 ` [PATCH 2/2] drivers: pwm: pwm-atmel: add support for pwm modes Claudiu Beznea 2017-08-21 8:25 ` [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding 2 siblings, 2 replies; 13+ messages in thread From: Claudiu Beznea @ 2017-05-09 12:15 UTC (permalink / raw) To: linux-arm-kernel Extends PWM framework to support PWM modes. The currently implemented PWM modes were called PWM complementary mode and PWM push-pull mode. For devices that have more than one output per PWM channel: - PWM complementary mode is standard working mode; in PWM complementary mode the rising and falling edges of the channels outputs have opposite levels, same duration and same starting time. - in PWM push-pull mode the channles outputs has same levels, same duration and the rising edges are delayed until the beginning of the next period. A new member was added in pwm_state structure in order to keep the new PWM argument. For PWM channels with inputs in DT, the mode could also be passed via DT. No new extra field need to be added in device tree; since the signal polarity is passed as a flag via DT, the field used to pass information for PWM channel polarity via DT is also used by PWM mode. Since, for the moment, only the complementary and push-pull modes are implemented, only one bit in flags used to pass polarity could also be used for PWM mode. The other drivers are not affected by this change. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- Documentation/pwm.txt | 24 +++++++++++++++++--- drivers/pwm/core.c | 13 +++++++++-- drivers/pwm/sysfs.c | 52 +++++++++++++++++++++++++++++++++++++++++++ include/dt-bindings/pwm/pwm.h | 1 + include/linux/pwm.h | 37 ++++++++++++++++++++++++++++-- 5 files changed, 120 insertions(+), 7 deletions(-) diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt index 789b27c..1b6fc59 100644 --- a/Documentation/pwm.txt +++ b/Documentation/pwm.txt @@ -59,9 +59,9 @@ In addition to the PWM state, the PWM API also exposes PWM arguments, which are the reference PWM config one should use on this PWM. PWM arguments are usually platform-specific and allows the PWM user to only care about dutycycle relatively to the full period (like, duty = 50% of the -period). struct pwm_args contains 2 fields (period and polarity) and should -be used to set the initial PWM config (usually done in the probe function -of the PWM user). PWM arguments are retrieved with pwm_get_args(). +period). struct pwm_args contains 3 fields (period, polarity and mode) and +should be used to set the initial PWM config (usually done in the probe +function of the PWM user). PWM arguments are retrieved with pwm_get_args(). Using PWMs with the sysfs interface ----------------------------------- @@ -100,6 +100,24 @@ enable - Enable/disable the PWM signal (read/write). 0 - disabled 1 - enabled +mode - Set PWM signal type (for channels with more than one output + per channel) + complementary - for an output signal as follow: + __ __ __ __ + PWMH1 __| |__| |__| |__| |__ + __ __ __ __ __ + PWML1 |__| |__| |__| |__| + <--T--> + Where T is the signal period. + + push-pull - for an output signal as follows: + __ __ + PWMH1 __| |________| |________ + __ __ + PWML1 ________| |________| |__ + <--T--> + Where T is the signal period. + Implementing a PWM driver ------------------------- diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index a0860b3..4efc92d 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -154,9 +154,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) pwm->args.period = args->args[1]; pwm->args.polarity = PWM_POLARITY_NORMAL; + pwm->args.mode = PWM_MODE_COMPLEMENTARY; - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) - pwm->args.polarity = PWM_POLARITY_INVERSED; + if (args->args_count > 2) { + if (args->args[2] & PWM_POLARITY_INVERTED) + pwm->args.polarity = PWM_POLARITY_INVERSED; + if (args->args[2] & PWM_MODE_PUSHPULL) + pwm->args.mode = PWM_MODE_PUSH_PULL; + } return pwm; } @@ -579,6 +584,8 @@ int pwm_adjust_config(struct pwm_device *pwm) pwm_get_args(pwm, &pargs); pwm_get_state(pwm, &state); + state.mode = pargs.mode; + /* * If the current period is zero it means that either the PWM driver * does not support initial state retrieval or the PWM has not yet @@ -997,6 +1004,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) seq_printf(s, " duty: %u ns", state.duty_cycle); seq_printf(s, " polarity: %s", state.polarity ? "inverse" : "normal"); + seq_printf(s, " mode: %s", + state.mode ? "push-pull" : "complementary"); seq_puts(s, "\n"); } diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index a813239..58f02bf 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -223,11 +223,62 @@ static ssize_t capture_show(struct device *child, return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); } +static ssize_t mode_show(struct device *child, + struct device_attribute *attr, + char *buf) +{ + const struct pwm_device *pwm = child_to_pwm_device(child); + const char *mode = "unknown"; + struct pwm_state state; + + pwm_get_state(pwm, &state); + + switch (state.mode) { + case PWM_MODE_COMPLEMENTARY: + mode = "complementary"; + break; + + case PWM_MODE_PUSH_PULL: + mode = "push-pull"; + break; + } + + return sprintf(buf, "%s\n", mode); +} + +static ssize_t mode_store(struct device *child, + struct device_attribute *attr, + const char *buf, size_t size) +{ + struct pwm_export *export = child_to_pwm_export(child); + struct pwm_device *pwm = export->pwm; + enum pwm_mode mode; + struct pwm_state state; + int ret; + + if (sysfs_streq(buf, "complementary")) + mode = PWM_MODE_COMPLEMENTARY; + else if (sysfs_streq(buf, "push-pull")) + mode = PWM_MODE_PUSH_PULL; + else + return -EINVAL; + + mutex_lock(&export->lock); + pwm_get_state(pwm, &state); + state.mode = mode; + ret = pwm_apply_state(pwm, &state); + mutex_unlock(&export->lock); + + return ret ? : size; +} + static DEVICE_ATTR_RW(period); static DEVICE_ATTR_RW(duty_cycle); static DEVICE_ATTR_RW(enable); static DEVICE_ATTR_RW(polarity); static DEVICE_ATTR_RO(capture); +static DEVICE_ATTR_RW(mode); static struct attribute *pwm_attrs[] = { &dev_attr_period.attr, @@ -235,6 +286,7 @@ static struct attribute *pwm_attrs[] = { &dev_attr_enable.attr, &dev_attr_polarity.attr, &dev_attr_capture.attr, + &dev_attr_mode.attr, NULL }; ATTRIBUTE_GROUPS(pwm); diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h index 96f49e8..83bb884 100644 --- a/include/dt-bindings/pwm/pwm.h +++ b/include/dt-bindings/pwm/pwm.h @@ -10,5 +10,6 @@ #define _DT_BINDINGS_PWM_PWM_H #define PWM_POLARITY_INVERTED (1 << 0) +#define PWM_MODE_PUSHPULL (1 << 1) #endif diff --git a/include/linux/pwm.h b/include/linux/pwm.h index 08fad7c..d924e55 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -25,9 +25,23 @@ enum pwm_polarity { }; /** + * enum pwm_mode - PWM mode + * @PWM_MODE_COMPLEMENTARY: rising and falling edges of the complementary + * PWM channels have opposite levels, same duration and same starting time + * @PWM_MODE_PUSH_PULL: rising and falling edges of the complementary PWM + * channels have same levels, same duration and are delayed until the + * beginning of the next period + */ +enum pwm_mode { + PWM_MODE_COMPLEMENTARY, + PWM_MODE_PUSH_PULL, +}; + +/** * struct pwm_args - board-dependent PWM arguments * @period: reference period * @polarity: reference polarity + * @mode: pwm mode * * This structure describes board-dependent arguments attached to a PWM * device. These arguments are usually retrieved from the PWM lookup table or @@ -40,6 +54,7 @@ enum pwm_polarity { struct pwm_args { unsigned int period; enum pwm_polarity polarity; + enum pwm_mode mode; }; enum { @@ -52,12 +67,14 @@ enum { * @period: PWM period (in nanoseconds) * @duty_cycle: PWM duty cycle (in nanoseconds) * @polarity: PWM polarity + * @mode: PWM mode * @enabled: PWM enabled status */ struct pwm_state { unsigned int period; unsigned int duty_cycle; enum pwm_polarity polarity; + enum pwm_mode mode; bool enabled; }; @@ -143,6 +160,21 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) return state.polarity; } +static inline void pwm_set_mode(struct pwm_device *pwm, enum pwm_mode mode) +{ + if (pwm) + pwm->state.mode = mode; +} + +static inline enum pwm_mode pwm_get_mode(const struct pwm_device *pwm) +{ + struct pwm_state state; + + pwm_get_state(pwm, &state); + + return state.mode; +} + static inline void pwm_get_args(const struct pwm_device *pwm, struct pwm_args *args) { @@ -156,8 +188,8 @@ static inline void pwm_get_args(const struct pwm_device *pwm, * * This functions prepares a state that can later be tweaked and applied * to the PWM device with pwm_apply_state(). This is a convenient function - * that first retrieves the current PWM state and the replaces the period - * and polarity fields with the reference values defined in pwm->args. + * that first retrieves the current PWM state and the replaces the period, + * polarity and mode fields with the reference values defined in pwm->args. * Once the function returns, you can adjust the ->enabled and ->duty_cycle * fields according to your needs before calling pwm_apply_state(). * @@ -180,6 +212,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->mode = args.mode; } /** -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/2] drivers: pwm: core: implement pwm mode 2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea @ 2017-05-27 22:11 ` Andy Shevchenko 2017-05-30 9:49 ` m18063 2017-07-19 12:13 ` m18063 1 sibling, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2017-05-27 22:11 UTC (permalink / raw) To: linux-arm-kernel On Tue, May 9, 2017 at 3:15 PM, Claudiu Beznea <claudiu.beznea@microchip.com> wrote: > Extends PWM framework to support PWM modes. The currently > implemented PWM modes were called PWM complementary mode > and PWM push-pull mode. For devices that have more than one > output per PWM channel: > - PWM complementary mode is standard working mode; in PWM > complementary mode the rising and falling edges of the > channels outputs have opposite levels, same duration and > same starting time. > - in PWM push-pull mode the channles outputs has same levels, > same duration and the rising edges are delayed until the > beginning of the next period. > A new member was added in pwm_state structure in order to > keep the new PWM argument. To me it sound over-engineered. It looks like polarity type I dunno if PWM supports linked / virtual channels, but it would be like that channel X (polarity P) effectively is channel Xa (polarity P) / channel Xb (polarity !P) and vise versa. Moreover, mode in your case doesn't fit to GPIO style of output which would be emulated or native mode for PWM (we have already a use case and one driver implements that). GPIO type of output is, obviously, duty=100% in case of emulation, though separate state for HW assisted kind of that. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] drivers: pwm: core: implement pwm mode 2017-05-27 22:11 ` Andy Shevchenko @ 2017-05-30 9:49 ` m18063 0 siblings, 0 replies; 13+ messages in thread From: m18063 @ 2017-05-30 9:49 UTC (permalink / raw) To: linux-arm-kernel Hi Andy, On 28.05.2017 01:11, Andy Shevchenko wrote: > On Tue, May 9, 2017 at 3:15 PM, Claudiu Beznea > <claudiu.beznea@microchip.com> wrote: >> Extends PWM framework to support PWM modes. The currently >> implemented PWM modes were called PWM complementary mode >> and PWM push-pull mode. For devices that have more than one >> output per PWM channel: >> - PWM complementary mode is standard working mode; in PWM >> complementary mode the rising and falling edges of the >> channels outputs have opposite levels, same duration and >> same starting time. >> - in PWM push-pull mode the channles outputs has same levels, >> same duration and the rising edges are delayed until the >> beginning of the next period. >> A new member was added in pwm_state structure in order to >> keep the new PWM argument. > > To me it sound over-engineered. > > It looks like polarity type Could you, please, give me more details on this? I don't understand your point. The PWM controller I worked with has more than one physical output per PWM channel (e.g. a linux PWM channel has 2 output pins, as you said below, one with polarity P, the other one with polarity !P). Different controllers could be configured to generate different kind of signals. PWM push-pull signal type is one of these and is a common characteristic. This kind of signal could be used, e.g., in half-bridge converter applications where you need delays between positive fronts of PWM signals which drives the transistors. It is a common characteristic of PWM controllers which is the reason I tried to implement it. The push-pull signal type looks like this in case of a controller with 2 outputs per PWM channel: __ __ channel Xa __| |________| |________ __ __ channel Xb ________| |________| |__ <--T--> > > I dunno if PWM supports linked / virtual channels, but it would be like that > > channel X (polarity P) effectively is > > channel Xa (polarity P) / channel Xb (polarity !P)> > and vise versa. The controller I worked for, the Atmel one, support 4 linux channels, but every channel has 2 physical outputs, 2 pins which outputs, by default, a signal with polarity P and a signal with polarity !P, exactly how you said. I called this in my patch the "complementary" mode. Signals looks like this: __ __ __ __ channel Xa __| |__| |__| |__| |__ __ __ __ __ __ channel Xb |__| |__| |__| |__| <--T--> > > Moreover, mode in your case doesn't fit to GPIO style of output which > would be emulated or native mode for PWM For the PWM controller I worked for, one linux PWM channel outputs on 2 physical pins. Those 2 pins are requested from pin controller to be used by PWM controller. The PWM controller is the one which generate the signal form on those pins. Could you please give more details on what what are you saying above? (we have already a use case > and one driver implements that). Could you, please, give me the driver name you are talking about? On PWM drivers I found only one driver which is started in push-pull, the drivers/pwm/pwm-bcm-kona.c one. The idea with the patches I send was to have a chip which could be user configurable to let the user choose how the chip to work. Thank you, Claudiu Beznea > > GPIO type of output is, obviously, duty=100% in case of emulation, > though separate state for HW assisted kind of that. > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] drivers: pwm: core: implement pwm mode 2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea 2017-05-27 22:11 ` Andy Shevchenko @ 2017-07-19 12:13 ` m18063 1 sibling, 0 replies; 13+ messages in thread From: m18063 @ 2017-07-19 12:13 UTC (permalink / raw) To: linux-arm-kernel Hello Thierry, I know you may be very busy but do you have any resolution regarding this series and the one with title "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" Thank you, Claudiu On 09.05.2017 15:15, Claudiu Beznea - M18063 wrote: > Extends PWM framework to support PWM modes. The currently > implemented PWM modes were called PWM complementary mode > and PWM push-pull mode. For devices that have more than one > output per PWM channel: > - PWM complementary mode is standard working mode; in PWM > complementary mode the rising and falling edges of the > channels outputs have opposite levels, same duration and > same starting time. > - in PWM push-pull mode the channles outputs has same levels, > same duration and the rising edges are delayed until the > beginning of the next period. > A new member was added in pwm_state structure in order to > keep the new PWM argument. > For PWM channels with inputs in DT, the mode could also be > passed via DT. No new extra field need to be added in device > tree; since the signal polarity is passed as a flag via DT, > the field used to pass information for PWM channel polarity > via DT is also used by PWM mode. Since, for the moment, only > the complementary and push-pull modes are implemented, only > one bit in flags used to pass polarity could also be used for > PWM mode. The other drivers are not affected by this change. > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > Documentation/pwm.txt | 24 +++++++++++++++++--- > drivers/pwm/core.c | 13 +++++++++-- > drivers/pwm/sysfs.c | 52 +++++++++++++++++++++++++++++++++++++++++++ > include/dt-bindings/pwm/pwm.h | 1 + > include/linux/pwm.h | 37 ++++++++++++++++++++++++++++-- > 5 files changed, 120 insertions(+), 7 deletions(-) > > diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt > index 789b27c..1b6fc59 100644 > --- a/Documentation/pwm.txt > +++ b/Documentation/pwm.txt > @@ -59,9 +59,9 @@ In addition to the PWM state, the PWM API also exposes PWM arguments, which > are the reference PWM config one should use on this PWM. > PWM arguments are usually platform-specific and allows the PWM user to only > care about dutycycle relatively to the full period (like, duty = 50% of the > -period). struct pwm_args contains 2 fields (period and polarity) and should > -be used to set the initial PWM config (usually done in the probe function > -of the PWM user). PWM arguments are retrieved with pwm_get_args(). > +period). struct pwm_args contains 3 fields (period, polarity and mode) and > +should be used to set the initial PWM config (usually done in the probe > +function of the PWM user). PWM arguments are retrieved with pwm_get_args(). > > Using PWMs with the sysfs interface > ----------------------------------- > @@ -100,6 +100,24 @@ enable - Enable/disable the PWM signal (read/write). > 0 - disabled > 1 - enabled > > +mode - Set PWM signal type (for channels with more than one output > + per channel) > + complementary - for an output signal as follow: > + __ __ __ __ > + PWMH1 __| |__| |__| |__| |__ > + __ __ __ __ __ > + PWML1 |__| |__| |__| |__| > + <--T--> > + Where T is the signal period. > + > + push-pull - for an output signal as follows: > + __ __ > + PWMH1 __| |________| |________ > + __ __ > + PWML1 ________| |________| |__ > + <--T--> > + Where T is the signal period. > + > Implementing a PWM driver > ------------------------- > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index a0860b3..4efc92d 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -154,9 +154,14 @@ of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args) > > pwm->args.period = args->args[1]; > pwm->args.polarity = PWM_POLARITY_NORMAL; > + pwm->args.mode = PWM_MODE_COMPLEMENTARY; > > - if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) > - pwm->args.polarity = PWM_POLARITY_INVERSED; > + if (args->args_count > 2) { > + if (args->args[2] & PWM_POLARITY_INVERTED) > + pwm->args.polarity = PWM_POLARITY_INVERSED; > + if (args->args[2] & PWM_MODE_PUSHPULL) > + pwm->args.mode = PWM_MODE_PUSH_PULL; > + } > > return pwm; > } > @@ -579,6 +584,8 @@ int pwm_adjust_config(struct pwm_device *pwm) > pwm_get_args(pwm, &pargs); > pwm_get_state(pwm, &state); > > + state.mode = pargs.mode; > + > /* > * If the current period is zero it means that either the PWM driver > * does not support initial state retrieval or the PWM has not yet > @@ -997,6 +1004,8 @@ static void pwm_dbg_show(struct pwm_chip *chip, struct seq_file *s) > seq_printf(s, " duty: %u ns", state.duty_cycle); > seq_printf(s, " polarity: %s", > state.polarity ? "inverse" : "normal"); > + seq_printf(s, " mode: %s", > + state.mode ? "push-pull" : "complementary"); > > seq_puts(s, "\n"); > } > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c > index a813239..58f02bf 100644 > --- a/drivers/pwm/sysfs.c > +++ b/drivers/pwm/sysfs.c > @@ -223,11 +223,62 @@ static ssize_t capture_show(struct device *child, > return sprintf(buf, "%u %u\n", result.period, result.duty_cycle); > } > > +static ssize_t mode_show(struct device *child, > + struct device_attribute *attr, > + char *buf) > +{ > + const struct pwm_device *pwm = child_to_pwm_device(child); > + const char *mode = "unknown"; > + struct pwm_state state; > + > + pwm_get_state(pwm, &state); > + > + switch (state.mode) { > + case PWM_MODE_COMPLEMENTARY: > + mode = "complementary"; > + break; > + > + case PWM_MODE_PUSH_PULL: > + mode = "push-pull"; > + break; > + } > + > + return sprintf(buf, "%s\n", mode); > +} > + > +static ssize_t mode_store(struct device *child, > + struct device_attribute *attr, > + const char *buf, size_t size) > +{ > + struct pwm_export *export = child_to_pwm_export(child); > + struct pwm_device *pwm = export->pwm; > + enum pwm_mode mode; > + struct pwm_state state; > + int ret; > + > + if (sysfs_streq(buf, "complementary")) > + mode = PWM_MODE_COMPLEMENTARY; > + else if (sysfs_streq(buf, "push-pull")) > + mode = PWM_MODE_PUSH_PULL; > + else > + return -EINVAL; > + > + mutex_lock(&export->lock); > + pwm_get_state(pwm, &state); > + state.mode = mode; > + ret = pwm_apply_state(pwm, &state); > + mutex_unlock(&export->lock); > + > + return ret ? : size; > +} > + > static DEVICE_ATTR_RW(period); > static DEVICE_ATTR_RW(duty_cycle); > static DEVICE_ATTR_RW(enable); > static DEVICE_ATTR_RW(polarity); > static DEVICE_ATTR_RO(capture); > +static DEVICE_ATTR_RW(mode); > > static struct attribute *pwm_attrs[] = { > &dev_attr_period.attr, > @@ -235,6 +286,7 @@ static struct attribute *pwm_attrs[] = { > &dev_attr_enable.attr, > &dev_attr_polarity.attr, > &dev_attr_capture.attr, > + &dev_attr_mode.attr, > NULL > }; > ATTRIBUTE_GROUPS(pwm); > diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h > index 96f49e8..83bb884 100644 > --- a/include/dt-bindings/pwm/pwm.h > +++ b/include/dt-bindings/pwm/pwm.h > @@ -10,5 +10,6 @@ > #define _DT_BINDINGS_PWM_PWM_H > > #define PWM_POLARITY_INVERTED (1 << 0) > +#define PWM_MODE_PUSHPULL (1 << 1) > > #endif > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index 08fad7c..d924e55 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -25,9 +25,23 @@ enum pwm_polarity { > }; > > /** > + * enum pwm_mode - PWM mode > + * @PWM_MODE_COMPLEMENTARY: rising and falling edges of the complementary > + * PWM channels have opposite levels, same duration and same starting time > + * @PWM_MODE_PUSH_PULL: rising and falling edges of the complementary PWM > + * channels have same levels, same duration and are delayed until the > + * beginning of the next period > + */ > +enum pwm_mode { > + PWM_MODE_COMPLEMENTARY, > + PWM_MODE_PUSH_PULL, > +}; > + > +/** > * struct pwm_args - board-dependent PWM arguments > * @period: reference period > * @polarity: reference polarity > + * @mode: pwm mode > * > * This structure describes board-dependent arguments attached to a PWM > * device. These arguments are usually retrieved from the PWM lookup table or > @@ -40,6 +54,7 @@ enum pwm_polarity { > struct pwm_args { > unsigned int period; > enum pwm_polarity polarity; > + enum pwm_mode mode; > }; > > enum { > @@ -52,12 +67,14 @@ enum { > * @period: PWM period (in nanoseconds) > * @duty_cycle: PWM duty cycle (in nanoseconds) > * @polarity: PWM polarity > + * @mode: PWM mode > * @enabled: PWM enabled status > */ > struct pwm_state { > unsigned int period; > unsigned int duty_cycle; > enum pwm_polarity polarity; > + enum pwm_mode mode; > bool enabled; > }; > > @@ -143,6 +160,21 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm) > return state.polarity; > } > > +static inline void pwm_set_mode(struct pwm_device *pwm, enum pwm_mode mode) > +{ > + if (pwm) > + pwm->state.mode = mode; > +} > + > +static inline enum pwm_mode pwm_get_mode(const struct pwm_device *pwm) > +{ > + struct pwm_state state; > + > + pwm_get_state(pwm, &state); > + > + return state.mode; > +} > + > static inline void pwm_get_args(const struct pwm_device *pwm, > struct pwm_args *args) > { > @@ -156,8 +188,8 @@ static inline void pwm_get_args(const struct pwm_device *pwm, > * > * This functions prepares a state that can later be tweaked and applied > * to the PWM device with pwm_apply_state(). This is a convenient function > - * that first retrieves the current PWM state and the replaces the period > - * and polarity fields with the reference values defined in pwm->args. > + * that first retrieves the current PWM state and the replaces the period, > + * polarity and mode fields with the reference values defined in pwm->args. > * Once the function returns, you can adjust the ->enabled and ->duty_cycle > * fields according to your needs before calling pwm_apply_state(). > * > @@ -180,6 +212,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->mode = args.mode; > } > > /** > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] drivers: pwm: pwm-atmel: add support for pwm modes 2017-05-09 12:15 [PATCH 0/2] extend PWM framework to support PWM dead-times Claudiu Beznea 2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea @ 2017-05-09 12:15 ` Claudiu Beznea 2017-08-21 8:25 ` [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding 2 siblings, 0 replies; 13+ messages in thread From: Claudiu Beznea @ 2017-05-09 12:15 UTC (permalink / raw) To: linux-arm-kernel Implement pwm modes by adding PWM controller specific configuration. Since this driver is used by controllers which supports PWM push-pull mode and controllers which doesn't, a new field was added in driver private data structure. Also, the driver private data structure was changed to adapt the new feature. Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/pwm/pwm-atmel.c | 103 +++++++++++++++++++++++++++++++++--------------- 1 file changed, 72 insertions(+), 31 deletions(-) diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c index 530d7dc..9d93cca 100644 --- a/drivers/pwm/pwm-atmel.c +++ b/drivers/pwm/pwm-atmel.c @@ -33,8 +33,11 @@ #define PWM_CMR 0x0 /* Bit field in CMR */ -#define PWM_CMR_CPOL (1 << 9) -#define PWM_CMR_UPD_CDTY (1 << 10) +#define PWM_CMR_CPOL BIT(9) +#define PWM_CMR_UPD_CDTY BIT(10) +#define PWM_CMR_DTHI BIT(17) +#define PWM_CMR_DTLI BIT(18) +#define PWM_CMR_PPM BIT(19) #define PWM_CMR_CPRE_MSK 0xF /* The following registers for PWM v1 */ @@ -65,11 +68,16 @@ struct atmel_pwm_registers { u8 duty_upd; }; +struct atmel_pwm_data { + struct atmel_pwm_registers regs; + bool ppm_supported; +}; + struct atmel_pwm_chip { struct pwm_chip chip; struct clk *clk; void __iomem *base; - const struct atmel_pwm_registers *regs; + const struct atmel_pwm_data *data; unsigned int updated_pwms; /* ISR is cleared when read, ensure only one thread does that */ @@ -150,15 +158,15 @@ static void atmel_pwm_update_cdty(struct pwm_chip *chip, struct pwm_device *pwm, struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); u32 val; - if (atmel_pwm->regs->duty_upd == - atmel_pwm->regs->period_upd) { + if (atmel_pwm->data->regs.duty_upd == + atmel_pwm->data->regs.period_upd) { val = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, PWM_CMR); val &= ~PWM_CMR_UPD_CDTY; atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); } atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, - atmel_pwm->regs->duty_upd, cdty); + atmel_pwm->data->regs.duty_upd, cdty); } static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip, @@ -168,9 +176,9 @@ static void atmel_pwm_set_cprd_cdty(struct pwm_chip *chip, struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip); atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, - atmel_pwm->regs->duty, cdty); + atmel_pwm->data->regs.duty, cdty); atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, - atmel_pwm->regs->period, cprd); + atmel_pwm->data->regs.period, cprd); } static void atmel_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm, @@ -223,9 +231,10 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, if (state->enabled) { if (cstate.enabled && cstate.polarity == state->polarity && - cstate.period == state->period) { + cstate.period == state->period && + cstate.mode == state->mode) { cprd = atmel_pwm_ch_readl(atmel_pwm, pwm->hwpwm, - atmel_pwm->regs->period); + atmel_pwm->data->regs.period); atmel_pwm_calculate_cdty(state, cprd, &cdty); atmel_pwm_update_cdty(chip, pwm, cdty); return 0; @@ -258,6 +267,23 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, val &= ~PWM_CMR_CPOL; else val |= PWM_CMR_CPOL; + + /* PWM mode. */ + if (atmel_pwm->data->ppm_supported) { + if (state->mode == PWM_MODE_PUSH_PULL) { + val |= PWM_CMR_PPM; + if (state->polarity == PWM_POLARITY_NORMAL) + val = (val & ~PWM_CMR_DTHI) | + PWM_CMR_DTLI; + else + val = (val & ~PWM_CMR_DTLI) | + PWM_CMR_DTHI; + } else { + val &= ~(PWM_CMR_PPM | PWM_CMR_DTLI | + PWM_CMR_DTHI); + } + } + atmel_pwm_ch_writel(atmel_pwm, pwm->hwpwm, PWM_CMR, val); atmel_pwm_set_cprd_cdty(chip, pwm, cprd, cdty); mutex_lock(&atmel_pwm->isr_lock); @@ -277,27 +303,42 @@ static const struct pwm_ops atmel_pwm_ops = { .owner = THIS_MODULE, }; -static const struct atmel_pwm_registers atmel_pwm_regs_v1 = { - .period = PWMV1_CPRD, - .period_upd = PWMV1_CUPD, - .duty = PWMV1_CDTY, - .duty_upd = PWMV1_CUPD, +static const struct atmel_pwm_data atmel_pwm_data_v1 = { + .regs = { + .period = PWMV1_CPRD, + .period_upd = PWMV1_CUPD, + .duty = PWMV1_CDTY, + .duty_upd = PWMV1_CUPD, + }, + .ppm_supported = false, }; -static const struct atmel_pwm_registers atmel_pwm_regs_v2 = { - .period = PWMV2_CPRD, - .period_upd = PWMV2_CPRDUPD, - .duty = PWMV2_CDTY, - .duty_upd = PWMV2_CDTYUPD, +static const struct atmel_pwm_data atmel_pwm_data_v2 = { + .regs = { + .period = PWMV2_CPRD, + .period_upd = PWMV2_CPRDUPD, + .duty = PWMV2_CDTY, + .duty_upd = PWMV2_CDTYUPD, + }, + .ppm_supported = false, +}; + +static const struct atmel_pwm_data atmel_pwm_data_v3 = { + .regs = { + .period = PWMV2_CPRD, + .period_upd = PWMV2_CPRDUPD, + .duty = PWMV2_CDTY, + .duty_upd = PWMV2_CDTYUPD, + }, + .ppm_supported = true, }; static const struct platform_device_id atmel_pwm_devtypes[] = { { .name = "at91sam9rl-pwm", - .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v1, + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v1, }, { .name = "sama5d3-pwm", - .driver_data = (kernel_ulong_t)&atmel_pwm_regs_v2, + .driver_data = (kernel_ulong_t)&atmel_pwm_data_v2, }, { /* sentinel */ }, @@ -307,20 +348,20 @@ MODULE_DEVICE_TABLE(platform, atmel_pwm_devtypes); static const struct of_device_id atmel_pwm_dt_ids[] = { { .compatible = "atmel,at91sam9rl-pwm", - .data = &atmel_pwm_regs_v1, + .data = &atmel_pwm_data_v1, }, { .compatible = "atmel,sama5d3-pwm", - .data = &atmel_pwm_regs_v2, + .data = &atmel_pwm_data_v2, }, { .compatible = "atmel,sama5d2-pwm", - .data = &atmel_pwm_regs_v2, + .data = &atmel_pwm_data_v3, }, { /* sentinel */ }, }; MODULE_DEVICE_TABLE(of, atmel_pwm_dt_ids); -static inline const struct atmel_pwm_registers * +static inline const struct atmel_pwm_data* atmel_pwm_get_driver_data(struct platform_device *pdev) { const struct platform_device_id *id; @@ -330,18 +371,18 @@ atmel_pwm_get_driver_data(struct platform_device *pdev) id = platform_get_device_id(pdev); - return (struct atmel_pwm_registers *)id->driver_data; + return (struct atmel_pwm_data *)id->driver_data; } static int atmel_pwm_probe(struct platform_device *pdev) { - const struct atmel_pwm_registers *regs; + const struct atmel_pwm_data *data; struct atmel_pwm_chip *atmel_pwm; struct resource *res; int ret; - regs = atmel_pwm_get_driver_data(pdev); - if (!regs) + data = atmel_pwm_get_driver_data(pdev); + if (!data) return -ENODEV; atmel_pwm = devm_kzalloc(&pdev->dev, sizeof(*atmel_pwm), GFP_KERNEL); @@ -373,7 +414,7 @@ static int atmel_pwm_probe(struct platform_device *pdev) atmel_pwm->chip.base = -1; atmel_pwm->chip.npwm = 4; - atmel_pwm->regs = regs; + atmel_pwm->data = data; atmel_pwm->updated_pwms = 0; mutex_init(&atmel_pwm->isr_lock); -- 2.7.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-05-09 12:15 [PATCH 0/2] extend PWM framework to support PWM dead-times Claudiu Beznea 2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea 2017-05-09 12:15 ` [PATCH 2/2] drivers: pwm: pwm-atmel: add support for pwm modes Claudiu Beznea @ 2017-08-21 8:25 ` Thierry Reding 2017-08-21 10:23 ` m18063 2 siblings, 1 reply; 13+ messages in thread From: Thierry Reding @ 2017-08-21 8:25 UTC (permalink / raw) To: linux-arm-kernel On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: > Hi all, > > Please give feedback on these patches which extends the PWM > framework in order to support multiple PWM signal types. > Since I didn't receive any inputs on RFC series I'm resending it as > normal patch series. > > The current patch series add the following PWM signal types: > - PWM complementary signals > - PWM push-pull signal > > Definition of PWM complementary mode: > For a PWM controllers with more than one outputs per PWM channel, > e.g. with 2 outputs per PWM channels, the PWM complementary signals > have opposite levels, same duration and same starting times, > as in the following diagram: > > __ __ __ __ > PWMH __| |__| |__| |__| |__ > __ __ __ __ __ > PWML |__| |__| |__| |__| > <--T--> > Where T is the signal period. > > Definition of PWM push-pull mode: > For PWM controllers with more than one outputs per PWM channel, > e.g. with 2 outputs per PWM channel, the PWM push-pull signals > have same levels, same duration and are delayed until the begining > of the next period, as in the following diagram: > > __ __ > PWMH __| |________| |________ > __ __ > PWML ________| |________| |__ > <--T--> > > Where T is the signal period. > > These output signals could be configured by setting PWM mode > (a new input in sysfs has been added in order to support this > operation). > > root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l > -r--r--r-- 1 root root 4096 Feb 9 10:54 capture > -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle > -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable > -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode > -rw-r--r-- 1 root root 4096 Feb 9 10:54 period > -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity > drwxr-xr-x 2 root root 0 Feb 9 10:54 power > -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent > > The PWM push-pull mode could be usefull in applications like > half bridge converters. Sorry for taking an absurdly long time to get back to you on this. One problem I see with this is that the PWM framework is built on the assumption that we have a single output per PWM channel. This becomes important when you start adding features such as this because now the users have no way of determining whether or not the PWM they retrieve will actually be able to do what they want. For example, let's say you have a half bridge converter driver in the kernel and it does a pwm_get() to obtain a PWM to use. There's nothing preventing it from using a simple one-output PWM and there's no way to check that we're actually doing something wrong. I think any in-kernel API would have to be more fully-featured, otherwise we're bound to run into problems. At the very least I think we'd have to expose some sort of capabilities list. A possibly simpler approach might be to restrict this to the driver that you need this for. It looks like you will be mainly using this via sysfs and in that case you might be able to just extend what information is exposed in sysfs for the particular PWM you're dealing with. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/7b2791ef/attachment.sig> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-21 8:25 ` [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding @ 2017-08-21 10:23 ` m18063 2017-08-21 11:25 ` Thierry Reding 0 siblings, 1 reply; 13+ messages in thread From: m18063 @ 2017-08-21 10:23 UTC (permalink / raw) To: linux-arm-kernel Hi Thierry, Thank you for your response. I added few comments below. Thank you, Claudiu On 21.08.2017 11:25, Thierry Reding wrote: > On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >> Hi all, >> >> Please give feedback on these patches which extends the PWM >> framework in order to support multiple PWM signal types. >> Since I didn't receive any inputs on RFC series I'm resending it as >> normal patch series. >> >> The current patch series add the following PWM signal types: >> - PWM complementary signals >> - PWM push-pull signal >> >> Definition of PWM complementary mode: >> For a PWM controllers with more than one outputs per PWM channel, >> e.g. with 2 outputs per PWM channels, the PWM complementary signals >> have opposite levels, same duration and same starting times, >> as in the following diagram: >> >> __ __ __ __ >> PWMH __| |__| |__| |__| |__ >> __ __ __ __ __ >> PWML |__| |__| |__| |__| >> <--T--> >> Where T is the signal period. >> >> Definition of PWM push-pull mode: >> For PWM controllers with more than one outputs per PWM channel, >> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >> have same levels, same duration and are delayed until the begining >> of the next period, as in the following diagram: >> >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> >> >> Where T is the signal period. >> >> These output signals could be configured by setting PWM mode >> (a new input in sysfs has been added in order to support this >> operation). >> >> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >> >> The PWM push-pull mode could be usefull in applications like >> half bridge converters. > > Sorry for taking an absurdly long time to get back to you on this. > > One problem I see with this is that the PWM framework is built on the > assumption that we have a single output per PWM channel. This becomes > important when you start adding features such as this because now the > users have no way of determining whether or not the PWM they retrieve > will actually be able to do what they want. I was thinking that the framework is there to offer support in configuring the underlying hardware without taking into account how many outputs per PWM channels are actually present. It is true I only worked with Atmel/Microchip PWM controllers which for instance, SAMA5 SoC has a PWM controller with 2 outputs per PWM channel. Taking into account that the driver is already mainlined I though that the user is aware of what he is using (either a one output per PWM channel controller, or 2 outputs or more than 2) and about the underlying hardware support. I understand your position on this. I'm just explaining myself on the approach I've chose for this implementation. > > For example, let's say you have a half bridge converter driver in the > kernel and it does a pwm_get() to obtain a PWM to use. There's nothing > preventing it from using a simple one-output PWM and there's no way to > check that we're actually doing something wrong. I understand your position here. I've chose this approach taking into account that the user of the PWM will be aware of the capabilities of the underlying hardware because I worked on a controller which already has a mainlined driver and which has more than one output per PWM channel and I believe there are also other controllers with this kind of capability and with already mainlined driver (e.g. reading the code of STM32 PWM driver I saw a bool type variable in driver specific data structure (struct stm32_pwm): "bool have_complementary_output" which let me think that their controller also could support more than one output per PWM channel (I will also try to find the controller specific datasheet). At a first look I saw that also TI ECAP PWM controller supports this (it is true that I am not aware of how it is initialized in kernel, I need to check the datasheet, if it is public). > > I think any in-kernel API would have to be more fully-featured, > otherwise we're bound to run into problems. At the very least I think > we'd have to expose some sort of capabilities list. About the exposing of these capabilities would you prefer to have the supported PWM modes registered in driver probe as a new field mask in "struct pwm_chip". e.g.: struct pwm_chip { struct device *dev; struct list_head list; const struct pwm_ops *ops; int base; unsigned int npwm; unsigned int pwm_modes_mask; /* bitfield of supported signal types */ struct pwm_device *pwms; struct pwm_device * (*of_xlate)(struct pwm_chip *pc, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; }; And having in driver_probe() something like this: driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; pwmchip_add(&driver_data->chip); Since the PWM push-pull mode imply more than one output per PWM channel. And validate the supported PWM modes when trying to configure one. Or registering, as a field in pwm_chip, how many outputs per channel are actually supported by the PWM controller and then validate the supported PWM mode based on this? e.g.: in "struct pwm_chip". e.g.: struct pwm_chip { struct device *dev; struct list_head list; const struct pwm_ops *ops; int base; unsigned int npwm; unsigned int pwm_outputs; /* Number of supported outputs per channel */ struct pwm_device *pwms; struct pwm_device * (*of_xlate)(struct pwm_chip *pc, const struct of_phandle_args *args); unsigned int of_pwm_n_cells; }; In driver_probe(): //... driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ pwmchip_add(&driver_data->chip); > > A possibly simpler approach might be to restrict this to the driver that > you need this for. It looks like you will be mainly using this via sysfs > and in that case you might be able to just extend what information is > exposed in sysfs for the particular PWM you're dealing with. By this you mean exporting the sysfs attributes only for my driver or possibly other drivers interested in this new feature? I may have another in kernel driver which may try to use this feature. Thank you, Claudiu > > Thierry > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-21 10:23 ` m18063 @ 2017-08-21 11:25 ` Thierry Reding 2017-08-22 12:11 ` m18063 0 siblings, 1 reply; 13+ messages in thread From: Thierry Reding @ 2017-08-21 11:25 UTC (permalink / raw) To: linux-arm-kernel On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: > Hi Thierry, > > Thank you for your response. I added few comments below. > > Thank you, > Claudiu > > On 21.08.2017 11:25, Thierry Reding wrote: > > On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: > >> Hi all, > >> > >> Please give feedback on these patches which extends the PWM > >> framework in order to support multiple PWM signal types. > >> Since I didn't receive any inputs on RFC series I'm resending it as > >> normal patch series. > >> > >> The current patch series add the following PWM signal types: > >> - PWM complementary signals > >> - PWM push-pull signal > >> > >> Definition of PWM complementary mode: > >> For a PWM controllers with more than one outputs per PWM channel, > >> e.g. with 2 outputs per PWM channels, the PWM complementary signals > >> have opposite levels, same duration and same starting times, > >> as in the following diagram: > >> > >> __ __ __ __ > >> PWMH __| |__| |__| |__| |__ > >> __ __ __ __ __ > >> PWML |__| |__| |__| |__| > >> <--T--> > >> Where T is the signal period. > >> > >> Definition of PWM push-pull mode: > >> For PWM controllers with more than one outputs per PWM channel, > >> e.g. with 2 outputs per PWM channel, the PWM push-pull signals > >> have same levels, same duration and are delayed until the begining > >> of the next period, as in the following diagram: > >> > >> __ __ > >> PWMH __| |________| |________ > >> __ __ > >> PWML ________| |________| |__ > >> <--T--> > >> > >> Where T is the signal period. > >> > >> These output signals could be configured by setting PWM mode > >> (a new input in sysfs has been added in order to support this > >> operation). > >> > >> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l > >> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture > >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle > >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable > >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode > >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period > >> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity > >> drwxr-xr-x 2 root root 0 Feb 9 10:54 power > >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent > >> > >> The PWM push-pull mode could be usefull in applications like > >> half bridge converters. > > > > Sorry for taking an absurdly long time to get back to you on this. > > > > One problem I see with this is that the PWM framework is built on the > > assumption that we have a single output per PWM channel. This becomes > > important when you start adding features such as this because now the > > users have no way of determining whether or not the PWM they retrieve > > will actually be able to do what they want. > I was thinking that the framework is there to offer support in configuring > the underlying hardware without taking into account how many outputs per > PWM channels are actually present. It is true I only worked with Atmel/Microchip > PWM controllers which for instance, SAMA5 SoC has a PWM controller > with 2 outputs per PWM channel. Taking into account that the driver is > already mainlined I though that the user is aware of what he is using > (either a one output per PWM channel controller, or 2 outputs or > more than 2) and about the underlying hardware support. I understand your > position on this. I'm just explaining myself on the approach I've chose > for this implementation. So currently we assume that there will be (at least) one output per PWM channel. However there are currently no drivers that actually use any output other than the "primary" one. That means, even if there are currently PWM controllers that support multiple outputs per PWM channel, we have no code making assumptions about it. Before we can add code that makes any assumptions about the number of outputs per PWM channel, we have to add facitilities for such code to detect capabilities, so that they can deal with PWMs that don't match what they need. > > For example, let's say you have a half bridge converter driver in the > > kernel and it does a pwm_get() to obtain a PWM to use. There's nothing > > preventing it from using a simple one-output PWM and there's no way to > > check that we're actually doing something wrong. > I understand your position here. I've chose this approach taking into > account that the user of the PWM will be aware of the capabilities > of the underlying hardware because I worked on a controller which already > has a mainlined driver and which has more than one output per PWM channel > and I believe there are also other controllers with this kind of capability > and with already mainlined driver (e.g. reading the code of STM32 PWM driver > I saw a bool type variable in driver specific data structure (struct stm32_pwm): > "bool have_complementary_output" which let me think that their controller > also could support more than one output per PWM channel (I will also try > to find the controller specific datasheet). At a first look I saw that also > TI ECAP PWM controller supports this (it is true that I am not aware of how > it is initialized in kernel, I need to check the datasheet, if it is public). Yes, it's quite possible that many of the controllers we currently support have this feature or not. But that's not really relevant. The only important bit is that none of the users know about it. We don't have the means to configure anything beyond just one output. So the only guarantee you will get with the current framework is that there will be one output signal per PWM channel. > > I think any in-kernel API would have to be more fully-featured, > > otherwise we're bound to run into problems. At the very least I think > > we'd have to expose some sort of capabilities list. > About the exposing of these capabilities would you prefer to have the > supported PWM modes registered in driver probe as a new field mask > in "struct pwm_chip". e.g.: > struct pwm_chip { > struct device *dev; > struct list_head list; > const struct pwm_ops *ops; > int base; > unsigned int npwm; > unsigned int pwm_modes_mask; /* bitfield of supported signal types */ > > struct pwm_device *pwms; > > struct pwm_device * (*of_xlate)(struct pwm_chip *pc, > const struct of_phandle_args *args); > unsigned int of_pwm_n_cells; > }; > > And having in driver_probe() something like this: > driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; > pwmchip_add(&driver_data->chip); > Since the PWM push-pull mode imply more than one output per PWM channel. And > validate the supported PWM modes when trying to configure one. > > Or registering, as a field in pwm_chip, how many outputs per channel are actually > supported by the PWM controller and then validate the supported PWM mode based > on this? > e.g.: > in "struct pwm_chip". e.g.: > struct pwm_chip { > struct device *dev; > struct list_head list; > const struct pwm_ops *ops; > int base; > unsigned int npwm; > unsigned int pwm_outputs; /* Number of supported outputs per channel */ > > struct pwm_device *pwms; > > struct pwm_device * (*of_xlate)(struct pwm_chip *pc, > const struct of_phandle_args *args); > unsigned int of_pwm_n_cells; > }; > > In driver_probe(): > //... > driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ > pwmchip_add(&driver_data->chip); I think I'd prefer something more flexible. I also think this can't be per-chip, but really has to be at the granularity of single PWM channels. How about something like this: struct pwm_caps { ... }; int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); >From a high-level perspective that would give users an easy way to obtain the capabilities of the PWM they're handed. Then we can start filling in that structure. struct pwm_caps { unsigned long modes; }; #define PWM_MODE_NORMAL (1 << 0) #define PWM_MODE_COMPLEMENTARY (1 << 1) #define PWM_MODE_PUSH_PULL (1 << 2) static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, unsigned long mode) { return (caps->modes & mode) != 0; } and maybe something like this as a shortcut: static inline bool pwm_supports_mode(struct pwm_device *pwm, unsigned long mode) { struct pwm_caps caps; int err; err = pwm_get_caps(pwm, &caps); if (err < 0) return false; return pwm_caps_supports_mode(&caps, mode); } I think that gives us a lot of flexibility and easy extensibility for other capabilities. The implementation of pwm_get_caps() should probably call back into the driver. But that would cause a lot of boilerplate for simple cases, and would be a lot of work to convert all existing drivers up front. So I think we can add helpers to solve the normal cases. Something along these lines perhaps: struct pwm_chip { ... int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); const struct pwm_caps *caps; }; static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) { if (!pwm || !caps || !pwm->chip->caps) return -EINVAL; *caps = *pwm->chip->caps; return 0; } And then perhaps even do something like this: const struct pwm_caps pwm_default_caps = { .modes = PWM_MODE_NORMAL, }; Now we can either have a patch that sets pwm_default_caps and pwm_simple_get_caps() for all existing drivers, or even easier add this to the pwmchip_add_with_polarity() function: int pwmchip_add_with_polarity(struct pwm_chip *chip, enum pwm_polarity polarity) { ... if (!chip->get_caps && !chip->caps) { chip->get_caps = pwm_simple_get_caps; chip->caps = pwm_default_caps; } ... } That way, every driver, unless upgraded with a custom ->get_caps() would get the defaults, which encode the current assumptions of the framework. I think what we'd also want to do is make sure that every driver always at least implement NORMAL mode and perhaps even fail to register those that don't. Not sure we want to go that far, but those are the current assumptions, so devices must be supporting it already anyway. > > A possibly simpler approach might be to restrict this to the driver that > > you need this for. It looks like you will be mainly using this via sysfs > > and in that case you might be able to just extend what information is > > exposed in sysfs for the particular PWM you're dealing with. > By this you mean exporting the sysfs attributes only for my driver or possibly > other drivers interested in this new feature? I may have another in kernel driver > which may try to use this feature. If you've got another driver that will want to use this, I'm leaning towards going with the fully featured approach. Ideally you would try to implement both at once so we can get a better feeling about whether or not the framework changes are adequate to cover at least the first two cases, which is much more reassuring than just being able to cover a single case. One thing to consider is still that if you only use this from sysfs, it'll be quite a lot of work to add in-kernel infrastructure when it's never needed. There's still an advantage to do it anyway because it allows us to abstract everything away properly from the start and avoid fragmentation of the sysfs interface. Thierry -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 833 bytes Desc: not available URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170821/ea16083c/attachment-0001.sig> ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-21 11:25 ` Thierry Reding @ 2017-08-22 12:11 ` m18063 2017-08-22 12:24 ` Benjamin Gaignard 0 siblings, 1 reply; 13+ messages in thread From: m18063 @ 2017-08-22 12:11 UTC (permalink / raw) To: linux-arm-kernel Hi Thierry, I added few other comments below. Please let me know what you think. Thank you, Claudiu On 21.08.2017 14:25, Thierry Reding wrote: > On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: >> Hi Thierry, >> >> Thank you for your response. I added few comments below. >> >> Thank you, >> Claudiu >> >> On 21.08.2017 11:25, Thierry Reding wrote: >>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >>>> Hi all, >>>> >>>> Please give feedback on these patches which extends the PWM >>>> framework in order to support multiple PWM signal types. >>>> Since I didn't receive any inputs on RFC series I'm resending it as >>>> normal patch series. >>>> >>>> The current patch series add the following PWM signal types: >>>> - PWM complementary signals >>>> - PWM push-pull signal >>>> >>>> Definition of PWM complementary mode: >>>> For a PWM controllers with more than one outputs per PWM channel, >>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals >>>> have opposite levels, same duration and same starting times, >>>> as in the following diagram: >>>> >>>> __ __ __ __ >>>> PWMH __| |__| |__| |__| |__ >>>> __ __ __ __ __ >>>> PWML |__| |__| |__| |__| >>>> <--T--> >>>> Where T is the signal period. >>>> >>>> Definition of PWM push-pull mode: >>>> For PWM controllers with more than one outputs per PWM channel, >>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >>>> have same levels, same duration and are delayed until the begining >>>> of the next period, as in the following diagram: >>>> >>>> __ __ >>>> PWMH __| |________| |________ >>>> __ __ >>>> PWML ________| |________| |__ >>>> <--T--> >>>> >>>> Where T is the signal period. >>>> >>>> These output signals could be configured by setting PWM mode >>>> (a new input in sysfs has been added in order to support this >>>> operation). >>>> >>>> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >>>> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >>>> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >>>> >>>> The PWM push-pull mode could be usefull in applications like >>>> half bridge converters. >>> >>> Sorry for taking an absurdly long time to get back to you on this. >>> >>> One problem I see with this is that the PWM framework is built on the >>> assumption that we have a single output per PWM channel. This becomes >>> important when you start adding features such as this because now the >>> users have no way of determining whether or not the PWM they retrieve >>> will actually be able to do what they want. >> I was thinking that the framework is there to offer support in configuring >> the underlying hardware without taking into account how many outputs per >> PWM channels are actually present. It is true I only worked with Atmel/Microchip >> PWM controllers which for instance, SAMA5 SoC has a PWM controller >> with 2 outputs per PWM channel. Taking into account that the driver is >> already mainlined I though that the user is aware of what he is using >> (either a one output per PWM channel controller, or 2 outputs or >> more than 2) and about the underlying hardware support. I understand your >> position on this. I'm just explaining myself on the approach I've chose >> for this implementation. > > So currently we assume that there will be (at least) one output per PWM > channel. However there are currently no drivers that actually use any > output other than the "primary" one. > > That means, even if there are currently PWM controllers that support > multiple outputs per PWM channel, we have no code making assumptions > about it. > > Before we can add code that makes any assumptions about the number of > outputs per PWM channel, we have to add facitilities for such code to > detect capabilities, so that they can deal with PWMs that don't match > what they need. > >>> For example, let's say you have a half bridge converter driver in the >>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing >>> preventing it from using a simple one-output PWM and there's no way to >>> check that we're actually doing something wrong. >> I understand your position here. I've chose this approach taking into >> account that the user of the PWM will be aware of the capabilities >> of the underlying hardware because I worked on a controller which already >> has a mainlined driver and which has more than one output per PWM channel >> and I believe there are also other controllers with this kind of capability >> and with already mainlined driver (e.g. reading the code of STM32 PWM driver >> I saw a bool type variable in driver specific data structure (struct stm32_pwm): >> "bool have_complementary_output" which let me think that their controller >> also could support more than one output per PWM channel (I will also try >> to find the controller specific datasheet). At a first look I saw that also >> TI ECAP PWM controller supports this (it is true that I am not aware of how >> it is initialized in kernel, I need to check the datasheet, if it is public). > > Yes, it's quite possible that many of the controllers we currently > support have this feature or not. But that's not really relevant. The > only important bit is that none of the users know about it. We don't > have the means to configure anything beyond just one output. So the only > guarantee you will get with the current framework is that there will be > one output signal per PWM channel. > >>> I think any in-kernel API would have to be more fully-featured, >>> otherwise we're bound to run into problems. At the very least I think >>> we'd have to expose some sort of capabilities list. >> About the exposing of these capabilities would you prefer to have the >> supported PWM modes registered in driver probe as a new field mask >> in "struct pwm_chip". e.g.: >> struct pwm_chip { >> struct device *dev; >> struct list_head list; >> const struct pwm_ops *ops; >> int base; >> unsigned int npwm; >> unsigned int pwm_modes_mask; /* bitfield of supported signal types */ >> >> struct pwm_device *pwms; >> >> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >> const struct of_phandle_args *args); >> unsigned int of_pwm_n_cells; >> }; >> >> And having in driver_probe() something like this: >> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; >> pwmchip_add(&driver_data->chip); >> Since the PWM push-pull mode imply more than one output per PWM channel. And >> validate the supported PWM modes when trying to configure one. >> >> Or registering, as a field in pwm_chip, how many outputs per channel are actually >> supported by the PWM controller and then validate the supported PWM mode based >> on this? >> e.g.: >> in "struct pwm_chip". e.g.: >> struct pwm_chip { >> struct device *dev; >> struct list_head list; >> const struct pwm_ops *ops; >> int base; >> unsigned int npwm; >> unsigned int pwm_outputs; /* Number of supported outputs per channel */>> >> struct pwm_device *pwms; >> >> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >> const struct of_phandle_args *args); >> unsigned int of_pwm_n_cells; >> }; >> >> In driver_probe(): >> //... >> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ >> pwmchip_add(&driver_data->chip); > > I think I'd prefer something more flexible. I also think this can't be > per-chip, but really has to be at the granularity of single PWM > channels. How about something like this: > > struct pwm_caps { > ... > }; > > int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); > > From a high-level perspective that would give users an easy way to > obtain the capabilities of the PWM they're handed. I forgot to mention, I don't know if you had time to look over the other patch series I send to extend the PWM framework: "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" (v2 since the auto build robot prompted me some compilation errors). In that series I proposed some extensions to be able to configure dead-times for PWM channels. Those changes are also specific to PWM controllers which has more than 1 output per PWM channel. Taking into account that: - this is also specific to PWM controllers with more than 1 output per PWM channel and - your proposed below to introduce capabilities specific to more than one output per PWM channel and not to introduce something to specify that the PWM chip or PWM device supports a number of X outputs per PWM channel and - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with lower delays b/w the edges of complementary outputs: Do you think it would be ok (in order to also have a way to configure PWM dead-times) to merge the changes of "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the changes that I will do for push-pull mode (with your below proposals) and have a single PWM mode for both PWM push-pull and PWM dead-times (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used to configure the dead-times that will work only when the PWM device supports push-pull mode and is configured in push-pull mode? Something like this: root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l -r--r--r-- 1 root root 4096 Feb 9 10:54 capture -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_re <<<< interface for configuring dead-time rising edge delay -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_fe <<<< interface for configuring dead-time falling edge delay -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode <<<< interface for PWM mode configuration -rw-r--r-- 1 root root 4096 Feb 9 10:54 period -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity drwxr-xr-x 2 root root 0 Feb 9 10:54 power -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent When user switch to push-pull mode it could start configure the dead-times (in nanoseconds) to obtain a signal of a form similar to: ____0 D____P ____ ____ ____ PWM signal __| |____| |____| |____| |____| |___ __ __ __ __ __ PWMH ____| |____re| |______| |______| |______| |___ __ __ __ __ __ __ PWML |______| |____fe| |______| |______| |______| (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover letter), and starting from a driver specific dead-time value (e.g. on Atmel/Microchip PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM outputs to actually switch to PWM push-pull mode (in this mode there is a delay of a period introduce on a PWM ouptut) to obtain a signal similar to this one: __ __ PWMH __| |________| |________ __ __ PWML ________| |________| |__ <--T--> Then we can start > filling in that structure. > > struct pwm_caps { > unsigned long modes; > }; > > #define PWM_MODE_NORMAL (1 << 0) > #define PWM_MODE_COMPLEMENTARY (1 << 1) > #define PWM_MODE_PUSH_PULL (1 << 2) > > static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, > unsigned long mode) > { > return (caps->modes & mode) != 0; > } > > and maybe something like this as a shortcut: > > static inline bool pwm_supports_mode(struct pwm_device *pwm, > unsigned long mode) > { > struct pwm_caps caps; > int err; > > err = pwm_get_caps(pwm, &caps); > if (err < 0) > return false; > > return pwm_caps_supports_mode(&caps, mode); > } > > I think that gives us a lot of flexibility and easy extensibility for > other capabilities. The implementation of pwm_get_caps() should probably > call back into the driver.> > But that would cause a lot of boilerplate for simple cases, and would be > a lot of work to convert all existing drivers up front. So I think we > can add helpers to solve the normal cases. Something along these lines > perhaps: > > struct pwm_chip { > ... > > int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); > > const struct pwm_caps *caps; > }; > > static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) > { > if (!pwm || !caps || !pwm->chip->caps) > return -EINVAL; > > *caps = *pwm->chip->caps; > > return 0; > } > > And then perhaps even do something like this: > > const struct pwm_caps pwm_default_caps = { > .modes = PWM_MODE_NORMAL, > }; > > Now we can either have a patch that sets pwm_default_caps and > pwm_simple_get_caps() for all existing drivers, or even easier add this > to the pwmchip_add_with_polarity() function: > > int pwmchip_add_with_polarity(struct pwm_chip *chip, > enum pwm_polarity polarity) > { > ... > > if (!chip->get_caps && !chip->caps) { > chip->get_caps = pwm_simple_get_caps; > chip->caps = pwm_default_caps; > } > > ... > } > What about having a new member in "struct pwm_ops" something like this: struct pwm_ops { // ... int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps); // ... }; to get from the driver the per PWM capabilities. And maybe to have, in core.c: int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) { struct pwm_chip *chip = pwm->chip; int ret = 0; if (!pwm || !chip || !chip->ops || !caps) return -EINVAL; if (chip->ops->get_caps) ret = chip->ops->get_caps(chip, pwm, caps); else caps = &pwm_default_caps; return ret; } > That way, every driver, unless upgraded with a custom ->get_caps() would > get the defaults, which encode the current assumptions of the framework. > > I think what we'd also want to do is make sure that every driver always > at least implement NORMAL mode and perhaps even fail to register those > that don't. Not sure we want to go that far, but those are the current > assumptions, so devices must be supporting it already anyway. > >>> A possibly simpler approach might be to restrict this to the driver that >>> you need this for. It looks like you will be mainly using this via sysfs >>> and in that case you might be able to just extend what information is >>> exposed in sysfs for the particular PWM you're dealing with. >> By this you mean exporting the sysfs attributes only for my driver or possibly >> other drivers interested in this new feature? I may have another in kernel driver >> which may try to use this feature. > > If you've got another driver that will want to use this, I'm leaning > towards going with the fully featured approach. I'm thinking at PWM regulator driver. I worked lately on adding code to PWM framework in order to support what our PWM controller calls PWM triggers. These are external input received directly by the PWM controller which let it change the PWM output waveform, as long as the trigger is active, only at the controller level (no interaction with kernel code interaction - no IRQ received on trigger activation). This could be used, e.g., in regulators with feedback. I'm thinking that this could be useful for PWM regulator driver when it is using a PWM channel in push-pull mode and with trigger configuration. I have create that code on top of push-pull mode modifications since I want to take advantage of push-pull mode while using PWM triggers. Of course this is something I didn't introduce (I can give more details on this if you want). Thank you, Claudiu Ideally you would try to > implement both at once so we can get a better feeling about whether or > not the framework changes are adequate to cover at least the first two > cases, which is much more reassuring than just being able to cover a > single case. > > One thing to consider is still that if you only use this from sysfs, > it'll be quite a lot of work to add in-kernel infrastructure when it's > never needed. There's still an advantage to do it anyway because it > allows us to abstract everything away properly from the start and avoid > fragmentation of the sysfs interface. > > Thierry > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-22 12:11 ` m18063 @ 2017-08-22 12:24 ` Benjamin Gaignard 2017-08-22 12:55 ` m18063 2017-09-06 14:09 ` m18063 0 siblings, 2 replies; 13+ messages in thread From: Benjamin Gaignard @ 2017-08-22 12:24 UTC (permalink / raw) To: linux-arm-kernel 2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@microchip.com>: > Hi Thierry, > > I added few other comments below. Please let me know what you think. > > Thank you, > Claudiu > > On 21.08.2017 14:25, Thierry Reding wrote: >> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: >>> Hi Thierry, >>> >>> Thank you for your response. I added few comments below. >>> >>> Thank you, >>> Claudiu >>> >>> On 21.08.2017 11:25, Thierry Reding wrote: >>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >>>>> Hi all, >>>>> >>>>> Please give feedback on these patches which extends the PWM >>>>> framework in order to support multiple PWM signal types. >>>>> Since I didn't receive any inputs on RFC series I'm resending it as >>>>> normal patch series. >>>>> >>>>> The current patch series add the following PWM signal types: >>>>> - PWM complementary signals >>>>> - PWM push-pull signal >>>>> >>>>> Definition of PWM complementary mode: >>>>> For a PWM controllers with more than one outputs per PWM channel, >>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals >>>>> have opposite levels, same duration and same starting times, >>>>> as in the following diagram: >>>>> >>>>> __ __ __ __ >>>>> PWMH __| |__| |__| |__| |__ >>>>> __ __ __ __ __ >>>>> PWML |__| |__| |__| |__| >>>>> <--T--> >>>>> Where T is the signal period. >>>>> >>>>> Definition of PWM push-pull mode: >>>>> For PWM controllers with more than one outputs per PWM channel, >>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >>>>> have same levels, same duration and are delayed until the begining >>>>> of the next period, as in the following diagram: >>>>> >>>>> __ __ >>>>> PWMH __| |________| |________ >>>>> __ __ >>>>> PWML ________| |________| |__ >>>>> <--T--> >>>>> >>>>> Where T is the signal period. >>>>> >>>>> These output signals could be configured by setting PWM mode >>>>> (a new input in sysfs has been added in order to support this >>>>> operation). >>>>> >>>>> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >>>>> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >>>>> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >>>>> >>>>> The PWM push-pull mode could be usefull in applications like >>>>> half bridge converters. >>>> >>>> Sorry for taking an absurdly long time to get back to you on this. >>>> >>>> One problem I see with this is that the PWM framework is built on the >>>> assumption that we have a single output per PWM channel. This becomes >>>> important when you start adding features such as this because now the >>>> users have no way of determining whether or not the PWM they retrieve >>>> will actually be able to do what they want. >>> I was thinking that the framework is there to offer support in configuring >>> the underlying hardware without taking into account how many outputs per >>> PWM channels are actually present. It is true I only worked with Atmel/Microchip >>> PWM controllers which for instance, SAMA5 SoC has a PWM controller >>> with 2 outputs per PWM channel. Taking into account that the driver is >>> already mainlined I though that the user is aware of what he is using >>> (either a one output per PWM channel controller, or 2 outputs or >>> more than 2) and about the underlying hardware support. I understand your >>> position on this. I'm just explaining myself on the approach I've chose >>> for this implementation. >> >> So currently we assume that there will be (at least) one output per PWM >> channel. However there are currently no drivers that actually use any >> output other than the "primary" one. >> >> That means, even if there are currently PWM controllers that support >> multiple outputs per PWM channel, we have no code making assumptions >> about it. >> >> Before we can add code that makes any assumptions about the number of >> outputs per PWM channel, we have to add facitilities for such code to >> detect capabilities, so that they can deal with PWMs that don't match >> what they need. >> >>>> For example, let's say you have a half bridge converter driver in the >>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing >>>> preventing it from using a simple one-output PWM and there's no way to >>>> check that we're actually doing something wrong. >>> I understand your position here. I've chose this approach taking into >>> account that the user of the PWM will be aware of the capabilities >>> of the underlying hardware because I worked on a controller which already >>> has a mainlined driver and which has more than one output per PWM channel >>> and I believe there are also other controllers with this kind of capability >>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver >>> I saw a bool type variable in driver specific data structure (struct stm32_pwm): >>> "bool have_complementary_output" which let me think that their controller >>> also could support more than one output per PWM channel (I will also try >>> to find the controller specific datasheet). At a first look I saw that also >>> TI ECAP PWM controller supports this (it is true that I am not aware of how >>> it is initialized in kernel, I need to check the datasheet, if it is public). >> >> Yes, it's quite possible that many of the controllers we currently >> support have this feature or not. But that's not really relevant. The >> only important bit is that none of the users know about it. We don't >> have the means to configure anything beyond just one output. So the only >> guarantee you will get with the current framework is that there will be >> one output signal per PWM channel. >> >>>> I think any in-kernel API would have to be more fully-featured, >>>> otherwise we're bound to run into problems. At the very least I think >>>> we'd have to expose some sort of capabilities list. >>> About the exposing of these capabilities would you prefer to have the >>> supported PWM modes registered in driver probe as a new field mask >>> in "struct pwm_chip". e.g.: >>> struct pwm_chip { >>> struct device *dev; >>> struct list_head list; >>> const struct pwm_ops *ops; >>> int base; >>> unsigned int npwm; >>> unsigned int pwm_modes_mask; /* bitfield of supported signal types */ >>> >>> struct pwm_device *pwms; >>> >>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>> const struct of_phandle_args *args); >>> unsigned int of_pwm_n_cells; >>> }; >>> >>> And having in driver_probe() something like this: >>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; >>> pwmchip_add(&driver_data->chip); >>> Since the PWM push-pull mode imply more than one output per PWM channel. And >>> validate the supported PWM modes when trying to configure one. >>> >>> Or registering, as a field in pwm_chip, how many outputs per channel are actually >>> supported by the PWM controller and then validate the supported PWM mode based >>> on this? >>> e.g.: >>> in "struct pwm_chip". e.g.: >>> struct pwm_chip { >>> struct device *dev; >>> struct list_head list; >>> const struct pwm_ops *ops; >>> int base; >>> unsigned int npwm; >>> unsigned int pwm_outputs; /* Number of supported outputs per channel */>> >>> struct pwm_device *pwms; >>> >>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>> const struct of_phandle_args *args); >>> unsigned int of_pwm_n_cells; >>> }; >>> >>> In driver_probe(): >>> //... >>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ >>> pwmchip_add(&driver_data->chip); >> >> I think I'd prefer something more flexible. I also think this can't be >> per-chip, but really has to be at the granularity of single PWM >> channels. How about something like this: >> >> struct pwm_caps { >> ... >> }; >> >> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); >> >> From a high-level perspective that would give users an easy way to >> obtain the capabilities of the PWM they're handed. > I forgot to mention, I don't know if you had time to look over the other > patch series I send to extend the PWM framework: > "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" > (v2 since the auto build robot prompted me some compilation errors). > In that series I proposed some extensions to be able to configure > dead-times for PWM channels. Those changes are also specific to PWM > controllers which has more than 1 output per PWM channel. Taking into > account that: > - this is also specific to PWM controllers with more than 1 output per > PWM channel and > - your proposed below to introduce capabilities specific to more than > one output per PWM channel and not to introduce something to specify that > the PWM chip or PWM device supports a number of X outputs per PWM channel and > - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with > lower delays b/w the edges of complementary outputs: > Do you think it would be ok (in order to also have a way to configure > PWM dead-times) to merge the changes of > "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the > changes that I will do for push-pull mode (with your below proposals) > and have a single PWM mode for both PWM push-pull and PWM dead-times > (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used > to configure the dead-times that will work only when the PWM device supports > push-pull mode and is configured in push-pull mode? > Something like this: > > root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l > -r--r--r-- 1 root root 4096 Feb 9 10:54 capture > -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle > -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_re <<<< interface for configuring dead-time rising edge delay > -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_fe <<<< interface for configuring dead-time falling edge delay > -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable > -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode <<<< interface for PWM mode configuration > -rw-r--r-- 1 root root 4096 Feb 9 10:54 period > -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity > drwxr-xr-x 2 root root 0 Feb 9 10:54 power > -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent > > When user switch to push-pull mode it could start configure > the dead-times (in nanoseconds) to obtain a signal of a form similar to: > ____0 D____P ____ ____ ____ > PWM signal __| |____| |____| |____| |____| |___ > __ __ __ __ __ > PWMH ____| |____re| |______| |______| |______| |___ > __ __ __ __ __ __ > PWML |______| |____fe| |______| |______| |______| > > (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover > letter), > and starting from a driver specific dead-time value (e.g. on Atmel/Microchip > PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM > outputs to actually switch to PWM push-pull mode (in this mode there is a delay > of a period introduce on a PWM ouptut) to obtain a signal similar to this one: > __ __ > PWMH __| |________| |________ > __ __ > PWML ________| |________| |__ > <--T--> > > Then we can start >> filling in that structure. >> >> struct pwm_caps { >> unsigned long modes; >> }; >> >> #define PWM_MODE_NORMAL (1 << 0) >> #define PWM_MODE_COMPLEMENTARY (1 << 1) >> #define PWM_MODE_PUSH_PULL (1 << 2) >> >> static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, >> unsigned long mode) >> { >> return (caps->modes & mode) != 0; >> } >> >> and maybe something like this as a shortcut: >> >> static inline bool pwm_supports_mode(struct pwm_device *pwm, >> unsigned long mode) >> { >> struct pwm_caps caps; >> int err; >> >> err = pwm_get_caps(pwm, &caps); >> if (err < 0) >> return false; >> >> return pwm_caps_supports_mode(&caps, mode); >> } >> >> I think that gives us a lot of flexibility and easy extensibility for >> other capabilities. The implementation of pwm_get_caps() should probably >> call back into the driver.> >> But that would cause a lot of boilerplate for simple cases, and would be >> a lot of work to convert all existing drivers up front. So I think we >> can add helpers to solve the normal cases. Something along these lines >> perhaps: >> >> struct pwm_chip { >> ... >> >> int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); >> >> const struct pwm_caps *caps; >> }; >> >> static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >> { >> if (!pwm || !caps || !pwm->chip->caps) >> return -EINVAL; >> >> *caps = *pwm->chip->caps; >> >> return 0; >> } >> >> And then perhaps even do something like this: >> >> const struct pwm_caps pwm_default_caps = { >> .modes = PWM_MODE_NORMAL, >> }; >> > Now we can either have a patch that sets pwm_default_caps and >> pwm_simple_get_caps() for all existing drivers, or even easier add this >> to the pwmchip_add_with_polarity() function: >> >> int pwmchip_add_with_polarity(struct pwm_chip *chip, >> enum pwm_polarity polarity) >> { >> ... >> >> if (!chip->get_caps && !chip->caps) { >> chip->get_caps = pwm_simple_get_caps; >> chip->caps = pwm_default_caps; >> } >> >> ... >> } >> > What about having a new member in "struct pwm_ops" something like this: > struct pwm_ops { > // ... > int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps); > // ... > }; > to get from the driver the per PWM capabilities. > > And maybe to have, in core.c: > int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) > { > struct pwm_chip *chip = pwm->chip; > int ret = 0; > > if (!pwm || !chip || !chip->ops || !caps) > return -EINVAL; > > if (chip->ops->get_caps) > ret = chip->ops->get_caps(chip, pwm, caps); > else > caps = &pwm_default_caps; > > return ret; > } > >> That way, every driver, unless upgraded with a custom ->get_caps() would >> get the defaults, which encode the current assumptions of the framework. >> >> I think what we'd also want to do is make sure that every driver always >> at least implement NORMAL mode and perhaps even fail to register those >> that don't. Not sure we want to go that far, but those are the current >> assumptions, so devices must be supporting it already anyway. >> >>>> A possibly simpler approach might be to restrict this to the driver that >>>> you need this for. It looks like you will be mainly using this via sysfs >>>> and in that case you might be able to just extend what information is >>>> exposed in sysfs for the particular PWM you're dealing with. >>> By this you mean exporting the sysfs attributes only for my driver or possibly >>> other drivers interested in this new feature? I may have another in kernel driver >>> which may try to use this feature. >> >> If you've got another driver that will want to use this, I'm leaning >> towards going with the fully featured approach. > I'm thinking at PWM regulator driver. I worked lately on adding code to PWM > framework in order to support what our PWM controller calls PWM triggers. > These are external input received directly by the PWM controller which let > it change the PWM output waveform, as long as the trigger is active, only > at the controller level (no interaction with kernel code interaction - > no IRQ received on trigger activation). > This could be used, e.g., in regulators with feedback. I'm thinking that this > could be useful for PWM regulator driver when it is using a PWM channel > in push-pull mode and with trigger configuration. > > I have create that code on top of push-pull mode modifications since > I want to take advantage of push-pull mode while using PWM triggers. > Of course this is something I didn't introduce (I can give more details > on this if you want). STM32 PWM have the same kind of triggers features but, since the harware block can do more than PWM, we have implemented the trigger part in IIO. It allow use to controls (start/stop) PWMs on events or levels. > > Thank you, > Claudiu > > Ideally you would try to >> implement both at once so we can get a better feeling about whether or >> not the framework changes are adequate to cover at least the first two >> cases, which is much more reassuring than just being able to cover a >> single case. >> >> One thing to consider is still that if you only use this from sysfs, >> it'll be quite a lot of work to add in-kernel infrastructure when it's >> never needed. There's still an advantage to do it anyway because it >> allows us to abstract everything away properly from the start and avoid >> fragmentation of the sysfs interface. >> >> Thierry >> > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel at lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- Benjamin Gaignard Graphic Study Group Linaro.org ? Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-22 12:24 ` Benjamin Gaignard @ 2017-08-22 12:55 ` m18063 2017-09-06 14:09 ` m18063 1 sibling, 0 replies; 13+ messages in thread From: m18063 @ 2017-08-22 12:55 UTC (permalink / raw) To: linux-arm-kernel On 22.08.2017 15:24, Benjamin Gaignard wrote: > 2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@microchip.com>: >> Hi Thierry, >> >> I added few other comments below. Please let me know what you think. >> >> Thank you, >> Claudiu >> >> On 21.08.2017 14:25, Thierry Reding wrote: >>> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: >>>> Hi Thierry, >>>> >>>> Thank you for your response. I added few comments below. >>>> >>>> Thank you, >>>> Claudiu >>>> >>>> On 21.08.2017 11:25, Thierry Reding wrote: >>>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >>>>>> Hi all, >>>>>> >>>>>> Please give feedback on these patches which extends the PWM >>>>>> framework in order to support multiple PWM signal types. >>>>>> Since I didn't receive any inputs on RFC series I'm resending it as >>>>>> normal patch series. >>>>>> >>>>>> The current patch series add the following PWM signal types: >>>>>> - PWM complementary signals >>>>>> - PWM push-pull signal >>>>>> >>>>>> Definition of PWM complementary mode: >>>>>> For a PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals >>>>>> have opposite levels, same duration and same starting times, >>>>>> as in the following diagram: >>>>>> >>>>>> __ __ __ __ >>>>>> PWMH __| |__| |__| |__| |__ >>>>>> __ __ __ __ __ >>>>>> PWML |__| |__| |__| |__| >>>>>> <--T--> >>>>>> Where T is the signal period. >>>>>> >>>>>> Definition of PWM push-pull mode: >>>>>> For PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >>>>>> have same levels, same duration and are delayed until the begining >>>>>> of the next period, as in the following diagram: >>>>>> >>>>>> __ __ >>>>>> PWMH __| |________| |________ >>>>>> __ __ >>>>>> PWML ________| |________| |__ >>>>>> <--T--> >>>>>> >>>>>> Where T is the signal period. >>>>>> >>>>>> These output signals could be configured by setting PWM mode >>>>>> (a new input in sysfs has been added in order to support this >>>>>> operation). >>>>>> >>>>>> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >>>>>> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >>>>>> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >>>>>> >>>>>> The PWM push-pull mode could be usefull in applications like >>>>>> half bridge converters. >>>>> >>>>> Sorry for taking an absurdly long time to get back to you on this. >>>>> >>>>> One problem I see with this is that the PWM framework is built on the >>>>> assumption that we have a single output per PWM channel. This becomes >>>>> important when you start adding features such as this because now the >>>>> users have no way of determining whether or not the PWM they retrieve >>>>> will actually be able to do what they want. >>>> I was thinking that the framework is there to offer support in configuring >>>> the underlying hardware without taking into account how many outputs per >>>> PWM channels are actually present. It is true I only worked with Atmel/Microchip >>>> PWM controllers which for instance, SAMA5 SoC has a PWM controller >>>> with 2 outputs per PWM channel. Taking into account that the driver is >>>> already mainlined I though that the user is aware of what he is using >>>> (either a one output per PWM channel controller, or 2 outputs or >>>> more than 2) and about the underlying hardware support. I understand your >>>> position on this. I'm just explaining myself on the approach I've chose >>>> for this implementation. >>> >>> So currently we assume that there will be (at least) one output per PWM >>> channel. However there are currently no drivers that actually use any >>> output other than the "primary" one. >>> >>> That means, even if there are currently PWM controllers that support >>> multiple outputs per PWM channel, we have no code making assumptions >>> about it. >>> >>> Before we can add code that makes any assumptions about the number of >>> outputs per PWM channel, we have to add facitilities for such code to >>> detect capabilities, so that they can deal with PWMs that don't match >>> what they need. >>> >>>>> For example, let's say you have a half bridge converter driver in the >>>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing >>>>> preventing it from using a simple one-output PWM and there's no way to >>>>> check that we're actually doing something wrong. >>>> I understand your position here. I've chose this approach taking into >>>> account that the user of the PWM will be aware of the capabilities >>>> of the underlying hardware because I worked on a controller which already >>>> has a mainlined driver and which has more than one output per PWM channel >>>> and I believe there are also other controllers with this kind of capability >>>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver >>>> I saw a bool type variable in driver specific data structure (struct stm32_pwm): >>>> "bool have_complementary_output" which let me think that their controller >>>> also could support more than one output per PWM channel (I will also try >>>> to find the controller specific datasheet). At a first look I saw that also >>>> TI ECAP PWM controller supports this (it is true that I am not aware of how >>>> it is initialized in kernel, I need to check the datasheet, if it is public). >>> >>> Yes, it's quite possible that many of the controllers we currently >>> support have this feature or not. But that's not really relevant. The >>> only important bit is that none of the users know about it. We don't >>> have the means to configure anything beyond just one output. So the only >>> guarantee you will get with the current framework is that there will be >>> one output signal per PWM channel. >>> >>>>> I think any in-kernel API would have to be more fully-featured, >>>>> otherwise we're bound to run into problems. At the very least I think >>>>> we'd have to expose some sort of capabilities list. >>>> About the exposing of these capabilities would you prefer to have the >>>> supported PWM modes registered in driver probe as a new field mask >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_modes_mask; /* bitfield of supported signal types */ >>>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> And having in driver_probe() something like this: >>>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; >>>> pwmchip_add(&driver_data->chip); >>>> Since the PWM push-pull mode imply more than one output per PWM channel. And >>>> validate the supported PWM modes when trying to configure one. >>>> >>>> Or registering, as a field in pwm_chip, how many outputs per channel are actually >>>> supported by the PWM controller and then validate the supported PWM mode based >>>> on this? >>>> e.g.: >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_outputs; /* Number of supported outputs per channel */>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> In driver_probe(): >>>> //... >>>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ >>>> pwmchip_add(&driver_data->chip); >>> >>> I think I'd prefer something more flexible. I also think this can't be >>> per-chip, but really has to be at the granularity of single PWM >>> channels. How about something like this: >>> >>> struct pwm_caps { >>> ... >>> }; >>> >>> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> From a high-level perspective that would give users an easy way to >>> obtain the capabilities of the PWM they're handed. >> I forgot to mention, I don't know if you had time to look over the other >> patch series I send to extend the PWM framework: >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" >> (v2 since the auto build robot prompted me some compilation errors). >> In that series I proposed some extensions to be able to configure >> dead-times for PWM channels. Those changes are also specific to PWM >> controllers which has more than 1 output per PWM channel. Taking into >> account that: >> - this is also specific to PWM controllers with more than 1 output per >> PWM channel and >> - your proposed below to introduce capabilities specific to more than >> one output per PWM channel and not to introduce something to specify that >> the PWM chip or PWM device supports a number of X outputs per PWM channel and >> - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with >> lower delays b/w the edges of complementary outputs: >> Do you think it would be ok (in order to also have a way to configure >> PWM dead-times) to merge the changes of >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the >> changes that I will do for push-pull mode (with your below proposals) >> and have a single PWM mode for both PWM push-pull and PWM dead-times >> (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used >> to configure the dead-times that will work only when the PWM device supports >> push-pull mode and is configured in push-pull mode? >> Something like this: >> >> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_re <<<< interface for configuring dead-time rising edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_fe <<<< interface for configuring dead-time falling edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode <<<< interface for PWM mode configuration >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >> >> When user switch to push-pull mode it could start configure >> the dead-times (in nanoseconds) to obtain a signal of a form similar to: >> ____0 D____P ____ ____ ____ >> PWM signal __| |____| |____| |____| |____| |___ >> __ __ __ __ __ >> PWMH ____| |____re| |______| |______| |______| |___ >> __ __ __ __ __ __ >> PWML |______| |____fe| |______| |______| |______| >> >> (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover >> letter), >> and starting from a driver specific dead-time value (e.g. on Atmel/Microchip >> PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM >> outputs to actually switch to PWM push-pull mode (in this mode there is a delay >> of a period introduce on a PWM ouptut) to obtain a signal similar to this one: >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> >> >> Then we can start >>> filling in that structure. >>> >>> struct pwm_caps { >>> unsigned long modes; >>> }; >>> >>> #define PWM_MODE_NORMAL (1 << 0) >>> #define PWM_MODE_COMPLEMENTARY (1 << 1) >>> #define PWM_MODE_PUSH_PULL (1 << 2) >>> >>> static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, >>> unsigned long mode) >>> { >>> return (caps->modes & mode) != 0; >>> } >>> >>> and maybe something like this as a shortcut: >>> >>> static inline bool pwm_supports_mode(struct pwm_device *pwm, >>> unsigned long mode) >>> { >>> struct pwm_caps caps; >>> int err; >>> >>> err = pwm_get_caps(pwm, &caps); >>> if (err < 0) >>> return false; >>> >>> return pwm_caps_supports_mode(&caps, mode); >>> } >>> >>> I think that gives us a lot of flexibility and easy extensibility for >>> other capabilities. The implementation of pwm_get_caps() should probably >>> call back into the driver.> >>> But that would cause a lot of boilerplate for simple cases, and would be >>> a lot of work to convert all existing drivers up front. So I think we >>> can add helpers to solve the normal cases. Something along these lines >>> perhaps: >>> >>> struct pwm_chip { >>> ... >>> >>> int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> const struct pwm_caps *caps; >>> }; >>> >>> static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >>> { >>> if (!pwm || !caps || !pwm->chip->caps) >>> return -EINVAL; >>> >>> *caps = *pwm->chip->caps; >>> >>> return 0; >>> } >>> >>> And then perhaps even do something like this: >>> >>> const struct pwm_caps pwm_default_caps = { >>> .modes = PWM_MODE_NORMAL, >>> }; >>>> Now we can either have a patch that sets pwm_default_caps and >>> pwm_simple_get_caps() for all existing drivers, or even easier add this >>> to the pwmchip_add_with_polarity() function: >>> >>> int pwmchip_add_with_polarity(struct pwm_chip *chip, >>> enum pwm_polarity polarity) >>> { >>> ... >>> >>> if (!chip->get_caps && !chip->caps) { >>> chip->get_caps = pwm_simple_get_caps; >>> chip->caps = pwm_default_caps; >>> } >>> >>> ... >>> } >>> >> What about having a new member in "struct pwm_ops" something like this: >> struct pwm_ops { >> // ... >> int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps); >> // ... >> }; >> to get from the driver the per PWM capabilities. >> >> And maybe to have, in core.c: >> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >> { >> struct pwm_chip *chip = pwm->chip; >> int ret = 0; >> >> if (!pwm || !chip || !chip->ops || !caps) >> return -EINVAL; >> >> if (chip->ops->get_caps) >> ret = chip->ops->get_caps(chip, pwm, caps); >> else >> caps = &pwm_default_caps; >> >> return ret; >> } >> >>> That way, every driver, unless upgraded with a custom ->get_caps() would >>> get the defaults, which encode the current assumptions of the framework. >>> >>> I think what we'd also want to do is make sure that every driver always >>> at least implement NORMAL mode and perhaps even fail to register those >>> that don't. Not sure we want to go that far, but those are the current >>> assumptions, so devices must be supporting it already anyway. >>> >>>>> A possibly simpler approach might be to restrict this to the driver that >>>>> you need this for. It looks like you will be mainly using this via sysfs >>>>> and in that case you might be able to just extend what information is >>>>> exposed in sysfs for the particular PWM you're dealing with. >>>> By this you mean exporting the sysfs attributes only for my driver or possibly >>>> other drivers interested in this new feature? I may have another in kernel driver >>>> which may try to use this feature. >>> >>> If you've got another driver that will want to use this, I'm leaning >>> towards going with the fully featured approach. >> I'm thinking at PWM regulator driver. I worked lately on adding code to PWM >> framework in order to support what our PWM controller calls PWM triggers. >> These are external input received directly by the PWM controller which let >> it change the PWM output waveform, as long as the trigger is active, only >> at the controller level (no interaction with kernel code interaction - >> no IRQ received on trigger activation). >> This could be used, e.g., in regulators with feedback. I'm thinking that this >> could be useful for PWM regulator driver when it is using a PWM channel >> in push-pull mode and with trigger configuration. >> >> I have create that code on top of push-pull mode modifications since >> I want to take advantage of push-pull mode while using PWM triggers. >> Of course this is something I didn't introduce (I can give more details >> on this if you want). > > STM32 PWM have the same kind of triggers features but, since the harware > block can do more than PWM, we have implemented the trigger part in IIO. > It allow use to controls (start/stop) PWMs on events or levels. It is true! I am aware of that and followed latest changes on that STM32 PWM driver. Atmel/Microchip PWM hardware is only PWM aware. I though at taking advantage of the IIO subsystem and integrate the PWM in there but it looked complicated to integrate both subsystems. Basically, on the Atmel/Microchip PWM controller setting a specific trigger involves setting a particular register bit. And, yes, as you said, based on external SoC events, the PWM output waveforms is disabled/enabled (based on the external events). Thank you, Claudiu > >> >> Thank you, >> Claudiu >> >> Ideally you would try to >>> implement both at once so we can get a better feeling about whether or >>> not the framework changes are adequate to cover at least the first two >>> cases, which is much more reassuring than just being able to cover a >>> single case. >>> >>> One thing to consider is still that if you only use this from sysfs, >>> it'll be quite a lot of work to add in-kernel infrastructure when it's >>> never needed. There's still an advantage to do it anyway because it >>> allows us to abstract everything away properly from the start and avoid >>> fragmentation of the sysfs interface. >>> >>> Thierry >>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 0/2] extend PWM framework to support PWM dead-times 2017-08-22 12:24 ` Benjamin Gaignard 2017-08-22 12:55 ` m18063 @ 2017-09-06 14:09 ` m18063 1 sibling, 0 replies; 13+ messages in thread From: m18063 @ 2017-09-06 14:09 UTC (permalink / raw) To: linux-arm-kernel Hi Thierry, Please, do you have any inputs on my previous proposals? Thank you, Claudiu On 22.08.2017 15:24, Benjamin Gaignard wrote: > 2017-08-22 14:11 GMT+02:00 m18063 <Claudiu.Beznea@microchip.com>: >> Hi Thierry, >> >> I added few other comments below. Please let me know what you think. >> >> Thank you, >> Claudiu >> >> On 21.08.2017 14:25, Thierry Reding wrote: >>> On Mon, Aug 21, 2017 at 01:23:08PM +0300, m18063 wrote: >>>> Hi Thierry, >>>> >>>> Thank you for your response. I added few comments below. >>>> >>>> Thank you, >>>> Claudiu >>>> >>>> On 21.08.2017 11:25, Thierry Reding wrote: >>>>> On Tue, May 09, 2017 at 03:15:51PM +0300, Claudiu Beznea wrote: >>>>>> Hi all, >>>>>> >>>>>> Please give feedback on these patches which extends the PWM >>>>>> framework in order to support multiple PWM signal types. >>>>>> Since I didn't receive any inputs on RFC series I'm resending it as >>>>>> normal patch series. >>>>>> >>>>>> The current patch series add the following PWM signal types: >>>>>> - PWM complementary signals >>>>>> - PWM push-pull signal >>>>>> >>>>>> Definition of PWM complementary mode: >>>>>> For a PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channels, the PWM complementary signals >>>>>> have opposite levels, same duration and same starting times, >>>>>> as in the following diagram: >>>>>> >>>>>> __ __ __ __ >>>>>> PWMH __| |__| |__| |__| |__ >>>>>> __ __ __ __ __ >>>>>> PWML |__| |__| |__| |__| >>>>>> <--T--> >>>>>> Where T is the signal period. >>>>>> >>>>>> Definition of PWM push-pull mode: >>>>>> For PWM controllers with more than one outputs per PWM channel, >>>>>> e.g. with 2 outputs per PWM channel, the PWM push-pull signals >>>>>> have same levels, same duration and are delayed until the begining >>>>>> of the next period, as in the following diagram: >>>>>> >>>>>> __ __ >>>>>> PWMH __| |________| |________ >>>>>> __ __ >>>>>> PWML ________| |________| |__ >>>>>> <--T--> >>>>>> >>>>>> Where T is the signal period. >>>>>> >>>>>> These output signals could be configured by setting PWM mode >>>>>> (a new input in sysfs has been added in order to support this >>>>>> operation). >>>>>> >>>>>> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >>>>>> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >>>>>> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >>>>>> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >>>>>> >>>>>> The PWM push-pull mode could be usefull in applications like >>>>>> half bridge converters. >>>>> >>>>> Sorry for taking an absurdly long time to get back to you on this. >>>>> >>>>> One problem I see with this is that the PWM framework is built on the >>>>> assumption that we have a single output per PWM channel. This becomes >>>>> important when you start adding features such as this because now the >>>>> users have no way of determining whether or not the PWM they retrieve >>>>> will actually be able to do what they want. >>>> I was thinking that the framework is there to offer support in configuring >>>> the underlying hardware without taking into account how many outputs per >>>> PWM channels are actually present. It is true I only worked with Atmel/Microchip >>>> PWM controllers which for instance, SAMA5 SoC has a PWM controller >>>> with 2 outputs per PWM channel. Taking into account that the driver is >>>> already mainlined I though that the user is aware of what he is using >>>> (either a one output per PWM channel controller, or 2 outputs or >>>> more than 2) and about the underlying hardware support. I understand your >>>> position on this. I'm just explaining myself on the approach I've chose >>>> for this implementation. >>> >>> So currently we assume that there will be (at least) one output per PWM >>> channel. However there are currently no drivers that actually use any >>> output other than the "primary" one. >>> >>> That means, even if there are currently PWM controllers that support >>> multiple outputs per PWM channel, we have no code making assumptions >>> about it. >>> >>> Before we can add code that makes any assumptions about the number of >>> outputs per PWM channel, we have to add facitilities for such code to >>> detect capabilities, so that they can deal with PWMs that don't match >>> what they need. >>> >>>>> For example, let's say you have a half bridge converter driver in the >>>>> kernel and it does a pwm_get() to obtain a PWM to use. There's nothing >>>>> preventing it from using a simple one-output PWM and there's no way to >>>>> check that we're actually doing something wrong. >>>> I understand your position here. I've chose this approach taking into >>>> account that the user of the PWM will be aware of the capabilities >>>> of the underlying hardware because I worked on a controller which already >>>> has a mainlined driver and which has more than one output per PWM channel >>>> and I believe there are also other controllers with this kind of capability >>>> and with already mainlined driver (e.g. reading the code of STM32 PWM driver >>>> I saw a bool type variable in driver specific data structure (struct stm32_pwm): >>>> "bool have_complementary_output" which let me think that their controller >>>> also could support more than one output per PWM channel (I will also try >>>> to find the controller specific datasheet). At a first look I saw that also >>>> TI ECAP PWM controller supports this (it is true that I am not aware of how >>>> it is initialized in kernel, I need to check the datasheet, if it is public). >>> >>> Yes, it's quite possible that many of the controllers we currently >>> support have this feature or not. But that's not really relevant. The >>> only important bit is that none of the users know about it. We don't >>> have the means to configure anything beyond just one output. So the only >>> guarantee you will get with the current framework is that there will be >>> one output signal per PWM channel. >>> >>>>> I think any in-kernel API would have to be more fully-featured, >>>>> otherwise we're bound to run into problems. At the very least I think >>>>> we'd have to expose some sort of capabilities list. >>>> About the exposing of these capabilities would you prefer to have the >>>> supported PWM modes registered in driver probe as a new field mask >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_modes_mask; /* bitfield of supported signal types */ >>>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> And having in driver_probe() something like this: >>>> driver_data->pwm_chip.pwm_modes = PWM_COMPLEMENTARY_MODE | PWM_PUSH_PULL_MODE; >>>> pwmchip_add(&driver_data->chip); >>>> Since the PWM push-pull mode imply more than one output per PWM channel. And >>>> validate the supported PWM modes when trying to configure one. >>>> >>>> Or registering, as a field in pwm_chip, how many outputs per channel are actually >>>> supported by the PWM controller and then validate the supported PWM mode based >>>> on this? >>>> e.g.: >>>> in "struct pwm_chip". e.g.: >>>> struct pwm_chip { >>>> struct device *dev; >>>> struct list_head list; >>>> const struct pwm_ops *ops; >>>> int base; >>>> unsigned int npwm; >>>> unsigned int pwm_outputs; /* Number of supported outputs per channel */>> >>>> struct pwm_device *pwms; >>>> >>>> struct pwm_device * (*of_xlate)(struct pwm_chip *pc, >>>> const struct of_phandle_args *args); >>>> unsigned int of_pwm_n_cells; >>>> }; >>>> >>>> In driver_probe(): >>>> //... >>>> driver_data->pwm_chip.pwm_outputs = 2; /* 2 outputs per PWM channel. */ >>>> pwmchip_add(&driver_data->chip); >>> >>> I think I'd prefer something more flexible. I also think this can't be >>> per-chip, but really has to be at the granularity of single PWM >>> channels. How about something like this: >>> >>> struct pwm_caps { >>> ... >>> }; >>> >>> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> From a high-level perspective that would give users an easy way to >>> obtain the capabilities of the PWM they're handed. >> I forgot to mention, I don't know if you had time to look over the other >> patch series I send to extend the PWM framework: >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" >> (v2 since the auto build robot prompted me some compilation errors). >> In that series I proposed some extensions to be able to configure >> dead-times for PWM channels. Those changes are also specific to PWM >> controllers which has more than 1 output per PWM channel. Taking into >> account that: >> - this is also specific to PWM controllers with more than 1 output per >> PWM channel and >> - your proposed below to introduce capabilities specific to more than >> one output per PWM channel and not to introduce something to specify that >> the PWM chip or PWM device supports a number of X outputs per PWM channel and >> - PWM dead-time is, in my opinion, some kind of PWM push-pull mode with >> lower delays b/w the edges of complementary outputs: >> Do you think it would be ok (in order to also have a way to configure >> PWM dead-times) to merge the changes of >> "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" with the >> changes that I will do for push-pull mode (with your below proposals) >> and have a single PWM mode for both PWM push-pull and PWM dead-times >> (e.g. the PWM_MODE_PUSH_PULL) and to have an interface which could be used >> to configure the dead-times that will work only when the PWM device supports >> push-pull mode and is configured in push-pull mode? >> Something like this: >> >> root at sama5d2-xplained:/sys/devices/platform/ahb/ahb:apb/f802c000.pwm/pwm/pwmchip0/pwm2# ls -l >> -r--r--r-- 1 root root 4096 Feb 9 10:54 capture >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 duty_cycle >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_re <<<< interface for configuring dead-time rising edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 deadtime_fe <<<< interface for configuring dead-time falling edge delay >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 enable >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 mode <<<< interface for PWM mode configuration >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 period >> -rw-r--r-- 1 root root 4096 Feb 9 10:55 polarity >> drwxr-xr-x 2 root root 0 Feb 9 10:54 power >> -rw-r--r-- 1 root root 4096 Feb 9 10:54 uevent >> >> When user switch to push-pull mode it could start configure >> the dead-times (in nanoseconds) to obtain a signal of a form similar to: >> ____0 D____P ____ ____ ____ >> PWM signal __| |____| |____| |____| |____| |___ >> __ __ __ __ __ >> PWMH ____| |____re| |______| |______| |______| |___ >> __ __ __ __ __ __ >> PWML |______| |____fe| |______| |______| |______| >> >> (see "[PATCH v2 0/2] extends PWM framework to support PWM dead-times" cover >> letter), >> and starting from a driver specific dead-time value (e.g. on Atmel/Microchip >> PWM controller starting with a dead-time value of ~0.05 microseconds) the PWM >> outputs to actually switch to PWM push-pull mode (in this mode there is a delay >> of a period introduce on a PWM ouptut) to obtain a signal similar to this one: >> __ __ >> PWMH __| |________| |________ >> __ __ >> PWML ________| |________| |__ >> <--T--> >> >> Then we can start >>> filling in that structure. >>> >>> struct pwm_caps { >>> unsigned long modes; >>> }; >>> >>> #define PWM_MODE_NORMAL (1 << 0) >>> #define PWM_MODE_COMPLEMENTARY (1 << 1) >>> #define PWM_MODE_PUSH_PULL (1 << 2) >>> >>> static inline bool pwm_caps_supports_mode(const struct pwm_caps *caps, >>> unsigned long mode) >>> { >>> return (caps->modes & mode) != 0; >>> } >>> >>> and maybe something like this as a shortcut: >>> >>> static inline bool pwm_supports_mode(struct pwm_device *pwm, >>> unsigned long mode) >>> { >>> struct pwm_caps caps; >>> int err; >>> >>> err = pwm_get_caps(pwm, &caps); >>> if (err < 0) >>> return false; >>> >>> return pwm_caps_supports_mode(&caps, mode); >>> } >>> >>> I think that gives us a lot of flexibility and easy extensibility for >>> other capabilities. The implementation of pwm_get_caps() should probably >>> call back into the driver.> >>> But that would cause a lot of boilerplate for simple cases, and would be >>> a lot of work to convert all existing drivers up front. So I think we >>> can add helpers to solve the normal cases. Something along these lines >>> perhaps: >>> >>> struct pwm_chip { >>> ... >>> >>> int (*get_caps)(struct pwm_device *pwm, struct pwm_caps *caps); >>> >>> const struct pwm_caps *caps; >>> }; >>> >>> static int pwm_simple_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >>> { >>> if (!pwm || !caps || !pwm->chip->caps) >>> return -EINVAL; >>> >>> *caps = *pwm->chip->caps; >>> >>> return 0; >>> } >>> >>> And then perhaps even do something like this: >>> >>> const struct pwm_caps pwm_default_caps = { >>> .modes = PWM_MODE_NORMAL, >>> }; >>>> Now we can either have a patch that sets pwm_default_caps and >>> pwm_simple_get_caps() for all existing drivers, or even easier add this >>> to the pwmchip_add_with_polarity() function: >>> >>> int pwmchip_add_with_polarity(struct pwm_chip *chip, >>> enum pwm_polarity polarity) >>> { >>> ... >>> >>> if (!chip->get_caps && !chip->caps) { >>> chip->get_caps = pwm_simple_get_caps; >>> chip->caps = pwm_default_caps; >>> } >>> >>> ... >>> } >>> >> What about having a new member in "struct pwm_ops" something like this: >> struct pwm_ops { >> // ... >> int (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_caps *caps); >> // ... >> }; >> to get from the driver the per PWM capabilities. >> >> And maybe to have, in core.c: >> int pwm_get_caps(struct pwm_device *pwm, struct pwm_caps *caps) >> { >> struct pwm_chip *chip = pwm->chip; >> int ret = 0; >> >> if (!pwm || !chip || !chip->ops || !caps) >> return -EINVAL; >> >> if (chip->ops->get_caps) >> ret = chip->ops->get_caps(chip, pwm, caps); >> else >> caps = &pwm_default_caps; >> >> return ret; >> } >> >>> That way, every driver, unless upgraded with a custom ->get_caps() would >>> get the defaults, which encode the current assumptions of the framework. >>> >>> I think what we'd also want to do is make sure that every driver always >>> at least implement NORMAL mode and perhaps even fail to register those >>> that don't. Not sure we want to go that far, but those are the current >>> assumptions, so devices must be supporting it already anyway. >>> >>>>> A possibly simpler approach might be to restrict this to the driver that >>>>> you need this for. It looks like you will be mainly using this via sysfs >>>>> and in that case you might be able to just extend what information is >>>>> exposed in sysfs for the particular PWM you're dealing with. >>>> By this you mean exporting the sysfs attributes only for my driver or possibly >>>> other drivers interested in this new feature? I may have another in kernel driver >>>> which may try to use this feature. >>> >>> If you've got another driver that will want to use this, I'm leaning >>> towards going with the fully featured approach. >> I'm thinking at PWM regulator driver. I worked lately on adding code to PWM >> framework in order to support what our PWM controller calls PWM triggers. >> These are external input received directly by the PWM controller which let >> it change the PWM output waveform, as long as the trigger is active, only >> at the controller level (no interaction with kernel code interaction - >> no IRQ received on trigger activation). >> This could be used, e.g., in regulators with feedback. I'm thinking that this >> could be useful for PWM regulator driver when it is using a PWM channel >> in push-pull mode and with trigger configuration. >> >> I have create that code on top of push-pull mode modifications since >> I want to take advantage of push-pull mode while using PWM triggers. >> Of course this is something I didn't introduce (I can give more details >> on this if you want). > > STM32 PWM have the same kind of triggers features but, since the harware > block can do more than PWM, we have implemented the trigger part in IIO. > It allow use to controls (start/stop) PWMs on events or levels. > >> >> Thank you, >> Claudiu >> >> Ideally you would try to >>> implement both at once so we can get a better feeling about whether or >>> not the framework changes are adequate to cover at least the first two >>> cases, which is much more reassuring than just being able to cover a >>> single case. >>> >>> One thing to consider is still that if you only use this from sysfs, >>> it'll be quite a lot of work to add in-kernel infrastructure when it's >>> never needed. There's still an advantage to do it anyway because it >>> allows us to abstract everything away properly from the start and avoid >>> fragmentation of the sysfs interface. >>> >>> Thierry >>> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel at lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-09-06 14:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-09 12:15 [PATCH 0/2] extend PWM framework to support PWM dead-times Claudiu Beznea 2017-05-09 12:15 ` [PATCH 1/2] drivers: pwm: core: implement pwm mode Claudiu Beznea 2017-05-27 22:11 ` Andy Shevchenko 2017-05-30 9:49 ` m18063 2017-07-19 12:13 ` m18063 2017-05-09 12:15 ` [PATCH 2/2] drivers: pwm: pwm-atmel: add support for pwm modes Claudiu Beznea 2017-08-21 8:25 ` [PATCH 0/2] extend PWM framework to support PWM dead-times Thierry Reding 2017-08-21 10:23 ` m18063 2017-08-21 11:25 ` Thierry Reding 2017-08-22 12:11 ` m18063 2017-08-22 12:24 ` Benjamin Gaignard 2017-08-22 12:55 ` m18063 2017-09-06 14:09 ` m18063
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).