[PATCH v2 00/11] pwm: Add support for PWM Capture

[PATCH v2 00/11] pwm: Add support for PWM Capture

[Lee Jones]

The first part of this set extends the current PWM API to allow external
code to request a PWM Capture.  Subsequent patches then make use of the
new API by providing a userspace offering via /sysfs.  The final part of
the set supplies PWM Capture functionality into the already existing STi
PWM driver.

This patch-set has been tested end to end via /sysfs.

v1 => v2:
  API change
  - Use a struct to carry the result back to the caller
  - Use 'struct pwm' to store device specific data
  - Make timeout configurable
  - Don't use clear_bit(), instead use raw bit logic
  - Propagate return value of platform_get_irq()
  - Don't cast to (void *)
  - Move to subsystem terminology (channels => devices)
  - Remove channel select feature
  - Enable Capture IP during capture

Lee Jones (11):
  pwm: Add PWM Capture support
  pwm: sti: Rename channel => device
  pwm: sysfs: Add PWM Capture support
  pwm: sti: Reorganise register names in preparation for new
    functionality
  pwm: sti: Only request clock rate when you need to
  pwm: sti: Supply PWM Capture register addresses and bit locations
  pwm: sti: Supply PWM Capture clock handling
  pwm: sti: Initialise PWM Capture device data
  pwm: sti: Add support for PWM Capture IRQs
  pwm: sti: Add PWM Capture call-back
  pwm: sti: Take the opportunity to conduct a little house keeping

 drivers/pwm/core.c    |  27 ++++
 drivers/pwm/pwm-sti.c | 411 +++++++++++++++++++++++++++++++++++++++++---------
 drivers/pwm/sysfs.c   |  17 +++
 include/linux/pwm.h   |  28 ++++
 4 files changed, 412 insertions(+), 71 deletions(-)

-- 
2.8.0

[[PATCH v2] 01/11] pwm: Add PWM Capture support

[Lee Jones]

Supply a PWM Capture call-back Op in order to pass back
information obtained by running analysis on PWM a signal.
This would normally (at least during testing) be called from
the Sysfs routines with a view to printing out PWM Capture
data which has been encoded into a string.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/core.c  | 27 +++++++++++++++++++++++++++
 include/linux/pwm.h | 28 ++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 7831bc6..160784d 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -494,6 +494,33 @@ unlock:
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 
 /**
+ * pwm_capture() - capture and report a PWM signal
+ * @pwm: PWM device
+ * @result: struct to fill with capture result
+ * @timeout_ms: time to wait, in milliseconds, before giving up on capture
+ *
+ * Returns: 0 on success or a negative error code on failure.
+ */
+int pwm_capture(struct pwm_device *pwm, struct pwm_capture *result,
+		unsigned int timeout_ms)
+{
+	int err;
+
+	if (!pwm || !pwm->chip->ops)
+		return -EINVAL;
+
+	if (!pwm->chip->ops->capture)
+		return -ENOSYS;
+
+	mutex_lock(&pwm->lock);
+	err = pwm->chip->ops->capture(pwm->chip, pwm, result, timeout_ms);
+	mutex_unlock(&pwm->lock);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(pwm_capture);
+
+/**
  * pwm_enable() - start a PWM output toggling
  * @pwm: PWM device
  *
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index cfc3ed4..49f9648 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -6,6 +6,7 @@
 #include <linux/of.h>
 
 struct pwm_device;
+struct pwm_capture;
 struct seq_file;
 
 #if IS_ENABLED(CONFIG_PWM)
@@ -33,6 +34,13 @@ int pwm_enable(struct pwm_device *pwm);
  * pwm_disable - stop a PWM output toggling
  */
 void pwm_disable(struct pwm_device *pwm);
+
+/*
+ * pwm_capture - capture and report a PWM signal
+ */
+int pwm_capture(struct pwm_device *pwm,
+		struct pwm_capture *result,
+		unsigned int timeout_ms);
 #else
 static inline struct pwm_device *pwm_request(int pwm_id, const char *label)
 {
@@ -56,6 +64,13 @@ static inline int pwm_enable(struct pwm_device *pwm)
 static inline void pwm_disable(struct pwm_device *pwm)
 {
 }
+
+static inline int pwm_capture(struct pwm_device *pwm,
+			      struct pwm_capture *result,
+			      unsigned int timeout_ms)
+{
+	return -EINVAL;
+}
 #endif
 
 struct pwm_chip;
@@ -107,6 +122,16 @@ struct pwm_device {
 	enum pwm_polarity polarity;
 };
 
+/**
+ * struct pwm_capture - PWM capture data
+ * @period: period of the PWM signal (in nanoseconds)
+ * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
+ */
+struct pwm_capture {
+	unsigned long long period;
+	unsigned long long duty_cycle;
+};
+
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 {
 	return test_bit(PWMF_ENABLED, &pwm->flags);
@@ -150,6 +175,7 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
  * @free: optional hook for freeing a PWM
  * @config: configure duty cycles and period length for this PWM
  * @set_polarity: configure the polarity of this PWM
+ * @capture: capture and report PWM signal
  * @enable: enable PWM output toggling
  * @disable: disable PWM output toggling
  * @dbg_show: optional routine to show contents in debugfs
@@ -162,6 +188,8 @@ struct pwm_ops {
 		      int duty_ns, int period_ns);
 	int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
 			    enum pwm_polarity polarity);
+	int (*capture)(struct pwm_chip *chip, struct pwm_device *pwm,
+		       struct pwm_capture *result, unsigned int timeout_ms);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*disable)(struct pwm_chip *chip, struct pwm_device *pwm);
 #ifdef CONFIG_DEBUG_FS
-- 
2.8.0

[[PATCH v2] 02/11] pwm: sti: Rename channel => device

[Lee Jones]

This is to bring the terminology used in the STi PWM driver more
into line with the PWM subsystem.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 92abbd5..3dae127 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -21,7 +21,7 @@
 #include <linux/slab.h>
 #include <linux/time.h>
 
-#define STI_DS_REG(ch)	(4 * (ch))	/* Channel's Duty Cycle register */
+#define STI_DS_REG(ch)	(4 * (ch))	/* Device's Duty Cycle register */
 #define STI_PWMCR	0x50		/* Control/Config register */
 #define STI_INTEN	0x54		/* Interrupt Enable/Disable register */
 #define PWM_PRESCALE_LOW_MASK		0x0f
@@ -40,7 +40,7 @@ enum {
 
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
-	unsigned int num_chan;
+	unsigned int num_devs;
 	unsigned int max_pwm_cnt;
 	unsigned int max_prescale;
 };
@@ -130,13 +130,13 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	/* Allow configuration changes if one of the
 	 * following conditions satisfy.
-	 * 1. No channels have been configured.
-	 * 2. Only one channel has been configured and the new request
-	 *    is for the same channel.
-	 * 3. Only one channel has been configured and the new request is
-	 *    for a new channel and period of the new channel is same as
+	 * 1. No devices have been configured.
+	 * 2. Only one device has been configured and the new request
+	 *    is for the same device.
+	 * 3. Only one device has been configured and the new request is
+	 *    for a new device and period of the new device is same as
 	 *    the current configured period.
-	 * 4. More than one channels are configured and period of the new
+	 * 4. More than one devices are configured and period of the new
 	 *    requestis the same as the current period.
 	 */
 	if (!ncfg ||
@@ -201,7 +201,7 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	int ret = 0;
 
 	/*
-	 * Since we have a common enable for all PWM channels,
+	 * Since we have a common enable for all PWM devices,
 	 * do not enable if already enabled.
 	 */
 	mutex_lock(&pc->sti_pwm_lock);
@@ -259,11 +259,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	const struct reg_field *reg_fields;
 	struct device_node *np = dev->of_node;
 	struct sti_pwm_compat_data *cdata = pc->cdata;
-	u32 num_chan;
+	u32 num_devs;
 
-	of_property_read_u32(np, "st,pwm-num-chan", &num_chan);
-	if (num_chan)
-		cdata->num_chan = num_chan;
+	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
+	if (num_devs)
+		cdata->num_devs = num_devs;
 
 	reg_fields = cdata->reg_fields;
 
@@ -330,7 +330,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	cdata->reg_fields   = &sti_pwm_regfields[0];
 	cdata->max_prescale = 0xff;
 	cdata->max_pwm_cnt  = 255;
-	cdata->num_chan     = 1;
+	cdata->num_devs     = 1;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
@@ -362,7 +362,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = pc->cdata->num_chan;
+	pc->chip.npwm = pc->cdata->num_devs;
 	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
@@ -381,7 +381,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 	struct sti_pwm_chip *pc = platform_get_drvdata(pdev);
 	unsigned int i;
 
-	for (i = 0; i < pc->cdata->num_chan; i++)
+	for (i = 0; i < pc->cdata->num_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->clk);
-- 
2.8.0

[[PATCH v2] 03/11] pwm: sysfs: Add PWM Capture support

[Lee Jones]

Allow a user to read PWM Capture results from /sysfs.

To start a capture and read the result, simply read the file:

  $ cat $PWMCHIP/capture

The output format is "<period>:<duty_cycle>".

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/sysfs.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 9c90886..ca0d886 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -167,16 +167,33 @@ static ssize_t polarity_store(struct device *child,
 	return ret ? : size;
 }
 
+static ssize_t capture_show(struct device *child,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	struct pwm_capture result;
+	int ret;
+
+	ret = pwm_capture(pwm, &result, jiffies_to_msecs(HZ));
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%llu %llu\n", result.period, result.duty_cycle);
+}
+
 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 struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
 	&dev_attr_duty_cycle.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
+	&dev_attr_capture.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
-- 
2.8.0

[[PATCH v2] 04/11] pwm: sti: Reorganise register names in preparation for new functionality

[Lee Jones]

Exciting functionality is on the way to this device.  But
before we can add it, we need to do some basic housekeeping
so the additions can be added cleanly.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 76 +++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 3dae127..5fbee61 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -21,18 +21,22 @@
 #include <linux/slab.h>
 #include <linux/time.h>
 
-#define STI_DS_REG(ch)	(4 * (ch))	/* Device's Duty Cycle register */
-#define STI_PWMCR	0x50		/* Control/Config register */
-#define STI_INTEN	0x54		/* Interrupt Enable/Disable register */
+#define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device's Duty Cycle register */
+
+#define STI_PWM_CTRL		0x50	/* Control/Config register */
+#define STI_INT_EN		0x54	/* Interrupt Enable/Disable register */
 #define PWM_PRESCALE_LOW_MASK		0x0f
 #define PWM_PRESCALE_HIGH_MASK		0xf0
 
 /* Regfield IDs */
 enum {
+	/* Bits in PWM_CTRL*/
 	PWMCLK_PRESCALE_LOW,
 	PWMCLK_PRESCALE_HIGH,
-	PWM_EN,
-	PWM_INT_EN,
+
+	PWM_OUT_EN,
+
+	PWM_CPT_INT_EN,
 
 	/* Keep last */
 	MAX_REGFIELDS
@@ -47,14 +51,14 @@ struct sti_pwm_compat_data {
 
 struct sti_pwm_chip {
 	struct device *dev;
-	struct clk *clk;
 	unsigned long clk_rate;
+	struct clk *pwm_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
 	struct regmap_field *prescale_low;
 	struct regmap_field *prescale_high;
-	struct regmap_field *pwm_en;
-	struct regmap_field *pwm_int_en;
+	struct regmap_field *pwm_out_en;
+	struct regmap_field *pwm_cpt_int_en;
 	struct pwm_chip chip;
 	struct pwm_device *cur;
 	unsigned long configured;
@@ -64,10 +68,10 @@ struct sti_pwm_chip {
 };
 
 static const struct reg_field sti_pwm_regfields[MAX_REGFIELDS] = {
-	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWMCR, 0, 3),
-	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWMCR, 11, 14),
-	[PWM_EN]		= REG_FIELD(STI_PWMCR, 9, 9),
-	[PWM_INT_EN]		= REG_FIELD(STI_INTEN, 0, 0),
+	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWM_CTRL, 0, 3),
+	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWM_CTRL, 11, 14),
+	[PWM_OUT_EN]		= REG_FIELD(STI_PWM_CTRL, 9, 9),
+	[PWM_CPT_INT_EN]	= REG_FIELD(STI_INT_EN, 1, 4),
 };
 
 static inline struct sti_pwm_chip *to_sti_pwmchip(struct pwm_chip *chip)
@@ -144,7 +148,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	    ((ncfg == 1) && (pwm->hwpwm != cur->hwpwm) && period_same) ||
 	    ((ncfg > 1) && period_same)) {
 		/* Enable clock before writing to PWM registers. */
-		ret = clk_enable(pc->clk);
+		ret = clk_enable(pc->pwm_clk);
 		if (ret)
 			return ret;
 
@@ -174,11 +178,12 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		 */
 		pwmvalx = cdata->max_pwm_cnt * duty_ns / period_ns;
 
-		ret = regmap_write(pc->regmap, STI_DS_REG(pwm->hwpwm), pwmvalx);
+		ret = regmap_write(pc->regmap,
+				   PWM_OUT_VAL(pwm->hwpwm), pwmvalx);
 		if (ret)
 			goto clk_dis;
 
-		ret = regmap_field_write(pc->pwm_int_en, 0);
+		ret = regmap_field_write(pc->pwm_cpt_int_en, 0);
 
 		set_bit(pwm->hwpwm, &pc->configured);
 		pc->cur = pwm;
@@ -190,7 +195,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	}
 
 clk_dis:
-	clk_disable(pc->clk);
+	clk_disable(pc->pwm_clk);
 	return ret;
 }
 
@@ -206,11 +211,11 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	 */
 	mutex_lock(&pc->sti_pwm_lock);
 	if (!pc->en_count) {
-		ret = clk_enable(pc->clk);
+		ret = clk_enable(pc->pwm_clk);
 		if (ret)
 			goto out;
 
-		ret = regmap_field_write(pc->pwm_en, 1);
+		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
 			dev_err(dev, "failed to enable PWM device:%d\n",
 				pwm->hwpwm);
@@ -232,9 +237,9 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 		mutex_unlock(&pc->sti_pwm_lock);
 		return;
 	}
-	regmap_field_write(pc->pwm_en, 0);
+	regmap_field_write(pc->pwm_out_en, 0);
 
-	clk_disable(pc->clk);
+	clk_disable(pc->pwm_clk);
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -277,15 +282,16 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->prescale_high))
 		return PTR_ERR(pc->prescale_high);
 
-	pc->pwm_en = devm_regmap_field_alloc(dev, pc->regmap,
-					     reg_fields[PWM_EN]);
-	if (IS_ERR(pc->pwm_en))
-		return PTR_ERR(pc->pwm_en);
 
-	pc->pwm_int_en = devm_regmap_field_alloc(dev, pc->regmap,
-						 reg_fields[PWM_INT_EN]);
-	if (IS_ERR(pc->pwm_int_en))
-		return PTR_ERR(pc->pwm_int_en);
+	pc->pwm_out_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_OUT_EN]);
+	if (IS_ERR(pc->pwm_out_en))
+		return PTR_ERR(pc->pwm_out_en);
+
+	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_CPT_INT_EN]);
+	if (IS_ERR(pc->pwm_cpt_int_en))
+		return PTR_ERR(pc->pwm_cpt_int_en);
 
 	return 0;
 }
@@ -341,19 +347,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	pc->clk = of_clk_get_by_name(dev->of_node, "pwm");
-	if (IS_ERR(pc->clk)) {
+	pc->pwm_clk = of_clk_get_by_name(dev->of_node, "pwm");
+	if (IS_ERR(pc->pwm_clk)) {
 		dev_err(dev, "failed to get PWM clock\n");
-		return PTR_ERR(pc->clk);
+		return PTR_ERR(pc->pwm_clk);
 	}
 
-	pc->clk_rate = clk_get_rate(pc->clk);
+	pc->clk_rate = clk_get_rate(pc->pwm_clk);
 	if (!pc->clk_rate) {
 		dev_err(dev, "failed to get clock rate\n");
 		return -EINVAL;
 	}
 
-	ret = clk_prepare(pc->clk);
+	ret = clk_prepare(pc->pwm_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
 		return ret;
@@ -367,7 +373,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
-		clk_unprepare(pc->clk);
+		clk_unprepare(pc->pwm_clk);
 		return ret;
 	}
 
@@ -384,7 +390,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 	for (i = 0; i < pc->cdata->num_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
-	clk_unprepare(pc->clk);
+	clk_unprepare(pc->pwm_clk);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
2.8.0

[[PATCH v2] 05/11] pwm: sti: Only request clock rate when you need to

[Lee Jones]

In the original code the clock rate was only obtained during
initialisation; however, the rate may change between then and
its use.  This patch ensures the correct rate is acquired just
before use.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 5fbee61..c4a34af 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -51,7 +51,6 @@ struct sti_pwm_compat_data {
 
 struct sti_pwm_chip {
 	struct device *dev;
-	unsigned long clk_rate;
 	struct clk *pwm_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
@@ -86,13 +85,20 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
 				unsigned int *prescale)
 {
 	struct sti_pwm_compat_data *cdata = pc->cdata;
+	unsigned long clk_rate;
 	unsigned long val;
 	unsigned int ps;
 
+	clk_rate = clk_get_rate(pc->pwm_clk);
+	if (!clk_rate) {
+		dev_err(pc->dev, "failed to get clock rate\n");
+		return -EINVAL;
+	}
+
 	/*
 	 * prescale = ((period_ns * clk_rate) / (10^9 * (max_pwm_count + 1)) - 1
 	 */
-	val = NSEC_PER_SEC / pc->clk_rate;
+	val = NSEC_PER_SEC / clk_rate;
 	val *= cdata->max_pwm_cnt + 1;
 
 	if (period % val) {
@@ -353,12 +359,6 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return PTR_ERR(pc->pwm_clk);
 	}
 
-	pc->clk_rate = clk_get_rate(pc->pwm_clk);
-	if (!pc->clk_rate) {
-		dev_err(dev, "failed to get clock rate\n");
-		return -EINVAL;
-	}
-
 	ret = clk_prepare(pc->pwm_clk);
 	if (ret) {
 		dev_err(dev, "failed to prepare clock\n");
-- 
2.8.0

[[PATCH v2] 06/11] pwm: sti: Supply PWM Capture register addresses and bit locations

[Lee Jones]

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index c4a34af..2f61e1e 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -22,26 +22,48 @@
 #include <linux/time.h>
 
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device's Duty Cycle register */
+#define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
+#define PWM_CPT_EDGE(x) (0x30 + (4 * (x))) /* Edge to capture on */
 
 #define STI_PWM_CTRL		0x50	/* Control/Config register */
 #define STI_INT_EN		0x54	/* Interrupt Enable/Disable register */
+#define STI_INT_STA		0x58	/* Interrupt Status register */
+#define PWM_INT_ACK			0x5c
 #define PWM_PRESCALE_LOW_MASK		0x0f
 #define PWM_PRESCALE_HIGH_MASK		0xf0
+#define PWM_CPT_EDGE_MASK		0x03
+#define PWM_INT_ACK_MASK		0x1ff
+
+#define STI_MAX_CPT_DEVS		4
+#define CPT_DC_MAX			0xff
 
 /* Regfield IDs */
 enum {
 	/* Bits in PWM_CTRL*/
 	PWMCLK_PRESCALE_LOW,
 	PWMCLK_PRESCALE_HIGH,
+	CPTCLK_PRESCALE,
 
 	PWM_OUT_EN,
+	PWM_CPT_EN,
 
 	PWM_CPT_INT_EN,
+	PWM_CPT_INT_STAT,
 
 	/* Keep last */
 	MAX_REGFIELDS
 };
 
+/* Each capture input can be programmed to detect rising-edge, falling-edge,
+ * either edge or neither egde
+ */
+enum sti_cpt_edge {
+	CPT_EDGE_DISABLED,
+	CPT_EDGE_RISING,
+	CPT_EDGE_FALLING,
+	CPT_EDGE_BOTH,
+};
+
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
 	unsigned int num_devs;
@@ -69,8 +91,11 @@ struct sti_pwm_chip {
 static const struct reg_field sti_pwm_regfields[MAX_REGFIELDS] = {
 	[PWMCLK_PRESCALE_LOW]	= REG_FIELD(STI_PWM_CTRL, 0, 3),
 	[PWMCLK_PRESCALE_HIGH]	= REG_FIELD(STI_PWM_CTRL, 11, 14),
+	[CPTCLK_PRESCALE]	= REG_FIELD(STI_PWM_CTRL, 4, 8),
 	[PWM_OUT_EN]		= REG_FIELD(STI_PWM_CTRL, 9, 9),
+	[PWM_CPT_EN]		= REG_FIELD(STI_PWM_CTRL, 10, 10),
 	[PWM_CPT_INT_EN]	= REG_FIELD(STI_INT_EN, 1, 4),
+	[PWM_CPT_INT_STAT]	= REG_FIELD(STI_INT_STA, 1, 4),
 };
 
 static inline struct sti_pwm_chip *to_sti_pwmchip(struct pwm_chip *chip)
-- 
2.8.0

[[PATCH v2] 07/11] pwm: sti: Supply PWM Capture clock handling

[Lee Jones]

ST's PWM IP is supplied by 2 different clocks.  One for PWM
Output and the other for Capture.  This patch provides clock
handling for the latter.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 2f61e1e..78979d0 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -74,6 +74,7 @@ struct sti_pwm_compat_data {
 struct sti_pwm_chip {
 	struct device *dev;
 	struct clk *pwm_clk;
+	struct clk *cpt_clk;
 	struct regmap *regmap;
 	struct sti_pwm_compat_data *cdata;
 	struct regmap_field *prescale_low;
@@ -183,6 +184,10 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		if (ret)
 			return ret;
 
+		ret = clk_enable(pc->cpt_clk);
+		if (ret)
+			return ret;
+
 		if (!period_same) {
 			ret = sti_pwm_get_prescale(pc, period_ns, &prescale);
 			if (ret)
@@ -227,6 +232,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 clk_dis:
 	clk_disable(pc->pwm_clk);
+	clk_disable(pc->cpt_clk);
 	return ret;
 }
 
@@ -246,6 +252,10 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 		if (ret)
 			goto out;
 
+		ret = clk_enable(pc->cpt_clk);
+		if (ret)
+			goto out;
+
 		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
 			dev_err(dev, "failed to enable PWM device:%d\n",
@@ -271,6 +281,7 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	regmap_field_write(pc->pwm_out_en, 0);
 
 	clk_disable(pc->pwm_clk);
+	clk_disable(pc->cpt_clk);
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -390,6 +401,18 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	pc->cpt_clk = of_clk_get_by_name(dev->of_node, "capture");
+	if (IS_ERR(pc->cpt_clk)) {
+		dev_err(dev, "failed to get PWM capture clock\n");
+		return PTR_ERR(pc->cpt_clk);
+	}
+
+	ret = clk_prepare(pc->cpt_clk);
+	if (ret) {
+		dev_err(dev, "failed to prepare clock\n");
+		return ret;
+	}
+
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
@@ -399,6 +422,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	ret = pwmchip_add(&pc->chip);
 	if (ret < 0) {
 		clk_unprepare(pc->pwm_clk);
+		clk_unprepare(pc->cpt_clk);
 		return ret;
 	}
 
@@ -416,6 +440,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->pwm_clk);
+	clk_unprepare(pc->cpt_clk);
 
 	return pwmchip_remove(&pc->chip);
 }
-- 
2.8.0

Re: [STLinux Kernel] [[PATCH v2] 07/11] pwm: sti: Supply PWM Capture clock handling

[Peter Griffin]

Hi Lee,

On Fri, 22 Apr 2016, Lee Jones wrote:

> ST's PWM IP is supplied by 2 different clocks.  One for PWM
> Output and the other for Capture.  This patch provides clock
> handling for the latter.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 2f61e1e..78979d0 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -74,6 +74,7 @@ struct sti_pwm_compat_data {
>  struct sti_pwm_chip {
>  	struct device *dev;
>  	struct clk *pwm_clk;
> +	struct clk *cpt_clk;
>  	struct regmap *regmap;
>  	struct sti_pwm_compat_data *cdata;
>  	struct regmap_field *prescale_low;
> @@ -183,6 +184,10 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  		if (ret)
>  			return ret;
>  
> +		ret = clk_enable(pc->cpt_clk);
> +		if (ret)
> +			return ret;
> +
>  		if (!period_same) {
>  			ret = sti_pwm_get_prescale(pc, period_ns, &prescale);
>  			if (ret)
> @@ -227,6 +232,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  
>  clk_dis:
>  	clk_disable(pc->pwm_clk);
> +	clk_disable(pc->cpt_clk);
>  	return ret;
>  }
>  
> @@ -246,6 +252,10 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>  		if (ret)
>  			goto out;
>  
> +		ret = clk_enable(pc->cpt_clk);
> +		if (ret)
> +			goto out;
> +
>  		ret = regmap_field_write(pc->pwm_out_en, 1);
>  		if (ret) {
>  			dev_err(dev, "failed to enable PWM device:%d\n",
> @@ -271,6 +281,7 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>  	regmap_field_write(pc->pwm_out_en, 0);
>  
>  	clk_disable(pc->pwm_clk);
> +	clk_disable(pc->cpt_clk);
>  	mutex_unlock(&pc->sti_pwm_lock);
>  }
>  
> @@ -390,6 +401,18 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	pc->cpt_clk = of_clk_get_by_name(dev->of_node, "capture");
> +	if (IS_ERR(pc->cpt_clk)) {
> +		dev_err(dev, "failed to get PWM capture clock\n");
> +		return PTR_ERR(pc->cpt_clk);
> +	}

The dt binding document pwm-st.txt also needs updating for this extra clock.

regards,

Peter.

[[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Lee Jones]

Each PWM Capture device is allocated a structure to hold its own
state.  During a capture the device may be partaking in one of 3
phases.  Initial (rising) phase change, a subsequent (falling)
phase change indicating end of the duty-cycle phase and finally
a final (rising) phase change indicating the end of the period.
The timer value snapshot each event is held in a variable of the
same name, and the phase number (0, 1, 2) is contained in the
index variable.  Other device specific information, such as GPIO
pin, the IRQ wait queue and locking is also contained in the
structure.  This patch initialises this structure for each of
the available devices.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 78979d0..9d597bb 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -11,6 +11,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/interrupt.h>
 #include <linux/math64.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -18,8 +19,10 @@
 #include <linux/platform_device.h>
 #include <linux/pwm.h>
 #include <linux/regmap.h>
+#include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/time.h>
+#include <linux/wait.h>
 
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device's Duty Cycle register */
 #define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
@@ -64,9 +67,17 @@ enum sti_cpt_edge {
 	CPT_EDGE_BOTH,
 };
 
+struct sti_cpt_ddata {
+	u32 snapshot[3];
+	int index;
+	struct mutex lock;
+	wait_queue_head_t wait;
+};
+
 struct sti_pwm_compat_data {
 	const struct reg_field *reg_fields;
-	unsigned int num_devs;
+	unsigned int pwm_num_devs;
+	unsigned int cpt_num_devs;
 	unsigned int max_pwm_cnt;
 	unsigned int max_prescale;
 };
@@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	struct device_node *np = dev->of_node;
 	struct sti_pwm_compat_data *cdata = pc->cdata;
 	u32 num_devs;
+	int ret;
 
-	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
-	if (num_devs)
-		cdata->num_devs = num_devs;
+	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
+	if (!ret)
+		cdata->pwm_num_devs = num_devs;
+
+	ret = of_property_read_u32(np, "st,capture-num-devs", &num_devs);
+	if (!ret)
+		cdata->cpt_num_devs = num_devs;
 
 	reg_fields = cdata->reg_fields;
 
@@ -350,6 +366,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_compat_data *cdata;
 	struct sti_pwm_chip *pc;
 	struct resource *res;
+	unsigned int devnum;
 	int ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
@@ -378,7 +395,8 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	cdata->reg_fields   = &sti_pwm_regfields[0];
 	cdata->max_prescale = 0xff;
 	cdata->max_pwm_cnt  = 255;
-	cdata->num_devs     = 1;
+	cdata->pwm_num_devs = 1;
+	cdata->cpt_num_devs = 0;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
@@ -416,7 +434,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	pc->chip.dev = dev;
 	pc->chip.ops = &sti_pwm_ops;
 	pc->chip.base = -1;
-	pc->chip.npwm = pc->cdata->num_devs;
+	pc->chip.npwm = pc->cdata->pwm_num_devs;
 	pc->chip.can_sleep = true;
 
 	ret = pwmchip_add(&pc->chip);
@@ -426,6 +444,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	for (devnum = 0; devnum < cdata->cpt_num_devs; devnum++) {
+		struct sti_cpt_ddata *ddata;
+
+		ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
+		if (!ddata)
+			return -ENOMEM;
+
+		init_waitqueue_head(&ddata->wait);
+		mutex_init(&ddata->lock);
+
+		pwm_set_chip_data(&pc->chip.pwms[devnum], ddata);
+	}
+
 	platform_set_drvdata(pdev, pc);
 
 	return 0;
@@ -436,7 +467,7 @@ static int sti_pwm_remove(struct platform_device *pdev)
 	struct sti_pwm_chip *pc = platform_get_drvdata(pdev);
 	unsigned int i;
 
-	for (i = 0; i < pc->cdata->num_devs; i++)
+	for (i = 0; i < pc->cdata->pwm_num_devs; i++)
 		pwm_disable(&pc->chip.pwms[i]);
 
 	clk_unprepare(pc->pwm_clk);
-- 
2.8.0

Re: [STLinux Kernel] [[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Peter Griffin]

Hi Lee,

On Fri, 22 Apr 2016, Lee Jones wrote:

> Each PWM Capture device is allocated a structure to hold its own
> state.  During a capture the device may be partaking in one of 3
> phases.  Initial (rising) phase change, a subsequent (falling)
> phase change indicating end of the duty-cycle phase and finally
> a final (rising) phase change indicating the end of the period.
> The timer value snapshot each event is held in a variable of the
> same name, and the phase number (0, 1, 2) is contained in the
> index variable.  Other device specific information, such as GPIO
> pin, the IRQ wait queue and locking is also contained in the
> structure.  This patch initialises this structure for each of
> the available devices.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
>  1 file changed, 38 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 78979d0..9d597bb 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -11,6 +11,7 @@
>   */
>  
>  #include <linux/clk.h>
> +#include <linux/interrupt.h>
>  #include <linux/math64.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/module.h>
> @@ -18,8 +19,10 @@
>  #include <linux/platform_device.h>
>  #include <linux/pwm.h>
>  #include <linux/regmap.h>
> +#include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/time.h>
> +#include <linux/wait.h>
>  
>  #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device's Duty Cycle register */
>  #define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
> @@ -64,9 +67,17 @@ enum sti_cpt_edge {
>  	CPT_EDGE_BOTH,
>  };
>  
> +struct sti_cpt_ddata {
> +	u32 snapshot[3];
> +	int index;
> +	struct mutex lock;
> +	wait_queue_head_t wait;
> +};
> +
>  struct sti_pwm_compat_data {
>  	const struct reg_field *reg_fields;
> -	unsigned int num_devs;
> +	unsigned int pwm_num_devs;
> +	unsigned int cpt_num_devs;
>  	unsigned int max_pwm_cnt;
>  	unsigned int max_prescale;
>  };
> @@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
>  	struct device_node *np = dev->of_node;
>  	struct sti_pwm_compat_data *cdata = pc->cdata;
>  	u32 num_devs;
> +	int ret;
>  
> -	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> -	if (num_devs)
> -		cdata->num_devs = num_devs;
> +	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> +	if (!ret)
> +		cdata->pwm_num_devs = num_devs;

dt binding document documents this as st,pwm-num-chan currently. Why do you need
this  binding anyway? Why can't you determine how many channels you have based on
the number of pinctrl groups you are given?

Also with this series I don't currently understand how the driver is configured to be
pwm-in or pwm-out. Can you explain how that decision is made?

For example at the moment I can't see any code in this series for handling the different
pinctrl groups which presumably are required to have this working.

If I look at pinctrl_pwm1_chan0_default and friends declared in
stih407-pinctrl.dtsi all are currently named pwm-out, and declared as outputs. For
pwm-in functionality I was expecting to see another set of pinctrl definitions,
declaring these pins as inputs, and something in the driver selecting the
correct pin config depending on how the device has been configured?

Maybe an updated dt example / bindings would make this clearer.

regards,

Peter

Re: [STLinux Kernel] [[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Lee Jones]

On Tue, 07 Jun 2016, Peter Griffin wrote:
> On Fri, 22 Apr 2016, Lee Jones wrote:
> 
> > Each PWM Capture device is allocated a structure to hold its own
> > state.  During a capture the device may be partaking in one of 3
> > phases.  Initial (rising) phase change, a subsequent (falling)
> > phase change indicating end of the duty-cycle phase and finally
> > a final (rising) phase change indicating the end of the period.
> > The timer value snapshot each event is held in a variable of the
> > same name, and the phase number (0, 1, 2) is contained in the
> > index variable.  Other device specific information, such as GPIO
> > pin, the IRQ wait queue and locking is also contained in the
> > structure.  This patch initialises this structure for each of
> > the available devices.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> >  1 file changed, 38 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > index 78979d0..9d597bb 100644
> > --- a/drivers/pwm/pwm-sti.c
> > +++ b/drivers/pwm/pwm-sti.c

[...]

> > @@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> >  	struct device_node *np = dev->of_node;
> >  	struct sti_pwm_compat_data *cdata = pc->cdata;
> >  	u32 num_devs;
> > +	int ret;
> >  
> > -	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > -	if (num_devs)
> > -		cdata->num_devs = num_devs;
> > +	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > +	if (!ret)
> > +		cdata->pwm_num_devs = num_devs;
> 
> dt binding document documents this as st,pwm-num-chan currently.

It does, but see PATCH 2.

> Why do you need
> this  binding anyway? Why can't you determine how many channels you have based on
> the number of pinctrl groups you are given?

That sounds like a horrible idea; a) I'm not even sure if that's
possible with the Pinctrl Consumer API and b) even if is it physically
possible, it sounds messy.  It's much better for code to be clear and
simple.  Code attempting to use clever inference techniques is usually
pretty hard to understand and maintain.  We use num-<stuff> a lot in
DT, and for good reason.  Besides, not every PWM channel is capable of
capture.

> Also with this series I don't currently understand how the driver is configured to be
> pwm-in or pwm-out. Can you explain how that decision is made?

This patch-set does not concern itself with the platform specific
side.  I have a subsequent patch-set for that.  Perhaps it might be
nice to move the documentation update into this set though.

> For example at the moment I can't see any code in this series for handling the different
> pinctrl groups which presumably are required to have this working.

To ease your curiosity, here is the patch which does that for the 407:

Author: Lee Jones <lee.jones@linaro.org>
Date:   Wed Feb 3 14:24:01 2016 +0000

    ARM: dts: STiH407: Declare PWM Capture data lines via Pinctrl
    
    Signed-off-by: Lee Jones <lee.jones@linaro.org>

diff --git a/arch/arm/boot/dts/stih407-pinctrl.dtsi b/arch/arm/boot/dts/stih407-pinctrl.dtsi
index a538ae5..bc22122 100644
--- a/arch/arm/boot/dts/stih407-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih407-pinctrl.dtsi
@@ -289,10 +289,12 @@
 			pinctrl_pwm1_chan0_default: pwm1-0-default {
 				st,pins {
 					pwm-out = <&pio3 0 ALT1 OUT>;
+					pwm-capturein = <&pio3 2 ALT1 IN>;
 				};
 			};
 			pinctrl_pwm1_chan1_default: pwm1-1-default {
 				st,pins {
+					pwm-capturein = <&pio4 3 ALT1 IN>;
 					pwm-out = <&pio4 4 ALT1 OUT>;
 				};
 			};
@@ -1030,6 +1032,7 @@
 		pwm0 {
 			pinctrl_pwm0_chan0_default: pwm0-0-default {
 				st,pins {
+					pwm-capturein = <&pio31 0 ALT1 IN>;
 					pwm-out = <&pio31 1 ALT1 OUT>;
 				};
 			};
				
> If I look at pinctrl_pwm1_chan0_default and friends declared in
> stih407-pinctrl.dtsi all are currently named pwm-out, and declared as outputs. For
> pwm-in functionality I was expecting to see another set of pinctrl definitions,
> declaring these pins as inputs, and something in the driver selecting the
> correct pin config depending on how the device has been configured?

See above.

> Maybe an updated dt example / bindings would make this clearer.

Happy to provide the DT documentation in this patch-set.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Peter Griffin]

Hi Lee,

On Tue, 07 Jun 2016, Lee Jones wrote:

> On Tue, 07 Jun 2016, Peter Griffin wrote:
> > On Fri, 22 Apr 2016, Lee Jones wrote:
> > 
> > > Each PWM Capture device is allocated a structure to hold its own
> > > state.  During a capture the device may be partaking in one of 3
> > > phases.  Initial (rising) phase change, a subsequent (falling)
> > > phase change indicating end of the duty-cycle phase and finally
> > > a final (rising) phase change indicating the end of the period.
> > > The timer value snapshot each event is held in a variable of the
> > > same name, and the phase number (0, 1, 2) is contained in the
> > > index variable.  Other device specific information, such as GPIO
> > > pin, the IRQ wait queue and locking is also contained in the
> > > structure.  This patch initialises this structure for each of
> > > the available devices.
> > > 
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > >  drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 38 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > index 78979d0..9d597bb 100644
> > > --- a/drivers/pwm/pwm-sti.c
> > > +++ b/drivers/pwm/pwm-sti.c
> 
> [...]
> 
> > > @@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> > >  	struct device_node *np = dev->of_node;
> > >  	struct sti_pwm_compat_data *cdata = pc->cdata;
> > >  	u32 num_devs;
> > > +	int ret;
> > >  
> > > -	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > -	if (num_devs)
> > > -		cdata->num_devs = num_devs;
> > > +	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > +	if (!ret)
> > > +		cdata->pwm_num_devs = num_devs;
> > 
> > dt binding document documents this as st,pwm-num-chan currently.
> 
> It does, but see PATCH 2.
> 
> > Why do you need
> > this  binding anyway? Why can't you determine how many channels you have based on
> > the number of pinctrl groups you are given?
> 
> That sounds like a horrible idea; a) I'm not even sure if that's
> possible with the Pinctrl Consumer API and 

It would be possible I believe.

> b) even if is it physically
> possible, it sounds messy.  It's much better for code to be clear and
> simple.

I'm not sure encoding the same info in 2 places in the dt node is
clear or simple. Currently you can have a mis-match between the pwm_num_devs
/ cpt_num_devs properties and the pinctrl configuration, and you can't detect
and warn / error if this happens, you just end up with something that doesn't
work.

>  Code attempting to use clever inference techniques is usually
> pretty hard to understand and maintain.

Agreed, currently you are using a inference technique though, that updating
pwm_num_devs / cpt_num_devs properties also requires corresponding updates to
pinctrl-0 and maybe also the actual pin group definitions of
pinctrl_pwm1_chan*_default and friends.

Having pinctrl-names such as 

pwm-in-1
pwm-in-2
pwm-out-1 etc

You just iterate round obtaining pins by name, and create a pwm in/out channel for each
pin group you are given. No nasty inference, plus you don't encode the same
information in two places in the node :)

As a seperate point looking at the current pwm dt nodes in v4.7-rc1, the pinctrl-0 is being set in
stih407-family.dtsi, which is wrong, it should be set in the board specific file.

Also I think the same applies to pwm_num_devs & cpt_num_devs dt properties.
How many pwm channels you have available is determined by the pin muxing which depends
on the board layout, so they should be set in the board file along with the
pinctrl-0 not in stih407-family.dtsi.

>  Besides, not every PWM channel is capable of
> capture.

OK, also currently the driver defaults to 0 if cpt_num_devs property is not supplied.

So it would make sense to only obtain & enable capture clock and capture irq
if cpt_num_devs >0?

Currently the code will error if capture clock and capture irq is not provided,
and enable capture clock even if no capture channels are being used.

> 
> > Also with this series I don't currently understand how the driver is configured to be
> > pwm-in or pwm-out. Can you explain how that decision is made?
> 
> This patch-set does not concern itself with the platform specific
> side.  I have a subsequent patch-set for that.  Perhaps it might be
> nice to move the documentation update into this set though.

It would definately be nice to update the dt documentation and code in lockstep.

Also IMHO the platform specific side should be included in this series,
because, you are changing the st,pwm-num-chan binding which will break the currently
merged pwm dt nodes. That change should ideally be changed as an atomic commit, so
we always have dt that matches the driver code.

> 
> > For example at the moment I can't see any code in this series for handling the different
> > pinctrl groups which presumably are required to have this working.
> 
> To ease your curiosity, here is the patch which does that for the 407:

OK thanks, that makes more sense knowing that pwm-in and pwm-out are different
pins so you don't need to support changing the in/out config on the fly.

fyi without the dt doc update or the corresponding platform side dt changes,
it does makes it harder to review the pwm-st driver parts of the
series.

regards,

Peter.

Re: [STLinux Kernel] [[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Lee Jones]

On Tue, 07 Jun 2016, Peter Griffin wrote:
> On Tue, 07 Jun 2016, Lee Jones wrote:
> > On Tue, 07 Jun 2016, Peter Griffin wrote:
> > > On Fri, 22 Apr 2016, Lee Jones wrote:
> > > 
> > > > Each PWM Capture device is allocated a structure to hold its own
> > > > state.  During a capture the device may be partaking in one of 3
> > > > phases.  Initial (rising) phase change, a subsequent (falling)
> > > > phase change indicating end of the duty-cycle phase and finally
> > > > a final (rising) phase change indicating the end of the period.
> > > > The timer value snapshot each event is held in a variable of the
> > > > same name, and the phase number (0, 1, 2) is contained in the
> > > > index variable.  Other device specific information, such as GPIO
> > > > pin, the IRQ wait queue and locking is also contained in the
> > > > structure.  This patch initialises this structure for each of
> > > > the available devices.
> > > > 
> > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > ---
> > > >  drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 38 insertions(+), 7 deletions(-)
> > > > 
> > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > index 78979d0..9d597bb 100644
> > > > --- a/drivers/pwm/pwm-sti.c
> > > > +++ b/drivers/pwm/pwm-sti.c
> > 
> > [...]
> > 
> > > > @@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> > > >  	struct device_node *np = dev->of_node;
> > > >  	struct sti_pwm_compat_data *cdata = pc->cdata;
> > > >  	u32 num_devs;
> > > > +	int ret;
> > > >  
> > > > -	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > > -	if (num_devs)
> > > > -		cdata->num_devs = num_devs;
> > > > +	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > > +	if (!ret)
> > > > +		cdata->pwm_num_devs = num_devs;
> > > 
> > > dt binding document documents this as st,pwm-num-chan currently.
> > 
> > It does, but see PATCH 2.
> > 
> > > Why do you need
> > > this  binding anyway? Why can't you determine how many channels you have based on
> > > the number of pinctrl groups you are given?
> > 
> > That sounds like a horrible idea; a) I'm not even sure if that's
> > possible with the Pinctrl Consumer API and 
> 
> It would be possible I believe.

Okay, so I paid this idea some lip service despite thinking that it's
crazy.  After looking at the API, I couldn't see anything suitable to
achieve what you are suggesting, so I had a conversation with the
Pinctrl maintainer who confirmed my thoughts.  In theory you could
write a DT parser to hop around DT by phandle to find the information
you're looking for, but it would be very complex and non-generic.  Way
more trouble than it would ever be worth.

> > b) even if is it physically
> > possible, it sounds messy.  It's much better for code to be clear and
> > simple.
> 
> I'm not sure encoding the same info in 2 places in the dt node is
> clear or simple. Currently you can have a mis-match between the pwm_num_devs
> / cpt_num_devs properties and the pinctrl configuration, and you can't detect
> and warn / error if this happens, you just end up with something that doesn't
> work.

I kinda see where you're coming from, but parsing the Pinctrl
configuration to derive device information is not the way to go.
Pinctrl's only aim is to simplify pin configuration (function,
direction, etc), not to start describing how many channels a device
has.

By the way, what do you mean that there is a current mismatch?

> >  Code attempting to use clever inference techniques is usually
> > pretty hard to understand and maintain.
> 
> Agreed, currently you are using a inference technique though, that updating
> pwm_num_devs / cpt_num_devs properties also requires corresponding updates to
> pinctrl-0 and maybe also the actual pin group definitions of
> pinctrl_pwm1_chan*_default and friends.

We currently treat pin configuration and device properties separately,
and rightly so in my opinion.  If you wish this to change, I'm sure
the Pinctrl maintainer will accept sensible patches to add this
functionality to the framework.  However today, this is not possible.

> Having pinctrl-names such as 
> 
> pwm-in-1
> pwm-in-2
> pwm-out-1 etc
> 
> You just iterate round obtaining pins by name, and create a pwm in/out channel for each
> pin group you are given. No nasty inference, plus you don't encode the same
> information in two places in the node :)

You can not obtain a pin by name currently, and I can't think of a
sane reason why you would want to.  It might solve very small corner
cases like this one, but in the real world, it's easier to solve them
with a '*-num-*' type property like we do for everything else:

  git grep num -- arch/arm/boot/dts/

> As a seperate point looking at the current pwm dt nodes in v4.7-rc1, the pinctrl-0 is being set in
> stih407-family.dtsi, which is wrong, it should be set in the board specific file.

The idea of the stih407-family board file is to cut down on
duplication.  To achieve this that file should contain everything
which each the STiH410 and the STiH407 has in common.  If there are
differences between the two platforms' PWM IP, then yes, I agree.  I
can take a look at that.

> Also I think the same applies to pwm_num_devs & cpt_num_devs dt properties.

Only if they differ.

> How many pwm channels you have available is determined by the pin muxing which depends
> on the board layout, so they should be set in the board file along with the
> pinctrl-0 not in stih407-family.dtsi.

I disagree with this point.  The number of channels the device
supports and the amount of pins set-up on the board are orthogonal.

> >  Besides, not every PWM channel is capable of
> > capture.
> 
> OK, also currently the driver defaults to 0 if cpt_num_devs property is not supplied.
> 
> So it would make sense to only obtain & enable capture clock and capture irq
> if cpt_num_devs >0?
> 
> Currently the code will error if capture clock and capture irq is not provided,
> and enable capture clock even if no capture channels are being used.

Good point.  Will fix.

> > > Also with this series I don't currently understand how the driver is configured to be
> > > pwm-in or pwm-out. Can you explain how that decision is made?
> > 
> > This patch-set does not concern itself with the platform specific
> > side.  I have a subsequent patch-set for that.  Perhaps it might be
> > nice to move the documentation update into this set though.
> 
> It would definately be nice to update the dt documentation and code in lockstep.
> 
> Also IMHO the platform specific side should be included in this series,
> because, you are changing the st,pwm-num-chan binding which will break the currently
> merged pwm dt nodes. That change should ideally be changed as an atomic commit, so
> we always have dt that matches the driver code.
> 
> > > For example at the moment I can't see any code in this series for handling the different
> > > pinctrl groups which presumably are required to have this working.
> > 
> > To ease your curiosity, here is the patch which does that for the 407:
> 
> OK thanks, that makes more sense knowing that pwm-in and pwm-out are different
> pins so you don't need to support changing the in/out config on the fly.
> 
> fyi without the dt doc update or the corresponding platform side dt changes,
> it does makes it harder to review the pwm-st driver parts of the
> series.

Very well.  In the next submission I will supply the entire set.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [STLinux Kernel] [[PATCH v2] 08/11] pwm: sti: Initialise PWM Capture device data

[Peter Griffin]

Hi Lee,

On Tue, 07 Jun 2016, Lee Jones wrote:

> On Tue, 07 Jun 2016, Peter Griffin wrote:
> > On Tue, 07 Jun 2016, Lee Jones wrote:
> > > On Tue, 07 Jun 2016, Peter Griffin wrote:
> > > > On Fri, 22 Apr 2016, Lee Jones wrote:
> > > > 
> > > > > Each PWM Capture device is allocated a structure to hold its own
> > > > > state.  During a capture the device may be partaking in one of 3
> > > > > phases.  Initial (rising) phase change, a subsequent (falling)
> > > > > phase change indicating end of the duty-cycle phase and finally
> > > > > a final (rising) phase change indicating the end of the period.
> > > > > The timer value snapshot each event is held in a variable of the
> > > > > same name, and the phase number (0, 1, 2) is contained in the
> > > > > index variable.  Other device specific information, such as GPIO
> > > > > pin, the IRQ wait queue and locking is also contained in the
> > > > > structure.  This patch initialises this structure for each of
> > > > > the available devices.
> > > > > 
> > > > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > > > ---
> > > > >  drivers/pwm/pwm-sti.c | 45 ++++++++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 38 insertions(+), 7 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > > > > index 78979d0..9d597bb 100644
> > > > > --- a/drivers/pwm/pwm-sti.c
> > > > > +++ b/drivers/pwm/pwm-sti.c
> > > 
> > > [...]
> > > 
> > > > > @@ -307,10 +318,15 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
> > > > >  	struct device_node *np = dev->of_node;
> > > > >  	struct sti_pwm_compat_data *cdata = pc->cdata;
> > > > >  	u32 num_devs;
> > > > > +	int ret;
> > > > >  
> > > > > -	of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > > > -	if (num_devs)
> > > > > -		cdata->num_devs = num_devs;
> > > > > +	ret = of_property_read_u32(np, "st,pwm-num-devs", &num_devs);
> > > > > +	if (!ret)
> > > > > +		cdata->pwm_num_devs = num_devs;
> > > > 
> > > > dt binding document documents this as st,pwm-num-chan currently.
> > > 
> > > It does, but see PATCH 2.
> > > 
> > > > Why do you need
> > > > this  binding anyway? Why can't you determine how many channels you have based on
> > > > the number of pinctrl groups you are given?
> > > 
> > > That sounds like a horrible idea; a) I'm not even sure if that's
> > > possible with the Pinctrl Consumer API and 
> > 
> > It would be possible I believe.
> 
> Okay, so I paid this idea some lip service despite thinking that it's
> crazy.

Thanks :-)

>  After looking at the API, I couldn't see anything suitable to
> achieve what you are suggesting, so I had a conversation with the
> Pinctrl maintainer who confirmed my thoughts.  In theory you could
> write a DT parser to hop around DT by phandle to find the information
> you're looking for, but it would be very complex and non-generic.  Way
> more trouble than it would ever be worth.

I thought it was possible to retrieve a group of pins by name. I think it
must have been the pinctrl_lookup_state() API I was thinking of.

> 
> > > b) even if is it physically
> > > possible, it sounds messy.  It's much better for code to be clear and
> > > simple.
> > 
> > I'm not sure encoding the same info in 2 places in the dt node is
> > clear or simple. Currently you can have a mis-match between the pwm_num_devs
> > / cpt_num_devs properties and the pinctrl configuration, and you can't detect
> > and warn / error if this happens, you just end up with something that doesn't
> > work.
> 
> I kinda see where you're coming from, but parsing the Pinctrl
> configuration to derive device information is not the way to go.
> Pinctrl's only aim is to simplify pin configuration (function,
> direction, etc), not to start describing how many channels a device
> has.

The number of useable channels the device has in this case is very closely linked with
the pin config though (see below).
> 
> By the way, what do you mean that there is a current mismatch?

It is possible to have a incoherent configuration whereby pwm_num_devs and
cpt_num_devs do not match up with the big list of pin groups you are providing
in pinctrl-0. Anyways it isn't really an issue if the pinctrl API doesn't exist to
allow you to do what I proposed.

> 
> > >  Code attempting to use clever inference techniques is usually
> > > pretty hard to understand and maintain.
> > 
> > Agreed, currently you are using a inference technique though, that updating
> > pwm_num_devs / cpt_num_devs properties also requires corresponding updates to
> > pinctrl-0 and maybe also the actual pin group definitions of
> > pinctrl_pwm1_chan*_default and friends.
> 
> We currently treat pin configuration and device properties separately,
> and rightly so in my opinion.  If you wish this to change, I'm sure
> the Pinctrl maintainer will accept sensible patches to add this
> functionality to the framework.  However today, this is not possible.
> 
> > Having pinctrl-names such as 
> > 
> > pwm-in-1
> > pwm-in-2
> > pwm-out-1 etc
> > 
> > You just iterate round obtaining pins by name, and create a pwm in/out channel for each
> > pin group you are given. No nasty inference, plus you don't encode the same
> > information in two places in the node :)
> 
> You can not obtain a pin by name currently, and I can't think of a
> sane reason why you would want to.  It might solve very small corner
> cases like this one, but in the real world, it's easier to solve them
> with a '*-num-*' type property like we do for everything else:
> 
>   git grep num -- arch/arm/boot/dts/

Looking through that grep, I can't see many examples where the '*-num-*' dt property
is directly linked to the number and amount of pins which should be configured.

The best example I can find is sd / emmc where <bus-width> describes the bus
width and the pinctrl-0 needs to correspond accordingly with the correct number
of pins. I guess this is similar.

> 
> > As a seperate point looking at the current pwm dt nodes in v4.7-rc1, the pinctrl-0 is being set in
> > stih407-family.dtsi, which is wrong, it should be set in the board specific file.
> 
> The idea of the stih407-family board file is to cut down on
> duplication.  To achieve this that file should contain everything
> which each the STiH410 and the STiH407 has in common.

It should contain everything which is common between these two SoC's which is
not board dependent. Things which vary due to board design need to live in
either stihxxx-b2120 if common between both SoC's and b2120 boards (like this will
be) or e.g. stih410-b2120.dts if it is more specific again to  a specific SoC and
board. Otherwise we will hit problems when adding new board variants in the
future.

>  If there are
> differences between the two platforms' PWM IP, then yes, I agree.  I
> can take a look at that.

> > Also I think the same applies to pwm_num_devs & cpt_num_devs dt properties.
> 
> Only if they differ.
> 
> > How many pwm channels you have available is determined by the pin muxing which depends
> > on the board layout, so they should be set in the board file along with the
> > pinctrl-0 not in stih407-family.dtsi.
> 
> I disagree with this point.  The number of channels the device
> supports and the amount of pins set-up on the board are orthogonal.

I guess this is the crux of the disagreement.

What happens in the pwm driver when you have more pwm channels declared in
pwm_num_devs & cpt_num_devs than physcially exist on the board?

Presumably at best you get garbage results for the pwm channels which
aren't connected to anything?

<snip>
> > 
> > > > For example at the moment I can't see any code in this series for handling the different
> > > > pinctrl groups which presumably are required to have this working.
> > > 
> > > To ease your curiosity, here is the patch which does that for the 407:
> > 
> > OK thanks, that makes more sense knowing that pwm-in and pwm-out are different
> > pins so you don't need to support changing the in/out config on the fly.
> > 
> > fyi without the dt doc update or the corresponding platform side dt changes,
> > it does makes it harder to review the pwm-st driver parts of the
> > series.
> 
> Very well.  In the next submission I will supply the entire set.
> 

ta, Regards,

Peter.

[[PATCH v2] 09/11] pwm: sti: Add support for PWM Capture IRQs

[Lee Jones]

Here we're requesting the PWM Capture IRQ and supplying the
handler which will be called in the event of an IRQ fire to
handle it.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 91 insertions(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 9d597bb..2230afb 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -91,7 +91,9 @@ struct sti_pwm_chip {
 	struct regmap_field *prescale_low;
 	struct regmap_field *prescale_high;
 	struct regmap_field *pwm_out_en;
+	struct regmap_field *pwm_cpt_en;
 	struct regmap_field *pwm_cpt_int_en;
+	struct regmap_field *pwm_cpt_int_stat;
 	struct pwm_chip chip;
 	struct pwm_device *cur;
 	unsigned long configured;
@@ -311,6 +313,76 @@ static const struct pwm_ops sti_pwm_ops = {
 	.owner = THIS_MODULE,
 };
 
+static irqreturn_t sti_pwm_interrupt(int irq, void *data)
+{
+	struct sti_pwm_chip *pc = data;
+	struct device *dev = pc->dev;
+	struct sti_cpt_ddata *ddata;
+	int devicenum;
+	unsigned int cpt_int_stat;
+	unsigned int reg;
+	int ret = IRQ_NONE;
+
+	ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
+	if (ret)
+		return ret;
+
+	while (cpt_int_stat) {
+		devicenum = ffs(cpt_int_stat) - 1;
+
+		ddata = pwm_get_chip_data(&pc->chip.pwms[devicenum]);
+
+		/*
+		 * Capture input:
+		 *    _______                   _______
+		 *   |       |                 |       |
+		 * __|       |_________________|       |________
+		 *   ^0      ^1                ^2
+		 *
+		 * Capture start by the first available rising edge
+		 * When a capture event occurs, capture value (CPT_VALx)
+		 * is stored, index incremented, capture edge changed.
+		 *
+		 * After the capture, if the index > 1, we have collected
+		 * the necessary data so we signal the thread waiting for it
+		 * and disable the capture by setting capture edge to none
+		 *
+		 */
+
+		regmap_read(pc->regmap,
+			    PWM_CPT_VAL(devicenum),
+			    &ddata->snapshot[ddata->index]);
+
+		switch (ddata->index) {
+		case 0:
+		case 1:
+			regmap_read(pc->regmap, PWM_CPT_EDGE(devicenum), &reg);
+			reg ^= PWM_CPT_EDGE_MASK;
+			regmap_write(pc->regmap, PWM_CPT_EDGE(devicenum), reg);
+
+			ddata->index++;
+			break;
+		case 2:
+			regmap_write(pc->regmap,
+				     PWM_CPT_EDGE(devicenum),
+				     CPT_EDGE_DISABLED);
+			wake_up(&ddata->wait);
+			break;
+		default:
+			dev_err(dev, "Internal error\n");
+		}
+
+		cpt_int_stat &= ~BIT_MASK(devicenum);
+
+		ret = IRQ_HANDLED;
+	}
+
+	/* Just ACK everything */
+	regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
+
+	return ret;
+}
+
 static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 {
 	struct device *dev = pc->dev;
@@ -351,6 +423,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->pwm_cpt_int_en))
 		return PTR_ERR(pc->pwm_cpt_int_en);
 
+	pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
+						reg_fields[PWM_CPT_INT_STAT]);
+	if (PTR_ERR_OR_ZERO(pc->pwm_cpt_int_stat))
+		return PTR_ERR(pc->pwm_cpt_int_stat);
+
 	return 0;
 }
 
@@ -367,7 +444,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	struct sti_pwm_chip *pc;
 	struct resource *res;
 	unsigned int devnum;
-	int ret;
+	int irq, ret;
 
 	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
 	if (!pc)
@@ -388,6 +465,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	if (IS_ERR(pc->regmap))
 		return PTR_ERR(pc->regmap);
 
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
+		return irq;
+	}
+
+	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
+			       0, pdev->name, pc);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Failed to request IRQ\n");
+		return ret;
+	}
+
 	/*
 	 * Setup PWM data with default values: some values could be replaced
 	 * with specific ones provided from Device Tree.
-- 
2.8.0

Re: [STLinux Kernel] [[PATCH v2] 09/11] pwm: sti: Add support for PWM Capture IRQs

[Peter Griffin]

Hi Lee,

On Fri, 22 Apr 2016, Lee Jones wrote:

> Here we're requesting the PWM Capture IRQ and supplying the
> handler which will be called in the event of an IRQ fire to
> handle it.
> 
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
>  drivers/pwm/pwm-sti.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 91 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> index 9d597bb..2230afb 100644
> --- a/drivers/pwm/pwm-sti.c
> +++ b/drivers/pwm/pwm-sti.c
> @@ -91,7 +91,9 @@ struct sti_pwm_chip {
>  	struct regmap_field *prescale_low;
>  	struct regmap_field *prescale_high;
>  	struct regmap_field *pwm_out_en;
> +	struct regmap_field *pwm_cpt_en;
>  	struct regmap_field *pwm_cpt_int_en;
> +	struct regmap_field *pwm_cpt_int_stat;
>  	struct pwm_chip chip;
>  	struct pwm_device *cur;
>  	unsigned long configured;
> @@ -311,6 +313,76 @@ static const struct pwm_ops sti_pwm_ops = {
>  	.owner = THIS_MODULE,
>  };
>  
> +static irqreturn_t sti_pwm_interrupt(int irq, void *data)
> +{
> +	struct sti_pwm_chip *pc = data;
> +	struct device *dev = pc->dev;
> +	struct sti_cpt_ddata *ddata;
> +	int devicenum;
> +	unsigned int cpt_int_stat;
> +	unsigned int reg;
> +	int ret = IRQ_NONE;
> +
> +	ret = regmap_field_read(pc->pwm_cpt_int_stat, &cpt_int_stat);
> +	if (ret)
> +		return ret;
> +
> +	while (cpt_int_stat) {
> +		devicenum = ffs(cpt_int_stat) - 1;
> +
> +		ddata = pwm_get_chip_data(&pc->chip.pwms[devicenum]);
> +
> +		/*
> +		 * Capture input:
> +		 *    _______                   _______
> +		 *   |       |                 |       |
> +		 * __|       |_________________|       |________
> +		 *   ^0      ^1                ^2
> +		 *
> +		 * Capture start by the first available rising edge
> +		 * When a capture event occurs, capture value (CPT_VALx)
> +		 * is stored, index incremented, capture edge changed.
> +		 *
> +		 * After the capture, if the index > 1, we have collected
> +		 * the necessary data so we signal the thread waiting for it
> +		 * and disable the capture by setting capture edge to none
> +		 *
> +		 */
> +
> +		regmap_read(pc->regmap,
> +			    PWM_CPT_VAL(devicenum),
> +			    &ddata->snapshot[ddata->index]);
> +
> +		switch (ddata->index) {
> +		case 0:
> +		case 1:
> +			regmap_read(pc->regmap, PWM_CPT_EDGE(devicenum), &reg);
> +			reg ^= PWM_CPT_EDGE_MASK;
> +			regmap_write(pc->regmap, PWM_CPT_EDGE(devicenum), reg);
> +
> +			ddata->index++;
> +			break;
> +		case 2:
> +			regmap_write(pc->regmap,
> +				     PWM_CPT_EDGE(devicenum),
> +				     CPT_EDGE_DISABLED);
> +			wake_up(&ddata->wait);
> +			break;
> +		default:
> +			dev_err(dev, "Internal error\n");
> +		}
> +
> +		cpt_int_stat &= ~BIT_MASK(devicenum);
> +
> +		ret = IRQ_HANDLED;
> +	}
> +
> +	/* Just ACK everything */
> +	regmap_write(pc->regmap, PWM_INT_ACK, PWM_INT_ACK_MASK);
> +
> +	return ret;
> +}
> +
>  static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
>  {
>  	struct device *dev = pc->dev;
> @@ -351,6 +423,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
>  	if (IS_ERR(pc->pwm_cpt_int_en))
>  		return PTR_ERR(pc->pwm_cpt_int_en);
>  
> +	pc->pwm_cpt_int_stat = devm_regmap_field_alloc(dev, pc->regmap,
> +						reg_fields[PWM_CPT_INT_STAT]);
> +	if (PTR_ERR_OR_ZERO(pc->pwm_cpt_int_stat))
> +		return PTR_ERR(pc->pwm_cpt_int_stat);
> +
>  	return 0;
>  }
>  
> @@ -367,7 +444,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	struct sti_pwm_chip *pc;
>  	struct resource *res;
>  	unsigned int devnum;
> -	int ret;
> +	int irq, ret;
>  
>  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
>  	if (!pc)
> @@ -388,6 +465,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
>  	if (IS_ERR(pc->regmap))
>  		return PTR_ERR(pc->regmap);
>  
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
> +			       0, pdev->name, pc);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "Failed to request IRQ\n");
> +		return ret;
> +	}
> +
>  	/*
>  	 * Setup PWM data with default values: some values could be replaced
>  	 * with specific ones provided from Device Tree.

pwm-st.txt dt binding document and pwm example needs updating to document this
irq.

regards,

Peter

Re: [STLinux Kernel] [[PATCH v2] 09/11] pwm: sti: Add support for PWM Capture IRQs

[Lee Jones]

On Tue, 07 Jun 2016, Peter Griffin wrote:

> Hi Lee,
> 
> On Fri, 22 Apr 2016, Lee Jones wrote:
> 
> > Here we're requesting the PWM Capture IRQ and supplying the
> > handler which will be called in the event of an IRQ fire to
> > handle it.
> > 
> > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > ---
> >  drivers/pwm/pwm-sti.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 91 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
> > index 9d597bb..2230afb 100644
> > --- a/drivers/pwm/pwm-sti.c
> > +++ b/drivers/pwm/pwm-sti.c

[...]

Please cut any large chunks of unnecessary cruft when reviewing.

> > @@ -367,7 +444,7 @@ static int sti_pwm_probe(struct platform_device *pdev)
> >  	struct sti_pwm_chip *pc;
> >  	struct resource *res;
> >  	unsigned int devnum;
> > -	int ret;
> > +	int irq, ret;
> >  
> >  	pc = devm_kzalloc(dev, sizeof(*pc), GFP_KERNEL);
> >  	if (!pc)
> > @@ -388,6 +465,19 @@ static int sti_pwm_probe(struct platform_device *pdev)
> >  	if (IS_ERR(pc->regmap))
> >  		return PTR_ERR(pc->regmap);
> >  
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0) {
> > +		dev_err(&pdev->dev, "Failed to obtain IRQ\n");
> > +		return irq;
> > +	}
> > +
> > +	ret = devm_request_irq(&pdev->dev, irq, sti_pwm_interrupt,
> > +			       0, pdev->name, pc);
> > +	if (ret < 0) {
> > +		dev_err(&pdev->dev, "Failed to request IRQ\n");
> > +		return ret;
> > +	}
> > +
> >  	/*
> >  	 * Setup PWM data with default values: some values could be replaced
> >  	 * with specific ones provided from Device Tree.
> 
> pwm-st.txt dt binding document and pwm example needs updating to document this
> irq.

Yes, it does.  I will update the documentation and place it into this
patch-set on next submission. 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[[PATCH v2] 10/11] pwm: sti: Add PWM Capture call-back

[Lee Jones]

Once a PWM Capture has been initiated, the capture call
enables a rising edge detection IRQ, then waits.  Once each
of the 3 phase changes have been recorded the thread then
wakes.  The remaining part of the call carries out the
relevant calculations and passes back a formatted string to
the caller.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 88 insertions(+)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index 2230afb..cecb6d4 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -24,6 +24,8 @@
 #include <linux/time.h>
 #include <linux/wait.h>
 
+#define SECS_TO_NANOSECS(x) ((x) * 1000 * 1000 * 1000)
+
 #define PWM_OUT_VAL(x)	(0x00 + (4 * (x))) /* Device's Duty Cycle register */
 #define PWM_CPT_VAL(x)	(0x10 + (4 * (x))) /* Capture value */
 #define PWM_CPT_EDGE(x) (0x30 + (4 * (x))) /* Edge to capture on */
@@ -305,7 +307,88 @@ static void sti_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm)
 	clear_bit(pwm->hwpwm, &pc->configured);
 }
 
+static int sti_pwm_capture(struct pwm_chip *chip, struct pwm_device *pwm,
+			   struct pwm_capture *result, unsigned int timeout_ms)
+{
+	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
+	struct sti_pwm_compat_data *cdata = pc->cdata;
+	struct sti_cpt_ddata *ddata = pwm_get_chip_data(pwm);
+	struct device *dev = pc->dev;
+	unsigned int effective_ticks;
+	unsigned long long high, low;
+	int ret;
+
+	if (pwm->hwpwm > cdata->cpt_num_devs - 1) {
+		dev_err(dev, "Device %d is not valid\n", pwm->hwpwm);
+		return -EINVAL;
+	}
+
+	mutex_lock(&ddata->lock);
+
+	/* Prepare capture measurement */
+	ddata->index = 0;
+	regmap_write(pc->regmap, PWM_CPT_EDGE(pwm->hwpwm), CPT_EDGE_RISING);
+	regmap_field_write(pc->pwm_cpt_int_en, BIT(pwm->hwpwm));
+
+	/* Enable capture */
+	ret = regmap_field_write(pc->pwm_cpt_en, 1);
+	if (ret) {
+		dev_err(dev, "failed to enable PWM capture %d\n", pwm->hwpwm);
+		goto out;
+	}
+
+	ret = wait_event_interruptible_timeout(ddata->wait,
+					       ddata->index > 1,
+					       msecs_to_jiffies(timeout_ms));
+
+	regmap_write(pc->regmap, PWM_CPT_EDGE(pwm->hwpwm), CPT_EDGE_DISABLED);
+
+	if (ret == -ERESTARTSYS)
+		goto out;
+
+	switch (ddata->index) {
+	case 0:
+	case 1:
+		/*
+		 * Getting here could mean :
+		 *  - input signal is constant of less than 1Hz
+		 *  - there is no input signal at all
+		 *
+		 * In such case the frequency is rounded down to 0
+		 */
+
+		result->period = 0;
+		result->duty_cycle = 0;
+
+		break;
+	case 2:
+		/* We have everying we need */
+		high = ddata->snapshot[1] - ddata->snapshot[0];
+		low  = ddata->snapshot[2] - ddata->snapshot[1];
+
+		effective_ticks = clk_get_rate(pc->cpt_clk);
+
+		result->period = SECS_TO_NANOSECS(high + low);
+		do_div(result->period, effective_ticks);
+
+		result->duty_cycle = SECS_TO_NANOSECS(high);
+		do_div(result->duty_cycle, effective_ticks);
+
+		break;
+	default:
+		dev_err(dev, "Internal error\n");
+	}
+
+out:
+	/* Disable capture */
+	regmap_field_write(pc->pwm_cpt_en, 0);
+
+	mutex_unlock(&ddata->lock);
+	return ret;
+}
+
 static const struct pwm_ops sti_pwm_ops = {
+	.capture = sti_pwm_capture,
 	.config = sti_pwm_config,
 	.enable = sti_pwm_enable,
 	.disable = sti_pwm_disable,
@@ -418,6 +501,11 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 	if (IS_ERR(pc->pwm_out_en))
 		return PTR_ERR(pc->pwm_out_en);
 
+	pc->pwm_cpt_en = devm_regmap_field_alloc(dev, pc->regmap,
+						 reg_fields[PWM_CPT_EN]);
+	if (IS_ERR(pc->pwm_cpt_en))
+		return PTR_ERR(pc->pwm_cpt_en);
+
 	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
 						 reg_fields[PWM_CPT_INT_EN]);
 	if (IS_ERR(pc->pwm_cpt_int_en))
-- 
2.8.0

[[PATCH v2] 11/11] pwm: sti: Take the opportunity to conduct a little house keeping

[Lee Jones]

This includes fixing some Coding Style issues and re-ordering/
simplifying a little code.

Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
 drivers/pwm/pwm-sti.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-sti.c b/drivers/pwm/pwm-sti.c
index cecb6d4..ac37973 100644
--- a/drivers/pwm/pwm-sti.c
+++ b/drivers/pwm/pwm-sti.c
@@ -1,8 +1,10 @@
 /*
- * PWM device driver for ST SoCs.
- * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ * PWM device driver for ST SoCs
+ *
+ * Copyright (C) 2013-2016 STMicroelectronics (R&D) Limited
  *
- * Copyright (C) 2013-2014 STMicroelectronics (R&D) Limited
+ * Author: Ajit Pal Singh <ajitpal.singh@st.com>
+ *         Lee Jones <lee.jones@linaro.org>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -142,13 +144,13 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
 	val = NSEC_PER_SEC / clk_rate;
 	val *= cdata->max_pwm_cnt + 1;
 
-	if (period % val) {
+	if (period % val)
 		return -EINVAL;
-	} else {
-		ps  = period / val - 1;
-		if (ps > cdata->max_prescale)
-			return -EINVAL;
-	}
+
+	ps  = period / val - 1;
+	if (ps > cdata->max_prescale)
+		return -EINVAL;
+
 	*prescale = ps;
 
 	return 0;
@@ -164,7 +166,7 @@ static int sti_pwm_get_prescale(struct sti_pwm_chip *pc, unsigned long period,
  * 256 values.
  */
 static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			 int duty_ns, int period_ns)
+			int duty_ns, int period_ns)
 {
 	struct sti_pwm_chip *pc = to_sti_pwmchip(chip);
 	struct sti_pwm_compat_data *cdata = pc->cdata;
@@ -210,7 +212,7 @@ static int sti_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 			ret =
 			regmap_field_write(pc->prescale_low,
-					   prescale & PWM_PRESCALE_LOW_MASK);
+				prescale & PWM_PRESCALE_LOW_MASK);
 			if (ret)
 				goto clk_dis;
 
@@ -273,7 +275,7 @@ static int sti_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 		ret = regmap_field_write(pc->pwm_out_en, 1);
 		if (ret) {
-			dev_err(dev, "failed to enable PWM device:%d\n",
+			dev_err(dev, "failed to enable PWM device %d\n",
 				pwm->hwpwm);
 			goto out;
 		}
@@ -293,10 +295,12 @@ static void sti_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 		mutex_unlock(&pc->sti_pwm_lock);
 		return;
 	}
+
 	regmap_field_write(pc->pwm_out_en, 0);
 
 	clk_disable(pc->pwm_clk);
 	clk_disable(pc->cpt_clk);
+
 	mutex_unlock(&pc->sti_pwm_lock);
 }
 
@@ -507,7 +511,7 @@ static int sti_pwm_probe_dt(struct sti_pwm_chip *pc)
 		return PTR_ERR(pc->pwm_cpt_en);
 
 	pc->pwm_cpt_int_en = devm_regmap_field_alloc(dev, pc->regmap,
-						 reg_fields[PWM_CPT_INT_EN]);
+						reg_fields[PWM_CPT_INT_EN]);
 	if (IS_ERR(pc->pwm_cpt_int_en))
 		return PTR_ERR(pc->pwm_cpt_int_en);
 
@@ -570,11 +574,11 @@ static int sti_pwm_probe(struct platform_device *pdev)
 	 * Setup PWM data with default values: some values could be replaced
 	 * with specific ones provided from Device Tree.
 	 */
-	cdata->reg_fields   = &sti_pwm_regfields[0];
-	cdata->max_prescale = 0xff;
-	cdata->max_pwm_cnt  = 255;
-	cdata->pwm_num_devs = 1;
-	cdata->cpt_num_devs = 0;
+	cdata->reg_fields	= &sti_pwm_regfields[0];
+	cdata->max_prescale	= 0xff;
+	cdata->max_pwm_cnt	= 255;
+	cdata->pwm_num_devs	= 1;
+	cdata->cpt_num_devs	= 0;
 
 	pc->cdata = cdata;
 	pc->dev = dev;
-- 
2.8.0

Re: [PATCH v2 00/11] pwm: Add support for PWM Capture

[Boris Brezillon]

Hi Lee,

On Fri, 22 Apr 2016 11:18:04 +0100
Lee Jones <lee.jones@linaro.org> wrote:

> The first part of this set extends the current PWM API to allow external
> code to request a PWM Capture.  Subsequent patches then make use of the
> new API by providing a userspace offering via /sysfs.  The final part of
> the set supplies PWM Capture functionality into the already existing STi
> PWM driver.

Is there a reason you decided to not put this driver in IIO? IMHO, it
would be more appropriate to make your PWM device an MFD that can either
bind to the PWM or the capture driver.
And BTW, IIO already has a sysfs interface (you may have to extend the
API to support your type of capture though).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 00/11] pwm: Add support for PWM Capture

[Lee Jones]

On Fri, 29 Apr 2016, Boris Brezillon wrote:

> Hi Lee,
> 
> On Fri, 22 Apr 2016 11:18:04 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > The first part of this set extends the current PWM API to allow external
> > code to request a PWM Capture.  Subsequent patches then make use of the
> > new API by providing a userspace offering via /sysfs.  The final part of
> > the set supplies PWM Capture functionality into the already existing STi
> > PWM driver.
> 
> Is there a reason you decided to not put this driver in IIO? IMHO, it
> would be more appropriate to make your PWM device an MFD that can either
> bind to the PWM or the capture driver.
> And BTW, IIO already has a sysfs interface (you may have to extend the
> API to support your type of capture though).

Multi-Function Device drivers can only be justified if the IP
contained does not and can not live in a single subsystem.  The IP
which controls both PWM-in and PWM-out in this device is the same.  I
can't fathom a sane reason why you would wish to separate this
functionality over multiple subsystems.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2 00/11] pwm: Add support for PWM Capture

[Boris Brezillon]

On Mon, 6 Jun 2016 16:32:31 +0100
Lee Jones <lee.jones@linaro.org> wrote:

> On Fri, 29 Apr 2016, Boris Brezillon wrote:
> 
> > Hi Lee,
> > 
> > On Fri, 22 Apr 2016 11:18:04 +0100
> > Lee Jones <lee.jones@linaro.org> wrote:
> >   
> > > The first part of this set extends the current PWM API to allow external
> > > code to request a PWM Capture.  Subsequent patches then make use of the
> > > new API by providing a userspace offering via /sysfs.  The final part of
> > > the set supplies PWM Capture functionality into the already existing STi
> > > PWM driver.  
> > 
> > Is there a reason you decided to not put this driver in IIO? IMHO, it
> > would be more appropriate to make your PWM device an MFD that can either
> > bind to the PWM or the capture driver.
> > And BTW, IIO already has a sysfs interface (you may have to extend the
> > API to support your type of capture though).  
> 
> Multi-Function Device drivers can only be justified if the IP
> contained does not and can not live in a single subsystem.  The IP
> which controls both PWM-in and PWM-out in this device is the same.  I
> can't fathom a sane reason why you would wish to separate this
> functionality over multiple subsystems.
> 

Well, I still think what you describe as PWM-in is actually a capture
device that would perfectly fit in the IIO subsystem, and I guess you
can't use the PWM IP as a capture and waveform generator device as the
same time, which is why I suggested the MFD approach to select the mode.

Anyway, I'm not the PWM or the IIO maintainer, so I'm just sharing my
opinion here.

Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 00/11] pwm: Add support for PWM Capture

[Lee Jones]

On Mon, 06 Jun 2016, Boris Brezillon wrote:

> On Mon, 6 Jun 2016 16:32:31 +0100
> Lee Jones <lee.jones@linaro.org> wrote:
> 
> > On Fri, 29 Apr 2016, Boris Brezillon wrote:
> > 
> > > Hi Lee,
> > > 
> > > On Fri, 22 Apr 2016 11:18:04 +0100
> > > Lee Jones <lee.jones@linaro.org> wrote:
> > >   
> > > > The first part of this set extends the current PWM API to allow external
> > > > code to request a PWM Capture.  Subsequent patches then make use of the
> > > > new API by providing a userspace offering via /sysfs.  The final part of
> > > > the set supplies PWM Capture functionality into the already existing STi
> > > > PWM driver.  
> > > 
> > > Is there a reason you decided to not put this driver in IIO? IMHO, it
> > > would be more appropriate to make your PWM device an MFD that can either
> > > bind to the PWM or the capture driver.
> > > And BTW, IIO already has a sysfs interface (you may have to extend the
> > > API to support your type of capture though).  
> > 
> > Multi-Function Device drivers can only be justified if the IP
> > contained does not and can not live in a single subsystem.  The IP
> > which controls both PWM-in and PWM-out in this device is the same.  I
> > can't fathom a sane reason why you would wish to separate this
> > functionality over multiple subsystems.
> > 
> 
> Well, I still think what you describe as PWM-in is actually a capture
> device that would perfectly fit in the IIO subsystem, and I guess you
> can't use the PWM IP as a capture and waveform generator device as the
> same time, which is why I suggested the MFD approach to select the mode.

We only tend to place devices in IIO if they do not fit anywhere
else.  There are lots of unidirectional and bidirectional capture
devices that belong in other subsystems.

This is a PWM device through and through, and the API fits in
perfectly with the remainder of the subsystem.  To attempt to manage
and maintain similar functionality spread over more than one subsystem
when there is no clear requirement (like there is with a chip
containing a GPIO, Regulator and HWMON components for inistance),
would be unnecessarily over-complicating matters.

> Anyway, I'm not the PWM or the IIO maintainer, so I'm just sharing my
> opinion here.
> 
> Regards,
> 
> Boris
> 

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog