[PATCH 0/2] extend PWM framework to support PWM dead-times

[PATCH 0/2] extend PWM framework to support PWM dead-times

[Claudiu Beznea]

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@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

[PATCH 1/2] drivers: pwm: core: implement pwm mode

[Claudiu Beznea]

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

[PATCH 2/2] drivers: pwm: pwm-atmel: add support for pwm modes

[Claudiu Beznea]

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

Re: [PATCH 1/2] drivers: pwm: core: implement pwm mode

[Andy Shevchenko]

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

Re: [PATCH 1/2] drivers: pwm: core: implement pwm mode

[m18063]

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.

> 

Re: [PATCH 1/2] drivers: pwm: core: implement pwm mode

[m18063]

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;
>  }
>  
>  /**
> 

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[Thierry Reding]

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

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@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

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

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[m18063]

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@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
> 

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[Thierry Reding]

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

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@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

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

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[m18063]

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@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@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
> 

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[Benjamin Gaignard]

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@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@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@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

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[m18063]

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@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@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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 

Re: [PATCH 0/2] extend PWM framework to support PWM dead-times

[m18063]

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@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@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@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
>