[RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Claudiu Beznea]

Hi,

Please give feedback on these patches which extends the PWM framework in
order to support multiple PWM modes of operations. This series is a rework
of [1] and [2].

The current patch series add the following PWM modes:
- PWM mode normal
- PWM mode complementary
- PWM mode push-pull

Normal mode - for PWM channels with one output; output waveforms looks like
this:
             __    __    __    __
    PWM   __|  |__|  |__|  |__|  |__
            <--T-->

    Where T is the signal period

Since PWMs with more than one output per channel could be used as one
output PWM the normal mode is the default mode for all PWMs (if not
specified otherwise).

Complementary mode - for PWM channels with two outputs; output waveforms
for a PWM channel in complementary mode looks line this:
             __    __    __    __
    PWMH1 __|  |__|  |__|  |__|  |__
          __    __    __    __    __
    PWML1   |__|  |__|  |__|  |__|
            <--T-->

    Where T is the signal period.

Push-pull mode - for PWM channels with two outputs; output waveforms for a
PWM channel in push-pull mode with normal polarity looks like this:
            __          __
    PWMH __|  |________|  |________
                  __          __
    PWML ________|  |________|  |__
           <--T-->

    If polarity is inversed:
         __    ________    ________
    PWMH   |__|        |__|
         ________    ________    __
    PWML         |__|        |__|
           <--T-->

    Where T is the signal period.

The PWM working modes are per PWM channel registered as PWM's capabilities.
The driver registers itself to PWM core a get_caps() function, in
struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
If this function is not registered in driver's probe, a default function
will be used to retrieve PWM capabilities. Currently, the default
capabilities includes only PWM normal mode.

PWM state has been updated to keep PWM mode. PWM mode could be configured
via sysfs or via DT. pwm_apply_state() will do the preliminary validation
for PWM mode to be applied.

In sysfs, user could get PWM modes by reading mode file of PWM device:
root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l
total 0
-r--r--r-- 1 root root 4096 Oct  9 09:07 capture
lrwxrwxrwx 1 root root    0 Oct  9 09:07 device -> ../../pwmchip0
-rw-r--r-- 1 root root 4096 Oct  9 08:42 duty_cycle
-rw-r--r-- 1 root root 4096 Oct  9 08:44 enable
--w------- 1 root root 4096 Oct  9 09:07 export
-rw-r--r-- 1 root root 4096 Oct  9 08:43 mode
-r--r--r-- 1 root root 4096 Oct  9 09:07 npwm
-rw-r--r-- 1 root root 4096 Oct  9 08:42 period
-rw-r--r-- 1 root root 4096 Oct  9 08:44 polarity
drwxr-xr-x 2 root root    0 Oct  9 09:07 power
lrwxrwxrwx 1 root root    0 Oct  9 09:07 subsystem -> ../../../../../../../../class/pwm
-rw-r--r-- 1 root root 4096 Oct  9 08:42 uevent
--w------- 1 root root 4096 Oct  9 09:07 unexport
root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
normal complementary [push-pull]

The mode enclosed in bracket is the currently active mode.

The mode could be set, via sysfs, by writing to mode file one of the modes
displayed at read:
root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode
root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
[normal] complementary push-pull 

The PWM push-pull mode could be usefull in applications like half bridge
converters.

This series also add PWM modes support for Atmel/Microchip SoCs.

Thank you,
Claudiu Beznea

[1] https://www.spinics.net/lists/arm-kernel/msg580275.html
[2] https://lkml.org/lkml/2018/1/12/359

Changes in v5:
- solved kbuild errors by removing dummy functions for case where
  CONFIG_PWM is not defined; adopted this approach since the removed
  function are used only when CONFIG_PWM is defined (in PWM core
  and few drivers from drivers/pwm/ directory)

Changes in v4:
- removed changes related to pwm_config() as per maintainer proposals
- added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device
  instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use
  pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1)
  (patches 2, 3, 4 from this series)
- renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with
  pwm-sun4i.c driver ('C' stands for capability)
- removed pwm_caps_valid() function
- renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in
  PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL

Changes in v3:
- removed changes related to only one of_xlate function for all PWM drivers
- switch to PWM capabilities per PWM channel nor per PWM chip
- squash documentation and bindings patches as requeted by reviewer
- introduced PWM_MODE(name) macro and used a bit enum for pwm modes
- related to DT bindings, used flags cell also for PWM modes
- updated of_xlate specific functions with "state->mode = mode;"
  instructions to avoid pwm_apply_state() failures
- use available modes for PWM channel in pwm_config() by first calling
  pwm_get_caps() to get caps.modes
- use loops through available modes in mode_store()/mode_show() and also in
  of_pwm_xlate_with_flags() instead of "if else" instructions; in this way,
  the addition of a new mode is independent of this code sections
- use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms
  for Atmel/Microchip PWM controller.

Changes in v2:
- remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT
  inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will
  make easy the addition of PWM mode support from DT
- add PWM mode normal
- register PWM modes as capabilities of PWM chips at driver probe and, in case
  driver doesn't provide these capabilities use default ones
- change the way PWM mode is pharsed via DT by using a new input for pwms
  binding property


Claudiu Beznea (9):
  pwm: extend PWM framework with PWM modes
  pwm: clps711x: populate PWM mode in of_xlate function
  pwm: cros-ec: populate PWM mode in of_xlate function
  pwm: pxa: populate PWM mode in of_xlate function
  pwm: add PWM modes
  pwm: atmel: add pwm capabilities
  pwm: add push-pull mode support
  pwm: add documentation for pwm push-pull mode
  pwm: atmel: add push-pull mode support

 Documentation/devicetree/bindings/pwm/pwm.txt |  11 ++-
 Documentation/pwm.txt                         |  42 ++++++++-
 drivers/pwm/core.c                            | 125 +++++++++++++++++++++++++-
 drivers/pwm/pwm-atmel.c                       | 118 +++++++++++++++++-------
 drivers/pwm/pwm-clps711x.c                    |  10 ++-
 drivers/pwm/pwm-cros-ec.c                     |   1 +
 drivers/pwm/pwm-pxa.c                         |   1 +
 drivers/pwm/sysfs.c                           |  61 +++++++++++++
 include/dt-bindings/pwm/pwm.h                 |   2 +
 include/linux/pwm.h                           |  64 +++++++++++++
 10 files changed, 395 insertions(+), 40 deletions(-)

-- 
2.7.4

[RESEND PATCH v5 1/9] pwm: extend PWM framework with PWM modes

[Claudiu Beznea]

Add basic PWM modes: normal and complementary. These modes should
differentiate the single output PWM channels from two outputs PWM
channels. These modes could be set as follow:
1. PWM channels with one output per channel:
- normal mode
2. PWM channels with two outputs per channel:
- normal mode
- complementary mode
Since users could use a PWM channel with two output as one output PWM
channel, the PWM normal mode is allowed to be set for PWM channels with
two outputs; in fact PWM normal mode should be supported by all PWMs.

The PWM capabilities were implemented per PWM channel. Every PWM controller
will register a function to get PWM capabilities. If this is not explicitly
set by the driver a default function will be used to retrieve the PWM
capabilities (in this case the PWM capabilities will contain only PWM
normal mode). This function is set in pwmchip_add_with_polarity() as a
member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps()
function could be used.

Every PWM channel have associated a mode in the PWM state. Proper
support was added to get/set PWM mode. The mode could also be set
from DT via flag cells. The valid DT modes are located in
include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be
set. If nothing is specified for a PWM channel, via DT, the first available
mode will be used (normally, this will be PWM normal mode).

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/core.c  | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 drivers/pwm/sysfs.c |  61 ++++++++++++++++++++++++++
 include/linux/pwm.h |  39 +++++++++++++++++
 3 files changed, 221 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 1581f6ab1b1f..59a9df9120de 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -136,6 +136,7 @@ struct pwm_device *
 of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
 {
 	struct pwm_device *pwm;
+	int modebit;
 
 	/* check, whether the driver supports a third cell for flags */
 	if (pc->of_pwm_n_cells < 3)
@@ -154,9 +155,23 @@ 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_get_valid(pc, pwm);
 
-	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;
+
+		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
+		     modebit < PWMC_MODE_CNT; modebit++) {
+			unsigned long mode = BIT(modebit);
+
+			if ((args->args[2] & mode) &&
+			    pwm_mode_valid(pwm, mode)) {
+				pwm->args.mode = mode;
+				break;
+			}
+		}
+	}
 
 	return pwm;
 }
@@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 		return pwm;
 
 	pwm->args.period = args->args[1];
+	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
 
 	return pwm;
 }
@@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops)
 }
 
 /**
+ * pwm_get_caps() - get PWM capabilities of a PWM device
+ * @chip: PWM chip
+ * @pwm: PWM device to get the capabilities for
+ * @caps: returned capabilities
+ *
+ * Returns: 0 on success or a negative error code on failure
+ */
+int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm,
+		 struct pwm_caps *caps)
+{
+	if (!chip || !pwm || !caps)
+		return -EINVAL;
+
+	if (chip->ops && chip->ops->get_caps)
+		pwm->chip->ops->get_caps(chip, pwm, caps);
+	else if (chip->get_default_caps)
+		chip->get_default_caps(caps);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pwm_get_caps);
+
+static void pwmchip_get_default_caps(struct pwm_caps *caps)
+{
+	static const struct pwm_caps default_caps = {
+		.modes = PWMC_MODE(NORMAL),
+	};
+
+	if (!caps)
+		return;
+
+	*caps = default_caps;
+}
+
+/**
+ * pwm_mode_get_valid() - get the first available valid mode for PWM
+ * @chip: PWM chip
+ * @pwm: PWM device to get the valid mode for
+ *
+ * Returns: first valid mode for PWM device
+ */
+unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct pwm_caps caps;
+
+	if (pwm_get_caps(chip, pwm, &caps))
+		return PWMC_MODE(NORMAL);
+
+	return BIT(ffs(caps.modes) - 1);
+}
+EXPORT_SYMBOL_GPL(pwm_mode_get_valid);
+
+/**
+ * pwm_mode_valid() - check if mode is valid for PWM device
+ * @pwm: PWM device
+ * @mode: PWM mode to check if valid
+ *
+ * Returns: true if mode is valid and false otherwise
+ */
+bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode)
+{
+	struct pwm_caps caps;
+
+	if (!pwm || !mode)
+		return false;
+
+	if (hweight_long(mode) != 1 || ffs(mode) - 1 >= PWMC_MODE_CNT)
+		return false;
+
+	if (pwm_get_caps(pwm->chip, pwm, &caps))
+		return false;
+
+	return (caps.modes & mode);
+}
+EXPORT_SYMBOL_GPL(pwm_mode_valid);
+
+const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
+{
+	static const char * const modes[] = {
+		"invalid",
+		"normal",
+		"complementary",
+	};
+
+	if (!pwm_mode_valid(pwm, mode))
+		return modes[0];
+
+	return modes[ffs(mode)];
+}
+
+/**
  * pwmchip_add_with_polarity() - register a new PWM chip
  * @chip: the PWM chip to add
  * @polarity: initial polarity of PWM channels
@@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 
 	mutex_lock(&pwm_lock);
 
+	chip->get_default_caps = pwmchip_get_default_caps;
+
 	ret = alloc_pwms(chip->base, chip->npwm);
 	if (ret < 0)
 		goto out;
@@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
 		pwm->pwm = chip->base + i;
 		pwm->hwpwm = i;
 		pwm->state.polarity = polarity;
+		pwm->state.mode = pwm_mode_get_valid(chip, pwm);
 
 		if (chip->ops->get_state)
 			chip->ops->get_state(chip, pwm, &pwm->state);
@@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 	int err;
 
 	if (!pwm || !state || !state->period ||
-	    state->duty_cycle > state->period)
+	    state->duty_cycle > state->period ||
+	    !pwm_mode_valid(pwm, state->mode))
 		return -EINVAL;
 
 	if (!memcmp(state, &pwm->state, sizeof(*state)))
@@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
 
 			pwm->state.enabled = state->enabled;
 		}
+
+		/* No mode support for non-atomic PWM. */
+		pwm->state.mode = state->mode;
 	}
 
 	return 0;
@@ -579,6 +693,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
@@ -850,6 +966,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id)
 
 	pwm->args.period = chosen->period;
 	pwm->args.polarity = chosen->polarity;
+	pwm->args.mode = pwm_mode_get_valid(chip, pwm);
 
 	return pwm;
 }
@@ -999,6 +1116,7 @@ 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", pwm_mode_desc(pwm, state.mode));
 
 		seq_puts(s, "\n");
 	}
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 83f2b0b15712..785eda0b1e67 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -223,11 +223,71 @@ 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)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	struct pwm_state state;
+	unsigned long mode;
+	int modebit, len = 0;
+
+	pwm_get_state(pwm, &state);
+
+	for (modebit = PWMC_MODE_NORMAL_BIT;
+	     modebit < PWMC_MODE_CNT; modebit++) {
+		mode = BIT(modebit);
+		if (pwm_mode_valid(pwm, mode)) {
+			if (state.mode == mode)
+				len += scnprintf(buf + len,
+						 PAGE_SIZE - len, "[%s] ",
+						 pwm_mode_desc(pwm, mode));
+			else
+				len += scnprintf(buf + len,
+						 PAGE_SIZE - len, "%s ",
+						 pwm_mode_desc(pwm, mode));
+		}
+	}
+
+	len += scnprintf(buf + len, PAGE_SIZE - len, "\n");
+	return len;
+}
+
+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;
+	struct pwm_state state;
+	unsigned long mode;
+	int modebit, ret;
+
+	for (modebit = PWMC_MODE_NORMAL_BIT;
+	     modebit < PWMC_MODE_CNT; modebit++) {
+		mode = BIT(modebit);
+		if (sysfs_streq(buf, pwm_mode_desc(pwm, mode)))
+			break;
+	}
+
+	if (modebit == PWMC_MODE_CNT)
+		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 +295,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/linux/pwm.h b/include/linux/pwm.h
index 56518adc31dd..a4ce4ad7edf0 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -26,9 +26,32 @@ enum pwm_polarity {
 };
 
 /**
+ * PWM modes capabilities
+ * @PWMC_MODE_NORMAL_BIT: PWM has one output
+ * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
+ * @PWMC_MODE_CNT: PWM modes count
+ */
+enum {
+	PWMC_MODE_NORMAL_BIT,
+	PWMC_MODE_COMPLEMENTARY_BIT,
+	PWMC_MODE_CNT,
+};
+
+#define PWMC_MODE(name)		BIT(PWMC_MODE_##name##_BIT)
+
+/**
+ * struct pwm_caps - PWM capabilities
+ * @modes: PWM modes
+ */
+struct pwm_caps {
+	unsigned long modes;
+};
+
+/**
  * struct pwm_args - board-dependent PWM arguments
  * @period: reference period
  * @polarity: reference polarity
+ * @mode: reference mode
  *
  * This structure describes board-dependent arguments attached to a PWM
  * device. These arguments are usually retrieved from the PWM lookup table or
@@ -41,6 +64,7 @@ enum pwm_polarity {
 struct pwm_args {
 	unsigned int period;
 	enum pwm_polarity polarity;
+	unsigned long mode;
 };
 
 enum {
@@ -53,12 +77,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;
+	unsigned long mode;
 	bool enabled;
 };
 
@@ -181,6 +207,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;
 }
 
 /**
@@ -254,6 +281,7 @@ pwm_set_relative_duty_cycle(struct pwm_state *state, unsigned int duty_cycle,
  * @get_state: get the current PWM state. This function is only
  *	       called once per PWM device when the PWM chip is
  *	       registered.
+ * @get_caps: get PWM capabilities.
  * @dbg_show: optional routine to show contents in debugfs
  * @owner: helps prevent removal of modules exporting active PWMs
  */
@@ -272,6 +300,8 @@ struct pwm_ops {
 		     struct pwm_state *state);
 	void (*get_state)(struct pwm_chip *chip, struct pwm_device *pwm,
 			  struct pwm_state *state);
+	void (*get_caps)(struct pwm_chip *chip, struct pwm_device *pwm,
+			 struct pwm_caps *caps);
 #ifdef CONFIG_DEBUG_FS
 	void (*dbg_show)(struct pwm_chip *chip, struct seq_file *s);
 #endif
@@ -287,6 +317,7 @@ struct pwm_ops {
  * @npwm: number of PWMs controlled by this chip
  * @pwms: array of PWM devices allocated by the framework
  * @of_xlate: request a PWM device given a device tree PWM specifier
+ * @get_default_caps: get default PWM capabilities
  * @of_pwm_n_cells: number of cells expected in the device tree PWM specifier
  */
 struct pwm_chip {
@@ -300,6 +331,7 @@ struct pwm_chip {
 
 	struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
 					const struct of_phandle_args *args);
+	void (*get_default_caps)(struct pwm_caps *caps);
 	unsigned int of_pwm_n_cells;
 };
 
@@ -438,6 +470,12 @@ struct pwm_device *pwm_request_from_chip(struct pwm_chip *chip,
 					 unsigned int index,
 					 const char *label);
 
+int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm,
+		 struct pwm_caps *caps);
+unsigned long pwm_mode_get_valid(struct pwm_chip *chip,
+				 struct pwm_device *pwm);
+bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode);
+const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode);
 struct pwm_device *of_pwm_xlate_with_flags(struct pwm_chip *pc,
 		const struct of_phandle_args *args);
 
@@ -592,6 +630,7 @@ static inline void pwm_apply_args(struct pwm_device *pwm)
 	state.enabled = false;
 	state.polarity = pwm->args.polarity;
 	state.period = pwm->args.period;
+	state.mode = pwm->args.mode;
 
 	pwm_apply_state(pwm, &state);
 }
-- 
2.7.4

[RESEND PATCH v5 2/9] pwm: clps711x: populate PWM mode in of_xlate function

[Claudiu Beznea]

Populate PWM mode in of_xlate function to avoid pwm_apply_state() failure.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-clps711x.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-clps711x.c b/drivers/pwm/pwm-clps711x.c
index 26ec24e457b1..d742e8e375c7 100644
--- a/drivers/pwm/pwm-clps711x.c
+++ b/drivers/pwm/pwm-clps711x.c
@@ -109,10 +109,18 @@ static const struct pwm_ops clps711x_pwm_ops = {
 static struct pwm_device *clps711x_pwm_xlate(struct pwm_chip *chip,
 					     const struct of_phandle_args *args)
 {
+	struct pwm_device *pwm;
+
 	if (args->args[0] >= chip->npwm)
 		return ERR_PTR(-EINVAL);
 
-	return pwm_request_from_chip(chip, args->args[0], NULL);
+	pwm = pwm_request_from_chip(chip, args->args[0], NULL);
+	if (IS_ERR(pwm))
+		return pwm;
+
+	pwm->args.mode = pwm_mode_get_valid(chip, pwm);
+
+	return pwm;
 }
 
 static int clps711x_pwm_probe(struct platform_device *pdev)
-- 
2.7.4

[RESEND PATCH v5 3/9] pwm: cros-ec: populate PWM mode in of_xlate function

[Claudiu Beznea]

Populate PWM mode in of_xlate function to avoid pwm_apply_state() failure.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-cros-ec.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 9c13694eaa24..78d28d60a468 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -147,6 +147,7 @@ cros_ec_pwm_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 
 	/* The EC won't let us change the period */
 	pwm->args.period = EC_PWM_MAX_DUTY;
+	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
 
 	return pwm;
 }
-- 
2.7.4

[RESEND PATCH v5 4/9] pwm: pxa: populate PWM mode in of_xlate function

[Claudiu Beznea]

Populate PWM mode in of_xlate function to avoid pwm_apply_state() failure.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-pxa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pwm/pwm-pxa.c b/drivers/pwm/pwm-pxa.c
index 4143a46684d2..4c88cb47d6ba 100644
--- a/drivers/pwm/pwm-pxa.c
+++ b/drivers/pwm/pwm-pxa.c
@@ -161,6 +161,7 @@ pxa_pwm_of_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
 		return pwm;
 
 	pwm->args.period = args->args[0];
+	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
 
 	return pwm;
 }
-- 
2.7.4

[RESEND PATCH v5 5/9] pwm: add PWM modes

[Claudiu Beznea]

Add PWM normal and complementary modes.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 Documentation/devicetree/bindings/pwm/pwm.txt |  9 +++++++--
 Documentation/pwm.txt                         | 26 +++++++++++++++++++++++---
 include/dt-bindings/pwm/pwm.h                 |  1 +
 3 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 8556263b8502..7c8aaac43f92 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -46,11 +46,16 @@ period in nanoseconds.
 Optionally, the pwm-specifier can encode a number of flags (defined in
 <dt-bindings/pwm/pwm.h>) in a third cell:
 - PWM_POLARITY_INVERTED: invert the PWM signal polarity
+- PWM_MODE_COMPLEMENTARY: PWM complementary working mode (for PWM channels
+with two outputs); if not specified, the default for PWM channel will be
+used
 
-Example with optional PWM specifier for inverse polarity
+Example with optional PWM specifier for inverse polarity and complementary
+mode:
 
 	bl: backlight {
-		pwms = <&pwm 0 5000000 PWM_POLARITY_INVERTED>;
+		pwms = <&pwm 0 5000000
+			(PWM_MODE_COMPLEMENTARY | PWM_POLARITY_INVERTED)>;
 		pwm-names = "backlight";
 	};
 
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 8fbf0aa3ba2d..912c43da8b48 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -61,9 +61,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
 -----------------------------------
@@ -110,6 +110,26 @@ channel that was exported. The following properties will then be available:
 	- 0 - disabled
 	- 1 - enabled
 
+  mode
+    Get/set PWM channel working mode.
+
+    Normal mode - for PWM channels with one output; this should be the
+        default working mode for every PWM channel; output waveforms looks
+        like this:
+             __    __    __    __
+    PWM   __|  |__|  |__|  |__|  |__
+            <--T-->
+
+    Complementary mode - for PWM channels with two outputs; output waveforms
+        looks line this:
+             __    __    __    __
+    PWMH1 __|  |__|  |__|  |__|  |__
+          __    __    __    __    __
+    PWML1   |__|  |__|  |__|  |__|
+            <--T-->
+
+    Where T is the signal period.
+
 Implementing a PWM driver
 -------------------------
 
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index ab9a077e3c7d..b82279cc1787 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -11,5 +11,6 @@
 #define _DT_BINDINGS_PWM_PWM_H
 
 #define PWM_POLARITY_INVERTED			(1 << 0)
+#define PWM_MODE_COMPLEMENTARY			(1 << 1)
 
 #endif
-- 
2.7.4

[RESEND PATCH v5 6/9] pwm: atmel: add pwm capabilities

[Claudiu Beznea]

Add pwm capabilities for Atmel/Microchip PWM controllers.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-atmel.c | 80 ++++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 28 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 530d7dc5f1b5..87ef54bd492c 100644
--- a/drivers/pwm/pwm-atmel.c
+++ b/drivers/pwm/pwm-atmel.c
@@ -65,11 +65,16 @@ struct atmel_pwm_registers {
 	u8 duty_upd;
 };
 
+struct atmel_pwm_data {
+	struct atmel_pwm_registers regs;
+	struct pwm_caps caps;
+};
+
 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 +155,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 +173,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,
@@ -225,7 +230,7 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 		    cstate.polarity == state->polarity &&
 		    cstate.period == state->period) {
 			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;
@@ -272,32 +277,51 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 	return 0;
 }
 
+static void atmel_pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_caps *caps)
+{
+	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
+
+	*caps = atmel_pwm->data->caps;
+}
+
 static const struct pwm_ops atmel_pwm_ops = {
 	.apply = atmel_pwm_apply,
+	.get_caps = atmel_pwm_get_caps,
 	.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,
+	},
+	.caps = {
+		.modes = PWMC_MODE(NORMAL),
+	},
 };
 
-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,
+	},
+	.caps = {
+		.modes = PWMC_MODE(NORMAL) | PWMC_MODE(COMPLEMENTARY),
+	},
 };
 
 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 +331,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_v2,
 	}, {
 		/* 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 +354,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 +397,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

[RESEND PATCH v5 7/9] pwm: add push-pull mode support

[Claudiu Beznea]

Add push-pull mode support. In push-pull mode the channels' outputs have
same polarities and the edges are complementary delayed for one period.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/core.c  | 1 +
 include/linux/pwm.h | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 59a9df9120de..5fde2e685ca7 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -348,6 +348,7 @@ const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
 		"invalid",
 		"normal",
 		"complementary",
+		"push-pull",
 	};
 
 	if (!pwm_mode_valid(pwm, mode))
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index a4ce4ad7edf0..eb170e2ab431 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -29,11 +29,14 @@ enum pwm_polarity {
  * PWM modes capabilities
  * @PWMC_MODE_NORMAL_BIT: PWM has one output
  * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
+ * @PWMC_MODE_PUSH_PULL_BIT: PWM has 2 outputs with same polarities and the
+ * edges are complementary delayed for one period
  * @PWMC_MODE_CNT: PWM modes count
  */
 enum {
 	PWMC_MODE_NORMAL_BIT,
 	PWMC_MODE_COMPLEMENTARY_BIT,
+	PWMC_MODE_PUSH_PULL_BIT,
 	PWMC_MODE_CNT,
 };
 
-- 
2.7.4

[RESEND PATCH v5 8/9] pwm: add documentation for pwm push-pull mode

[Claudiu Beznea]

Add documentation for PWM push-pull mode.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/pwm/pwm.txt |  2 ++
 Documentation/pwm.txt                         | 16 ++++++++++++++++
 include/dt-bindings/pwm/pwm.h                 |  1 +
 3 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
index 7c8aaac43f92..6a60c0fca112 100644
--- a/Documentation/devicetree/bindings/pwm/pwm.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm.txt
@@ -49,6 +49,8 @@ Optionally, the pwm-specifier can encode a number of flags (defined in
 - PWM_MODE_COMPLEMENTARY: PWM complementary working mode (for PWM channels
 with two outputs); if not specified, the default for PWM channel will be
 used
+- PWM_MODE_PUSH_PULL: PWM push-pull working modes (for PWM channels with
+two outputs); if not specified the default for PWM channel will be used
 
 Example with optional PWM specifier for inverse polarity and complementary
 mode:
diff --git a/Documentation/pwm.txt b/Documentation/pwm.txt
index 912c43da8b48..675f9351fed1 100644
--- a/Documentation/pwm.txt
+++ b/Documentation/pwm.txt
@@ -128,6 +128,22 @@ channel that was exported. The following properties will then be available:
     PWML1   |__|  |__|  |__|  |__|
             <--T-->
 
+    Push-pull mode - for PWM channels with two outputs; output waveforms
+        for a PWM channel in push-pull mode, with normal polarity looks like
+        this:
+            __          __
+    PWMH __|  |________|  |________
+                  __          __
+    PWML ________|  |________|  |__
+           <--T-->
+
+    If polarity is inversed:
+         __    ________    ________
+    PWMH   |__|        |__|
+         ________    ________    __
+    PWML         |__|        |__|
+           <--T-->
+
     Where T is the signal period.
 
 Implementing a PWM driver
diff --git a/include/dt-bindings/pwm/pwm.h b/include/dt-bindings/pwm/pwm.h
index b82279cc1787..cd4265bce968 100644
--- a/include/dt-bindings/pwm/pwm.h
+++ b/include/dt-bindings/pwm/pwm.h
@@ -12,5 +12,6 @@
 
 #define PWM_POLARITY_INVERTED			(1 << 0)
 #define PWM_MODE_COMPLEMENTARY			(1 << 1)
+#define PWM_MODE_PUSH_PULL			(1 << 2)
 
 #endif
-- 
2.7.4

[RESEND PATCH v5 9/9] pwm: atmel: add push-pull mode support

[Claudiu Beznea]

Add support for PWM push-pull mode. This is only supported by SAMA5D2 SoCs.

Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
---
 drivers/pwm/pwm-atmel.c | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-atmel.c b/drivers/pwm/pwm-atmel.c
index 87ef54bd492c..aaafc4dd30f2 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 */
@@ -219,16 +222,19 @@ static int atmel_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct atmel_pwm_chip *atmel_pwm = to_atmel_pwm_chip(chip);
 	struct pwm_state cstate;
+	struct pwm_caps caps;
 	unsigned long cprd, cdty;
 	u32 pres, val;
 	int ret;
 
 	pwm_get_state(pwm, &cstate);
+	pwm_get_caps(chip, pwm, &caps);
 
 	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->data->regs.period);
 			atmel_pwm_calculate_cdty(state, cprd, &cdty);
@@ -263,6 +269,18 @@ 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 (caps.modes & PWMC_MODE(PUSH_PULL)) {
+			if (state->mode == PWMC_MODE(PUSH_PULL)) {
+				val |= PWM_CMR_PPM | PWM_CMR_DTLI;
+				val &= ~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);
@@ -315,6 +333,20 @@ static const struct atmel_pwm_data atmel_pwm_data_v2 = {
 	},
 };
 
+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,
+	},
+	.caps = {
+		.modes = PWMC_MODE(NORMAL) |
+			 PWMC_MODE(COMPLEMENTARY) |
+			 PWMC_MODE(PUSH_PULL),
+	},
+};
+
 static const struct platform_device_id atmel_pwm_devtypes[] = {
 	{
 		.name = "at91sam9rl-pwm",
@@ -337,7 +369,7 @@ static const struct of_device_id atmel_pwm_dt_ids[] = {
 		.data = &atmel_pwm_data_v2,
 	}, {
 		.compatible = "atmel,sama5d2-pwm",
-		.data = &atmel_pwm_data_v2,
+		.data = &atmel_pwm_data_v3,
 	}, {
 		/* sentinel */
 	},
-- 
2.7.4

Re: [RESEND PATCH v5 5/9] pwm: add PWM modes

[Rob Herring]

On Tue, Aug 28, 2018 at 04:01:22PM +0300, Claudiu Beznea wrote:
> Add PWM normal and complementary modes.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm.txt |  9 +++++++--

Acked-by: Rob Herring <robh@kernel.org>

>  Documentation/pwm.txt                         | 26 +++++++++++++++++++++++---
>  include/dt-bindings/pwm/pwm.h                 |  1 +
>  3 files changed, 31 insertions(+), 5 deletions(-)

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Nicolas Ferre]

Thierry,

On 28/08/2018 at 15:01, Claudiu Beznea wrote:
> Hi,
> 
> Please give feedback on these patches which extends the PWM framework in
> order to support multiple PWM modes of operations. This series is a rework
> of [1] and [2].

This series started with a RFC back on 5 April 2017 "extend PWM 
framework to support PWM modes". The continuous work starting with v2 of 
this series on January 12, 2018.

Then Claudiu tried to address all comments up to v4 which didn't have 
any more reviews. He posted a v5 without comments since May 22, 2018. 
This series is basically a resent of the v5 (as said in the $subject).

We would like to know what is preventing this series to be included in 
the PWM sub-system. Note that if some issue still remain with it, we are 
ready to help to solve them.

Without feedback from you side, we fear that we would miss a merge 
window again for no obvious reason (DT part is Acked by Rob: patch 5/9).

Best regards,
   Nicolas


> The current patch series add the following PWM modes:
> - PWM mode normal
> - PWM mode complementary
> - PWM mode push-pull
> 
> Normal mode - for PWM channels with one output; output waveforms looks like
> this:
>               __    __    __    __
>      PWM   __|  |__|  |__|  |__|  |__
>              <--T-->
> 
>      Where T is the signal period
> 
> Since PWMs with more than one output per channel could be used as one
> output PWM the normal mode is the default mode for all PWMs (if not
> specified otherwise).
> 
> Complementary mode - for PWM channels with two outputs; output waveforms
> for a PWM channel in complementary mode looks line this:
>               __    __    __    __
>      PWMH1 __|  |__|  |__|  |__|  |__
>            __    __    __    __    __
>      PWML1   |__|  |__|  |__|  |__|
>              <--T-->
> 
>      Where T is the signal period.
> 
> Push-pull mode - for PWM channels with two outputs; output waveforms for a
> PWM channel in push-pull mode with normal polarity looks like this:
>              __          __
>      PWMH __|  |________|  |________
>                    __          __
>      PWML ________|  |________|  |__
>             <--T-->
> 
>      If polarity is inversed:
>           __    ________    ________
>      PWMH   |__|        |__|
>           ________    ________    __
>      PWML         |__|        |__|
>             <--T-->
> 
>      Where T is the signal period.
> 
> The PWM working modes are per PWM channel registered as PWM's capabilities.
> The driver registers itself to PWM core a get_caps() function, in
> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
> If this function is not registered in driver's probe, a default function
> will be used to retrieve PWM capabilities. Currently, the default
> capabilities includes only PWM normal mode.
> 
> PWM state has been updated to keep PWM mode. PWM mode could be configured
> via sysfs or via DT. pwm_apply_state() will do the preliminary validation
> for PWM mode to be applied.
> 
> In sysfs, user could get PWM modes by reading mode file of PWM device:
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l
> total 0
> -r--r--r-- 1 root root 4096 Oct  9 09:07 capture
> lrwxrwxrwx 1 root root    0 Oct  9 09:07 device -> ../../pwmchip0
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 duty_cycle
> -rw-r--r-- 1 root root 4096 Oct  9 08:44 enable
> --w------- 1 root root 4096 Oct  9 09:07 export
> -rw-r--r-- 1 root root 4096 Oct  9 08:43 mode
> -r--r--r-- 1 root root 4096 Oct  9 09:07 npwm
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 period
> -rw-r--r-- 1 root root 4096 Oct  9 08:44 polarity
> drwxr-xr-x 2 root root    0 Oct  9 09:07 power
> lrwxrwxrwx 1 root root    0 Oct  9 09:07 subsystem -> ../../../../../../../../class/pwm
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 uevent
> --w------- 1 root root 4096 Oct  9 09:07 unexport
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
> normal complementary [push-pull]
> 
> The mode enclosed in bracket is the currently active mode.
> 
> The mode could be set, via sysfs, by writing to mode file one of the modes
> displayed at read:
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
> [normal] complementary push-pull
> 
> The PWM push-pull mode could be usefull in applications like half bridge
> converters.
> 
> This series also add PWM modes support for Atmel/Microchip SoCs.
> 
> Thank you,
> Claudiu Beznea
> 
> [1] https://www.spinics.net/lists/arm-kernel/msg580275.html
> [2] https://lkml.org/lkml/2018/1/12/359
> 
> Changes in v5:
> - solved kbuild errors by removing dummy functions for case where
>    CONFIG_PWM is not defined; adopted this approach since the removed
>    function are used only when CONFIG_PWM is defined (in PWM core
>    and few drivers from drivers/pwm/ directory)
> 
> Changes in v4:
> - removed changes related to pwm_config() as per maintainer proposals
> - added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device
>    instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use
>    pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1)
>    (patches 2, 3, 4 from this series)
> - renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with
>    pwm-sun4i.c driver ('C' stands for capability)
> - removed pwm_caps_valid() function
> - renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in
>    PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL
> 
> Changes in v3:
> - removed changes related to only one of_xlate function for all PWM drivers
> - switch to PWM capabilities per PWM channel nor per PWM chip
> - squash documentation and bindings patches as requeted by reviewer
> - introduced PWM_MODE(name) macro and used a bit enum for pwm modes
> - related to DT bindings, used flags cell also for PWM modes
> - updated of_xlate specific functions with "state->mode = mode;"
>    instructions to avoid pwm_apply_state() failures
> - use available modes for PWM channel in pwm_config() by first calling
>    pwm_get_caps() to get caps.modes
> - use loops through available modes in mode_store()/mode_show() and also in
>    of_pwm_xlate_with_flags() instead of "if else" instructions; in this way,
>    the addition of a new mode is independent of this code sections
> - use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms
>    for Atmel/Microchip PWM controller.
> 
> Changes in v2:
> - remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT
>    inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will
>    make easy the addition of PWM mode support from DT
> - add PWM mode normal
> - register PWM modes as capabilities of PWM chips at driver probe and, in case
>    driver doesn't provide these capabilities use default ones
> - change the way PWM mode is pharsed via DT by using a new input for pwms
>    binding property
> 
> 
> Claudiu Beznea (9):
>    pwm: extend PWM framework with PWM modes
>    pwm: clps711x: populate PWM mode in of_xlate function
>    pwm: cros-ec: populate PWM mode in of_xlate function
>    pwm: pxa: populate PWM mode in of_xlate function
>    pwm: add PWM modes
>    pwm: atmel: add pwm capabilities
>    pwm: add push-pull mode support
>    pwm: add documentation for pwm push-pull mode
>    pwm: atmel: add push-pull mode support
> 
>   Documentation/devicetree/bindings/pwm/pwm.txt |  11 ++-
>   Documentation/pwm.txt                         |  42 ++++++++-
>   drivers/pwm/core.c                            | 125 +++++++++++++++++++++++++-
>   drivers/pwm/pwm-atmel.c                       | 118 +++++++++++++++++-------
>   drivers/pwm/pwm-clps711x.c                    |  10 ++-
>   drivers/pwm/pwm-cros-ec.c                     |   1 +
>   drivers/pwm/pwm-pxa.c                         |   1 +
>   drivers/pwm/sysfs.c                           |  61 +++++++++++++
>   include/dt-bindings/pwm/pwm.h                 |   2 +
>   include/linux/pwm.h                           |  64 +++++++++++++
>   10 files changed, 395 insertions(+), 40 deletions(-)
> 


-- 
Nicolas Ferre

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Nicolas Ferre]

On 14/09/2018 at 18:20, Nicolas Ferre wrote:
> Thierry,
> 
> On 28/08/2018 at 15:01, Claudiu Beznea wrote:
>> Hi,
>>
>> Please give feedback on these patches which extends the PWM framework in
>> order to support multiple PWM modes of operations. This series is a rework
>> of [1] and [2].
> 
> This series started with a RFC back on 5 April 2017 "extend PWM
> framework to support PWM modes". The continuous work starting with v2 of
> this series on January 12, 2018.
> 
> Then Claudiu tried to address all comments up to v4 which didn't have
> any more reviews. He posted a v5 without comments since May 22, 2018.
> This series is basically a resent of the v5 (as said in the $subject).
> 
> We would like to know what is preventing this series to be included in
> the PWM sub-system. Note that if some issue still remain with it, we are
> ready to help to solve them.
> 
> Without feedback from you side, we fear that we would miss a merge
> window again for no obvious reason (DT part is Acked by Rob: patch 5/9).

3 weeks no news about my previous ping...

ping again!

> Best regards,
>     Nicolas
> 
> 
>> The current patch series add the following PWM modes:
>> - PWM mode normal
>> - PWM mode complementary
>> - PWM mode push-pull
>>
>> Normal mode - for PWM channels with one output; output waveforms looks like
>> this:
>>                __    __    __    __
>>       PWM   __|  |__|  |__|  |__|  |__
>>               <--T-->
>>
>>       Where T is the signal period
>>
>> Since PWMs with more than one output per channel could be used as one
>> output PWM the normal mode is the default mode for all PWMs (if not
>> specified otherwise).
>>
>> Complementary mode - for PWM channels with two outputs; output waveforms
>> for a PWM channel in complementary mode looks line this:
>>                __    __    __    __
>>       PWMH1 __|  |__|  |__|  |__|  |__
>>             __    __    __    __    __
>>       PWML1   |__|  |__|  |__|  |__|
>>               <--T-->
>>
>>       Where T is the signal period.
>>
>> Push-pull mode - for PWM channels with two outputs; output waveforms for a
>> PWM channel in push-pull mode with normal polarity looks like this:
>>               __          __
>>       PWMH __|  |________|  |________
>>                     __          __
>>       PWML ________|  |________|  |__
>>              <--T-->
>>
>>       If polarity is inversed:
>>            __    ________    ________
>>       PWMH   |__|        |__|
>>            ________    ________    __
>>       PWML         |__|        |__|
>>              <--T-->
>>
>>       Where T is the signal period.
>>
>> The PWM working modes are per PWM channel registered as PWM's capabilities.
>> The driver registers itself to PWM core a get_caps() function, in
>> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
>> If this function is not registered in driver's probe, a default function
>> will be used to retrieve PWM capabilities. Currently, the default
>> capabilities includes only PWM normal mode.
>>
>> PWM state has been updated to keep PWM mode. PWM mode could be configured
>> via sysfs or via DT. pwm_apply_state() will do the preliminary validation
>> for PWM mode to be applied.
>>
>> In sysfs, user could get PWM modes by reading mode file of PWM device:
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l
>> total 0
>> -r--r--r-- 1 root root 4096 Oct  9 09:07 capture
>> lrwxrwxrwx 1 root root    0 Oct  9 09:07 device -> ../../pwmchip0
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 duty_cycle
>> -rw-r--r-- 1 root root 4096 Oct  9 08:44 enable
>> --w------- 1 root root 4096 Oct  9 09:07 export
>> -rw-r--r-- 1 root root 4096 Oct  9 08:43 mode
>> -r--r--r-- 1 root root 4096 Oct  9 09:07 npwm
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 period
>> -rw-r--r-- 1 root root 4096 Oct  9 08:44 polarity
>> drwxr-xr-x 2 root root    0 Oct  9 09:07 power
>> lrwxrwxrwx 1 root root    0 Oct  9 09:07 subsystem -> ../../../../../../../../class/pwm
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 uevent
>> --w------- 1 root root 4096 Oct  9 09:07 unexport
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
>> normal complementary [push-pull]
>>
>> The mode enclosed in bracket is the currently active mode.
>>
>> The mode could be set, via sysfs, by writing to mode file one of the modes
>> displayed at read:
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
>> [normal] complementary push-pull
>>
>> The PWM push-pull mode could be usefull in applications like half bridge
>> converters.
>>
>> This series also add PWM modes support for Atmel/Microchip SoCs.
>>
>> Thank you,
>> Claudiu Beznea
>>
>> [1] https://www.spinics.net/lists/arm-kernel/msg580275.html
>> [2] https://lkml.org/lkml/2018/1/12/359
>>
>> Changes in v5:
>> - solved kbuild errors by removing dummy functions for case where
>>     CONFIG_PWM is not defined; adopted this approach since the removed
>>     function are used only when CONFIG_PWM is defined (in PWM core
>>     and few drivers from drivers/pwm/ directory)
>>
>> Changes in v4:
>> - removed changes related to pwm_config() as per maintainer proposals
>> - added pwm_mode_get_valid() to retrieve a valid PWM mode fror PWM device
>>     instead of using BIT(ffs(caps.mode) - 1) and changed drivers to use
>>     pwm_mode_get_valid() instead of pwm_get_caps() + BIT(ffs(caps.mode) - 1)
>>     (patches 2, 3, 4 from this series)
>> - renamed PWM_MODE() macro in PWMC_MODE() to avoid conflicts with
>>     pwm-sun4i.c driver ('C' stands for capability)
>> - removed pwm_caps_valid() function
>> - renamed PWM_DTMODE_COMPLEMENTARY and PWM_DTMODE_PUSH_PULL macros in
>>     PWM_MODE_COMPLEMENTARY and PWM_MODE_PUSH_PULL
>>
>> Changes in v3:
>> - removed changes related to only one of_xlate function for all PWM drivers
>> - switch to PWM capabilities per PWM channel nor per PWM chip
>> - squash documentation and bindings patches as requeted by reviewer
>> - introduced PWM_MODE(name) macro and used a bit enum for pwm modes
>> - related to DT bindings, used flags cell also for PWM modes
>> - updated of_xlate specific functions with "state->mode = mode;"
>>     instructions to avoid pwm_apply_state() failures
>> - use available modes for PWM channel in pwm_config() by first calling
>>     pwm_get_caps() to get caps.modes
>> - use loops through available modes in mode_store()/mode_show() and also in
>>     of_pwm_xlate_with_flags() instead of "if else" instructions; in this way,
>>     the addition of a new mode is independent of this code sections
>> - use DTLI=1, DTHI=0 register settings to obtain push-pull mode waveforms
>>     for Atmel/Microchip PWM controller.
>>
>> Changes in v2:
>> - remove of_xlate and of_pwm_n_cells and use generic functions to pharse DT
>>     inputs; this is done in patches 1, 2, 3, 4, 5, 6, 7 of this series; this will
>>     make easy the addition of PWM mode support from DT
>> - add PWM mode normal
>> - register PWM modes as capabilities of PWM chips at driver probe and, in case
>>     driver doesn't provide these capabilities use default ones
>> - change the way PWM mode is pharsed via DT by using a new input for pwms
>>     binding property
>>
>>
>> Claudiu Beznea (9):
>>     pwm: extend PWM framework with PWM modes
>>     pwm: clps711x: populate PWM mode in of_xlate function
>>     pwm: cros-ec: populate PWM mode in of_xlate function
>>     pwm: pxa: populate PWM mode in of_xlate function
>>     pwm: add PWM modes
>>     pwm: atmel: add pwm capabilities
>>     pwm: add push-pull mode support
>>     pwm: add documentation for pwm push-pull mode
>>     pwm: atmel: add push-pull mode support
>>
>>    Documentation/devicetree/bindings/pwm/pwm.txt |  11 ++-
>>    Documentation/pwm.txt                         |  42 ++++++++-
>>    drivers/pwm/core.c                            | 125 +++++++++++++++++++++++++-
>>    drivers/pwm/pwm-atmel.c                       | 118 +++++++++++++++++-------
>>    drivers/pwm/pwm-clps711x.c                    |  10 ++-
>>    drivers/pwm/pwm-cros-ec.c                     |   1 +
>>    drivers/pwm/pwm-pxa.c                         |   1 +
>>    drivers/pwm/sysfs.c                           |  61 +++++++++++++
>>    include/dt-bindings/pwm/pwm.h                 |   2 +
>>    include/linux/pwm.h                           |  64 +++++++++++++
>>    10 files changed, 395 insertions(+), 40 deletions(-)
>>
> 
> 


-- 
Nicolas Ferre

Re: [RESEND PATCH v5 8/9] pwm: add documentation for pwm push-pull mode

[Thierry Reding]

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

On Tue, Aug 28, 2018 at 04:01:25PM +0300, Claudiu Beznea wrote:
> Add documentation for PWM push-pull mode.
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/pwm/pwm.txt |  2 ++
>  Documentation/pwm.txt                         | 16 ++++++++++++++++
>  include/dt-bindings/pwm/pwm.h                 |  1 +
>  3 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
> index 7c8aaac43f92..6a60c0fca112 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
> @@ -49,6 +49,8 @@ Optionally, the pwm-specifier can encode a number of flags (defined in
>  - PWM_MODE_COMPLEMENTARY: PWM complementary working mode (for PWM channels
>  with two outputs); if not specified, the default for PWM channel will be
>  used
> +- PWM_MODE_PUSH_PULL: PWM push-pull working modes (for PWM channels with
> +two outputs); if not specified the default for PWM channel will be used

What if somebody has this in the DT:

	PWM_MODE_COMPLEMENTARY | PWM_MODE_PUSH_PULL

which one takes precedence, or do we reject it?

Wouldn't it be preferable to either move the modes into an extra field
within the flags field, or perhaps even add another field?

I guess since Rob's already acked this, that concern may be unfounded.

Thierry

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

Re: [RESEND PATCH v5 8/9] pwm: add documentation for pwm push-pull mode

[Claudiu.Beznea]

On 12.10.2018 15:15, Thierry Reding wrote:
> On Tue, Aug 28, 2018 at 04:01:25PM +0300, Claudiu Beznea wrote:
>> Add documentation for PWM push-pull mode.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm.txt |  2 ++
>>  Documentation/pwm.txt                         | 16 ++++++++++++++++
>>  include/dt-bindings/pwm/pwm.h                 |  1 +
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm.txt b/Documentation/devicetree/bindings/pwm/pwm.txt
>> index 7c8aaac43f92..6a60c0fca112 100644
>> --- a/Documentation/devicetree/bindings/pwm/pwm.txt
>> +++ b/Documentation/devicetree/bindings/pwm/pwm.txt
>> @@ -49,6 +49,8 @@ Optionally, the pwm-specifier can encode a number of flags (defined in
>>  - PWM_MODE_COMPLEMENTARY: PWM complementary working mode (for PWM channels
>>  with two outputs); if not specified, the default for PWM channel will be
>>  used
>> +- PWM_MODE_PUSH_PULL: PWM push-pull working modes (for PWM channels with
>> +two outputs); if not specified the default for PWM channel will be used
> 
> What if somebody has this in the DT:
> 
> 	PWM_MODE_COMPLEMENTARY | PWM_MODE_PUSH_PULL
> 
> which one takes precedence, or do we reject it?

The first valid one will be selected.

In patch 1/1 from this series, changes added to of_pwm_xlate_with_flags()
function, there is this code:

+		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
+		     modebit < PWMC_MODE_CNT; modebit++) {
+			unsigned long mode = BIT(modebit);
+
+			if ((args->args[2] & mode) &&
+			    pwm_mode_valid(pwm, mode)) {
+				pwm->args.mode = mode;
+				break;
+			}
+		}

And since the modes bits are defined as follows:

enum {
	PWMC_MODE_NORMAL_BIT,
	PWMC_MODE_COMPLEMENTARY_BIT,
	PWMC_MODE_PUSH_PULL_BIT,
	PWMC_MODE_CNT,
};

in your proposed scenario: PWM_MODE_COMPLEMENTARY | PWM_MODE_PUSH_PULL
the PWM_MODE_COMPLEMENTARY mode will be selected since it is the first
valid one.

> 
> Wouldn't it be preferable to either move the modes into an extra field
> within the flags field, or perhaps even add another field?

This approach was proposed in version 2 of this series and based on the
discussions I had with Rob Herring [1] I decided to use the remaining space
from cell specific to PWM flags.

Thank you,
Claudiu Beznea

[1] https://lkml.org/lkml/2018/1/22/655

> 
> I guess since Rob's already acked this, that concern may be unfounded.
> 
> Thierry
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Thierry Reding]

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

On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote:
> Thierry,
> 
> On 28/08/2018 at 15:01, Claudiu Beznea wrote:
> > Hi,
> > 
> > Please give feedback on these patches which extends the PWM framework in
> > order to support multiple PWM modes of operations. This series is a rework
> > of [1] and [2].
> 
> This series started with a RFC back on 5 April 2017 "extend PWM framework to
> support PWM modes". The continuous work starting with v2 of this series on
> January 12, 2018.
> 
> Then Claudiu tried to address all comments up to v4 which didn't have any
> more reviews. He posted a v5 without comments since May 22, 2018. This
> series is basically a resent of the v5 (as said in the $subject).
> 
> We would like to know what is preventing this series to be included in the
> PWM sub-system. Note that if some issue still remain with it, we are ready
> to help to solve them.
> 
> Without feedback from you side, we fear that we would miss a merge window
> again for no obvious reason (DT part is Acked by Rob: patch 5/9).

First off, apologies for not getting around to this earlier.

I think this series is mostly fine, but I still have doubts about the DT
aspects of this. In particular, Rob raised a concern about this here:

	https://lkml.org/lkml/2018/1/22/655

and it seems like that particular question was never fully resolved as
the discussion veered off that particular topic. I know that Rob acked
the DT parts of this, but I suspect that this might have been glossed
over.

To restate the concern: these extended modes have special uses and none
of the users in the kernel, other than sysfs, can use anything other
than the normal mode. They may work fine with other modes, but only if
they ignore the extras that come with them. Therefore I think it's safe
to say that anyone who would want to use these modes would want to
explicitly say so. For example the sysfs interface already does that by
changing the mode only after the "mode" attribute is written. Any users
for special use-cases would want to do the same thing, that is, drive a
PWM in a specific mode, on purpose. You wouldn't have a "generic" user
such as pwm-backlight or leds-pwm request anything other than the normal
mode.

So my question is, do we really need to represent these modes in DT? The
series currently doesn't contain any patches that add users of these new
modes. Are such patches available somewhere, or is the only user of this
supposed to be sysfs?

I'm hesitant to move forward with this as-is without seeing how it will
be used. The PWM specifier flags are somewhat abused by adding modes to
them. I think this hasn't been completely thought through, because the
only reason to specify a mode is to actually set that mode. But then the
DT ABI allows a bitmask of modes to be requested via DT. I know that
only the first of those modes will end up being used, but then why even
allow it in the first place?

And again, even if we allow the mode to be specified in DT, how do the
consumer drivers know that the correct mode was being set in DT. Let's
say we have a consumer that requires the PWM to be driven in
complementary mode. Should it rely on the DT to contain the correct
specification for the mode? And if it knows that it needs complementary
mode, why not just set that mode itself? That way there's no margin for
error.

Thierry

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

Re: [RESEND PATCH v5 1/9] pwm: extend PWM framework with PWM modes

[Thierry Reding]

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

On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote:
> Add basic PWM modes: normal and complementary. These modes should
> differentiate the single output PWM channels from two outputs PWM
> channels. These modes could be set as follow:
> 1. PWM channels with one output per channel:
> - normal mode
> 2. PWM channels with two outputs per channel:
> - normal mode
> - complementary mode
> Since users could use a PWM channel with two output as one output PWM
> channel, the PWM normal mode is allowed to be set for PWM channels with
> two outputs; in fact PWM normal mode should be supported by all PWMs.
> 
> The PWM capabilities were implemented per PWM channel. Every PWM controller
> will register a function to get PWM capabilities. If this is not explicitly
> set by the driver a default function will be used to retrieve the PWM
> capabilities (in this case the PWM capabilities will contain only PWM
> normal mode). This function is set in pwmchip_add_with_polarity() as a
> member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps()
> function could be used.
> 
> Every PWM channel have associated a mode in the PWM state. Proper
> support was added to get/set PWM mode. The mode could also be set
> from DT via flag cells. The valid DT modes are located in
> include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be
> set. If nothing is specified for a PWM channel, via DT, the first available
> mode will be used (normally, this will be PWM normal mode).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
> ---
>  drivers/pwm/core.c  | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  drivers/pwm/sysfs.c |  61 ++++++++++++++++++++++++++
>  include/linux/pwm.h |  39 +++++++++++++++++
>  3 files changed, 221 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 1581f6ab1b1f..59a9df9120de 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -136,6 +136,7 @@ struct pwm_device *
>  of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
>  {
>  	struct pwm_device *pwm;
> +	int modebit;
>  
>  	/* check, whether the driver supports a third cell for flags */
>  	if (pc->of_pwm_n_cells < 3)
> @@ -154,9 +155,23 @@ 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_get_valid(pc, pwm);
>  
> -	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;
> +
> +		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
> +		     modebit < PWMC_MODE_CNT; modebit++) {
> +			unsigned long mode = BIT(modebit);
> +
> +			if ((args->args[2] & mode) &&
> +			    pwm_mode_valid(pwm, mode)) {
> +				pwm->args.mode = mode;
> +				break;
> +			}
> +		}
> +	}
>  
>  	return pwm;
>  }
> @@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
>  		return pwm;
>  
>  	pwm->args.period = args->args[1];
> +	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
>  
>  	return pwm;
>  }
> @@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops)
>  }
>  
>  /**
> + * pwm_get_caps() - get PWM capabilities of a PWM device
> + * @chip: PWM chip
> + * @pwm: PWM device to get the capabilities for
> + * @caps: returned capabilities
> + *
> + * Returns: 0 on success or a negative error code on failure
> + */
> +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm,
> +		 struct pwm_caps *caps)
> +{
> +	if (!chip || !pwm || !caps)
> +		return -EINVAL;
> +
> +	if (chip->ops && chip->ops->get_caps)
> +		pwm->chip->ops->get_caps(chip, pwm, caps);
> +	else if (chip->get_default_caps)
> +		chip->get_default_caps(caps);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pwm_get_caps);

I'm confused by the way ->get_default_caps() is used here. This always
points at pwmchip_get_default_caps() if I understand correctly, so why
bother with the indirection?

I think it would be better to either export pwmchip_get_default_caps()
as a default implementation of ->get_caps(), something like:

	void pwm_get_default_caps(struct pwm_chip *chip, struct pwm_device *pwm,
				  struct pwm_caps *caps)
	{
		...
	}
	EXPORT_SYMBOL_GPL(pwm_get_default_caps);

This could be used by the individual drivers as the implementation if
they don't support any modes.

Another, probably simpler approach would be to just get rid of the
chip->get_default_caps() pointer and directly call
pwmchip_get_default_caps() from pwm_get_caps():

	int pwm_get_caps(...)
	{
		/*
		 * Note that you don't need the check for chip->ops because
		 * the core checks for that upon registration of the chip.
		 */
		if (chip->ops->get_caps)
			return chip->ops->get_caps(...);

		return pwm_get_default_caps(...);
	}

And if we do that, might as well fold pwm_get_default_caps() into
pwm_get_caps().

> +/**
> + * pwm_mode_get_valid() - get the first available valid mode for PWM
> + * @chip: PWM chip
> + * @pwm: PWM device to get the valid mode for
> + *
> + * Returns: first valid mode for PWM device
> + */
> +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm)

This is a little ambiguous. Perhaps pwm_get_default_mode()?

> +/**
> + * pwm_mode_valid() - check if mode is valid for PWM device
> + * @pwm: PWM device
> + * @mode: PWM mode to check if valid
> + *
> + * Returns: true if mode is valid and false otherwise
> + */
> +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode)

Again, this is slightly ambiguous because the name suggests that it
merely tests a mode for validity, not that it is really supported by the
given device. Perhaps something like pwm_supports_mode()?

> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
> +{
> +	static const char * const modes[] = {
> +		"invalid",
> +		"normal",
> +		"complementary",
> +	};
> +
> +	if (!pwm_mode_valid(pwm, mode))
> +		return modes[0];
> +
> +	return modes[ffs(mode)];
> +}

Do we really need to be able to get the name of the mode in the context
of a given PWM channel? Couldn't we drop the pwm parameter and simply
return the name (pwm_get_mode_name()?) and at the same time remove the
extra "invalid" mode in there? I'm not sure what the use-case here is,
but it seems to me like the code should always check for supported modes
first before reporting their names in any way.

> +/**
>   * pwmchip_add_with_polarity() - register a new PWM chip
>   * @chip: the PWM chip to add
>   * @polarity: initial polarity of PWM channels
> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  
>  	mutex_lock(&pwm_lock);
>  
> +	chip->get_default_caps = pwmchip_get_default_caps;
> +
>  	ret = alloc_pwms(chip->base, chip->npwm);
>  	if (ret < 0)
>  		goto out;
> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>  		pwm->pwm = chip->base + i;
>  		pwm->hwpwm = i;
>  		pwm->state.polarity = polarity;
> +		pwm->state.mode = pwm_mode_get_valid(chip, pwm);
>  
>  		if (chip->ops->get_state)
>  			chip->ops->get_state(chip, pwm, &pwm->state);
> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  	int err;
>  
>  	if (!pwm || !state || !state->period ||
> -	    state->duty_cycle > state->period)
> +	    state->duty_cycle > state->period ||
> +	    !pwm_mode_valid(pwm, state->mode))
>  		return -EINVAL;
>  
>  	if (!memcmp(state, &pwm->state, sizeof(*state)))
> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>  
>  			pwm->state.enabled = state->enabled;
>  		}
> +
> +		/* No mode support for non-atomic PWM. */
> +		pwm->state.mode = state->mode;

That comment seems misplaced. This is actually part of atomic PWM, so
maybe just reverse the logic and say "mode support only for atomic PWM"
or something. I would personally just leave it away. The legacy API has
no way of setting the mode, which is indication enough that we don't
support it.

> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> index 56518adc31dd..a4ce4ad7edf0 100644
> --- a/include/linux/pwm.h
> +++ b/include/linux/pwm.h
> @@ -26,9 +26,32 @@ enum pwm_polarity {
>  };
>  
>  /**
> + * PWM modes capabilities
> + * @PWMC_MODE_NORMAL_BIT: PWM has one output
> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
> + * @PWMC_MODE_CNT: PWM modes count
> + */
> +enum {
> +	PWMC_MODE_NORMAL_BIT,
> +	PWMC_MODE_COMPLEMENTARY_BIT,
> +	PWMC_MODE_CNT,
> +};
> +
> +#define PWMC_MODE(name)		BIT(PWMC_MODE_##name##_BIT)

Why the C in the prefix? Why not just PWM_MODE_* for all of the above?

> +
> +/**
> + * struct pwm_caps - PWM capabilities
> + * @modes: PWM modes

This should probably say that it's a bitmask of PWM_MODE_*.

> +struct pwm_caps {
> +	unsigned long modes;
> +};
> +
> +/**
>   * struct pwm_args - board-dependent PWM arguments
>   * @period: reference period
>   * @polarity: reference polarity
> + * @mode: reference mode

As discussed in my reply to the cover letter, what is the use for the
reference mode? Drivers want to use either normal or some other mode.
What good is it to get this from board-dependent arguments if the
driver already knows which one it wants or needs?

> @@ -300,6 +331,7 @@ struct pwm_chip {
>  
>  	struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>  					const struct of_phandle_args *args);
> +	void (*get_default_caps)(struct pwm_caps *caps);

As mentioned above, I don't think we need this.

Thierry

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

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Claudiu.Beznea]

On 16.10.2018 15:03, Thierry Reding wrote:
> On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote:
>> Thierry,
>>
>> On 28/08/2018 at 15:01, Claudiu Beznea wrote:
>>> Hi,
>>>
>>> Please give feedback on these patches which extends the PWM framework in
>>> order to support multiple PWM modes of operations. This series is a rework
>>> of [1] and [2].
>>
>> This series started with a RFC back on 5 April 2017 "extend PWM framework to
>> support PWM modes". The continuous work starting with v2 of this series on
>> January 12, 2018.
>>
>> Then Claudiu tried to address all comments up to v4 which didn't have any
>> more reviews. He posted a v5 without comments since May 22, 2018. This
>> series is basically a resent of the v5 (as said in the $subject).
>>
>> We would like to know what is preventing this series to be included in the
>> PWM sub-system. Note that if some issue still remain with it, we are ready
>> to help to solve them.
>>
>> Without feedback from you side, we fear that we would miss a merge window
>> again for no obvious reason (DT part is Acked by Rob: patch 5/9).
> 
> First off, apologies for not getting around to this earlier.
> 
> I think this series is mostly fine, but I still have doubts about the DT
> aspects of this. In particular, Rob raised a concern about this here:
> 
> 	https://lkml.org/lkml/2018/1/22/655
> 
> and it seems like that particular question was never fully resolved as
> the discussion veered off that particular topic.

1/ If you are talking about this sentence:
"Yes, but you have to make "normal" be no bit set to be compatible with
everything already out there."

The current implementation consider that if no mode is provided then, the
old approach is considered, meaning the normal mode will be used by every
PWM in-kernel clients.

In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what
pwm_mode_get_valid() returns. In case of controllers which does not
implement something special for PWM modes the PWM normal mode will be
returned (pwmchip_get_default_caps() function has to be called in the end).
Otherwise the pwm->args.mode will be populated with what user provided as
input from DT, if what was provided from DT is valid for PWM channel.
Please see that pwm_mode_valid() is used to validate user input, otherwise
PWM normal mode will be used.

+	pwm->args.mode = pwm_mode_get_valid(pc, pwm);

-	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;
+
+		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
+		     modebit < PWMC_MODE_CNT; modebit++) {
+			unsigned long mode = BIT(modebit);
+
+			if ((args->args[2] & mode) &&
+			    pwm_mode_valid(pwm, mode)) {
+				pwm->args.mode = mode;
+				break;
+			}
+		}
+	}


2/ If you are talking about this sentence:
"Thinking about this some more, shouldn't the new modes just be
implied? A client is going to require one of these modes or it won't
work right."

As explained at point 1, if there is no mode requested from DT the default
mode for channel will be used, which, in case of PWM controller which are
not implementing the new modes, will be PWM normal mode.

3/ If you are talking about:
"Also complementary mode could be accomplished with a single pwm output
and a board level inverter, right? How would that be handled when the
PWM driver doesn't support that mode?"
This complicates the things and I think it requires extra device tree
bindings to describe extra per-pwm channel capabilities. For the moment,
with this series I've stopped to only have the capabilities registered from
driver. My understanding is that this could be accomplished with extra
device tree bindings in PWM controller to describe PWM channels
capabilities. If you also want me to look into this direction please let me
know. And also, suggestions would be appreciated.

I know that Rob acked
> the DT parts of this, but I suspect that this might have been glossed
> over.

If this is about point 3 I've emphasize above I would like to have some
inputs from your side, if you agree with a behavior like having extra DT
bindings. The intention wasn't to left this over but to have a better
understanding of the subject. I'm thinking if it is ok to have modules
outside of the SoC that model a behavior that could not be controlled from
software (it could be only hardware) but to behave in a single way. Take
for instance this scenario:

- new DT bindings are implemented to specify this behavior per channel
- hardware modules are soldered outside of a PWM channel with one output
- the new DT bindings are not specified for the soldered PWM channel
- user enables this channel, it will have only normal mode available for it
to configure (because the new DT bindings were not provided)
- but the real output the user will see would be in complementary or even
push-pull mode.

> 
> To restate the concern: these extended modes have special uses and none
> of the users in the kernel, other than sysfs, can use anything other
> than the normal mode. They may work fine with other modes, but only if
> they ignore the extras that come with them. Therefore I think it's safe
> to say that anyone who would want to use these modes would want to
> explicitly say so. For example the sysfs interface already does that by
> changing the mode only after the "mode" attribute is written. Any users
> for special use-cases would want to do the same thing, that is, drive a
> PWM in a specific mode, on purpose. You wouldn't have a "generic" user
> such as pwm-backlight or leds-pwm request anything other than the normal
> mode.
> 
> So my question is, do we really need to represent these modes in DT? The
> series currently doesn't contain any patches that add users of these new
> modes. Are such patches available somewhere, or is the only user of this
> supposed to be sysfs?

For the moment, no, there is no in-kernel user for this, only sysfs. I had
in mind to adapt the use of these new mode for PWM regulator for scenarios
described in [1] page 2126. Anyway, since these patches doesn't introduce
any user other that sysfs it will not disturbed me to drop the changes. By
the time I or someone else will do some other changes that requires this,
the DT part should also be introduced.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

> 
> I'm hesitant to move forward with this as-is without seeing how it will
> be used.

For the moment only sysfs is supposed to use this.

The PWM specifier flags are somewhat abused by adding modes to
> them. 

I see.

I think this hasn't been completely thought through, because the
> only reason to specify a mode is to actually set that mode.

Maybe it wasn't clear understand, with the current implementation if no
mode will be specified the default mode will be used. There is no impose to
specify a mode.

 But then the
> DT ABI allows a bitmask of modes to be requested via DT. I know that
> only the first of those modes will end up being used, but then why even
> allow it in the first place?

I had in mind that I will change the PWM regulator driver to work in
scenarios like the one specified in the link above.

> 
> And again, even if we allow the mode to be specified in DT, how do the
> consumer drivers know that the correct mode was being set in DT. 

PWM user will call at some point devm_pwm_get() which, in turn, will call
of_pwm_get() which in turn will initialize PWM args with inputs from DT.
After that PWM user will, at some point, apply a PWM state; if it is
initialized with PWM args initialized when devm_pwm_get() was called then
pwm_apply_state() would fail if no good mode is provided as input via DT.

Same thing happens if a bad period is provided via DT.

Let's
> say we have a consumer that requires the PWM to be driven in
> complementary mode. Should it rely on the DT to contain the correct
> specification for the mode? And if it knows that it needs complementary
> mode, why not just set that mode itself?

I'm thinking it's the same way as is with PWM period which could also be
provided from DT. In the end a bad period value could be provided from
device tree. E.g. Atmel PWM controller could generate PWM signals who's
periods could not be higher than ~0.6 seconds. If a bad value is provided
the pwm_apply_state() will fail.

Thank you,
Claudiu Beznea

That way there's no margin for
> error.


> 
> Thierry
> 

Re: [RESEND PATCH v5 1/9] pwm: extend PWM framework with PWM modes

[Claudiu.Beznea]

On 16.10.2018 15:25, Thierry Reding wrote:
> On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote:
>> Add basic PWM modes: normal and complementary. These modes should
>> differentiate the single output PWM channels from two outputs PWM
>> channels. These modes could be set as follow:
>> 1. PWM channels with one output per channel:
>> - normal mode
>> 2. PWM channels with two outputs per channel:
>> - normal mode
>> - complementary mode
>> Since users could use a PWM channel with two output as one output PWM
>> channel, the PWM normal mode is allowed to be set for PWM channels with
>> two outputs; in fact PWM normal mode should be supported by all PWMs.
>>
>> The PWM capabilities were implemented per PWM channel. Every PWM controller
>> will register a function to get PWM capabilities. If this is not explicitly
>> set by the driver a default function will be used to retrieve the PWM
>> capabilities (in this case the PWM capabilities will contain only PWM
>> normal mode). This function is set in pwmchip_add_with_polarity() as a
>> member of "struct pwm_chip". To retrieve capabilities the pwm_get_caps()
>> function could be used.
>>
>> Every PWM channel have associated a mode in the PWM state. Proper
>> support was added to get/set PWM mode. The mode could also be set
>> from DT via flag cells. The valid DT modes are located in
>> include/dt-bindings/pwm/pwm.h. Only modes supported by PWM channel could be
>> set. If nothing is specified for a PWM channel, via DT, the first available
>> mode will be used (normally, this will be PWM normal mode).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com>
>> ---
>>  drivers/pwm/core.c  | 124 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  drivers/pwm/sysfs.c |  61 ++++++++++++++++++++++++++
>>  include/linux/pwm.h |  39 +++++++++++++++++
>>  3 files changed, 221 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 1581f6ab1b1f..59a9df9120de 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -136,6 +136,7 @@ struct pwm_device *
>>  of_pwm_xlate_with_flags(struct pwm_chip *pc, const struct of_phandle_args *args)
>>  {
>>  	struct pwm_device *pwm;
>> +	int modebit;
>>  
>>  	/* check, whether the driver supports a third cell for flags */
>>  	if (pc->of_pwm_n_cells < 3)
>> @@ -154,9 +155,23 @@ 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_get_valid(pc, pwm);
>>  
>> -	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;
>> +
>> +		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
>> +		     modebit < PWMC_MODE_CNT; modebit++) {
>> +			unsigned long mode = BIT(modebit);
>> +
>> +			if ((args->args[2] & mode) &&
>> +			    pwm_mode_valid(pwm, mode)) {
>> +				pwm->args.mode = mode;
>> +				break;
>> +			}
>> +		}
>> +	}
>>  
>>  	return pwm;
>>  }
>> @@ -183,6 +198,7 @@ of_pwm_simple_xlate(struct pwm_chip *pc, const struct of_phandle_args *args)
>>  		return pwm;
>>  
>>  	pwm->args.period = args->args[1];
>> +	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
>>  
>>  	return pwm;
>>  }
>> @@ -250,6 +266,97 @@ static bool pwm_ops_check(const struct pwm_ops *ops)
>>  }
>>  
>>  /**
>> + * pwm_get_caps() - get PWM capabilities of a PWM device
>> + * @chip: PWM chip
>> + * @pwm: PWM device to get the capabilities for
>> + * @caps: returned capabilities
>> + *
>> + * Returns: 0 on success or a negative error code on failure
>> + */
>> +int pwm_get_caps(struct pwm_chip *chip, struct pwm_device *pwm,
>> +		 struct pwm_caps *caps)
>> +{
>> +	if (!chip || !pwm || !caps)
>> +		return -EINVAL;
>> +
>> +	if (chip->ops && chip->ops->get_caps)
>> +		pwm->chip->ops->get_caps(chip, pwm, caps);
>> +	else if (chip->get_default_caps)
>> +		chip->get_default_caps(caps);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(pwm_get_caps);
> 
> I'm confused by the way ->get_default_caps() is used here. This always
> points at pwmchip_get_default_caps() if I understand correctly, so why
> bother with the indirection?

Yep, looking again at this it also looks ugly to me and I dont't remember
why I chose this approach. I looked over emails and so but didn't manage to
find the reason for this. I agree with you that there is no need to keep a
pointer in struct pwm_chip for this.

> 
> I think it would be better to either export pwmchip_get_default_caps()
> as a default implementation of ->get_caps(), something like:
> 
> 	void pwm_get_default_caps(struct pwm_chip *chip, struct pwm_device *pwm,
> 				  struct pwm_caps *caps)
> 	{
> 		...
> 	}
> 	EXPORT_SYMBOL_GPL(pwm_get_default_caps);
> 
> This could be used by the individual drivers as the implementation if
> they don't support any modes.
> 
> Another, probably simpler approach would be to just get rid of the
> chip->get_default_caps() pointer and directly call
> pwmchip_get_default_caps() from pwm_get_caps():
> 
> 	int pwm_get_caps(...)
> 	{
> 		/*
> 		 * Note that you don't need the check for chip->ops because
> 		 * the core checks for that upon registration of the chip.
> 		 */
> 		if (chip->ops->get_caps)
> 			return chip->ops->get_caps(...);
> 
> 		return pwm_get_default_caps(...);
> 	}
> 
> And if we do that, might as well fold pwm_get_default_caps() into
> pwm_get_caps().

I am also in favor of this approach.

> 
>> +/**
>> + * pwm_mode_get_valid() - get the first available valid mode for PWM
>> + * @chip: PWM chip
>> + * @pwm: PWM device to get the valid mode for
>> + *
>> + * Returns: first valid mode for PWM device
>> + */
>> +unsigned long pwm_mode_get_valid(struct pwm_chip *chip, struct pwm_device *pwm)
> 
> This is a little ambiguous. Perhaps pwm_get_default_mode()?


Ok, will change it to pwm_get_default_mode(). I placed pwm_mode in front of
every function in this patch to emphasize the functions are dealing with
PWM modes. But being inside PWM core and using this approach might be
confusing.

> 
>> +/**
>> + * pwm_mode_valid() - check if mode is valid for PWM device
>> + * @pwm: PWM device
>> + * @mode: PWM mode to check if valid
>> + *
>> + * Returns: true if mode is valid and false otherwise
>> + */
>> +bool pwm_mode_valid(struct pwm_device *pwm, unsigned long mode)
> 
> Again, this is slightly ambiguous because the name suggests that it
> merely tests a mode for validity, not that it is really supported by the
> given device. Perhaps something like pwm_supports_mode()?

Ok, understand. I'll use pwm_supports_mode() instead.

> 
>> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
>> +{
>> +	static const char * const modes[] = {
>> +		"invalid",
>> +		"normal",
>> +		"complementary",
>> +	};
>> +
>> +	if (!pwm_mode_valid(pwm, mode))
>> +		return modes[0];
>> +
>> +	return modes[ffs(mode)];
>> +}
> 
> Do we really need to be able to get the name of the mode in the context
> of a given PWM channel? Couldn't we drop the pwm parameter and simply
> return the name (pwm_get_mode_name()?) and at the same time remove the
> extra "invalid" mode in there? I'm not sure what the use-case here is,
> but it seems to me like the code should always check for supported modes
> first before reporting their names in any way.

Looking back at this code, the main use case for checking PWM mode validity
in pwm_mode_desc() was only with regards to mode_store(). But there is not
need for this checking since the same thing will be checked in
pwm_apply_state() and, in case user provides an invalid mode via sysfs the
pwm_apply_state() will fail.

To conclude, I will change this function in something like:

if (mode == PWM_MODE_NORMAL)
	return "normal";
else if (mode == PWM_MODE_COMPLEMENTARY)
	return "complementary";
else if (mode == PWM_MODE_PUSH_PULL)
	return "push-pull";
else
	return "invalid";

Please let me know if it is OK for you.

> 
>> +/**
>>   * pwmchip_add_with_polarity() - register a new PWM chip
>>   * @chip: the PWM chip to add
>>   * @polarity: initial polarity of PWM channels
>> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>  
>>  	mutex_lock(&pwm_lock);
>>  
>> +	chip->get_default_caps = pwmchip_get_default_caps;
>> +
>>  	ret = alloc_pwms(chip->base, chip->npwm);
>>  	if (ret < 0)
>>  		goto out;
>> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>  		pwm->pwm = chip->base + i;
>>  		pwm->hwpwm = i;
>>  		pwm->state.polarity = polarity;
>> +		pwm->state.mode = pwm_mode_get_valid(chip, pwm);
>>  
>>  		if (chip->ops->get_state)
>>  			chip->ops->get_state(chip, pwm, &pwm->state);
>> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>>  	int err;
>>  
>>  	if (!pwm || !state || !state->period ||
>> -	    state->duty_cycle > state->period)
>> +	    state->duty_cycle > state->period ||
>> +	    !pwm_mode_valid(pwm, state->mode))
>>  		return -EINVAL;
>>  
>>  	if (!memcmp(state, &pwm->state, sizeof(*state)))
>> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>>  
>>  			pwm->state.enabled = state->enabled;
>>  		}
>> +
>> +		/* No mode support for non-atomic PWM. */
>> +		pwm->state.mode = state->mode;
> 
> That comment seems misplaced. This is actually part of atomic PWM, so
> maybe just reverse the logic and say "mode support only for atomic PWM"
> or something. I would personally just leave it away.

Ok, sure. I will remove the comment. But the code has to be there to avoid
unassigned mode value for PWM state (normal mode means BIT(0)) and so to
avoid future PWM applies failure.

The legacy API has
> no way of setting the mode, which is indication enough that we don't
> support it.
> 
>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>> index 56518adc31dd..a4ce4ad7edf0 100644
>> --- a/include/linux/pwm.h
>> +++ b/include/linux/pwm.h
>> @@ -26,9 +26,32 @@ enum pwm_polarity {
>>  };
>>  
>>  /**
>> + * PWM modes capabilities
>> + * @PWMC_MODE_NORMAL_BIT: PWM has one output
>> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
>> + * @PWMC_MODE_CNT: PWM modes count
>> + */
>> +enum {
>> +	PWMC_MODE_NORMAL_BIT,
>> +	PWMC_MODE_COMPLEMENTARY_BIT,
>> +	PWMC_MODE_CNT,
>> +};
>> +
>> +#define PWMC_MODE(name)		BIT(PWMC_MODE_##name##_BIT)
> 
> Why the C in the prefix? Why not just PWM_MODE_* for all of the above?

Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time
I wrote this patch... So I choose to add extra C to this macro, "C" meaning
"constant" in my mind. I was not sure it was the best solution at that time
either.

> 
>> +
>> +/**
>> + * struct pwm_caps - PWM capabilities
>> + * @modes: PWM modes
> 
> This should probably say that it's a bitmask of PWM_MODE_*.

Ok, I'll update.

> 
>> +struct pwm_caps {
>> +	unsigned long modes;
>> +};
>> +
>> +/**
>>   * struct pwm_args - board-dependent PWM arguments
>>   * @period: reference period
>>   * @polarity: reference polarity
>> + * @mode: reference mode
> 
> As discussed in my reply to the cover letter, what is the use for the
> reference mode? Drivers want to use either normal or some other mode.
> What good is it to get this from board-dependent arguments if the
> driver already knows which one it wants or needs?

For the moment, no, there is no in-kernel user. Only sysfs. I was thinking
I would also adapt, e.g., the pwm-regulator driver to also make use of this
features in setups like the one described in [1], page 2126
(Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation).
But, for the moment there is no in-kernel user.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

> 
>> @@ -300,6 +331,7 @@ struct pwm_chip {
>>  
>>  	struct pwm_device * (*of_xlate)(struct pwm_chip *pc,
>>  					const struct of_phandle_args *args);
>> +	void (*get_default_caps)(struct pwm_caps *caps);
> 
> As mentioned above, I don't think we need this.

Ok, I'll update.

> 
> Thierry
> 
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [RESEND PATCH v5 1/9] pwm: extend PWM framework with PWM modes

[Thierry Reding]

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

On Wed, Oct 17, 2018 at 12:42:00PM +0000, Claudiu.Beznea@microchip.com wrote:
> On 16.10.2018 15:25, Thierry Reding wrote:
> > On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote:
[...]
> >> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
> >> +{
> >> +	static const char * const modes[] = {
> >> +		"invalid",
> >> +		"normal",
> >> +		"complementary",
> >> +	};
> >> +
> >> +	if (!pwm_mode_valid(pwm, mode))
> >> +		return modes[0];
> >> +
> >> +	return modes[ffs(mode)];
> >> +}
> > 
> > Do we really need to be able to get the name of the mode in the context
> > of a given PWM channel? Couldn't we drop the pwm parameter and simply
> > return the name (pwm_get_mode_name()?) and at the same time remove the
> > extra "invalid" mode in there? I'm not sure what the use-case here is,
> > but it seems to me like the code should always check for supported modes
> > first before reporting their names in any way.
> 
> Looking back at this code, the main use case for checking PWM mode validity
> in pwm_mode_desc() was only with regards to mode_store(). But there is not
> need for this checking since the same thing will be checked in
> pwm_apply_state() and, in case user provides an invalid mode via sysfs the
> pwm_apply_state() will fail.
> 
> To conclude, I will change this function in something like:
> 
> if (mode == PWM_MODE_NORMAL)
> 	return "normal";
> else if (mode == PWM_MODE_COMPLEMENTARY)
> 	return "complementary";
> else if (mode == PWM_MODE_PUSH_PULL)
> 	return "push-pull";
> else
> 	return "invalid";
> 
> Please let me know if it is OK for you.

Do we even have to check here for validity of the mode in the first
place? Shouldn't this already happen at a higher level? I mean we do
need to check for valid input in mode_store(), but whatever mode we
pass into this could already have been validated, so that this would
never return "invalid".

For example, you already define an enum for the PWM modes. I think it'd
be best if we then used that enum to pass the modes around. That way it
becomes easy to check for validity.

So taking one step back, I think we can remove some of the ambiguities
by making sure we only ever specify one mode. When the mode is
explicitly being set, we only ever want one, right? The only point in
time where we can store more than one is for the capabilities. So I
think being more explicit about that would be useful. That way we remove
any uncertainties about what the unsigned long might contain at any
point in time.

> >> +/**
> >>   * pwmchip_add_with_polarity() - register a new PWM chip
> >>   * @chip: the PWM chip to add
> >>   * @polarity: initial polarity of PWM channels
> >> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >>  
> >>  	mutex_lock(&pwm_lock);
> >>  
> >> +	chip->get_default_caps = pwmchip_get_default_caps;
> >> +
> >>  	ret = alloc_pwms(chip->base, chip->npwm);
> >>  	if (ret < 0)
> >>  		goto out;
> >> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
> >>  		pwm->pwm = chip->base + i;
> >>  		pwm->hwpwm = i;
> >>  		pwm->state.polarity = polarity;
> >> +		pwm->state.mode = pwm_mode_get_valid(chip, pwm);
> >>  
> >>  		if (chip->ops->get_state)
> >>  			chip->ops->get_state(chip, pwm, &pwm->state);
> >> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> >>  	int err;
> >>  
> >>  	if (!pwm || !state || !state->period ||
> >> -	    state->duty_cycle > state->period)
> >> +	    state->duty_cycle > state->period ||
> >> +	    !pwm_mode_valid(pwm, state->mode))
> >>  		return -EINVAL;
> >>  
> >>  	if (!memcmp(state, &pwm->state, sizeof(*state)))
> >> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
> >>  
> >>  			pwm->state.enabled = state->enabled;
> >>  		}
> >> +
> >> +		/* No mode support for non-atomic PWM. */
> >> +		pwm->state.mode = state->mode;
> > 
> > That comment seems misplaced. This is actually part of atomic PWM, so
> > maybe just reverse the logic and say "mode support only for atomic PWM"
> > or something. I would personally just leave it away.
> 
> Ok, sure. I will remove the comment. But the code has to be there to avoid
> unassigned mode value for PWM state (normal mode means BIT(0)) and so to
> avoid future PWM applies failure.

Oh yeah, definitely keep the code around. I was only commenting on
the... comment. =)

> The legacy API has
> > no way of setting the mode, which is indication enough that we don't
> > support it.
> > 
> >> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
> >> index 56518adc31dd..a4ce4ad7edf0 100644
> >> --- a/include/linux/pwm.h
> >> +++ b/include/linux/pwm.h
> >> @@ -26,9 +26,32 @@ enum pwm_polarity {
> >>  };
> >>  
> >>  /**
> >> + * PWM modes capabilities
> >> + * @PWMC_MODE_NORMAL_BIT: PWM has one output
> >> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
> >> + * @PWMC_MODE_CNT: PWM modes count
> >> + */
> >> +enum {
> >> +	PWMC_MODE_NORMAL_BIT,
> >> +	PWMC_MODE_COMPLEMENTARY_BIT,
> >> +	PWMC_MODE_CNT,
> >> +};
> >> +
> >> +#define PWMC_MODE(name)		BIT(PWMC_MODE_##name##_BIT)
> > 
> > Why the C in the prefix? Why not just PWM_MODE_* for all of the above?
> 
> Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time
> I wrote this patch... So I choose to add extra C to this macro, "C" meaning
> "constant" in my mind. I was not sure it was the best solution at that time
> either.

We can always rename definitions in drivers if we want to use
conflicting names in the core. In this particular case, and given what I
said above regarding the PWM mode definitions, I think it'd be better to
drop the _BIT suffix from the enum values, so that we get something like
this:

	enum pwm_mode {
		PWM_MODE_NORMAL,
		PWM_MODE_COMPLEMENTARY,
		PWM_MODE_COUNT
	};

Then in order to create the actual bitmasks we can use a macro that is
explicit about creating bitmasks:

	#define PWM_MODE_MASK(name) BIT(PWM_MODE_##name##)

With that, the conflict with pwm-sun4i conveniently goes away.

> >> +struct pwm_caps {
> >> +	unsigned long modes;
> >> +};
> >> +
> >> +/**
> >>   * struct pwm_args - board-dependent PWM arguments
> >>   * @period: reference period
> >>   * @polarity: reference polarity
> >> + * @mode: reference mode
> > 
> > As discussed in my reply to the cover letter, what is the use for the
> > reference mode? Drivers want to use either normal or some other mode.
> > What good is it to get this from board-dependent arguments if the
> > driver already knows which one it wants or needs?
> 
> For the moment, no, there is no in-kernel user. Only sysfs. I was thinking
> I would also adapt, e.g., the pwm-regulator driver to also make use of this
> features in setups like the one described in [1], page 2126
> (Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation).
> But, for the moment there is no in-kernel user.
> 
> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

Okay, so that means we don't have any use-cases where we even need the
mode in DT? If so, I don't think we should add that code yet. We'd be
adding an ABI that we don't know how it will be used or if it is even
sufficient. Granted, as long as nobody uses it we could probably just
silently drop it again, but why risk if it's just dead code anyway.

If I understand the half-bridge converter application correctly you
would want to model that with pwm-regulator and instead of using a
regular PWM (normal mode) you'd use a PWM_MODE_PUSH_PULL instead. Is
that really all there is to support this? Does the voltage output of
the half-bridge converter vary linearly with the duty-cycle? I suppose
one could always combine the push-pull PWM with a voltage table to make
it work. I'm not opposed to the idea, just trying to figure out if the
pwm-regulator driver would be viable or whether there are other things
we need to make these setups work.

Thierry

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

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Thierry Reding]

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

On Wed, Oct 17, 2018 at 12:41:53PM +0000, Claudiu.Beznea@microchip.com wrote:
> 
> 
> On 16.10.2018 15:03, Thierry Reding wrote:
> > On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote:
> >> Thierry,
> >>
> >> On 28/08/2018 at 15:01, Claudiu Beznea wrote:
> >>> Hi,
> >>>
> >>> Please give feedback on these patches which extends the PWM framework in
> >>> order to support multiple PWM modes of operations. This series is a rework
> >>> of [1] and [2].
> >>
> >> This series started with a RFC back on 5 April 2017 "extend PWM framework to
> >> support PWM modes". The continuous work starting with v2 of this series on
> >> January 12, 2018.
> >>
> >> Then Claudiu tried to address all comments up to v4 which didn't have any
> >> more reviews. He posted a v5 without comments since May 22, 2018. This
> >> series is basically a resent of the v5 (as said in the $subject).
> >>
> >> We would like to know what is preventing this series to be included in the
> >> PWM sub-system. Note that if some issue still remain with it, we are ready
> >> to help to solve them.
> >>
> >> Without feedback from you side, we fear that we would miss a merge window
> >> again for no obvious reason (DT part is Acked by Rob: patch 5/9).
> > 
> > First off, apologies for not getting around to this earlier.
> > 
> > I think this series is mostly fine, but I still have doubts about the DT
> > aspects of this. In particular, Rob raised a concern about this here:
> > 
> > 	https://lkml.org/lkml/2018/1/22/655
> > 
> > and it seems like that particular question was never fully resolved as
> > the discussion veered off that particular topic.
> 
> 1/ If you are talking about this sentence:
> "Yes, but you have to make "normal" be no bit set to be compatible with
> everything already out there."
> 
> The current implementation consider that if no mode is provided then, the
> old approach is considered, meaning the normal mode will be used by every
> PWM in-kernel clients.
> 
> In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what
> pwm_mode_get_valid() returns. In case of controllers which does not
> implement something special for PWM modes the PWM normal mode will be
> returned (pwmchip_get_default_caps() function has to be called in the end).
> Otherwise the pwm->args.mode will be populated with what user provided as
> input from DT, if what was provided from DT is valid for PWM channel.
> Please see that pwm_mode_valid() is used to validate user input, otherwise
> PWM normal mode will be used.

No, that part looks fine.

> 
> +	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
> 
> -	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;
> +
> +		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
> +		     modebit < PWMC_MODE_CNT; modebit++) {
> +			unsigned long mode = BIT(modebit);
> +
> +			if ((args->args[2] & mode) &&
> +			    pwm_mode_valid(pwm, mode)) {
> +				pwm->args.mode = mode;
> +				break;
> +			}
> +		}
> +	}
> 
> 
> 2/ If you are talking about this sentence:
> "Thinking about this some more, shouldn't the new modes just be
> implied? A client is going to require one of these modes or it won't
> work right."
> 
> As explained at point 1, if there is no mode requested from DT the default
> mode for channel will be used, which, in case of PWM controller which are
> not implementing the new modes, will be PWM normal mode.

I don't think that's an issue. I think what Rob was referring to and
which mirrors my concern is that these modes are a feature that doesn't
extend to typical use-cases. So for all existing use-cases (like LED or
backlight) we always assume a PWM running in normal mode. Now, if you
write a driver for some particular piece of hardware that needs a mode
that is not the normal mode, the question is: wouldn't that driver know
that it wants exactly push-pull or complementary mode? Wouldn't it have
to explicitly check that the PWM supports it and select it (i.e. in the
driver code)?

Say you have a driver that requires push-pull mode. It doesn't really
make sense to require the mode to be encoded in DT, because the driver
will only work with one specific mode anyway. So might as well require
it and have the driver check for support and fail if the PWM is not
compatible. This would likely never happen, because hardware engineers
couldn't have validated the design in that case, but there's no reason
for the mode to be specified in DT because it is fixed by the very use-
case anyway.

Also, leaving it out of DT simplifies things. If you allow the mode to
be specified in DT you could end up with a situation where the driver
required push-pull mode, but the DT specifies complementary mode. What
do you do in such a situation? Warn about it and just go with push-pull
anyway? Then why give the users a way of screwing things up in the first
place?

> 3/ If you are talking about:
> "Also complementary mode could be accomplished with a single pwm output
> and a board level inverter, right? How would that be handled when the
> PWM driver doesn't support that mode?"
> This complicates the things and I think it requires extra device tree
> bindings to describe extra per-pwm channel capabilities. For the moment,
> with this series I've stopped to only have the capabilities registered from
> driver. My understanding is that this could be accomplished with extra
> device tree bindings in PWM controller to describe PWM channels
> capabilities. If you also want me to look into this direction please let me
> know. And also, suggestions would be appreciated.

I think this is very interesting, but I don't think this needs to be
done as part of this series.

> I know that Rob acked
> > the DT parts of this, but I suspect that this might have been glossed
> > over.
> 
> If this is about point 3 I've emphasize above I would like to have some
> inputs from your side, if you agree with a behavior like having extra DT
> bindings. The intention wasn't to left this over but to have a better
> understanding of the subject. I'm thinking if it is ok to have modules
> outside of the SoC that model a behavior that could not be controlled from
> software (it could be only hardware) but to behave in a single way. Take
> for instance this scenario:
> 
> - new DT bindings are implemented to specify this behavior per channel
> - hardware modules are soldered outside of a PWM channel with one output
> - the new DT bindings are not specified for the soldered PWM channel
> - user enables this channel, it will have only normal mode available for it
> to configure (because the new DT bindings were not provided)
> - but the real output the user will see would be in complementary or even
> push-pull mode.

I think we could possible model this as a "logical" PWM in DT. We could
for example have something like this:

	/ {
		...

		pwms {
			pwm@0 {
				compatible = "pwm-fixed"; /* or whatever */
				pwms = <&pwm 0 40000>; /* input PWM */
				mode = <PWM_MODE_COMPLEMENTARY>;
			};

			...
		};

		...
	};

The above would model a logical PWM that is driven by the specified PWM
in normal mode but which is effectively complementary because of some
additional circuitry on the board.

> > To restate the concern: these extended modes have special uses and none
> > of the users in the kernel, other than sysfs, can use anything other
> > than the normal mode. They may work fine with other modes, but only if
> > they ignore the extras that come with them. Therefore I think it's safe
> > to say that anyone who would want to use these modes would want to
> > explicitly say so. For example the sysfs interface already does that by
> > changing the mode only after the "mode" attribute is written. Any users
> > for special use-cases would want to do the same thing, that is, drive a
> > PWM in a specific mode, on purpose. You wouldn't have a "generic" user
> > such as pwm-backlight or leds-pwm request anything other than the normal
> > mode.
> > 
> > So my question is, do we really need to represent these modes in DT? The
> > series currently doesn't contain any patches that add users of these new
> > modes. Are such patches available somewhere, or is the only user of this
> > supposed to be sysfs?
> 
> For the moment, no, there is no in-kernel user for this, only sysfs. I had
> in mind to adapt the use of these new mode for PWM regulator for scenarios
> described in [1] page 2126. Anyway, since these patches doesn't introduce
> any user other that sysfs it will not disturbed me to drop the changes. By
> the time I or someone else will do some other changes that requires this,
> the DT part should also be introduced.
> 
> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

Yes, I'd like that. The half-bridge converter certainly sounds like
something that may be able to use the DT bindings that you specified,
but I'd be less concerned about these changes if we had actual users.

> > I'm hesitant to move forward with this as-is without seeing how it will
> > be used.
> 
> For the moment only sysfs is supposed to use this.
> 
> The PWM specifier flags are somewhat abused by adding modes to
> > them. 
> 
> I see.
> 
> I think this hasn't been completely thought through, because the
> > only reason to specify a mode is to actually set that mode.
> 
> Maybe it wasn't clear understand, with the current implementation if no
> mode will be specified the default mode will be used. There is no impose to
> specify a mode.
> 
>  But then the
> > DT ABI allows a bitmask of modes to be requested via DT. I know that
> > only the first of those modes will end up being used, but then why even
> > allow it in the first place?
> 
> I had in mind that I will change the PWM regulator driver to work in
> scenarios like the one specified in the link above.

Yeah, that sounds like it would be reasonable from a quick look.
However, what I don't quite understand yet is why the mode in the PWM
specifier would need to be a bitmask. Take for example the pwm-regulator
case for a half-bridge converter. If your board uses such a setup, then
you absolutely must drive the PWM in push-pull mode, otherwise the
converter will not work, right? So you want exactly one mode to be
applied. Then why complicate matters by allowing the mode to be a
bitmask? We could just as easily reserve, say, 8 bits (24-31) for the
mode, which could then be identical to enum pwm_mode.

> > And again, even if we allow the mode to be specified in DT, how do the
> > consumer drivers know that the correct mode was being set in DT. 
> 
> PWM user will call at some point devm_pwm_get() which, in turn, will call
> of_pwm_get() which in turn will initialize PWM args with inputs from DT.
> After that PWM user will, at some point, apply a PWM state; if it is
> initialized with PWM args initialized when devm_pwm_get() was called then
> pwm_apply_state() would fail if no good mode is provided as input via DT.
> 
> Same thing happens if a bad period is provided via DT.

But that only checks that the DT specified a supported mode, it doesn't
mean that it's the correct one. For cases like pwm-regulator this may be
fine because the driver ultimately doesn't care about the exact mode. If
you have a driver that only works with a specific mode, however, it can
be problematic.

> Let's
> > say we have a consumer that requires the PWM to be driven in
> > complementary mode. Should it rely on the DT to contain the correct
> > specification for the mode? And if it knows that it needs complementary
> > mode, why not just set that mode itself?
> 
> I'm thinking it's the same way as is with PWM period which could also be
> provided from DT. In the end a bad period value could be provided from
> device tree. E.g. Atmel PWM controller could generate PWM signals who's
> periods could not be higher than ~0.6 seconds. If a bad value is provided
> the pwm_apply_state() will fail.

I understand that. And it's good to validate these things in the driver.
However, the PWM driver can only validate for the PWM that it is
driving. It doesn't know if the consumer has any special requirements
regarding the mode. So if the PWM supports push-pull mode and the DT
contains PWM_MODE_PUSH_PULL, then everything is fine as far as the PWM
driver is concerned. However, if the consumer driver strictly requires
complementary mode, there's nothing the PWM driver can do about it. So
we either need the consumer to be able to query the mode if it has any
specific needs and refuse to use a PWM that has the wrong mode in the
specifier, or the consumer needs to explicitly set a mode, in which case
there's no need to have it in DT and the PWM driver needs to reject it
if the PWM doesn't support it.

Thierry

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

Re: [RESEND PATCH v5 1/9] pwm: extend PWM framework with PWM modes

[Claudiu Beznea]

On 18.10.2018 18:32, Thierry Reding wrote:
> On Wed, Oct 17, 2018 at 12:42:00PM +0000, Claudiu.Beznea@microchip.com wrote:
>> On 16.10.2018 15:25, Thierry Reding wrote:
>>> On Tue, Aug 28, 2018 at 04:01:18PM +0300, Claudiu Beznea wrote:
> [...]
>>>> +const char *pwm_mode_desc(struct pwm_device *pwm, unsigned long mode)
>>>> +{
>>>> +	static const char * const modes[] = {
>>>> +		"invalid",
>>>> +		"normal",
>>>> +		"complementary",
>>>> +	};
>>>> +
>>>> +	if (!pwm_mode_valid(pwm, mode))
>>>> +		return modes[0];
>>>> +
>>>> +	return modes[ffs(mode)];
>>>> +}
>>>
>>> Do we really need to be able to get the name of the mode in the context
>>> of a given PWM channel? Couldn't we drop the pwm parameter and simply
>>> return the name (pwm_get_mode_name()?) and at the same time remove the
>>> extra "invalid" mode in there? I'm not sure what the use-case here is,
>>> but it seems to me like the code should always check for supported modes
>>> first before reporting their names in any way.
>>
>> Looking back at this code, the main use case for checking PWM mode validity
>> in pwm_mode_desc() was only with regards to mode_store(). But there is not
>> need for this checking since the same thing will be checked in
>> pwm_apply_state() and, in case user provides an invalid mode via sysfs the
>> pwm_apply_state() will fail.
>>
>> To conclude, I will change this function in something like:
>>
>> if (mode == PWM_MODE_NORMAL)
>> 	return "normal";
>> else if (mode == PWM_MODE_COMPLEMENTARY)
>> 	return "complementary";
>> else if (mode == PWM_MODE_PUSH_PULL)
>> 	return "push-pull";
>> else
>> 	return "invalid";
>>
>> Please let me know if it is OK for you.
> 
> Do we even have to check here for validity of the mode in the first
> place? Shouldn't this already happen at a higher level? I mean we do
> need to check for valid input in mode_store(), but whatever mode we
> pass into this could already have been validated, so that this would
> never return "invalid".

Ok, now I see your point. I agree, there is no need here for "invalid" here
neither since in mode_store there is a look through all the defined modes
and, anyway, if nothing valid matches with what user provides, yes, the:

+	if (modebit == PWMC_MODE_CNT)
+		return -EINVAL;

will return anyway.

And every place this pwm_mode_desc() is used, the mode has been already
checked and it is valid.

> 
> For example, you already define an enum for the PWM modes. I think it'd
> be best if we then used that enum to pass the modes around. That way it
> becomes easy to check for validity.

Ok.

> 
> So taking one step back, I think we can remove some of the ambiguities
> by making sure we only ever specify one mode. When the mode is
> explicitly being set, we only ever want one, right?

Right!

> The only point in
> time where we can store more than one is for the capabilities. So I
> think being more explicit about that would be useful.

Ok.

> That way we remove
> any uncertainties about what the unsigned long might contain at any
> point in time.
> 
>>>> +/**
>>>>   * pwmchip_add_with_polarity() - register a new PWM chip
>>>>   * @chip: the PWM chip to add
>>>>   * @polarity: initial polarity of PWM channels
>>>> @@ -275,6 +382,8 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>>>  
>>>>  	mutex_lock(&pwm_lock);
>>>>  
>>>> +	chip->get_default_caps = pwmchip_get_default_caps;
>>>> +
>>>>  	ret = alloc_pwms(chip->base, chip->npwm);
>>>>  	if (ret < 0)
>>>>  		goto out;
>>>> @@ -294,6 +403,7 @@ int pwmchip_add_with_polarity(struct pwm_chip *chip,
>>>>  		pwm->pwm = chip->base + i;
>>>>  		pwm->hwpwm = i;
>>>>  		pwm->state.polarity = polarity;
>>>> +		pwm->state.mode = pwm_mode_get_valid(chip, pwm);
>>>>  
>>>>  		if (chip->ops->get_state)
>>>>  			chip->ops->get_state(chip, pwm, &pwm->state);
>>>> @@ -469,7 +579,8 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>>>>  	int err;
>>>>  
>>>>  	if (!pwm || !state || !state->period ||
>>>> -	    state->duty_cycle > state->period)
>>>> +	    state->duty_cycle > state->period ||
>>>> +	    !pwm_mode_valid(pwm, state->mode))
>>>>  		return -EINVAL;
>>>>  
>>>>  	if (!memcmp(state, &pwm->state, sizeof(*state)))
>>>> @@ -530,6 +641,9 @@ int pwm_apply_state(struct pwm_device *pwm, struct pwm_state *state)
>>>>  
>>>>  			pwm->state.enabled = state->enabled;
>>>>  		}
>>>> +
>>>> +		/* No mode support for non-atomic PWM. */
>>>> +		pwm->state.mode = state->mode;
>>>
>>> That comment seems misplaced. This is actually part of atomic PWM, so
>>> maybe just reverse the logic and say "mode support only for atomic PWM"
>>> or something. I would personally just leave it away.
>>
>> Ok, sure. I will remove the comment. But the code has to be there to avoid
>> unassigned mode value for PWM state (normal mode means BIT(0)) and so to
>> avoid future PWM applies failure.
> 
> Oh yeah, definitely keep the code around. I was only commenting on
> the... comment. =)
> 
>> The legacy API has
>>> no way of setting the mode, which is indication enough that we don't
>>> support it.
>>>
>>>> diff --git a/include/linux/pwm.h b/include/linux/pwm.h
>>>> index 56518adc31dd..a4ce4ad7edf0 100644
>>>> --- a/include/linux/pwm.h
>>>> +++ b/include/linux/pwm.h
>>>> @@ -26,9 +26,32 @@ enum pwm_polarity {
>>>>  };
>>>>  
>>>>  /**
>>>> + * PWM modes capabilities
>>>> + * @PWMC_MODE_NORMAL_BIT: PWM has one output
>>>> + * @PWMC_MODE_COMPLEMENTARY_BIT: PWM has 2 outputs with opposite polarities
>>>> + * @PWMC_MODE_CNT: PWM modes count
>>>> + */
>>>> +enum {
>>>> +	PWMC_MODE_NORMAL_BIT,
>>>> +	PWMC_MODE_COMPLEMENTARY_BIT,
>>>> +	PWMC_MODE_CNT,
>>>> +};
>>>> +
>>>> +#define PWMC_MODE(name)		BIT(PWMC_MODE_##name##_BIT)
>>>
>>> Why the C in the prefix? Why not just PWM_MODE_* for all of the above?
>>
>> Well... PWM_MODE() macro was already took by pwm-sun4i.c driver at the time
>> I wrote this patch... So I choose to add extra C to this macro, "C" meaning
>> "constant" in my mind. I was not sure it was the best solution at that time
>> either.
> 
> We can always rename definitions in drivers if we want to use
> conflicting names in the core. In this particular case, and given what I
> said above regarding the PWM mode definitions, I think it'd be better to
> drop the _BIT suffix from the enum values, so that we get something like
> this:
> 
> 	enum pwm_mode {
> 		PWM_MODE_NORMAL,
> 		PWM_MODE_COMPLEMENTARY,
> 		PWM_MODE_COUNT
> 	};
> 
> Then in order to create the actual bitmasks we can use a macro that is
> explicit about creating bitmasks:
> 
> 	#define PWM_MODE_MASK(name) BIT(PWM_MODE_##name##)
> 
> With that, the conflict with pwm-sun4i conveniently goes away.

Ok, thanks!

> 
>>>> +struct pwm_caps {
>>>> +	unsigned long modes;
>>>> +};
>>>> +
>>>> +/**
>>>>   * struct pwm_args - board-dependent PWM arguments
>>>>   * @period: reference period
>>>>   * @polarity: reference polarity
>>>> + * @mode: reference mode
>>>
>>> As discussed in my reply to the cover letter, what is the use for the
>>> reference mode? Drivers want to use either normal or some other mode.
>>> What good is it to get this from board-dependent arguments if the
>>> driver already knows which one it wants or needs?
>>
>> For the moment, no, there is no in-kernel user. Only sysfs. I was thinking
>> I would also adapt, e.g., the pwm-regulator driver to also make use of this
>> features in setups like the one described in [1], page 2126
>> (Figure 56-13: Half-Bridge Converter Application: No Feedback Regulation).
>> But, for the moment there is no in-kernel user.
>>
>> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf
> 
> Okay, so that means we don't have any use-cases where we even need the
> mode in DT? If so, I don't think we should add that code yet.

Agree!

> We'd be
> adding an ABI that we don't know how it will be used or if it is even
> sufficient. Granted, as long as nobody uses it we could probably just
> silently drop it again, but why risk if it's just dead code anyway.

I agree with you. My bad that I added without having a user. I've just had
in mind that I will work with it around PWM regulator.

> 
> If I understand the half-bridge converter application correctly you
> would want to model that with pwm-regulator and instead of using a
> regular PWM (normal mode) you'd use a PWM_MODE_PUSH_PULL instead. 

Yes, that was the idea.

> Is
> that really all there is to support this?

Than and the the variation at page 2127, same datasheet [1] (figure
Figure 56-14: Half-Bridge Converter Application: Feedback Regulation).
Also, complementary could also be used in motor control applications.

[1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf

> Does the voltage output of
> the half-bridge converter vary linearly with the duty-cycle?

That's my understanding.

> I suppose
> one could always combine the push-pull PWM with a voltage table to make
> it work. I'm not opposed to the idea, just trying to figure out if the
> pwm-regulator driver would be viable or whether there are other things
> we need to make these setups work.

Till now I didn't do any changes on PWM regulator to check how it the
current behavior with push-pull mode.

Thank you,
Claudiu Beznea

> 
> Thierry
> 

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Claudiu Beznea]

On 18.10.2018 19:00, Thierry Reding wrote:
> On Wed, Oct 17, 2018 at 12:41:53PM +0000, Claudiu.Beznea@microchip.com wrote:
>>
>>
>> On 16.10.2018 15:03, Thierry Reding wrote:
>>> On Fri, Sep 14, 2018 at 06:20:48PM +0200, Nicolas Ferre wrote:
>>>> Thierry,
>>>>
>>>> On 28/08/2018 at 15:01, Claudiu Beznea wrote:
>>>>> Hi,
>>>>>
>>>>> Please give feedback on these patches which extends the PWM framework in
>>>>> order to support multiple PWM modes of operations. This series is a rework
>>>>> of [1] and [2].
>>>>
>>>> This series started with a RFC back on 5 April 2017 "extend PWM framework to
>>>> support PWM modes". The continuous work starting with v2 of this series on
>>>> January 12, 2018.
>>>>
>>>> Then Claudiu tried to address all comments up to v4 which didn't have any
>>>> more reviews. He posted a v5 without comments since May 22, 2018. This
>>>> series is basically a resent of the v5 (as said in the $subject).
>>>>
>>>> We would like to know what is preventing this series to be included in the
>>>> PWM sub-system. Note that if some issue still remain with it, we are ready
>>>> to help to solve them.
>>>>
>>>> Without feedback from you side, we fear that we would miss a merge window
>>>> again for no obvious reason (DT part is Acked by Rob: patch 5/9).
>>>
>>> First off, apologies for not getting around to this earlier.
>>>
>>> I think this series is mostly fine, but I still have doubts about the DT
>>> aspects of this. In particular, Rob raised a concern about this here:
>>>
>>> 	https://lkml.org/lkml/2018/1/22/655
>>>
>>> and it seems like that particular question was never fully resolved as
>>> the discussion veered off that particular topic.
>>
>> 1/ If you are talking about this sentence:
>> "Yes, but you have to make "normal" be no bit set to be compatible with
>> everything already out there."
>>
>> The current implementation consider that if no mode is provided then, the
>> old approach is considered, meaning the normal mode will be used by every
>> PWM in-kernel clients.
>>
>> In of_pwm_xlate_with_flags() the pmw->args.mode is initialized with what
>> pwm_mode_get_valid() returns. In case of controllers which does not
>> implement something special for PWM modes the PWM normal mode will be
>> returned (pwmchip_get_default_caps() function has to be called in the end).
>> Otherwise the pwm->args.mode will be populated with what user provided as
>> input from DT, if what was provided from DT is valid for PWM channel.
>> Please see that pwm_mode_valid() is used to validate user input, otherwise
>> PWM normal mode will be used.
> 
> No, that part looks fine.
> 
>>
>> +	pwm->args.mode = pwm_mode_get_valid(pc, pwm);
>>
>> -	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;
>> +
>> +		for (modebit = PWMC_MODE_COMPLEMENTARY_BIT;
>> +		     modebit < PWMC_MODE_CNT; modebit++) {
>> +			unsigned long mode = BIT(modebit);
>> +
>> +			if ((args->args[2] & mode) &&
>> +			    pwm_mode_valid(pwm, mode)) {
>> +				pwm->args.mode = mode;
>> +				break;
>> +			}
>> +		}
>> +	}
>>
>>
>> 2/ If you are talking about this sentence:
>> "Thinking about this some more, shouldn't the new modes just be
>> implied? A client is going to require one of these modes or it won't
>> work right."
>>
>> As explained at point 1, if there is no mode requested from DT the default
>> mode for channel will be used, which, in case of PWM controller which are
>> not implementing the new modes, will be PWM normal mode.
> 
> I don't think that's an issue. I think what Rob was referring to and
> which mirrors my concern is that these modes are a feature that doesn't
> extend to typical use-cases. So for all existing use-cases (like LED or
> backlight) we always assume a PWM running in normal mode. Now, if you
> write a driver for some particular piece of hardware that needs a mode
> that is not the normal mode, the question is: wouldn't that driver know
> that it wants exactly push-pull or complementary mode?

It should, yes.

Wouldn't it have
> to explicitly check that the PWM supports it and select it (i.e. in the
> driver code)?

Yes, that should be the flow. I added the DT changes for cases where a
driver could use more than one mode and to be able to choose one of the
modes it may needs.

> 
> Say you have a driver that requires push-pull mode. It doesn't really
> make sense to require the mode to be encoded in DT, because the driver
> will only work with one specific mode anyway. So might as well require
> it and have the driver check for support and fail if the PWM is not
> compatible. This would likely never happen, because hardware engineers
> couldn't have validated the design in that case, but there's no reason
> for the mode to be specified in DT because it is fixed by the very use-
> case anyway.

Yes, agree.

> 
> Also, leaving it out of DT simplifies things.

Agree.

> If you allow the mode to
> be specified in DT you could end up with a situation where the driver
> required push-pull mode, but the DT specifies complementary mode. What
> do you do in such a situation? Warn about it and just go with push-pull
> anyway? Then why give the users a way of screwing things up in the first
> place?

I only introduce this because I had in mind the PWM regulator and I was
thinking to make it work for either push-pull mode and normal mode. But
since there is no code for this at the moment I totally agree with you to
not introduce the DT part. My bad I push it here without a use case.

> 
>> 3/ If you are talking about:
>> "Also complementary mode could be accomplished with a single pwm output
>> and a board level inverter, right? How would that be handled when the
>> PWM driver doesn't support that mode?"
>> This complicates the things and I think it requires extra device tree
>> bindings to describe extra per-pwm channel capabilities. For the moment,
>> with this series I've stopped to only have the capabilities registered from
>> driver. My understanding is that this could be accomplished with extra
>> device tree bindings in PWM controller to describe PWM channels
>> capabilities. If you also want me to look into this direction please let me
>> know. And also, suggestions would be appreciated.
> 
> I think this is very interesting, but I don't think this needs to be
> done as part of this series.
> 
>> I know that Rob acked
>>> the DT parts of this, but I suspect that this might have been glossed
>>> over.
>>
>> If this is about point 3 I've emphasize above I would like to have some
>> inputs from your side, if you agree with a behavior like having extra DT
>> bindings. The intention wasn't to left this over but to have a better
>> understanding of the subject. I'm thinking if it is ok to have modules
>> outside of the SoC that model a behavior that could not be controlled from
>> software (it could be only hardware) but to behave in a single way. Take
>> for instance this scenario:
>>
>> - new DT bindings are implemented to specify this behavior per channel
>> - hardware modules are soldered outside of a PWM channel with one output
>> - the new DT bindings are not specified for the soldered PWM channel
>> - user enables this channel, it will have only normal mode available for it
>> to configure (because the new DT bindings were not provided)
>> - but the real output the user will see would be in complementary or even
>> push-pull mode.
> 
> I think we could possible model this as a "logical" PWM in DT. We could
> for example have something like this:
> 
> 	/ {
> 		...
> 
> 		pwms {
> 			pwm@0 {
> 				compatible = "pwm-fixed"; /* or whatever */
> 				pwms = <&pwm 0 40000>; /* input PWM */
> 				mode = <PWM_MODE_COMPLEMENTARY>;
> 			};
> 
> 			...
> 		};
> 
> 		...
> 	};
> 
> The above would model a logical PWM that is driven by the specified PWM
> in normal mode but which is effectively complementary because of some
> additional circuitry on the board.

Ok, i see it. Sounds good to me.

> 
>>> To restate the concern: these extended modes have special uses and none
>>> of the users in the kernel, other than sysfs, can use anything other
>>> than the normal mode. They may work fine with other modes, but only if
>>> they ignore the extras that come with them. Therefore I think it's safe
>>> to say that anyone who would want to use these modes would want to
>>> explicitly say so. For example the sysfs interface already does that by
>>> changing the mode only after the "mode" attribute is written. Any users
>>> for special use-cases would want to do the same thing, that is, drive a
>>> PWM in a specific mode, on purpose. You wouldn't have a "generic" user
>>> such as pwm-backlight or leds-pwm request anything other than the normal
>>> mode.
>>>
>>> So my question is, do we really need to represent these modes in DT? The
>>> series currently doesn't contain any patches that add users of these new
>>> modes. Are such patches available somewhere, or is the only user of this
>>> supposed to be sysfs?
>>
>> For the moment, no, there is no in-kernel user for this, only sysfs. I had
>> in mind to adapt the use of these new mode for PWM regulator for scenarios
>> described in [1] page 2126. Anyway, since these patches doesn't introduce
>> any user other that sysfs it will not disturbed me to drop the changes. By
>> the time I or someone else will do some other changes that requires this,
>> the DT part should also be introduced.
>>
>> [1] http://ww1.microchip.com/downloads/en/DeviceDoc/DS60001476B.pdf
> 
> Yes, I'd like that. The half-bridge converter certainly sounds like
> something that may be able to use the DT bindings that you specified,
> but I'd be less concerned about these changes if we had actual users.

I understand.

Now, thinking again at what you proposed above with regards to logical PWM
channels I'm wondering if, for future, if needed, would be good for PWM
clients that could used a PWM channel in more than one PWM mode, to have
specified in device tree, as a child of PWM controller, the mode that the
client would use that channel. E.g. if DriverX wants to use PWM0 in
complementary mode:

pwm@00aabbcc {
	// ...
	pwm@0 {
		mode = <PWM_MODE_COMPLEMENTARY>;
		// this being the only mode that could be used for
		// PWM channel 0
	};
}

driverx@00ffffff {
	pwms=<pwm 0 50000>;
}

For future reference, do you find this feasible?

> 
>>> I'm hesitant to move forward with this as-is without seeing how it will
>>> be used.
>>
>> For the moment only sysfs is supposed to use this.
>>
>> The PWM specifier flags are somewhat abused by adding modes to
>>> them. 
>>
>> I see.
>>
>> I think this hasn't been completely thought through, because the
>>> only reason to specify a mode is to actually set that mode.
>>
>> Maybe it wasn't clear understand, with the current implementation if no
>> mode will be specified the default mode will be used. There is no impose to
>> specify a mode.
>>
>>  But then the
>>> DT ABI allows a bitmask of modes to be requested via DT. I know that
>>> only the first of those modes will end up being used, but then why even
>>> allow it in the first place?
>>
>> I had in mind that I will change the PWM regulator driver to work in
>> scenarios like the one specified in the link above.
> 
> Yeah, that sounds like it would be reasonable from a quick look.
> However, what I don't quite understand yet is why the mode in the PWM
> specifier would need to be a bitmask.

You are talking to have them as bitmask in pwm-flags cell right?
I though to stick this to the current way to request the PWM mode.

Take for example the pwm-regulator
> case for a half-bridge converter. If your board uses such a setup, then
> you absolutely must drive the PWM in push-pull mode, otherwise the
> converter will not work, right?

Right!

So you want exactly one mode to be
> applied. Then why complicate matters by allowing the mode to be a
> bitmask? 

Just to have everything behaving almost in the same way as it was
previously.  I'm saying to request the PWM channel from a PWM client (via
DT) as it was previously done but just adding the PWM mode (in pwm-flags
cell as per this version).

I also was not sure about this: in the 2nd version of this series I
introduced a new cell for PWM modes but this new cell was after pwm-flags
cell, and pwm-flags cell is optional, so to have simpler code, in scenarios
with PWM modes user would have also specified the pwm-flags cell (although
maybe it was not necessary).

> We could just as easily reserve, say, 8 bits (24-31) for the
> mode, which could then be identical to enum pwm_mode.

In pwm-flags cell, right?


> 
>>> And again, even if we allow the mode to be specified in DT, how do the
>>> consumer drivers know that the correct mode was being set in DT. 
>>
>> PWM user will call at some point devm_pwm_get() which, in turn, will call
>> of_pwm_get() which in turn will initialize PWM args with inputs from DT.
>> After that PWM user will, at some point, apply a PWM state; if it is
>> initialized with PWM args initialized when devm_pwm_get() was called then
>> pwm_apply_state() would fail if no good mode is provided as input via DT.
>>
>> Same thing happens if a bad period is provided via DT.
> 
> But that only checks that the DT specified a supported mode, it doesn't
> mean that it's the correct one. For cases like pwm-regulator this may be
> fine because the driver ultimately doesn't care about the exact mode. If
> you have a driver that only works with a specific mode, however, it can
> be problematic.

Yes, agree.

> 
>> Let's
>>> say we have a consumer that requires the PWM to be driven in
>>> complementary mode. Should it rely on the DT to contain the correct
>>> specification for the mode? And if it knows that it needs complementary
>>> mode, why not just set that mode itself?
>>
>> I'm thinking it's the same way as is with PWM period which could also be
>> provided from DT. In the end a bad period value could be provided from
>> device tree. E.g. Atmel PWM controller could generate PWM signals who's
>> periods could not be higher than ~0.6 seconds. If a bad value is provided
>> the pwm_apply_state() will fail.
> 
> I understand that. And it's good to validate these things in the driver.
> However, the PWM driver can only validate for the PWM that it is
> driving. It doesn't know if the consumer has any special requirements
> regarding the mode. So if the PWM supports push-pull mode and the DT
> contains PWM_MODE_PUSH_PULL, then everything is fine as far as the PWM
> driver is concerned. However, if the consumer driver strictly requires
> complementary mode, there's nothing the PWM driver can do about it. So
> we either need the consumer to be able to query the mode if it has any
> specific needs and refuse to use a PWM that has the wrong mode in the
> specifier, or the consumer needs to explicitly set a mode, in which case
> there's no need to have it in DT and the PWM driver needs to reject it
> if the PWM doesn't support it.

Ok, I see your point and understand that DT part may be risky and
complicates the things. And I agree to remove it from this series since,
anyway, there is no in-kernel user for that.

Thank you,
Claudiu Beznea


> 
> Thierry
> 

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Uwe Kleine-König]

Hello Claudiu,

On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote:
> Please give feedback on these patches which extends the PWM framework in
> order to support multiple PWM modes of operations. This series is a rework
> of [1] and [2].
> 
> The current patch series add the following PWM modes:
> - PWM mode normal
> - PWM mode complementary
> - PWM mode push-pull
> 
> Normal mode - for PWM channels with one output; output waveforms looks like
> this:
>              __    __    __    __
>     PWM   __|  |__|  |__|  |__|  |__
>             <--T-->
> 
>     Where T is the signal period

In my discussion about some suggested changes to the PWM framework I
used slightly different way to show the wave-forms in ASCII which are
IMHO slightly better. Also I think it is valuable to not use a 50% duty
cycle in the examples to remove some ambiguity.

With a duty cycle of 1/3 the normal mode looks as follows in "my" way:

          __       __       __
 PWM   __/  \_____/  \_____/  \_____/
         ^        ^        ^        ^

The carets mark always the start of a period. And note the rising and
falling edges are already part of the active and inactive phases
respectively which matches reality.
 
> Since PWMs with more than one output per channel could be used as one
> output PWM the normal mode is the default mode for all PWMs (if not
> specified otherwise).
> 
> Complementary mode - for PWM channels with two outputs; output waveforms
> for a PWM channel in complementary mode looks line this:
>              __    __    __    __
>     PWMH1 __|  |__|  |__|  |__|  |__
>           __    __    __    __    __
>     PWML1   |__|  |__|  |__|  |__|
>             <--T-->
> 
>     Where T is the signal period.

So this translates to (I think):

           __       __       __       __       __
 PWMH   __/  \_____/  \_____/  \_____/  \_____/  \_____/
        __    _____    _____    _____    _____    _____
 PWML     \__/     \__/     \__/     \__/     \__/     \
          ^        ^        ^        ^        ^        ^

That is PWML always pulls in the opposite direction of PWMH. Maybe we
could come up with better terms than PWMH and PWML (which might be
specific for the Atmel implementation?). Maybe "normal" and
"complement"?

> Push-pull mode - for PWM channels with two outputs; output waveforms for a
> PWM channel in push-pull mode with normal polarity looks like this:
>             __          __
>     PWMH __|  |________|  |________
>                   __          __
>     PWML ________|  |________|  |__
>            <--T-->

That again with the alternative display method and duty cycle 1/3:

           __                __                __
 PWMA   __/  \______________/  \______________/  \______
                    __                __
 PWMB   ___________/  \______________/  \______________/
          ^        ^        ^        ^        ^        ^

That is PWMA and PWMB are active only every 2nd period taking alternate
turns, right?


>     If polarity is inversed:
>          __    ________    ________
>     PWMH   |__|        |__|
>          ________    ________    __
>     PWML         |__|        |__|
>            <--T-->

That's again with duty cycle 1/3:

        __    ______________    ______________    ______
 PWMA     \__/              \__/              \__/
        ___________    ______________    ______________
 PWMB              \__/              \__/              \
          ^        ^        ^        ^        ^        ^

Given that the start of period isn't externally visible this is
equivalent to using a duty cycle 2/3 and not inverting which results in:

        __    ______________    ______________    ______
 PWMA     \__/              \__/              \__/
        ___________    ______________    ______________
 PWMB              \__/              \__/              \
             ^        ^        ^        ^        ^

I would really like if a more detailed description of the modes would be
created as part of this series. Currently there are a few implied
properties hidden in the PWM API (unrelated to this series) which I try
to resolve together with Thierry. Getting this documented right from the
start would be great here.

I didn't look in detail into the driver implementation, but from the
PWMs implemented in the STM32F4 family I would have chosen a different
model which makes me wonder if we should stick to a more general way to
describe two outputs from a single PWM channel.

I would use four values with nanosecond resolution to describe these:

  .period
  .duty_cycle
  .alt_duty_cycle
  .alt_offset

period and duty_cycle is as before for the primary output and then the
alt_* values describe offset and duty cycle of the secondary output.

What you called "normal mode" would then be specified using

  .period = $period
  .duty_cycle = $duty_cycle
  .alt_duty_cycle = 0
  .alt_offset = dontcare

Your "push pull mode" would be:

  .period = 2 * $period
  .duty_cycle = $duty_cycle
  .alt_duty_cycle = $duty_cycle
  .alt_offset = $period

and complementary mode would be specified using:

  .period = $period
  .duty_cycle = $duty_cycle
  .alt_duty_cycle = $period - $duty_cycle
  .alt_offset = $duty_cycle

With this abstraction stuff like "complementary output with dead-time
insertion" (something like:

           __                __                __
 PWMA   __/  \______________/  \______________/  \______
                __________        __________          __
 PWMB   _______/          \______/          \______/
          ^                 ^                 ^

) could be modelled.

> The PWM working modes are per PWM channel registered as PWM's capabilities.
> The driver registers itself to PWM core a get_caps() function, in
> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
> If this function is not registered in driver's probe, a default function
> will be used to retrieve PWM capabilities. Currently, the default
> capabilities includes only PWM normal mode.

In the i2c framework this is a function, too, and I wonder if simplicity
is better served when this is just a flags member in the pwm_ops
structure.

> PWM state has been updated to keep PWM mode. PWM mode could be configured
> via sysfs or via DT. pwm_apply_state() will do the preliminary validation
> for PWM mode to be applied.
> 
> In sysfs, user could get PWM modes by reading mode file of PWM device:
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l
> total 0
> -r--r--r-- 1 root root 4096 Oct  9 09:07 capture
> lrwxrwxrwx 1 root root    0 Oct  9 09:07 device -> ../../pwmchip0
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 duty_cycle
> -rw-r--r-- 1 root root 4096 Oct  9 08:44 enable
> --w------- 1 root root 4096 Oct  9 09:07 export
> -rw-r--r-- 1 root root 4096 Oct  9 08:43 mode
> -r--r--r-- 1 root root 4096 Oct  9 09:07 npwm
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 period
> -rw-r--r-- 1 root root 4096 Oct  9 08:44 polarity
> drwxr-xr-x 2 root root    0 Oct  9 09:07 power
> lrwxrwxrwx 1 root root    0 Oct  9 09:07 subsystem -> ../../../../../../../../class/pwm
> -rw-r--r-- 1 root root 4096 Oct  9 08:42 uevent
> --w------- 1 root root 4096 Oct  9 09:07 unexport
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
> normal complementary [push-pull]
> 
> The mode enclosed in bracket is the currently active mode.
> 
> The mode could be set, via sysfs, by writing to mode file one of the modes
> displayed at read:
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode
> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
> [normal] complementary push-pull 

Getting a simple user of this into the kernel would be beneficial, too.
In my discussion with Thierry I'm faced with arguments like: You're
simplifying stuff which might destroy a use-case for sysfs users. This
is hard to argue against because it involves some guesswork to
reconstruct or contradict such arguments.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Claudiu.Beznea]

Hi Uwe,

Thank you for your inputs and sorry for the late response. Please see my
answers inline.

On 22.10.2018 11:29, Uwe Kleine-König wrote:
> Hello Claudiu,
> 
> On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote:
>> Please give feedback on these patches which extends the PWM framework in
>> order to support multiple PWM modes of operations. This series is a rework
>> of [1] and [2].
>>
>> The current patch series add the following PWM modes:
>> - PWM mode normal
>> - PWM mode complementary
>> - PWM mode push-pull
>>
>> Normal mode - for PWM channels with one output; output waveforms looks like
>> this:
>>              __    __    __    __
>>     PWM   __|  |__|  |__|  |__|  |__
>>             <--T-->
>>
>>     Where T is the signal period
> 
> In my discussion about some suggested changes to the PWM framework I
> used slightly different way to show the wave-forms in ASCII which are
> IMHO slightly better. 

It is true that the slope version is more like in real world but, on the
other hand, the datasheets and documentations mostly uses the digital
waveform formats (with no slopes) with regards to, at least PWM.

> Also I think it is valuable to not use a 50% duty
> cycle in the examples to remove some ambiguity.

Ok, sure, I will use a 1/3 duty cycle on next version.

> 
> With a duty cycle of 1/3 the normal mode looks as follows in "my" way:
> 
>           __       __       __
>  PWM   __/  \_____/  \_____/  \_____/
>          ^        ^        ^        ^
> 
> The carets mark always the start of a period. And note the rising and
> falling edges are already part of the active and inactive phases
> respectively which matches reality.

Ok, I'll use the carets on next version.

>  
>> Since PWMs with more than one output per channel could be used as one
>> output PWM the normal mode is the default mode for all PWMs (if not
>> specified otherwise).
>>
>> Complementary mode - for PWM channels with two outputs; output waveforms
>> for a PWM channel in complementary mode looks line this:
>>              __    __    __    __
>>     PWMH1 __|  |__|  |__|  |__|  |__
>>           __    __    __    __    __
>>     PWML1   |__|  |__|  |__|  |__|
>>             <--T-->
>>
>>     Where T is the signal period.
> 
> So this translates to (I think):
> 
>            __       __       __       __       __
>  PWMH   __/  \_____/  \_____/  \_____/  \_____/  \_____/
>         __    _____    _____    _____    _____    _____
>  PWML     \__/     \__/     \__/     \__/     \__/     \
>           ^        ^        ^        ^        ^        ^
> 
> That is PWML always pulls in the opposite direction of PWMH. Maybe we
> could come up with better terms than PWMH and PWML (which might be
> specific for the Atmel implementation?).

Yes, this is Atmel implementation naming.

> Maybe "normal" and
> "complement"?

I will think about it try to come with new naming. Normal and Complement
may be confusing for users with regards to PWM modes.

> 
>> Push-pull mode - for PWM channels with two outputs; output waveforms for a
>> PWM channel in push-pull mode with normal polarity looks like this:
>>             __          __
>>     PWMH __|  |________|  |________
>>                   __          __
>>     PWML ________|  |________|  |__
>>            <--T-->
> 
> That again with the alternative display method and duty cycle 1/3:
> 
>            __                __                __
>  PWMA   __/  \______________/  \______________/  \______
>                     __                __
>  PWMB   ___________/  \______________/  \______________/
>           ^        ^        ^        ^        ^        ^
Ok.

> 
> That is PWMA and PWMB are active only every 2nd period taking alternate
> turns, right?

Yes.

> 
> 
>>     If polarity is inversed:
>>          __    ________    ________
>>     PWMH   |__|        |__|
>>          ________    ________    __
>>     PWML         |__|        |__|
>>            <--T-->
> 
> That's again with duty cycle 1/3:
> 
>         __    ______________    ______________    ______
>  PWMA     \__/              \__/              \__/
>         ___________    ______________    ______________
>  PWMB              \__/              \__/              \
>           ^        ^        ^        ^        ^        ^
> 

Ok.

> Given that the start of period isn't externally visible this is
> equivalent to using a duty cycle 2/3 and not inverting which results in:
> 
>         __    ______________    ______________    ______
>  PWMA     \__/              \__/              \__/
>         ___________    ______________    ______________
>  PWMB              \__/              \__/              \
>              ^        ^        ^        ^        ^
> 
> I would really like if a more detailed description of the modes would be
> created as part of this series.

Sure, I will try to document it better.

> Currently there are a few implied
> properties hidden in the PWM API (unrelated to this series) which I try
> to resolve together with Thierry. Getting this documented right from the
> start would be great here.

Could you tell me if you want something specific to be touch as part of
documentation process for these PWM modes?

> 
> I didn't look in detail into the driver implementation, but from the
> PWMs implemented in the STM32F4 family I would have chosen a different
> model which makes me wonder if we should stick to a more general way to
> describe two outputs from a single PWM channel.
> 
> I would use four values with nanosecond resolution to describe these:
> 
>   .period
>   .duty_cycle
>   .alt_duty_cycle
>   .alt_offset
> 
> period and duty_cycle is as before for the primary output and then the
> alt_* values describe offset and duty cycle of the secondary output.
> 
> What you called "normal mode" would then be specified using
> 
>   .period = $period
>   .duty_cycle = $duty_cycle
>   .alt_duty_cycle = 0
>   .alt_offset = dontcare
> 
> Your "push pull mode" would be:
> 
>   .period = 2 * $period
>   .duty_cycle = $duty_cycle
>   .alt_duty_cycle = $duty_cycle
>   .alt_offset = $period
> 
> and complementary mode would be specified using:
> 
>   .period = $period
>   .duty_cycle = $duty_cycle
>   .alt_duty_cycle = $period - $duty_cycle
>   .alt_offset = $duty_cycle
> 

On Atmel PWM controller the push-pull mode is hardware generated based on
period and duty cycles that are setup for only one channel. The hardware
will take care of the synchronization b/w the outputs so that the push-pull
characteristic to be generated.

Having different configuration for every output part of the push-pull
waveform will allow users to generate every kind of outputs. But for IPs
that are capable of push-pull or complementary modes the generation of the
2 outputs are done in hardware (true in case of Atmel PWM controller). In
case of STM32F4 as far as I can see from [1] "The advanced-control timers
(TIM1 and TIM8 ) can output two complementary signals and
manage the switching-off and the switching-on instants of the outputs."
Maybe, in this case, if there are 2 hardware blocks that could be synced to
work together, e.g. in complementary mode, the setting of these two timers
should be done in driver so that the hardware blocks to be configured
together, atomically, so that the complementary characteristics to be obtained.

From my point of view it is better to implement in PWM core the concepts,
e.g. push-pull, complementary, even dead-time (I had something in my queue
for this) and the driver to do what it takes so that the IP to generate
implemented concepts. Mostly, the applications of PWM will use the PWM in
normal mode, push-pull, complementary or complementary with dead-time
insertion.

[1]
https://www.st.com/content/ccc/resource/technical/document/reference_manual/3d/6d/5a/66/b4/99/40/d4/DM00031020.pdf/files/DM00031020.pdf/jcr:content/translations/en.DM00031020.pdf


> With this abstraction stuff like "complementary output with dead-time
> insertion" (something like:
> 
>            __                __                __
>  PWMA   __/  \______________/  \______________/  \______
>                 __________        __________          __
>  PWMB   _______/          \______/          \______/
>           ^                 ^                 ^
> 
> ) could be modelled.

Same for this, my opinion is that we should implement generic things in
core and drivers should configure properly the IP so that it generates the
proper signals.

> 
>> The PWM working modes are per PWM channel registered as PWM's capabilities.
>> The driver registers itself to PWM core a get_caps() function, in
>> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
>> If this function is not registered in driver's probe, a default function
>> will be used to retrieve PWM capabilities. Currently, the default
>> capabilities includes only PWM normal mode.
> 
> In the i2c framework this is a function, too, and I wonder if simplicity
> is better served when this is just a flags member in the pwm_ops
> structure.

Thierry proposed this so that we could retrieve capabilities per PWM channel.

> 
>> PWM state has been updated to keep PWM mode. PWM mode could be configured
>> via sysfs or via DT. pwm_apply_state() will do the preliminary validation
>> for PWM mode to be applied.
>>
>> In sysfs, user could get PWM modes by reading mode file of PWM device:
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# ls -l
>> total 0
>> -r--r--r-- 1 root root 4096 Oct  9 09:07 capture
>> lrwxrwxrwx 1 root root    0 Oct  9 09:07 device -> ../../pwmchip0
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 duty_cycle
>> -rw-r--r-- 1 root root 4096 Oct  9 08:44 enable
>> --w------- 1 root root 4096 Oct  9 09:07 export
>> -rw-r--r-- 1 root root 4096 Oct  9 08:43 mode
>> -r--r--r-- 1 root root 4096 Oct  9 09:07 npwm
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 period
>> -rw-r--r-- 1 root root 4096 Oct  9 08:44 polarity
>> drwxr-xr-x 2 root root    0 Oct  9 09:07 power
>> lrwxrwxrwx 1 root root    0 Oct  9 09:07 subsystem -> ../../../../../../../../class/pwm
>> -rw-r--r-- 1 root root 4096 Oct  9 08:42 uevent
>> --w------- 1 root root 4096 Oct  9 09:07 unexport
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
>> normal complementary [push-pull]
>>
>> The mode enclosed in bracket is the currently active mode.
>>
>> The mode could be set, via sysfs, by writing to mode file one of the modes
>> displayed at read:
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# echo normal > mode
>> root@sama5d2-xplained:/sys/class/pwm/pwmchip0/pwm2# cat mode
>> [normal] complementary push-pull 
> 
> Getting a simple user of this into the kernel would be beneficial, too.
> In my discussion with Thierry I'm faced with arguments like: You're
> simplifying stuff which might destroy a use-case for sysfs users. This
> is hard to argue against because it involves some guesswork to
> reconstruct or contradict such arguments.

Yep, currently only sysfs is using this. I will see what I can do about
this to also have an in kernel user (I'll have to check, at least, how
pwm-regulator is working with this).

Thank you,
Claudiu Beznea

> 
> Best regards
> Uwe
> 

Re: [RESEND PATCH v5 0/9] extend PWM framework to support PWM modes

[Uwe Kleine-König]

Hello Claudiu,

On Fri, Oct 26, 2018 at 10:44:43AM +0000, Claudiu.Beznea@microchip.com wrote:
> Thank you for your inputs and sorry for the late response. Please see my
> answers inline.

No problems, I didn't held my breath :-)

> On 22.10.2018 11:29, Uwe Kleine-König wrote:
> > On Tue, Aug 28, 2018 at 04:01:17PM +0300, Claudiu Beznea wrote:
> >> Since PWMs with more than one output per channel could be used as one
> >> output PWM the normal mode is the default mode for all PWMs (if not
> >> specified otherwise).
> >>
> >> Complementary mode - for PWM channels with two outputs; output waveforms
> >> for a PWM channel in complementary mode looks line this:
> >>              __    __    __    __
> >>     PWMH1 __|  |__|  |__|  |__|  |__
> >>           __    __    __    __    __
> >>     PWML1   |__|  |__|  |__|  |__|
> >>             <--T-->
> >>
> >>     Where T is the signal period.
> > 
> > So this translates to (I think):
> > 
> >            __       __       __       __       __
> >  PWMH   __/  \_____/  \_____/  \_____/  \_____/  \_____/
> >         __    _____    _____    _____    _____    _____
> >  PWML     \__/     \__/     \__/     \__/     \__/     \
> >           ^        ^        ^        ^        ^        ^
> > 
> > That is PWML always pulls in the opposite direction of PWMH. Maybe we
> > could come up with better terms than PWMH and PWML (which might be
> > specific for the Atmel implementation?).
> 
> Yes, this is Atmel implementation naming.
> 
> > Maybe "normal" and
> > "complement"?
> 
> I will think about it try to come with new naming. Normal and Complement
> may be confusing for users with regards to PWM modes.
> 
> > 
> >> Push-pull mode - for PWM channels with two outputs; output waveforms for a
> >> PWM channel in push-pull mode with normal polarity looks like this:
> >>             __          __
> >>     PWMH __|  |________|  |________
> >>                   __          __
> >>     PWML ________|  |________|  |__
> >>            <--T-->
> > 
> > That again with the alternative display method and duty cycle 1/3:
> > 
> >            __                __                __
> >  PWMA   __/  \______________/  \______________/  \______
> >                     __                __
> >  PWMB   ___________/  \______________/  \______________/
> >           ^        ^        ^        ^        ^        ^
> Ok.
> 
> > That is PWMA and PWMB are active only every 2nd period taking alternate
> > turns, right?
> 
> Yes.
> 
> > 
> > 
> >>     If polarity is inversed:
> >>          __    ________    ________
> >>     PWMH   |__|        |__|
> >>          ________    ________    __
> >>     PWML         |__|        |__|
> >>            <--T-->
> > 
> > That's again with duty cycle 1/3:
> > 
> >         __    ______________    ______________    ______
> >  PWMA     \__/              \__/              \__/
> >         ___________    ______________    ______________
> >  PWMB              \__/              \__/              \
> >           ^        ^        ^        ^        ^        ^
> > 
> 
> Ok.
> 
> > Given that the start of period isn't externally visible this is
> > equivalent to using a duty cycle 2/3 and not inverting which results in:
> > 
> >         __    ______________    ______________    ______
> >  PWMA     \__/              \__/              \__/
> >         ___________    ______________    ______________
> >  PWMB              \__/              \__/              \
> >              ^        ^        ^        ^        ^
> > 
> > I would really like if a more detailed description of the modes would be
> > created as part of this series.
> 
> Sure, I will try to document it better.

Note here I just leaked my belief that the PWM framework shouldn't
necessarily expose inversion to users which is part of my discussion
with Thierry. I think it would be sensible to drop it here.

> > Currently there are a few implied
> > properties hidden in the PWM API (unrelated to this series) which I try
> > to resolve together with Thierry. Getting this documented right from the
> > start would be great here.
> 
> Could you tell me if you want something specific to be touch as part of
> documentation process for these PWM modes?

At least having something about the expectations in Documenation/ would
be great.

> > I didn't look in detail into the driver implementation, but from the
> > PWMs implemented in the STM32F4 family I would have chosen a different
> > model which makes me wonder if we should stick to a more general way to
> > describe two outputs from a single PWM channel.
> > 
> > I would use four values with nanosecond resolution to describe these:
> > 
> >   .period
> >   .duty_cycle
> >   .alt_duty_cycle
> >   .alt_offset
> > 
> > period and duty_cycle is as before for the primary output and then the
> > alt_* values describe offset and duty cycle of the secondary output.
> > 
> > What you called "normal mode" would then be specified using
> > 
> >   .period = $period
> >   .duty_cycle = $duty_cycle
> >   .alt_duty_cycle = 0
> >   .alt_offset = dontcare
> > 
> > Your "push pull mode" would be:
> > 
> >   .period = 2 * $period
> >   .duty_cycle = $duty_cycle
> >   .alt_duty_cycle = $duty_cycle
> >   .alt_offset = $period
> > 
> > and complementary mode would be specified using:
> > 
> >   .period = $period
> >   .duty_cycle = $duty_cycle
> >   .alt_duty_cycle = $period - $duty_cycle
> >   .alt_offset = $duty_cycle
> > 
> 
> On Atmel PWM controller the push-pull mode is hardware generated based on
> period and duty cycles that are setup for only one channel. The hardware
> will take care of the synchronization b/w the outputs so that the push-pull
> characteristic to be generated.
> 
> Having different configuration for every output part of the push-pull
> waveform will allow users to generate every kind of outputs. But for IPs
> that are capable of push-pull or complementary modes the generation of the
> 2 outputs are done in hardware (true in case of Atmel PWM controller). In
> case of STM32F4 as far as I can see from [1] "The advanced-control timers
> (TIM1 and TIM8 ) can output two complementary signals and
> manage the switching-off and the switching-on instants of the outputs."
> Maybe, in this case, if there are 2 hardware blocks that could be synced to
> work together, e.g. in complementary mode, the setting of these two timers
> should be done in driver so that the hardware blocks to be configured
> together, atomically, so that the complementary characteristics to be obtained.

The upside I see in my approach with .alt_duty_cycle and .alt_offset
over your .mode is that it allows to describe more use-cases. If I
wanted to support complementary with dead-time I'd need another member
in pwm_state to specify the dead time. Then the next controller can only
add dead time on one end of secondary output needing yet another mode
enum. With my approach I think you can specify all sensible(TM)
waveforms already now. Then a driver must not be adapted again when
someone adds support for another mode.

The downside is that if your PWM only supports complementary mode with
no dead-time you have to find out from .alt_duty_cycle and .alt_offset
that the specified wave form is indeed matching complementary mode. From
from the framework's POV this is more elegant though (but YMMV).

> > With this abstraction stuff like "complementary output with dead-time
> > insertion" (something like:
> > 
> >            __                __                __
> >  PWMA   __/  \______________/  \______________/  \______
> >                 __________        __________          __
> >  PWMB   _______/          \______/          \______/
> >           ^                 ^                 ^
> > 
> > ) could be modelled.
> 
> Same for this, my opinion is that we should implement generic things in
> core and drivers should configure properly the IP so that it generates the
> proper signals.

This is common to both our approaches. Just the way the PWM user
specifies his/her wishes (and so the input for hw drivers) is different.
 
> >> The PWM working modes are per PWM channel registered as PWM's capabilities.
> >> The driver registers itself to PWM core a get_caps() function, in
> >> struct pwm_ops, that will be used by PWM core to retrieve PWM capabilities.
> >> If this function is not registered in driver's probe, a default function
> >> will be used to retrieve PWM capabilities. Currently, the default
> >> capabilities includes only PWM normal mode.
> > 
> > In the i2c framework this is a function, too, and I wonder if simplicity
> > is better served when this is just a flags member in the pwm_ops
> > structure.
> 
> Thierry proposed this so that we could retrieve capabilities per PWM channel.

I don't have a complete overview over the different hardware
implementations, but I'd expect that if two different implementations
share the operations then the return value of .get_caps is shared by
both. As long this is the case not introducing a callback for that is
the easier path. Adding a callback later when (and if) this is required
is trivial then.
 
Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |