[PATCH 00/10]

[PATCH 00/10]

[Olliver Schinagl]

Hey Thierry,

With this patch set I wanted to add some new features to the PWM framework,
while doing so, I ran into 2 minor things with the lpc18xx and sunxi driver.

The lpc18xx I bumped into because I was trying to learn from existing drivers.
Here I only added the setter to set the chip_data, since I think it is bad
practice to set data directly in the struct, I'll be happy to stand corrected
however and that patch not applied.

Since I use the sunxi platform a lot, I used this platform to test things with
and to also ran into some issues with the hardware PWM as can be seen in its
commit log.

The major feature of this patch-set however is the introduction of the Generic
GPIO based PWM driver based on the high-resolution timers. On an Olimex Lime2
featuring an Allwinner A20 SoC, I can toggle quite reliably at 100 kHz. At 150
kHz the SoC started to show signs of being to slow and the scope traces started
to become inconsistent.
Naturally of course, a GPIO based PWM is heavily dependent on system load and
on the SoC. For some use cases a bit-banged PWM might not be ideal, audio for
example does not sound 'pitch-perfect' but does work pretty alright. It really
is up to the users what one might one to use a bit-banged GPIO driver for.

Finally, I've started to add a 'pulse' option to the PWM framework and
added it to the sunxi and gpio variants. It currently does not work and I
did not wanted to submit it until Boris's PWM patches where in, but I also
felt an early review on the intention would be a good idea.
The idea is using this new feature, we can start emitting a fixed number of
pulses, using the PWM 'hardware' guaranteeing properly timed output.
Not all hardware may support this at all (might be emulated however) and the
sunxi hardware for example is limited to 1 pulse max (but may emulate more with
timers for example). The GPIO driver is able to produce a lot of timed pulses
however.

Thanks,

Olliver

Signed-off-by: Olliver Schinagl <oliver@schinagl.nL>

Olliver Schinagl (10):
  pwm: lpc18xx_pwm: use pwm_set_chip_data
  pwm: sunxi: fix whitespace issue
  pwm: sunxi: Yield some time to the pwm-block to become ready
  pwm: core: use bitops
  pwm: sysfs: do not unnecessarily store result in var
  pwm: sysfs: make use of the DEVICE_ATTR_[RW][WO] macro's
  pwm: gpio: Add a generic gpio based PWM driver
  pwm: core: add pulse feature to the PWM framework
  pwm: pwm_gpio: add pulse option
  pwm: sunxi: Add possibility to pulse the sunxi pwm output

 Documentation/devicetree/bindings/pwm/pwm-gpio.txt |  18 ++
 MAINTAINERS                                        |   5 +
 drivers/pwm/Kconfig                                |  15 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/core.c                                 |  30 ++-
 drivers/pwm/pwm-gpio.c                             | 276 +++++++++++++++++++++
 drivers/pwm/pwm-lpc18xx-sct.c                      |  11 +-
 drivers/pwm/pwm-sun4i.c                            |  73 ++++--
 drivers/pwm/sysfs.c                                | 133 +++++++---
 include/linux/pwm.h                                |  71 +++++-
 10 files changed, 556 insertions(+), 77 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.txt
 create mode 100644 drivers/pwm/pwm-gpio.c

-- 
2.6.1

[PATCH 01/10] pwm: lpc18xx_pwm: use pwm_set_chip_data

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

The lpc18xx driver currently manipulates the pwm_device struct directly
rather then using the pwm_set_chip_data. While the current method may
save a clock cycle or two, it is more obvious that data is set to the
chip pointer (especially since it is only a single int holding struct.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-lpc18xx-sct.c | 11 +++++++----
 drivers/pwm/pwm-sun4i.c       | 11 +++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/pwm/pwm-lpc18xx-sct.c b/drivers/pwm/pwm-lpc18xx-sct.c
index 9163085..0ab59f1 100644
--- a/drivers/pwm/pwm-lpc18xx-sct.c
+++ b/drivers/pwm/pwm-lpc18xx-sct.c
@@ -408,14 +408,17 @@ static int lpc18xx_pwm_probe(struct platform_device *pdev)
 	}
 
 	for (i = 0; i < lpc18xx_pwm->chip.npwm; i++) {
+		struct lpc18xx_pwm_data *lpc18xx_data;
+
 		pwm = &lpc18xx_pwm->chip.pwms[i];
-		pwm->chip_data = devm_kzalloc(lpc18xx_pwm->dev,
-					      sizeof(struct lpc18xx_pwm_data),
-					      GFP_KERNEL);
-		if (!pwm->chip_data) {
+		lpc18xx_data = devm_kzalloc(lpc18xx_pwm->dev,
+					    sizeof(struct lpc18xx_pwm_data),
+					    GFP_KERNEL);
+		if (!lpc18xx_data) {
 			ret = -ENOMEM;
 			goto remove_pwmchip;
 		}
+		pwm_set_chip_data(pwm, lpc18xx_data);
 	}
 
 	platform_set_drvdata(pdev, lpc18xx_pwm);
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index cd9dde5..5ec4e8e 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -8,6 +8,7 @@
 
 #include <linux/bitops.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/io.h>
 #include <linux/module.h>
@@ -244,6 +245,16 @@ static void sun4i_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_EN, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
+
+	/* Allow for the PWM hardware to finish its last toggle. The pulse
+	 * may have just started and thus we should wait a full period.
+	 */
+	ndelay(pwm_get_period(pwm));
+
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
-- 
2.6.1

[PATCH 02/10] pwm: sunxi: fix whitespace issue

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

This patch changes no code, it just fixes the whitespacing

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-sun4i.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 5ec4e8e..58ff424 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -115,7 +115,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 		 * is not an integer so round it half up instead of
 		 * truncating to get less surprising values.
 		 */
-		div = clk_rate * period_ns + NSEC_PER_SEC/2;
+		div = clk_rate * period_ns + NSEC_PER_SEC / 2;
 		do_div(div, NSEC_PER_SEC);
 		if (div - 1 > PWM_PRD_MASK)
 			prescaler = 0;
-- 
2.6.1

[PATCH 03/10] pwm: sunxi: Yield some time to the pwm-block to become ready

[Olliver Schinagl]

The pwm-block of some of the sunxi chips feature a 'ready' flag to
indicate the software that it is ready for new commands.

Right now, when we call pwm_config and set the period, we write the
values to the registers, and turn off the clock to the IP. Because of
this, the hardware does not have time to configure the hardware and set
the 'ready' flag.

By running the clock just before making new changes and before checking
if the hardware is ready, the hardware has time to reconfigure itself
and set the clear the flag appropriately.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
---
 drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 18 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 58ff424..4d84d9d 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	u64 clk_rate, div = 0;
 	unsigned int prescaler = 0;
 	int err;
+	int ret = 0;
+
+	/* Let the PWM hardware run before making any changes. We do this to
+	 * allow the hardware to have some time to clear the 'ready' flag.
+	 */
+	err = clk_prepare_enable(sun4i_pwm->clk);
+	if (err) {
+		dev_err(chip->dev, "failed to enable PWM clock\n");
+		return err;
+	}
+	spin_lock(&sun4i_pwm->ctrl_lock);
+	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+	spin_unlock(&sun4i_pwm->ctrl_lock);
 
 	clk_rate = clk_get_rate(sun4i_pwm->clk);
 
@@ -136,7 +152,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 		if (div - 1 > PWM_PRD_MASK) {
 			dev_err(chip->dev, "period exceeds the maximum value\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			spin_lock(&sun4i_pwm->ctrl_lock);
+			goto out;
 		}
 	}
 
@@ -145,26 +163,14 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	do_div(div, period_ns);
 	dty = div;
 
-	err = clk_prepare_enable(sun4i_pwm->clk);
-	if (err) {
-		dev_err(chip->dev, "failed to enable PWM clock\n");
-		return err;
-	}
-
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
-		spin_unlock(&sun4i_pwm->ctrl_lock);
-		clk_disable_unprepare(sun4i_pwm->clk);
-		return -EBUSY;
-	}
-
-	clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-	if (clk_gate) {
-		val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
-		sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
+		ret = -EBUSY;
+		goto out;
 	}
+	val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
+	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 	val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
@@ -174,6 +180,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
 
+out:
 	if (clk_gate) {
 		val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
 		val |= clk_gate;
@@ -183,7 +190,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 	spin_unlock(&sun4i_pwm->ctrl_lock);
 	clk_disable_unprepare(sun4i_pwm->clk);
 
-	return 0;
+	return ret;
 }
 
 static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
-- 
2.6.1

[PATCH 04/10] pwm: core: use bitops

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

The pwm header defines bits manually while there is a nice bitops.h with
a BIT() macro. Use the BIT() macro to set bits in pwm.h

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 include/linux/pwm.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index d681f68..29315ad 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -1,6 +1,7 @@
 #ifndef __LINUX_PWM_H
 #define __LINUX_PWM_H
 
+#include <linux/bitops.h>
 #include <linux/err.h>
 #include <linux/of.h>
 
@@ -74,9 +75,9 @@ enum pwm_polarity {
 };
 
 enum {
-	PWMF_REQUESTED = 1 << 0,
-	PWMF_ENABLED = 1 << 1,
-	PWMF_EXPORTED = 1 << 2,
+	PWMF_REQUESTED = BIT(0),
+	PWMF_ENABLED = BIT(1),
+	PWMF_EXPORTED = BIT(2),
 };
 
 /**
-- 
2.6.1

[PATCH 05/10] pwm: sysfs: do not unnecessarily store result in var

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

Use the result of pwm_is_enabled directly instead of storing it first.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/sysfs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index c472772..ba67845 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -97,9 +97,8 @@ static ssize_t pwm_enable_show(struct device *child,
 			       char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
-	int enabled = pwm_is_enabled(pwm);
 
-	return sprintf(buf, "%d\n", enabled);
+	return sprintf(buf, "%d\n", pwm_is_enabled(pwm));
 }
 
 static ssize_t pwm_enable_store(struct device *child,
-- 
2.6.1

[PATCH 06/10] pwm: sysfs: make use of the DEVICE_ATTR_[RW][WO] macro's

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

For the npwm property the pwm sysfs interface already made use of the
DEVICE_ATTR_RO macro. This patch expands this to the other sysfs
properties so that the code base is concise and makes use of this
helpful macro.

This has the advantage of slightly reducing the code size, improving
readability and no longer using magic values for permissions.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/sysfs.c | 72 ++++++++++++++++++++++++++---------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index ba67845..9c90886 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -40,18 +40,18 @@ static struct pwm_device *child_to_pwm_device(struct device *child)
 	return export->pwm;
 }
 
-static ssize_t pwm_period_show(struct device *child,
-			       struct device_attribute *attr,
-			       char *buf)
+static ssize_t period_show(struct device *child,
+			   struct device_attribute *attr,
+			   char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
 	return sprintf(buf, "%u\n", pwm_get_period(pwm));
 }
 
-static ssize_t pwm_period_store(struct device *child,
-				struct device_attribute *attr,
-				const char *buf, size_t size)
+static ssize_t period_store(struct device *child,
+			    struct device_attribute *attr,
+			    const char *buf, size_t size)
 {
 	struct pwm_device *pwm = child_to_pwm_device(child);
 	unsigned int val;
@@ -66,18 +66,18 @@ static ssize_t pwm_period_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t pwm_duty_cycle_show(struct device *child,
-				   struct device_attribute *attr,
-				   char *buf)
+static ssize_t duty_cycle_show(struct device *child,
+			       struct device_attribute *attr,
+			       char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
 	return sprintf(buf, "%u\n", pwm_get_duty_cycle(pwm));
 }
 
-static ssize_t pwm_duty_cycle_store(struct device *child,
-				    struct device_attribute *attr,
-				    const char *buf, size_t size)
+static ssize_t duty_cycle_store(struct device *child,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
 {
 	struct pwm_device *pwm = child_to_pwm_device(child);
 	unsigned int val;
@@ -92,18 +92,18 @@ static ssize_t pwm_duty_cycle_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t pwm_enable_show(struct device *child,
-			       struct device_attribute *attr,
-			       char *buf)
+static ssize_t enable_show(struct device *child,
+			   struct device_attribute *attr,
+			   char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 
 	return sprintf(buf, "%d\n", pwm_is_enabled(pwm));
 }
 
-static ssize_t pwm_enable_store(struct device *child,
-				struct device_attribute *attr,
-				const char *buf, size_t size)
+static ssize_t enable_store(struct device *child,
+			    struct device_attribute *attr,
+			    const char *buf, size_t size)
 {
 	struct pwm_device *pwm = child_to_pwm_device(child);
 	int val, ret;
@@ -127,9 +127,9 @@ static ssize_t pwm_enable_store(struct device *child,
 	return ret ? : size;
 }
 
-static ssize_t pwm_polarity_show(struct device *child,
-				 struct device_attribute *attr,
-				 char *buf)
+static ssize_t polarity_show(struct device *child,
+			     struct device_attribute *attr,
+			     char *buf)
 {
 	const struct pwm_device *pwm = child_to_pwm_device(child);
 	const char *polarity = "unknown";
@@ -147,9 +147,9 @@ static ssize_t pwm_polarity_show(struct device *child,
 	return sprintf(buf, "%s\n", polarity);
 }
 
-static ssize_t pwm_polarity_store(struct device *child,
-				  struct device_attribute *attr,
-				  const char *buf, size_t size)
+static ssize_t polarity_store(struct device *child,
+			      struct device_attribute *attr,
+			      const char *buf, size_t size)
 {
 	struct pwm_device *pwm = child_to_pwm_device(child);
 	enum pwm_polarity polarity;
@@ -167,10 +167,10 @@ static ssize_t pwm_polarity_store(struct device *child,
 	return ret ? : size;
 }
 
-static DEVICE_ATTR(period, 0644, pwm_period_show, pwm_period_store);
-static DEVICE_ATTR(duty_cycle, 0644, pwm_duty_cycle_show, pwm_duty_cycle_store);
-static DEVICE_ATTR(enable, 0644, pwm_enable_show, pwm_enable_store);
-static DEVICE_ATTR(polarity, 0644, pwm_polarity_show, pwm_polarity_store);
+static DEVICE_ATTR_RW(period);
+static DEVICE_ATTR_RW(duty_cycle);
+static DEVICE_ATTR_RW(enable);
+static DEVICE_ATTR_RW(polarity);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
@@ -244,9 +244,9 @@ static int pwm_unexport_child(struct device *parent, struct pwm_device *pwm)
 	return 0;
 }
 
-static ssize_t pwm_export_store(struct device *parent,
-				struct device_attribute *attr,
-				const char *buf, size_t len)
+static ssize_t export_store(struct device *parent,
+			    struct device_attribute *attr,
+			    const char *buf, size_t len)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 	struct pwm_device *pwm;
@@ -270,11 +270,11 @@ static ssize_t pwm_export_store(struct device *parent,
 
 	return ret ? : len;
 }
-static DEVICE_ATTR(export, 0200, NULL, pwm_export_store);
+static DEVICE_ATTR_WO(export);
 
-static ssize_t pwm_unexport_store(struct device *parent,
-				  struct device_attribute *attr,
-				  const char *buf, size_t len)
+static ssize_t unexport_store(struct device *parent,
+			      struct device_attribute *attr,
+			      const char *buf, size_t len)
 {
 	struct pwm_chip *chip = dev_get_drvdata(parent);
 	unsigned int hwpwm;
@@ -291,7 +291,7 @@ static ssize_t pwm_unexport_store(struct device *parent,
 
 	return ret ? : len;
 }
-static DEVICE_ATTR(unexport, 0200, NULL, pwm_unexport_store);
+static DEVICE_ATTR_WO(unexport);
 
 static ssize_t npwm_show(struct device *parent, struct device_attribute *attr,
 			 char *buf)
-- 
2.6.1

[PATCH 07/10] pwm: gpio: Add a generic gpio based PWM driver

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

This patch adds a bit-banging gpio PWM driver. It makes use of hrtimers,
to allow nano-second resolution, though it obviously strongly depends on
the switching speed of the gpio pins, hrtimer and system load.

Each pwm node can have 1 or more "pwm-gpio" entries, which will be
treated as pwm's as part of a pwm chip.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 Documentation/devicetree/bindings/pwm/pwm-gpio.txt |  18 ++
 MAINTAINERS                                        |   5 +
 drivers/pwm/Kconfig                                |  15 ++
 drivers/pwm/Makefile                               |   1 +
 drivers/pwm/pwm-gpio.c                             | 253 +++++++++++++++++++++
 5 files changed, 292 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.txt
 create mode 100644 drivers/pwm/pwm-gpio.c

diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.txt b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
new file mode 100644
index 0000000..336f61f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
@@ -0,0 +1,18 @@
+Generic GPIO bit-banged PWM driver
+
+Required properties:
+  - compatible: should be "pwm-gpio"
+  - #pwm-cells: should be 3, see pwm.txt in this directory for a general
+    description of the cells format.
+  - pwm-gpios: one or more gpios describing the used gpio, see the gpio
+    bindings for the used gpio driver.
+
+Example:
+#include <dt-bindings/gpio/gpio.h>
+
+	pwm: pwm@0 {
+		compatible = "pwm-gpio";
+		#pwm-cells = 3;
+		pwm-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>;
+		pwm-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
+	};
diff --git a/MAINTAINERS b/MAINTAINERS
index 7ba7ab7..0ae7fbf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4555,6 +4555,11 @@ F:	drivers/i2c/muxes/i2c-mux-gpio.c
 F:	include/linux/i2c-mux-gpio.h
 F:	Documentation/i2c/muxes/i2c-mux-gpio
 
+GENERIC GPIO PWM DRIVER
+M:	Olliver Schinagl <oliver@schinagl.nl>
+S:	Maintained
+F:	drivers/pwm/pwm-gpio.c
+
 GENERIC HDLC (WAN) DRIVERS
 M:	Krzysztof Halasa <khc@pm.waw.pl>
 W:	http://www.kernel.org/pub/linux/utils/net/hdlc/
diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 0cfaf6b..c0bc296 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -170,6 +170,21 @@ config PWM_IMG
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-img
 
+config PWM_GPIO
+	tristate "Generic GPIO bit-banged PWM driver"
+	depends on OF
+	depends on GPIOLIB
+	help
+	  Some platforms do not offer any hardware PWM capabilities but do have
+	  General Purpose Input Output (GPIO) pins available. Using the kernels
+	  High-Resolution Timer API this driver tries to toggle GPIO using the
+	  generic kernel PWM framework. The maximum frequency and/or accuracy
+	  is dependent on several factors such as system load and the maximum
+	  speed a pin can be toggled at the hardware.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-gpio.
+
 config PWM_IMX
 	tristate "i.MX PWM support"
 	depends on ARCH_MXC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 69b8275..96aa9aa 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
+obj-$(CONFIG_PWM_GPIO)		+= pwm-gpio.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
new file mode 100644
index 0000000..8b588fb
--- /dev/null
+++ b/drivers/pwm/pwm-gpio.c
@@ -0,0 +1,253 @@
+/*
+ * Copyright (c) 2015 Olliver Schinagl <oliver@schinagl.nl>
+ *
+ * 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
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * This driver adds a high-resolution timer based PWM driver. Since this is a
+ * bit-banged driver, accuracy will always depend on a lot of factors, such as
+ * GPIO toggle speed and system load.
+ */
+
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/property.h>
+#include <linux/pwm.h>
+#include <linux/errno.h>
+#include <linux/err.h>
+#include <linux/device.h>
+#include <linux/hrtimer.h>
+#include <linux/ktime.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+
+#define DRV_NAME "pwm-gpio"
+
+struct gpio_pwm_data {
+	struct hrtimer timer;
+	struct gpio_desc *gpiod;
+	bool polarity;
+	bool pin_on;
+	int on_time;
+	int off_time;
+	bool run;
+};
+
+struct gpio_pwm_chip {
+	struct pwm_chip chip;
+};
+
+static void gpio_pwm_off(struct gpio_pwm_data *gpio_data)
+{
+	gpiod_set_value_cansleep(gpio_data->gpiod, gpio_data->polarity ? 0 : 1);
+}
+
+static void gpio_pwm_on(struct gpio_pwm_data *gpio_data)
+{
+	gpiod_set_value_cansleep(gpio_data->gpiod, gpio_data->polarity ? 1 : 0);
+}
+
+enum hrtimer_restart gpio_pwm_timer(struct hrtimer *timer)
+{
+	struct gpio_pwm_data *gpio_data = container_of(timer,
+						      struct gpio_pwm_data,
+						      timer);
+	if (!gpio_data->run) {
+		gpio_pwm_off(gpio_data);
+		gpio_data->pin_on = false;
+		return HRTIMER_NORESTART;
+	}
+
+	if (!gpio_data->pin_on) {
+		hrtimer_forward_now(&gpio_data->timer,
+				    ns_to_ktime(gpio_data->on_time));
+		gpio_pwm_on(gpio_data);
+		gpio_data->pin_on = true;
+	} else {
+		hrtimer_forward_now(&gpio_data->timer,
+				    ns_to_ktime(gpio_data->off_time));
+		gpio_pwm_off(gpio_data);
+		gpio_data->pin_on = false;
+	}
+
+	return HRTIMER_RESTART;
+}
+
+static int gpio_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			    int duty_ns, int period_ns)
+{
+	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
+
+	gpio_data->on_time = duty_ns;
+	gpio_data->off_time = period_ns - duty_ns;
+
+	return 0;
+}
+
+static int gpio_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
+				 enum pwm_polarity polarity)
+{
+	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
+
+	gpio_data->polarity = (polarity != PWM_POLARITY_NORMAL) ? true : false;
+
+	return 0;
+}
+
+static int gpio_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
+
+	if (gpio_data->run)
+		return -EBUSY;
+
+	gpio_data->run = true;
+	if (gpio_data->off_time) {
+		hrtimer_start(&gpio_data->timer, ktime_set(0, 0),
+			      HRTIMER_MODE_REL);
+	} else {
+		if (gpio_data->on_time)
+			gpio_pwm_on(gpio_data);
+		else
+			gpio_pwm_off(gpio_data);
+	}
+
+	return 0;
+}
+
+static void gpio_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
+
+	gpio_data->run = false;
+	if (!gpio_data->off_time)
+		gpio_pwm_off(gpio_data);
+}
+
+static const struct pwm_ops gpio_pwm_ops = {
+	.config = gpio_pwm_config,
+	.set_polarity = gpio_pwm_set_polarity,
+	.enable = gpio_pwm_enable,
+	.disable = gpio_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int gpio_pwm_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct gpio_pwm_chip *gpio_chip;
+	int npwm, i;
+	int hrtimer = 0;
+
+	npwm = of_gpio_named_count(pdev->dev.of_node, "pwm-gpios");
+	if (npwm < 1)
+		return -ENODEV;
+
+	gpio_chip = devm_kzalloc(&pdev->dev, sizeof(*gpio_chip), GFP_KERNEL);
+	if (!gpio_chip)
+		return -ENOMEM;
+
+	gpio_chip->chip.dev = &pdev->dev;
+	gpio_chip->chip.ops = &gpio_pwm_ops;
+	gpio_chip->chip.base = -1;
+	gpio_chip->chip.npwm = npwm;
+	gpio_chip->chip.of_xlate = of_pwm_xlate_with_flags;
+	gpio_chip->chip.of_pwm_n_cells = 3;
+	gpio_chip->chip.can_sleep = true;
+
+	ret = pwmchip_add(&gpio_chip->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
+		return ret;
+	}
+
+	for (i = 0; i < npwm; i++) {
+		struct gpio_desc *gpiod;
+		struct gpio_pwm_data *gpio_data;
+
+		gpiod = devm_gpiod_get_index(&pdev->dev, "pwm", i,
+					     GPIOD_OUT_LOW);
+		if (IS_ERR(gpiod)) {
+			int error;
+
+			error = PTR_ERR(gpiod);
+			if (error != -EPROBE_DEFER)
+				dev_err(&pdev->dev,
+					"failed to get gpio flags, error: %d\n",
+					error);
+			return error;
+		}
+
+		gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data),
+					 GFP_KERNEL);
+
+		hrtimer_init(&gpio_data->timer,
+			     CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+		gpio_data->timer.function = &gpio_pwm_timer;
+		gpio_data->gpiod = gpiod;
+		gpio_data->pin_on = false;
+		gpio_data->run = false;
+
+		if (hrtimer_is_hres_active(&gpio_data->timer))
+			hrtimer++;
+
+		pwm_set_chip_data(&gpio_chip->chip.pwms[i], gpio_data);
+	}
+	if (!hrtimer)
+		dev_warn(&pdev->dev, "unable to use High-Resolution timer,");
+		dev_warn(&pdev->dev, "%s is restricted to low resolution.",
+			 DRV_NAME);
+
+	platform_set_drvdata(pdev, gpio_chip);
+
+	dev_info(&pdev->dev, "%d gpio pwms loaded\n", npwm);
+
+	return 0;
+}
+
+static int gpio_pwm_remove(struct platform_device *pdev)
+{
+	struct gpio_pwm_chip *gpio_chip;
+	int i;
+
+	gpio_chip = platform_get_drvdata(pdev);
+	for (i = 0; i < gpio_chip->chip.npwm; i++) {
+		struct gpio_pwm_data *gpio_data;
+
+		gpio_data = pwm_get_chip_data(&gpio_chip->chip.pwms[i]);
+
+		hrtimer_cancel(&gpio_data->timer);
+	}
+
+	return pwmchip_remove(&gpio_chip->chip);
+}
+
+static const struct of_device_id gpio_pwm_of_match[] = {
+	{ .compatible = DRV_NAME, },
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, gpio_pwm_of_match);
+
+static struct platform_driver gpio_pwm_driver = {
+	.probe = gpio_pwm_probe,
+	.remove = gpio_pwm_remove,
+	.driver = {
+		.name = DRV_NAME,
+		.of_match_table = of_match_ptr(gpio_pwm_of_match),
+	},
+};
+module_platform_driver(gpio_pwm_driver);
+
+MODULE_AUTHOR("Olliver Schinagl <oliver@schinagl.nl>");
+MODULE_DESCRIPTION("Generic GPIO bit-banged PWM driver");
+MODULE_LICENSE("GPL");
-- 
2.6.1

[PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

Some hardware PWM's have the possibility to only send out one (or more)
pulses. This can be quite a useful feature in case one wants or needs
only a single pulse, but at the exact width.

Additionally, if multiple pulses are possible, outputting a fixed amount
of pulses can be useful for various timing specific purposes.

A few new functions have been expanded or added for this new behavior.

* pwm_config()	now takes an additional parameter to setup the number of
		pulses to output. The driver may force this to 0 or 1
		for if example if this feature is not or only partially
		supported
* pwm_[sg]et_pulse_count()	get or set the number of pulses the pwm
				framework is configured for
* pwm_get_pulse_count_max()	get the maximum number of pulses the pwm
				driver supports
* pwm_pulse()		Tell the PWM to emit a pre-configured number of pulses
* pwm_pulse_done()	an internal function for drivers to call when
			they have completed their pre-configured number
			of pulses
* pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
			yielding a little more information than just
			pwm_is_enabled()

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/core.c      | 30 +++++++++++++++++++----
 drivers/pwm/pwm-gpio.c  |  3 ++-
 drivers/pwm/pwm-sun4i.c |  3 ++-
 drivers/pwm/sysfs.c     | 58 ++++++++++++++++++++++++++++++++++++++++++--
 include/linux/pwm.h     | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
 5 files changed, 147 insertions(+), 11 deletions(-)

diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
index 3f9df3e..e2c1c0a 100644
--- a/drivers/pwm/core.c
+++ b/drivers/pwm/core.c
@@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free);
  * @pwm: PWM device
  * @duty_ns: "on" time (in nanoseconds)
  * @period_ns: duration (in nanoseconds) of one cycle
+ * @pulse_count: number of pulses (periods) to output on pwm_pulse
  *
  * Returns: 0 on success or a negative error code on failure.
  */
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns,
+	       unsigned int pulse_count)
 {
 	int err;
 
 	if (!pwm || duty_ns < 0 || period_ns <= 0 || duty_ns > period_ns)
 		return -EINVAL;
 
-	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns, period_ns);
+	if (pulse_count > pwm->pulse_count_max)
+		return -EINVAL;
+
+	err = pwm->chip->ops->config(pwm->chip, pwm, duty_ns,
+				     period_ns, pulse_count);
 	if (err)
 		return err;
 
 	pwm->duty_cycle = duty_ns;
 	pwm->period = period_ns;
+	pwm->pulse_count = pulse_count;
 
 	return 0;
 }
@@ -487,6 +494,18 @@ int pwm_set_polarity(struct pwm_device *pwm, enum pwm_polarity polarity)
 EXPORT_SYMBOL_GPL(pwm_set_polarity);
 
 /**
+ * pwm_pulse_done() - notify the PWM framework that pulse_count pulses are done
+ * @pwm: PWM device
+ */
+void pwm_pulse_done(struct pwm_device *pwm)
+{
+	if (pwm && !test_and_clear_bit(PWMF_ENABLED | PWMF_PULSING,
+				       &pwm->flags))
+		return pwm->chip->ops->disable(pwm->chip, pwm);
+}
+EXPORT_SYMBOL_GPL(pwm_pulse_done);
+
+/**
  * pwm_enable() - start a PWM output toggling
  * @pwm: PWM device
  *
@@ -494,7 +513,9 @@ EXPORT_SYMBOL_GPL(pwm_set_polarity);
  */
 int pwm_enable(struct pwm_device *pwm)
 {
-	if (pwm && !test_and_set_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm && !test_and_set_bit(
+			PWMF_ENABLED | !pwm->pulse_count ? : PWMF_PULSING,
+			&pwm->flags))
 		return pwm->chip->ops->enable(pwm->chip, pwm);
 
 	return pwm ? 0 : -EINVAL;
@@ -507,7 +528,8 @@ EXPORT_SYMBOL_GPL(pwm_enable);
  */
 void pwm_disable(struct pwm_device *pwm)
 {
-	if (pwm && test_and_clear_bit(PWMF_ENABLED, &pwm->flags))
+	if (pwm && test_and_clear_bit(PWMF_ENABLED | PWMF_PULSING,
+				      &pwm->flags))
 		pwm->chip->ops->disable(pwm->chip, pwm);
 }
 EXPORT_SYMBOL_GPL(pwm_disable);
diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
index 8b588fb..cf4b170 100644
--- a/drivers/pwm/pwm-gpio.c
+++ b/drivers/pwm/pwm-gpio.c
@@ -84,7 +84,7 @@ enum hrtimer_restart gpio_pwm_timer(struct hrtimer *timer)
 }
 
 static int gpio_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			    int duty_ns, int period_ns)
+			    int duty_ns, int period_ns, unsigned int count)
 {
 	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
 
@@ -202,6 +202,7 @@ static int gpio_pwm_probe(struct platform_device *pdev)
 			hrtimer++;
 
 		pwm_set_chip_data(&gpio_chip->chip.pwms[i], gpio_data);
+		pwm_set_pulse_count_max(&gpio_chip->chip.pwms[i], UINT_MAX);
 	}
 	if (!hrtimer)
 		dev_warn(&pdev->dev, "unable to use High-Resolution timer,");
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 4d84d9d..6347ca8 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -97,7 +97,8 @@ static inline void sun4i_pwm_writel(struct sun4i_pwm_chip *chip,
 }
 
 static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
-			    int duty_ns, int period_ns)
+			    int duty_ns, int period_ns,
+			    unsigned int pulse_count)
 {
 	struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip);
 	u32 prd, dty, val, clk_gate;
diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
index 9c90886..9b7413c 100644
--- a/drivers/pwm/sysfs.c
+++ b/drivers/pwm/sysfs.c
@@ -61,7 +61,8 @@ static ssize_t period_store(struct device *child,
 	if (ret)
 		return ret;
 
-	ret = pwm_config(pwm, pwm_get_duty_cycle(pwm), val);
+	ret = pwm_config(pwm, pwm_get_duty_cycle(pwm),
+			 val, pwm_get_pulse_count(pwm));
 
 	return ret ? : size;
 }
@@ -87,7 +88,8 @@ static ssize_t duty_cycle_store(struct device *child,
 	if (ret)
 		return ret;
 
-	ret = pwm_config(pwm, val, pwm_get_period(pwm));
+	ret = pwm_config(pwm, val, pwm_get_period(pwm),
+			 pwm_get_pulse_count(pwm));
 
 	return ret ? : size;
 }
@@ -167,16 +169,68 @@ static ssize_t polarity_store(struct device *child,
 	return ret ? : size;
 }
 
+static ssize_t pulse_count_show(struct device *child,
+				struct device_attribute *attr,
+				char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm_get_pulse_count(pwm));
+}
+
+static ssize_t pulse_count_store(struct device *child,
+				 struct device_attribute *attr,
+				 const char *buf, size_t size)
+{
+	struct pwm_device *pwm = child_to_pwm_device(child);
+	unsigned int val;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	ret = pwm_config(pwm, pwm_get_duty_cycle(pwm),
+			 pwm_get_period(pwm), val);
+
+	return ret ? : size;
+
+}
+
+static ssize_t pulse_count_max_show(struct device *child,
+				    struct device_attribute *attr,
+				    char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%u\n", pwm_get_pulse_count_max(pwm));
+}
+
+static ssize_t pulsing_show(struct device *child,
+			    struct device_attribute *attr,
+			    char *buf)
+{
+	const struct pwm_device *pwm = child_to_pwm_device(child);
+
+	return sprintf(buf, "%d\n", pwm_is_pulsing(pwm));
+}
+
 static DEVICE_ATTR_RW(period);
 static DEVICE_ATTR_RW(duty_cycle);
 static DEVICE_ATTR_RW(enable);
 static DEVICE_ATTR_RW(polarity);
+static DEVICE_ATTR_RW(pulse_count);
+static DEVICE_ATTR_RO(pulse_count_max);
+static DEVICE_ATTR_RO(pulsing);
 
 static struct attribute *pwm_attrs[] = {
 	&dev_attr_period.attr,
 	&dev_attr_duty_cycle.attr,
 	&dev_attr_enable.attr,
 	&dev_attr_polarity.attr,
+	&dev_attr_pulse_count.attr,
+	&dev_attr_pulse_count_max.attr,
+	&dev_attr_pulsing.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(pwm);
diff --git a/include/linux/pwm.h b/include/linux/pwm.h
index 29315ad..c6042cf 100644
--- a/include/linux/pwm.h
+++ b/include/linux/pwm.h
@@ -22,7 +22,13 @@ void pwm_free(struct pwm_device *pwm);
 /*
  * pwm_config - change a PWM device configuration
  */
-int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns);
+int pwm_config(struct pwm_device *pwm, int duty_ns,
+	       int period_ns, unsigned int pulse_count);
+
+/*
+ * pwm_pulse_done - notify the PWM framework that pulse_count pulses are done
+ */
+void pwm_pulse_done(struct pwm_device *pwm);
 
 /*
  * pwm_enable - start a PWM output toggling
@@ -43,7 +49,8 @@ static inline void pwm_free(struct pwm_device *pwm)
 {
 }
 
-static inline int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
+static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
+			     int period_ns, unsigned int pulse_count)
 {
 	return -EINVAL;
 }
@@ -53,6 +60,11 @@ static inline int pwm_enable(struct pwm_device *pwm)
 	return -EINVAL;
 }
 
+static inline int pwm_pulse_done(struct pwm_device *pwm)
+{
+	return -EINVAL;
+}
+
 static inline void pwm_disable(struct pwm_device *pwm)
 {
 }
@@ -78,6 +90,7 @@ enum {
 	PWMF_REQUESTED = BIT(0),
 	PWMF_ENABLED = BIT(1),
 	PWMF_EXPORTED = BIT(2),
+	PWMF_PULSING = BIT(3),
 };
 
 /**
@@ -91,6 +104,8 @@ enum {
  * @period: period of the PWM signal (in nanoseconds)
  * @duty_cycle: duty cycle of the PWM signal (in nanoseconds)
  * @polarity: polarity of the PWM signal
+ * @pulse_count: number of PWM pulses to toggle
+ * @pulse_count_max: maximum number of pulses that can be set to pulse
  */
 struct pwm_device {
 	const char *label;
@@ -103,6 +118,8 @@ struct pwm_device {
 	unsigned int period;
 	unsigned int duty_cycle;
 	enum pwm_polarity polarity;
+	unsigned int pulse_count;
+	unsigned int pulse_count_max;
 };
 
 static inline bool pwm_is_enabled(const struct pwm_device *pwm)
@@ -110,6 +127,11 @@ static inline bool pwm_is_enabled(const struct pwm_device *pwm)
 	return test_bit(PWMF_ENABLED, &pwm->flags);
 }
 
+static inline bool pwm_is_pulsing(const struct pwm_device *pwm)
+{
+	return test_bit(PWMF_ENABLED | PWMF_PULSING, &pwm->flags);
+}
+
 static inline void pwm_set_period(struct pwm_device *pwm, unsigned int period)
 {
 	if (pwm)
@@ -142,6 +164,42 @@ static inline enum pwm_polarity pwm_get_polarity(const struct pwm_device *pwm)
 	return pwm ? pwm->polarity : PWM_POLARITY_NORMAL;
 }
 
+/*
+ * pwm_set_pulse_count - configure the number of pulses of a pwm_pulse
+ */
+static inline void pwm_set_pulse_count(struct pwm_device *pwm,
+				       unsigned int pulse_count)
+{
+	if (pwm)
+		pwm->pulse_count = 0;
+}
+
+/*
+ * pwm_get_pulse_count - retrieve the number of pules to pulse of a pwm_pulse
+ */
+static inline unsigned int pwm_get_pulse_count(const struct pwm_device *pwm)
+{
+	return pwm ? pwm->pulse_count : 0;
+}
+
+/*
+ * pwm_get_pulse_count_max - retrieve the maximum number of pulses
+ */
+static inline unsigned int pwm_get_pulse_count_max(const struct pwm_device *pwm)
+{
+	return pwm ? pwm->pulse_count_max : 0;
+}
+
+/*
+ * pwm_set_pulse_count_max - set the maximum number of pulses
+ */
+static inline void pwm_set_pulse_count_max(struct pwm_device *pwm,
+					   unsigned int pulse_count_max)
+{
+	if (pwm)
+		pwm->pulse_count_max = pulse_count_max;
+}
+
 /**
  * struct pwm_ops - PWM controller operations
  * @request: optional hook for requesting a PWM
@@ -157,7 +215,7 @@ struct pwm_ops {
 	int (*request)(struct pwm_chip *chip, struct pwm_device *pwm);
 	void (*free)(struct pwm_chip *chip, struct pwm_device *pwm);
 	int (*config)(struct pwm_chip *chip, struct pwm_device *pwm,
-		      int duty_ns, int period_ns);
+		      int duty_ns, int period_ns, unsigned int pulse_count);
 	int (*set_polarity)(struct pwm_chip *chip, struct pwm_device *pwm,
 			    enum pwm_polarity polarity);
 	int (*enable)(struct pwm_chip *chip, struct pwm_device *pwm);
-- 
2.6.1

[PATCH 09/10] pwm: pwm_gpio: add pulse option

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

With the newly added pwm_pulse option added to the PWM framework, this
patch adds the pulse functionality to the gpio_pwm driver.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-gpio.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
index cf4b170..24c27b1 100644
--- a/drivers/pwm/pwm-gpio.c
+++ b/drivers/pwm/pwm-gpio.c
@@ -40,6 +40,8 @@ struct gpio_pwm_data {
 	bool pin_on;
 	int on_time;
 	int off_time;
+	unsigned int count;
+	bool pulse;
 	bool run;
 };
 
@@ -80,6 +82,16 @@ enum hrtimer_restart gpio_pwm_timer(struct hrtimer *timer)
 		gpio_data->pin_on = false;
 	}
 
+	if (gpio_data->count > 0)
+		gpio_data->count--;
+	if (!gpio_data->count && gpio_data->pulse) {
+		struct pwm_device *pwm = container_of((void *)gpio_data,
+						      struct pwm_device,
+						      chip_data);
+		pwm_pulse_done(pwm);
+		return HRTIMER_NORESTART;
+	}
+
 	return HRTIMER_RESTART;
 }
 
@@ -88,6 +100,10 @@ static int gpio_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 {
 	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
 
+	/* A full pulse is both the on and off time, and since counting each
+	 * iteration of the timer is easy and clean, we need double the count.
+	 */
+	gpio_data->count = count * 2;
 	gpio_data->on_time = duty_ns;
 	gpio_data->off_time = period_ns - duty_ns;
 
@@ -111,6 +127,9 @@ static int gpio_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 	if (gpio_data->run)
 		return -EBUSY;
 
+	if (pwm->pulse_count)
+		gpio_data->pulse = true;
+	gpio_data->count = pwm->pulse_count;
 	gpio_data->run = true;
 	if (gpio_data->off_time) {
 		hrtimer_start(&gpio_data->timer, ktime_set(0, 0),
@@ -130,6 +149,7 @@ static void gpio_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
 	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
 
 	gpio_data->run = false;
+	gpio_data->pulse = false;
 	if (!gpio_data->off_time)
 		gpio_pwm_off(gpio_data);
 }
@@ -196,6 +216,8 @@ static int gpio_pwm_probe(struct platform_device *pdev)
 		gpio_data->timer.function = &gpio_pwm_timer;
 		gpio_data->gpiod = gpiod;
 		gpio_data->pin_on = false;
+		gpio_data->count = 0;
+		gpio_data->pulse = false;
 		gpio_data->run = false;
 
 		if (hrtimer_is_hres_active(&gpio_data->timer))
-- 
2.6.1

[PATCH 10/10] pwm: sunxi: Add possibility to pulse the sunxi pwm output

[Olliver Schinagl]

From: Olliver Schinagl <oliver@schinagl.nl>

With the new pulse mode addition to the PWM framework, we can make use
of this for the sunxi PWM.

WARNING: Do not merge yet, currently, we can only pulse once and a
manual disable is required to 'reset' the PWM framework. I haven't
thought through how to automatically 'disable' the PWM after the pulse
is finished and need some guidance here.

Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
---
 drivers/pwm/pwm-sun4i.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 6347ca8..8370cca 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -32,7 +32,7 @@
 #define PWM_EN			BIT(4)
 #define PWM_ACT_STATE		BIT(5)
 #define PWM_CLK_GATING		BIT(6)
-#define PWM_MODE		BIT(7)
+#define PWM_MODE_PULSE		BIT(7)
 #define PWM_PULSE		BIT(8)
 #define PWM_BYPASS		BIT(9)
 
@@ -166,6 +166,8 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
+	if (pulse_count)
+		val |= BIT_CH(PWM_MODE_PULSE, pwm->hwpwm);
 	if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {
 		ret = -EBUSY;
 		goto out;
@@ -237,7 +239,10 @@ static int sun4i_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
 
 	spin_lock(&sun4i_pwm->ctrl_lock);
 	val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
-	val |= BIT_CH(PWM_EN, pwm->hwpwm);
+	if (pwm->pulse_count)
+		val |= BIT_CH(PWM_PULSE, pwm->hwpwm);
+	else
+		val |= BIT_CH(PWM_EN, pwm->hwpwm);
 	val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
 	sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
 	spin_unlock(&sun4i_pwm->ctrl_lock);
@@ -350,9 +355,12 @@ static int sun4i_pwm_probe(struct platform_device *pdev)
 	}
 
 	val = sun4i_pwm_readl(pwm, PWM_CTRL_REG);
-	for (i = 0; i < pwm->chip.npwm; i++)
+	for (i = 0; i < pwm->chip.npwm; i++) {
 		if (!(val & BIT_CH(PWM_ACT_STATE, i)))
 			pwm->chip.pwms[i].polarity = PWM_POLARITY_INVERSED;
+
+		pwm_set_pulse_count_max(&pwm->chip.pwms[i], 1);
+	}
 	clk_disable_unprepare(pwm->clk);
 
 	return 0;
-- 
2.6.1

Re: [PATCH 01/10] pwm: lpc18xx_pwm: use pwm_set_chip_data

[Ezequiel Garcia]

(+ Ariel)

Hi Oliver,

Not sure why there's some many people in Cc for such a silly change.
I guess you are using get_maintainers.pl on the entire patchset and get
this rather long list.

IMO, the value of submitting patches as part of a larger series is to be able to
push patches that need to be applied in some given order, or otherwise
have some kind of logical relationship between them.

However, this is not the case: it's just a very small change and has
no relation to the rest of the patches in the series.
I think a simple standalone patch would be better here.

Also, get_maintainer.pl is just a hint, and not meant to be used as-is.
In particular, you are missing the driver's author.

On 26 October 2015 at 18:32, Olliver Schinagl <o.schinagl@ultimaker.com> wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
>
> The lpc18xx driver currently manipulates the pwm_device struct directly
> rather then using the pwm_set_chip_data. While the current method may
> save a clock cycle or two, it is more obvious that data is set to the
> chip pointer (especially since it is only a single int holding struct.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/pwm-lpc18xx-sct.c | 11 +++++++----
>  drivers/pwm/pwm-sun4i.c       | 11 +++++++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
>

...and this diffstat is obviously fishy.
-- 
Ezequiel García, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[kbuild test robot]

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

Hi Olliver,

[auto build test ERROR on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/pwm-lpc18xx_pwm-use-pwm_set_chip_data/20151027-053853
config: x86_64-randconfig-n0-10270521 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_set_backlight':
>> drivers/gpu/drm/i915/intel_panel.c:652:2: error: too few arguments to function 'pwm_config'
     pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);
     ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:52:19: note: declared here
    static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
                      ^
   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_disable_backlight':
   drivers/gpu/drm/i915/intel_panel.c:797:2: error: too few arguments to function 'pwm_config'
     pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
     ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:52:19: note: declared here
    static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
                      ^
   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_setup_backlight':
   drivers/gpu/drm/i915/intel_panel.c:1442:11: error: too few arguments to function 'pwm_config'
     retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,
              ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:52:19: note: declared here
    static inline int pwm_config(struct pwm_device *pwm, int duty_ns,
                      ^

vim +/pwm_config +652 drivers/gpu/drm/i915/intel_panel.c

0fb890c0 Vandana Kannan 2015-05-05  646  
b029e66f Shobhit Kumar  2015-06-26  647  static void pwm_set_backlight(struct intel_connector *connector, u32 level)
b029e66f Shobhit Kumar  2015-06-26  648  {
b029e66f Shobhit Kumar  2015-06-26  649  	struct intel_panel *panel = &connector->panel;
b029e66f Shobhit Kumar  2015-06-26  650  	int duty_ns = DIV_ROUND_UP(level * CRC_PMIC_PWM_PERIOD_NS, 100);
b029e66f Shobhit Kumar  2015-06-26  651  
b029e66f Shobhit Kumar  2015-06-26 @652  	pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);
b029e66f Shobhit Kumar  2015-06-26  653  }
b029e66f Shobhit Kumar  2015-06-26  654  
7bd688cd Jani Nikula    2013-11-08  655  static void

:::::: The code at line 652 was first introduced by commit
:::::: b029e66fa8e39ba10dcc47b114be8da8b082493b drm/i915: Backlight control using CRC PMIC based PWM driver

:::::: TO: Shobhit Kumar <shobhit.kumar@intel.com>
:::::: CC: Daniel Vetter <daniel.vetter@ffwll.ch>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 27099 bytes --]

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[kbuild test robot]

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

Hi Olliver,

[auto build test WARNING on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/pwm-lpc18xx_pwm-use-pwm_set_chip_data/20151027-053853
config: avr32-atstk1006_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=avr32 

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-atmel.c:278: warning: initialization from incompatible pointer type

vim +278 drivers/pwm/pwm-atmel.c

472ac3dc Alexandre Belloni 2015-05-25  262  	mutex_lock(&atmel_pwm->isr_lock);
472ac3dc Alexandre Belloni 2015-05-25  263  	atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
472ac3dc Alexandre Belloni 2015-05-25  264  
472ac3dc Alexandre Belloni 2015-05-25  265  	while (!(atmel_pwm->updated_pwms & (1 << pwm->hwpwm)) &&
472ac3dc Alexandre Belloni 2015-05-25  266  	       time_before(jiffies, timeout)) {
472ac3dc Alexandre Belloni 2015-05-25  267  		usleep_range(10, 100);
472ac3dc Alexandre Belloni 2015-05-25  268  		atmel_pwm->updated_pwms |= atmel_pwm_readl(atmel_pwm, PWM_ISR);
472ac3dc Alexandre Belloni 2015-05-25  269  	}
32b16d46 Bo Shen           2013-12-13  270  
472ac3dc Alexandre Belloni 2015-05-25  271  	mutex_unlock(&atmel_pwm->isr_lock);
32b16d46 Bo Shen           2013-12-13  272  	atmel_pwm_writel(atmel_pwm, PWM_DIS, 1 << pwm->hwpwm);
32b16d46 Bo Shen           2013-12-13  273  
32b16d46 Bo Shen           2013-12-13  274  	clk_disable(atmel_pwm->clk);
32b16d46 Bo Shen           2013-12-13  275  }
32b16d46 Bo Shen           2013-12-13  276  
32b16d46 Bo Shen           2013-12-13  277  static const struct pwm_ops atmel_pwm_ops = {
32b16d46 Bo Shen           2013-12-13 @278  	.config = atmel_pwm_config,
32b16d46 Bo Shen           2013-12-13  279  	.set_polarity = atmel_pwm_set_polarity,
32b16d46 Bo Shen           2013-12-13  280  	.enable = atmel_pwm_enable,
32b16d46 Bo Shen           2013-12-13  281  	.disable = atmel_pwm_disable,
32b16d46 Bo Shen           2013-12-13  282  	.owner = THIS_MODULE,
32b16d46 Bo Shen           2013-12-13  283  };
32b16d46 Bo Shen           2013-12-13  284  
32b16d46 Bo Shen           2013-12-13  285  struct atmel_pwm_data {
32b16d46 Bo Shen           2013-12-13  286  	void (*config)(struct pwm_chip *chip, struct pwm_device *pwm,

:::::: The code at line 278 was first introduced by commit
:::::: 32b16d46e4154d6f9d9c1d5dd35f64cdf08b7aea pwm: atmel-pwm: Add Atmel PWM controller driver

:::::: TO: Bo Shen <voice.shen@atmel.com>
:::::: CC: Thierry Reding <thierry.reding@gmail.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 13364 bytes --]

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[kbuild test robot]

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

Hi Olliver,

[auto build test ERROR on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/pwm-lpc18xx_pwm-use-pwm_set_chip_data/20151027-053853
config: i386-randconfig-s1-201543 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/input/misc/max77693-haptic.c: In function 'max77693_haptic_set_duty_cycle':
>> drivers/input/misc/max77693-haptic.c:76:10: error: too few arguments to function 'pwm_config'
     error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period);
             ^
   In file included from drivers/input/misc/max77693-haptic.c:23:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
   drivers/input/misc/max8997_haptic.c: In function 'max8997_haptic_set_duty_cycle':
>> drivers/input/misc/max8997_haptic.c:76:9: error: too few arguments to function 'pwm_config'
      ret = pwm_config(chip->pwm, duty, chip->pwm_period);
            ^
   In file included from drivers/input/misc/max8997_haptic.c:29:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^

vim +/pwm_config +76 drivers/input/misc/max77693-haptic.c

a3b3ca75 Jaewon Kim 2014-09-11  70  
a3b3ca75 Jaewon Kim 2014-09-11  71  static int max77693_haptic_set_duty_cycle(struct max77693_haptic *haptic)
a3b3ca75 Jaewon Kim 2014-09-11  72  {
a3b3ca75 Jaewon Kim 2014-09-11  73  	int delta = (haptic->pwm_dev->period + haptic->pwm_duty) / 2;
a3b3ca75 Jaewon Kim 2014-09-11  74  	int error;
a3b3ca75 Jaewon Kim 2014-09-11  75  
a3b3ca75 Jaewon Kim 2014-09-11 @76  	error = pwm_config(haptic->pwm_dev, delta, haptic->pwm_dev->period);
a3b3ca75 Jaewon Kim 2014-09-11  77  	if (error) {
a3b3ca75 Jaewon Kim 2014-09-11  78  		dev_err(haptic->dev, "failed to configure pwm: %d\n", error);
a3b3ca75 Jaewon Kim 2014-09-11  79  		return error;

:::::: The code at line 76 was first introduced by commit
:::::: a3b3ca753cdc92c7d5f57404afed3115b3b79cc6 Input: add haptic driver on max77693

:::::: TO: Jaewon Kim <jaewon02.kim@samsung.com>
:::::: CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 24764 bytes --]

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[kbuild test robot]

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

Hi Olliver,

[auto build test WARNING on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/pwm-lpc18xx_pwm-use-pwm_set_chip_data/20151027-053853
config: arm-at91_dt_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

>> drivers/pwm/pwm-atmel-tcb.c:359:2: warning: initialization from incompatible pointer type
     .config = atmel_tcb_pwm_config,
     ^
   drivers/pwm/pwm-atmel-tcb.c:359:2: warning: (near initialization for 'atmel_tcb_pwm_ops.config')

vim +359 drivers/pwm/pwm-atmel-tcb.c

9421bade Boris BREZILLON 2013-01-08  343  	}
9421bade Boris BREZILLON 2013-01-08  344  
9421bade Boris BREZILLON 2013-01-08  345  	tcbpwm->period = period;
9421bade Boris BREZILLON 2013-01-08  346  	tcbpwm->div = i;
9421bade Boris BREZILLON 2013-01-08  347  	tcbpwm->duty = duty;
9421bade Boris BREZILLON 2013-01-08  348  
9421bade Boris BREZILLON 2013-01-08  349  	/* If the PWM is enabled, call enable to apply the new conf */
5c31252c Boris Brezillon 2015-07-01  350  	if (pwm_is_enabled(pwm))
9421bade Boris BREZILLON 2013-01-08  351  		atmel_tcb_pwm_enable(chip, pwm);
9421bade Boris BREZILLON 2013-01-08  352  
9421bade Boris BREZILLON 2013-01-08  353  	return 0;
9421bade Boris BREZILLON 2013-01-08  354  }
9421bade Boris BREZILLON 2013-01-08  355  
9421bade Boris BREZILLON 2013-01-08  356  static const struct pwm_ops atmel_tcb_pwm_ops = {
9421bade Boris BREZILLON 2013-01-08  357  	.request = atmel_tcb_pwm_request,
9421bade Boris BREZILLON 2013-01-08  358  	.free = atmel_tcb_pwm_free,
9421bade Boris BREZILLON 2013-01-08 @359  	.config = atmel_tcb_pwm_config,
9421bade Boris BREZILLON 2013-01-08  360  	.set_polarity = atmel_tcb_pwm_set_polarity,
9421bade Boris BREZILLON 2013-01-08  361  	.enable = atmel_tcb_pwm_enable,
9421bade Boris BREZILLON 2013-01-08  362  	.disable = atmel_tcb_pwm_disable,
83c80dc5 Axel Lin        2013-03-31  363  	.owner = THIS_MODULE,
9421bade Boris BREZILLON 2013-01-08  364  };
9421bade Boris BREZILLON 2013-01-08  365  
9421bade Boris BREZILLON 2013-01-08  366  static int atmel_tcb_pwm_probe(struct platform_device *pdev)
9421bade Boris BREZILLON 2013-01-08  367  {

:::::: The code at line 359 was first introduced by commit
:::::: 9421bade0765d8ffb86b8a99213b611278a3542a pwm: atmel: add Timer Counter Block PWM driver

:::::: TO: Boris BREZILLON <linux-arm@overkiz.com>
:::::: CC: Thierry Reding <thierry.reding@avionic-design.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 20010 bytes --]

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[kbuild test robot]

Hi Olliver,

[auto build test WARNING on pwm/for-next -- if it's inappropriate base, please suggest rules for selecting the more suitable base]

url:    https://github.com/0day-ci/linux/commits/Olliver-Schinagl/pwm-lpc18xx_pwm-use-pwm_set_chip_data/20151027-053853
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/clk/clk-pwm.c:89:25: sparse: not enough arguments for function pwm_config
   drivers/clk/clk-pwm.c: In function 'clk_pwm_probe':
   drivers/clk/clk-pwm.c:89:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(pwm, (pwm->period + 1) >> 1, pwm->period);
           ^
   In file included from drivers/clk/clk-pwm.c:15:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/gpu/drm/i915/intel_panel.c:652:19: sparse: not enough arguments for function pwm_config
   drivers/gpu/drm/i915/intel_panel.c:797:19: sparse: not enough arguments for function pwm_config
   drivers/gpu/drm/i915/intel_panel.c:1442:28: sparse: not enough arguments for function pwm_config
   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_set_backlight':
   drivers/gpu/drm/i915/intel_panel.c:652:2: error: too few arguments to function 'pwm_config'
     pwm_config(panel->backlight.pwm, duty_ns, CRC_PMIC_PWM_PERIOD_NS);
     ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_disable_backlight':
   drivers/gpu/drm/i915/intel_panel.c:797:2: error: too few arguments to function 'pwm_config'
     pwm_config(panel->backlight.pwm, 0, CRC_PMIC_PWM_PERIOD_NS);
     ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/gpu/drm/i915/intel_panel.c: In function 'pwm_setup_backlight':
   drivers/gpu/drm/i915/intel_panel.c:1442:11: error: too few arguments to function 'pwm_config'
     retval = pwm_config(panel->backlight.pwm, CRC_PMIC_PWM_PERIOD_NS,
              ^
   In file included from drivers/gpu/drm/i915/intel_panel.c:35:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/hwmon/pwm-fan.c:51:25: sparse: not enough arguments for function pwm_config
   drivers/hwmon/pwm-fan.c:240:25: sparse: not enough arguments for function pwm_config
   drivers/hwmon/pwm-fan.c:313:25: sparse: not enough arguments for function pwm_config
   drivers/hwmon/pwm-fan.c: In function '__set_pwm':
   drivers/hwmon/pwm-fan.c:51:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
           ^
   In file included from drivers/hwmon/pwm-fan.c:25:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/hwmon/pwm-fan.c: In function 'pwm_fan_probe':
   drivers/hwmon/pwm-fan.c:240:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(ctx->pwm, duty_cycle, ctx->pwm->period);
           ^
   In file included from drivers/hwmon/pwm-fan.c:25:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/hwmon/pwm-fan.c: In function 'pwm_fan_resume':
   drivers/hwmon/pwm-fan.c:313:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(ctx->pwm, duty, ctx->pwm->period);
           ^
   In file included from drivers/hwmon/pwm-fan.c:25:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/input/misc/pwm-beeper.c:56:33: sparse: not enough arguments for function pwm_config
   drivers/input/misc/pwm-beeper.c:161:27: sparse: not enough arguments for function pwm_config
   drivers/input/misc/pwm-beeper.c: In function 'pwm_beeper_event':
   drivers/input/misc/pwm-beeper.c:56:9: error: too few arguments to function 'pwm_config'
      ret = pwm_config(beeper->pwm, period / 2, period);
            ^
   In file included from drivers/input/misc/pwm-beeper.c:21:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/input/misc/pwm-beeper.c: In function 'pwm_beeper_resume':
   drivers/input/misc/pwm-beeper.c:161:3: error: too few arguments to function 'pwm_config'
      pwm_config(beeper->pwm, beeper->period / 2, beeper->period);
      ^
   In file included from drivers/input/misc/pwm-beeper.c:21:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/leds/leds-pwm.c:46:19: sparse: not enough arguments for function pwm_config
   drivers/leds/leds-pwm.c: In function '__led_pwm_set':
   drivers/leds/leds-pwm.c:46:2: error: too few arguments to function 'pwm_config'
     pwm_config(led_dat->pwm, new_duty, led_dat->period);
     ^
   In file included from drivers/leds/leds-pwm.c:22:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/regulator/pwm-regulator.c:64:25: sparse: not enough arguments for function pwm_config
   drivers/regulator/pwm-regulator.c:123:25: sparse: not enough arguments for function pwm_config
   drivers/regulator/pwm-regulator.c: In function 'pwm_regulator_set_voltage_sel':
   drivers/regulator/pwm-regulator.c:64:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(drvdata->pwm, dutycycle, pwm_reg_period);
           ^
   In file included from drivers/regulator/pwm-regulator.c:22:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/regulator/pwm-regulator.c: In function 'pwm_regulator_set_voltage':
   drivers/regulator/pwm-regulator.c:123:8: error: too few arguments to function 'pwm_config'
     ret = pwm_config(drvdata->pwm, (period / 100) * duty_cycle, period);
           ^
   In file included from drivers/regulator/pwm-regulator.c:22:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/video/backlight/lm3630a_bl.c:168:19: sparse: not enough arguments for function pwm_config
   drivers/video/backlight/lm3630a_bl.c: In function 'lm3630a_pwm_ctrl':
   drivers/video/backlight/lm3630a_bl.c:168:2: error: too few arguments to function 'pwm_config'
     pwm_config(pchip->pwmd, duty, period);
     ^
   In file included from drivers/video/backlight/lm3630a_bl.c:19:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/video/backlight/lp855x_bl.c:251:19: sparse: not enough arguments for function pwm_config
   drivers/video/backlight/lp855x_bl.c: In function 'lp855x_pwm_ctrl':
   drivers/video/backlight/lp855x_bl.c:251:2: error: too few arguments to function 'pwm_config'
     pwm_config(lp->pwm, duty, period);
     ^
   In file included from drivers/video/backlight/lp855x_bl.c:19:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
>> drivers/video/backlight/pwm_bl.c:69:19: sparse: not enough arguments for function pwm_config
   drivers/video/backlight/pwm_bl.c:108:27: sparse: not enough arguments for function pwm_config
   drivers/video/backlight/pwm_bl.c: In function 'pwm_backlight_power_off':
   drivers/video/backlight/pwm_bl.c:69:2: error: too few arguments to function 'pwm_config'
     pwm_config(pb->pwm, 0, pb->period);
     ^
   In file included from drivers/video/backlight/pwm_bl.c:22:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
   drivers/video/backlight/pwm_bl.c: In function 'pwm_backlight_update_status':
   drivers/video/backlight/pwm_bl.c:108:3: error: too few arguments to function 'pwm_config'
      pwm_config(pb->pwm, duty_cycle, pb->period);
      ^
   In file included from drivers/video/backlight/pwm_bl.c:22:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^
--
   drivers/video/fbdev/ssd1307fb.c:150:29: sparse: incorrect type in initializer (different address spaces)
   drivers/video/fbdev/ssd1307fb.c:150:29:    expected unsigned char [usertype] *vmem
   drivers/video/fbdev/ssd1307fb.c:150:29:    got char [noderef] <asn:2>*screen_base
   drivers/video/fbdev/ssd1307fb.c:226:13: sparse: incorrect type in assignment (different address spaces)
   drivers/video/fbdev/ssd1307fb.c:226:13:    expected unsigned char [noderef] [usertype] <asn:2>*dst
   drivers/video/fbdev/ssd1307fb.c:226:13:    got void *<noident>
   drivers/video/fbdev/ssd1307fb.c:228:28: sparse: incorrect type in argument 1 (different address spaces)
   drivers/video/fbdev/ssd1307fb.c:228:28:    expected void *to
   drivers/video/fbdev/ssd1307fb.c:228:28:    got unsigned char [noderef] [usertype] <asn:2>*dst
>> drivers/video/fbdev/ssd1307fb.c:299:27: sparse: not enough arguments for function pwm_config
   drivers/video/fbdev/ssd1307fb.c: In function 'ssd1307fb_init':
   drivers/video/fbdev/ssd1307fb.c:299:3: error: too few arguments to function 'pwm_config'
      pwm_config(par->pwm, par->pwm_period / 2, par->pwm_period);
      ^
   In file included from drivers/video/fbdev/ssd1307fb.c:17:0:
   include/linux/pwm.h:25:5: note: declared here
    int pwm_config(struct pwm_device *pwm, int duty_ns,
        ^

vim +89 drivers/clk/clk-pwm.c

9a74ccdb Philipp Zabel 2015-02-13  73  
9a74ccdb Philipp Zabel 2015-02-13  74  	if (!pwm->period) {
9a74ccdb Philipp Zabel 2015-02-13  75  		dev_err(&pdev->dev, "invalid PWM period\n");
9a74ccdb Philipp Zabel 2015-02-13  76  		return -EINVAL;
9a74ccdb Philipp Zabel 2015-02-13  77  	}
9a74ccdb Philipp Zabel 2015-02-13  78  
9a74ccdb Philipp Zabel 2015-02-13  79  	if (of_property_read_u32(node, "clock-frequency", &clk_pwm->fixed_rate))
9a74ccdb Philipp Zabel 2015-02-13  80  		clk_pwm->fixed_rate = NSEC_PER_SEC / pwm->period;
9a74ccdb Philipp Zabel 2015-02-13  81  
9a74ccdb Philipp Zabel 2015-02-13  82  	if (pwm->period != NSEC_PER_SEC / clk_pwm->fixed_rate &&
9a74ccdb Philipp Zabel 2015-02-13  83  	    pwm->period != DIV_ROUND_UP(NSEC_PER_SEC, clk_pwm->fixed_rate)) {
9a74ccdb Philipp Zabel 2015-02-13  84  		dev_err(&pdev->dev,
9a74ccdb Philipp Zabel 2015-02-13  85  			"clock-frequency does not match PWM period\n");
9a74ccdb Philipp Zabel 2015-02-13  86  		return -EINVAL;
9a74ccdb Philipp Zabel 2015-02-13  87  	}
9a74ccdb Philipp Zabel 2015-02-13  88  
9a74ccdb Philipp Zabel 2015-02-13 @89  	ret = pwm_config(pwm, (pwm->period + 1) >> 1, pwm->period);
9a74ccdb Philipp Zabel 2015-02-13  90  	if (ret < 0)
9a74ccdb Philipp Zabel 2015-02-13  91  		return ret;
9a74ccdb Philipp Zabel 2015-02-13  92  
9a74ccdb Philipp Zabel 2015-02-13  93  	clk_name = node->name;
9a74ccdb Philipp Zabel 2015-02-13  94  	of_property_read_string(node, "clock-output-names", &clk_name);
9a74ccdb Philipp Zabel 2015-02-13  95  
9a74ccdb Philipp Zabel 2015-02-13  96  	init.name = clk_name;
9a74ccdb Philipp Zabel 2015-02-13  97  	init.ops = &clk_pwm_ops;

:::::: The code at line 89 was first introduced by commit
:::::: 9a74ccdbbb8fa6302ae1ba606f2ef0c03d3242ab clk: Add PWM clock driver

:::::: TO: Philipp Zabel <p.zabel@pengutronix.de>
:::::: CC: Michael Turquette <mturquette@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 01/10] pwm: lpc18xx_pwm: use pwm_set_chip_data

[Olliver Schinagl]

Hey Ezequiel,

On October 26, 2015 10:58:18 PM CET, Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:
>(+ Ariel)
>
>Hi Oliver,
>
>Not sure why there's some many people in Cc for such a silly change.
>I guess you are using get_maintainers.pl on the entire patchset and get
>this rather long list.
>
I did indeed use the script and for v2 i'll split it up in several patches. The grouping that made sense to me was it was all pwm related. I'll do better next time. Sorry.

>IMO, the value of submitting patches as part of a larger series is to
>be able to
>push patches that need to be applied in some given order, or otherwise
>have some kind of logical relationship between them.
>
>However, this is not the case: it's just a very small change and has
>no relation to the rest of the patches in the series.
>I think a simple standalone patch would be better here.
>
>Also, get_maintainer.pl is just a hint, and not meant to be used as-is.
>In particular, you are missing the driver's author.
>
>On 26 October 2015 at 18:32, Olliver Schinagl
><o.schinagl@ultimaker.com> wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> The lpc18xx driver currently manipulates the pwm_device struct
>directly
>> rather then using the pwm_set_chip_data. While the current method may
>> save a clock cycle or two, it is more obvious that data is set to the
>> chip pointer (especially since it is only a single int holding
>struct.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>  drivers/pwm/pwm-lpc18xx-sct.c | 11 +++++++----
>>  drivers/pwm/pwm-sun4i.c       | 11 +++++++++++
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>
>...and this diffstat is obviously fishy.
It does.indeed, somehow i.must have accidentally merged two patches, stupid me. This.will also be addressed in the v2.

Olliver

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 07/10] pwm: gpio: Add a generic gpio based PWM driver

[Rob Herring]

On Mon, Oct 26, 2015 at 4:32 PM, Olliver Schinagl
<o.schinagl@ultimaker.com> wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
>
> This patch adds a bit-banging gpio PWM driver. It makes use of hrtimers,
> to allow nano-second resolution, though it obviously strongly depends on
> the switching speed of the gpio pins, hrtimer and system load.
>
> Each pwm node can have 1 or more "pwm-gpio" entries, which will be
> treated as pwm's as part of a pwm chip.
>
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-gpio.txt |  18 ++
>  MAINTAINERS                                        |   5 +
>  drivers/pwm/Kconfig                                |  15 ++
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-gpio.c                             | 253 +++++++++++++++++++++
>  5 files changed, 292 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>  create mode 100644 drivers/pwm/pwm-gpio.c
>
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.txt b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> new file mode 100644
> index 0000000..336f61f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> @@ -0,0 +1,18 @@
> +Generic GPIO bit-banged PWM driver
> +
> +Required properties:
> +  - compatible: should be "pwm-gpio"
> +  - #pwm-cells: should be 3, see pwm.txt in this directory for a general
> +    description of the cells format.
> +  - pwm-gpios: one or more gpios describing the used gpio, see the gpio
> +    bindings for the used gpio driver.

I'm not sure there is really much advantage to having multiple gpios
per node. It would simplify the driver a bit not to, but I don't feel
strongly either way.

> +
> +Example:
> +#include <dt-bindings/gpio/gpio.h>
> +
> +       pwm: pwm@0 {

Unit address should be associated with a reg property, so drop it.

> +               compatible = "pwm-gpio";
> +               #pwm-cells = 3;
> +               pwm-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>;
> +               pwm-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;

This would not actually compile. You can't have 2 properties of the same name.

> +       };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 7ba7ab7..0ae7fbf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4555,6 +4555,11 @@ F:       drivers/i2c/muxes/i2c-mux-gpio.c
>  F:     include/linux/i2c-mux-gpio.h
>  F:     Documentation/i2c/muxes/i2c-mux-gpio
>
> +GENERIC GPIO PWM DRIVER
> +M:     Olliver Schinagl <oliver@schinagl.nl>
> +S:     Maintained
> +F:     drivers/pwm/pwm-gpio.c

Can you add the binding doc too.

Rob

Re: [PATCH 07/10] pwm: gpio: Add a generic gpio based PWM driver

[Olliver Schinagl]

Hey Rob,

On October 27, 2015 8:42:48 AM CET, Rob Herring <robh+dt@kernel.org> wrote:
>On Mon, Oct 26, 2015 at 4:32 PM, Olliver Schinagl
><o.schinagl@ultimaker.com> wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> This patch adds a bit-banging gpio PWM driver. It makes use of
>hrtimers,
>> to allow nano-second resolution, though it obviously strongly depends
>on
>> the switching speed of the gpio pins, hrtimer and system load.
>>
>> Each pwm node can have 1 or more "pwm-gpio" entries, which will be
>> treated as pwm's as part of a pwm chip.
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>  Documentation/devicetree/bindings/pwm/pwm-gpio.txt |  18 ++
>>  MAINTAINERS                                        |   5 +
>>  drivers/pwm/Kconfig                                |  15 ++
>>  drivers/pwm/Makefile                               |   1 +
>>  drivers/pwm/pwm-gpio.c                             | 253
>+++++++++++++++++++++
>>  5 files changed, 292 insertions(+)
>>  create mode 100644
>Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>>  create mode 100644 drivers/pwm/pwm-gpio.c
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>> new file mode 100644
>> index 0000000..336f61f
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>> @@ -0,0 +1,18 @@
>> +Generic GPIO bit-banged PWM driver
>> +
>> +Required properties:
>> +  - compatible: should be "pwm-gpio"
>> +  - #pwm-cells: should be 3, see pwm.txt in this directory for a
>general
>> +    description of the cells format.
>> +  - pwm-gpios: one or more gpios describing the used gpio, see the
>gpio
>> +    bindings for the used gpio driver.
>
>I'm not sure there is really much advantage to having multiple gpios
>per node. It would simplify the driver a bit not to, but I don't feel
>strongly either way.
I figured this way you have one (or more) gpio 'chips' and then per chip 1 or more outputs. I actually thing pne would use several gpios rather then several 'chips'. Most hardware oprates the same way i thought.
>
>> +
>> +Example:
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +       pwm: pwm@0 {
>
>Unit address should be associated with a reg property, so drop it.
Done

>
>> +               compatible = "pwm-gpio";
>> +               #pwm-cells = 3;
>> +               pwm-gpios = <&pio 7 1 GPIO_ACTIVE_LOW>;
>> +               pwm-gpios = <&pio 7 2 GPIO_ACTIVE_LOW>;
>
>This would not actually compile. You can't have 2 properties of the
>same name.
A bad example is still a bug. Fixed.
>
>> +       };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7ba7ab7..0ae7fbf 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -4555,6 +4555,11 @@ F:       drivers/i2c/muxes/i2c-mux-gpio.c
>>  F:     include/linux/i2c-mux-gpio.h
>>  F:     Documentation/i2c/muxes/i2c-mux-gpio
>>
>> +GENERIC GPIO PWM DRIVER
>> +M:     Olliver Schinagl <oliver@schinagl.nl>
>> +S:     Maintained
>> +F:     drivers/pwm/pwm-gpio.c
>
>Can you add the binding doc too.
Sure, done.

>
>Rob

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [PATCH 04/10] pwm: core: use bitops

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:35PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> The pwm header defines bits manually while there is a nice bitops.h with
> a BIT() macro. Use the BIT() macro to set bits in pwm.h
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  include/linux/pwm.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)

I don't think this is a useful change. The BIT() macro needs the same
number of characters to type at the expense of requiring an additional
include.

Thierry

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

Re: [PATCH 04/10] pwm: core: use bitops

[Olliver Schinagl]

Hey Thierry,

but why have the bit macro at all then :)

But that choice I guess I leave to you, as it's your section, I know 
some submaintainers prefer it and want it to be used, so I guess it's 
something in general kernel wide that should be desided on, BIT() macro 
preferred or not.

Olliver

On 06-11-15 15:46, Thierry Reding wrote:
> On Mon, Oct 26, 2015 at 10:32:35PM +0100, Olliver Schinagl wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> The pwm header defines bits manually while there is a nice bitops.h with
>> a BIT() macro. Use the BIT() macro to set bits in pwm.h
>>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   include/linux/pwm.h | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
> I don't think this is a useful change. The BIT() macro needs the same
> number of characters to type at the expense of requiring an additional
> include.
>
> Thierry

-- 
Met vriendelijke groeten, Kind regards, 与亲切的问候

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

Re: [PATCH 05/10] pwm: sysfs: do not unnecessarily store result in var

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:36PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> Use the result of pwm_is_enabled directly instead of storing it first.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/sysfs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 06/10] pwm: sysfs: make use of the DEVICE_ATTR_[RW][WO] macro's

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:37PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> For the npwm property the pwm sysfs interface already made use of the
> DEVICE_ATTR_RO macro. This patch expands this to the other sysfs
> properties so that the code base is concise and makes use of this
> helpful macro.
> 
> This has the advantage of slightly reducing the code size, improving
> readability and no longer using magic values for permissions.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/sysfs.c | 72 ++++++++++++++++++++++++++---------------------------
>  1 file changed, 36 insertions(+), 36 deletions(-)

Applied, thanks.

Thierry

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

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> Some hardware PWM's have the possibility to only send out one (or more)
> pulses. This can be quite a useful feature in case one wants or needs
> only a single pulse, but at the exact width.
> 
> Additionally, if multiple pulses are possible, outputting a fixed amount
> of pulses can be useful for various timing specific purposes.

I see how theoretically this would be nice to have. But I'm reluctant to
merge this feature if there aren't any users. What drivers in the kernel
would want to use this feature? Are there new drivers being worked on
that will need this?

> A few new functions have been expanded or added for this new behavior.
> 
> * pwm_config()	now takes an additional parameter to setup the number of
> 		pulses to output. The driver may force this to 0 or 1
> 		for if example if this feature is not or only partially
> 		supported

This is problematic because you need to atomically update all drivers
and users (the kbuild robot already told you that you didn't do this).
To make things easier I suggest you wait with this change until the
atomic PWM patches have been merged, at which point it should become a
lot easier to deal with this kind of extension.

> * pwm_[sg]et_pulse_count()	get or set the number of pulses the pwm
> 				framework is configured for
> * pwm_get_pulse_count_max()	get the maximum number of pulses the pwm
> 				driver supports
> * pwm_pulse()		Tell the PWM to emit a pre-configured number of pulses

Isn't this essentially the same as pwm_enable()? I'd think that if the
PWM is configured to output pulses, then pwm_enable() would simply do
what it's been configured to do (emit the pulses). Why the need for an
additional function?

> * pwm_pulse_done()	an internal function for drivers to call when
> 			they have completed their pre-configured number
> 			of pulses
> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
> 			yielding a little more information than just
> 			pwm_is_enabled()

Similarily, I'd think that once the PWM is done executing the series of
pulses that it was configured for it would be automatically disabled. A
consumer could then simply use pwm_is_enabled() and drivers could call
pwm_disable() on their PWM to mark them as disabled when they're done
pulsing.

> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/core.c      | 30 +++++++++++++++++++----
>  drivers/pwm/pwm-gpio.c  |  3 ++-
>  drivers/pwm/pwm-sun4i.c |  3 ++-
>  drivers/pwm/sysfs.c     | 58 ++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/pwm.h     | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 147 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
> index 3f9df3e..e2c1c0a 100644
> --- a/drivers/pwm/core.c
> +++ b/drivers/pwm/core.c
> @@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free);
>   * @pwm: PWM device
>   * @duty_ns: "on" time (in nanoseconds)
>   * @period_ns: duration (in nanoseconds) of one cycle
> + * @pulse_count: number of pulses (periods) to output on pwm_pulse
>   *
>   * Returns: 0 on success or a negative error code on failure.
>   */
> -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns,
> +	       unsigned int pulse_count)

Like I said, this is problematic because every driver and every consumer
now needs to be aware of pulsing. Once the PWM atomic patches are merged
this will become easier to do because the pulse configuration would be a
part of the atomic state, and hence can be conveniently ignored by users
and driver alike.

Thierry

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

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Olliver Schinagl]

Hey Thierry,

On 06-11-15 16:18, Thierry Reding wrote:
> On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
>> From: Olliver Schinagl <oliver@schinagl.nl>
>>
>> Some hardware PWM's have the possibility to only send out one (or more)
>> pulses. This can be quite a useful feature in case one wants or needs
>> only a single pulse, but at the exact width.
>>
>> Additionally, if multiple pulses are possible, outputting a fixed amount
>> of pulses can be useful for various timing specific purposes.
> I see how theoretically this would be nice to have. But I'm reluctant to
> merge this feature if there aren't any users. What drivers in the kernel
> would want to use this feature? Are there new drivers being worked on
> that will need this?
I should have brought this up as to why I added this, I'm working on a 
stepper driver framework (inspired by the pwm framework actually) and 
rotating moters by x degree's you do by sending pulses, using controlled 
pulses (timing wise) you can precisely move stepper motors. Yes we can 
do this reasonably accurate in software, but doing it in hardware is so 
much nicer.
>
>> A few new functions have been expanded or added for this new behavior.
>>
>> * pwm_config()	now takes an additional parameter to setup the number of
>> 		pulses to output. The driver may force this to 0 or 1
>> 		for if example if this feature is not or only partially
>> 		supported
> This is problematic because you need to atomically update all drivers
> and users (the kbuild robot already told you that you didn't do this).
> To make things easier I suggest you wait with this change until the
> atomic PWM patches have been merged, at which point it should become a
> lot easier to deal with this kind of extension.
yes, I think i mentioned this in the cover letter, I wanted to get your 
input whilst waiting for Boris's patches. So I deffinatly want to 
combine it then, just getting some head work started :)
>
>> * pwm_[sg]et_pulse_count()	get or set the number of pulses the pwm
>> 				framework is configured for
>> * pwm_get_pulse_count_max()	get the maximum number of pulses the pwm
>> 				driver supports
>> * pwm_pulse()		Tell the PWM to emit a pre-configured number of pulses
> Isn't this essentially the same as pwm_enable()? I'd think that if the
> PWM is configured to output pulses, then pwm_enable() would simply do
> what it's been configured to do (emit the pulses). Why the need for an
> additional function?
pwm_pulse() should be dropped, I think I accidentally left that in the 
documentation, sorry.
>
>> * pwm_pulse_done()	an internal function for drivers to call when
>> 			they have completed their pre-configured number
>> 			of pulses
>> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
>> 			yielding a little more information than just
>> 			pwm_is_enabled()
> Similarily, I'd think that once the PWM is done executing the series of
> pulses that it was configured for it would be automatically disabled. A
> consumer could then simply use pwm_is_enabled() and drivers could call
> pwm_disable() on their PWM to mark them as disabled when they're done
> pulsing.
I agree, pulseating can be dropped too as we know that a) the pulse flag 
is set, b) we are enabled. But I'm not sure now if the flag is exported 
to sysfs, in any case, sysfs should just check the pulseating flag?
>
>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>> ---
>>   drivers/pwm/core.c      | 30 +++++++++++++++++++----
>>   drivers/pwm/pwm-gpio.c  |  3 ++-
>>   drivers/pwm/pwm-sun4i.c |  3 ++-
>>   drivers/pwm/sysfs.c     | 58 ++++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/pwm.h     | 64 ++++++++++++++++++++++++++++++++++++++++++++++---
>>   5 files changed, 147 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c
>> index 3f9df3e..e2c1c0a 100644
>> --- a/drivers/pwm/core.c
>> +++ b/drivers/pwm/core.c
>> @@ -432,22 +432,29 @@ EXPORT_SYMBOL_GPL(pwm_free);
>>    * @pwm: PWM device
>>    * @duty_ns: "on" time (in nanoseconds)
>>    * @period_ns: duration (in nanoseconds) of one cycle
>> + * @pulse_count: number of pulses (periods) to output on pwm_pulse
>>    *
>>    * Returns: 0 on success or a negative error code on failure.
>>    */
>> -int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns)
>> +int pwm_config(struct pwm_device *pwm, int duty_ns, int period_ns,
>> +	       unsigned int pulse_count)
> Like I said, this is problematic because every driver and every consumer
> now needs to be aware of pulsing. Once the PWM atomic patches are merged
> this will become easier to do because the pulse configuration would be a
> part of the atomic state, and hence can be conveniently ignored by users
> and driver alike.
I agree :) I'll take your initial comments and work with those so far in 
cleaning stuff up. Feel free to get back to me about the validity of the 
pwm_pulse for steppers generally
> Thierry

-- 
Met vriendelijke groeten, Kind regards, 与亲切的问候

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

Re: [PATCH 07/10] pwm: gpio: Add a generic gpio based PWM driver

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:38PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> This patch adds a bit-banging gpio PWM driver. It makes use of hrtimers,
> to allow nano-second resolution, though it obviously strongly depends on
> the switching speed of the gpio pins, hrtimer and system load.
> 
> Each pwm node can have 1 or more "pwm-gpio" entries, which will be
> treated as pwm's as part of a pwm chip.
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-gpio.txt |  18 ++
>  MAINTAINERS                                        |   5 +
>  drivers/pwm/Kconfig                                |  15 ++
>  drivers/pwm/Makefile                               |   1 +
>  drivers/pwm/pwm-gpio.c                             | 253 +++++++++++++++++++++
>  5 files changed, 292 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.txt
>  create mode 100644 drivers/pwm/pwm-gpio.c
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.txt b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> new file mode 100644
> index 0000000..336f61f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.txt
> @@ -0,0 +1,18 @@
> +Generic GPIO bit-banged PWM driver
> +
> +Required properties:
> +  - compatible: should be "pwm-gpio"
> +  - #pwm-cells: should be 3, see pwm.txt in this directory for a general
> +    description of the cells format.
> +  - pwm-gpios: one or more gpios describing the used gpio, see the gpio
> +    bindings for the used gpio driver.

I agree with Rob that a single GPIO per PWM chip would be more
straightforward here. Having multiple GPIOs per chip, and hence a
multi-channel chip, suggests that they are in some fashion grouped
whereas in reality they are really completely separate.

> +Example:
> +#include <dt-bindings/gpio/gpio.h>
> +
> +	pwm: pwm@0 {
> +		compatible = "pwm-gpio";
> +		#pwm-cells = 3;

I think the correct syntax for this would be: #pwm-cells = <3>;

> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 0cfaf6b..c0bc296 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -170,6 +170,21 @@ config PWM_IMG
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-img
>  
> +config PWM_GPIO
> +	tristate "Generic GPIO bit-banged PWM driver"
> +	depends on OF
> +	depends on GPIOLIB
> +	help
> +	  Some platforms do not offer any hardware PWM capabilities but do have
> +	  General Purpose Input Output (GPIO) pins available. Using the kernels
> +	  High-Resolution Timer API this driver tries to toggle GPIO using the
> +	  generic kernel PWM framework. The maximum frequency and/or accuracy
> +	  is dependent on several factors such as system load and the maximum
> +	  speed a pin can be toggled at the hardware.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-gpio.
> +

This is oddly sorted. Entries should be ordered alphabetically.

>  config PWM_IMX
>  	tristate "i.MX PWM support"
>  	depends on ARCH_MXC
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 69b8275..96aa9aa 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
>  obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> +obj-$(CONFIG_PWM_GPIO)		+= pwm-gpio.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
>  obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o

The ordering is correct here. Perhaps just something that got introduced
by oversight in a rebase?

> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 0000000..8b588fb
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,253 @@
> +/*
> + * Copyright (c) 2015 Olliver Schinagl <oliver@schinagl.nl>
> + *
> + * 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
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * This driver adds a high-resolution timer based PWM driver. Since this is a
> + * bit-banged driver, accuracy will always depend on a lot of factors, such as
> + * GPIO toggle speed and system load.

I'm not sure this is helpful. The Kconfig snippet already has this
information.

> + */
> +
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/errno.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/ktime.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>

Please keep these sorted alphabetically.

> +
> +#define DRV_NAME "pwm-gpio"

You only have two locations where you use this and it's going to be ABI
immediately, hence you will never be able to change it anyway. So there
is really no need for this define.

> +
> +struct gpio_pwm_data {

In my opinion the _data suffix here is redundant.

> +	struct hrtimer timer;
> +	struct gpio_desc *gpiod;
> +	bool polarity;

I think this should probably be called "invert" judging by how it's
used.

> +	bool pin_on;
> +	int on_time;
> +	int off_time;

These can probably be unsigned.

> +	bool run;

How is this different from pwm_is_enabled()?

> +};
> +
> +struct gpio_pwm_chip {
> +	struct pwm_chip chip;
> +};

If you restrict the driver to a single PWM channel you can merge the
gpio_pwm_data structure into this and vastly simplify the management of
resources.

> +
> +static void gpio_pwm_off(struct gpio_pwm_data *gpio_data)
> +{
> +	gpiod_set_value_cansleep(gpio_data->gpiod, gpio_data->polarity ? 0 : 1);
> +}
> +
> +static void gpio_pwm_on(struct gpio_pwm_data *gpio_data)
> +{
> +	gpiod_set_value_cansleep(gpio_data->gpiod, gpio_data->polarity ? 1 : 0);
> +}

According to this, polarity == true means that polarity is normal.

> +static int gpio_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> +				 enum pwm_polarity polarity)
> +{
> +	struct gpio_pwm_data *gpio_data = pwm_get_chip_data(pwm);
> +
> +	gpio_data->polarity = (polarity != PWM_POLARITY_NORMAL) ? true : false;
> +
> +	return 0;
> +}

But here polarity == true only if the PWM is configured for inverse
polarity.

> +static int gpio_pwm_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +	struct gpio_pwm_chip *gpio_chip;
> +	int npwm, i;

i can be unsigned int.

> +	int hrtimer = 0;

This can be unsigned int.

> +
> +	npwm = of_gpio_named_count(pdev->dev.of_node, "pwm-gpios");
> +	if (npwm < 1)
> +		return -ENODEV;

Don't you want to propagate error codes from of_gpio_named_count()? So
this should probably be:

	if (npwm < 0)
		return npwm;

	if (npwm < 1)
		return -ENODEV;

> +
> +	gpio_chip = devm_kzalloc(&pdev->dev, sizeof(*gpio_chip), GFP_KERNEL);
> +	if (!gpio_chip)
> +		return -ENOMEM;
> +
> +	gpio_chip->chip.dev = &pdev->dev;
> +	gpio_chip->chip.ops = &gpio_pwm_ops;
> +	gpio_chip->chip.base = -1;
> +	gpio_chip->chip.npwm = npwm;
> +	gpio_chip->chip.of_xlate = of_pwm_xlate_with_flags;
> +	gpio_chip->chip.of_pwm_n_cells = 3;
> +	gpio_chip->chip.can_sleep = true;
> +
> +	ret = pwmchip_add(&gpio_chip->chip);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to add PWM chip: %d\n", ret);
> +		return ret;
> +	}

You should call pwmchip_add() as late as possible. The pwmchip_add()
will register the device with sysfs and hence make it available to
userspace. Userspace may immediately start using it, but the below can
easily lead to the device going away shortly after it was made
available.

> +	for (i = 0; i < npwm; i++) {
> +		struct gpio_desc *gpiod;
> +		struct gpio_pwm_data *gpio_data;
> +
> +		gpiod = devm_gpiod_get_index(&pdev->dev, "pwm", i,
> +					     GPIOD_OUT_LOW);
> +		if (IS_ERR(gpiod)) {
> +			int error;
> +
> +			error = PTR_ERR(gpiod);
> +			if (error != -EPROBE_DEFER)
> +				dev_err(&pdev->dev,
> +					"failed to get gpio flags, error: %d\n",
> +					error);
> +			return error;
> +		}
> +
> +		gpio_data = devm_kzalloc(&pdev->dev, sizeof(*gpio_data),
> +					 GFP_KERNEL);
> +
> +		hrtimer_init(&gpio_data->timer,
> +			     CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +		gpio_data->timer.function = &gpio_pwm_timer;
> +		gpio_data->gpiod = gpiod;
> +		gpio_data->pin_on = false;
> +		gpio_data->run = false;
> +
> +		if (hrtimer_is_hres_active(&gpio_data->timer))
> +			hrtimer++;
> +
> +		pwm_set_chip_data(&gpio_chip->chip.pwms[i], gpio_data);
> +	}

It seems that the only reason why you need to call pwmchip_add() early
is so that the PWMs are allocated when you initialize them here. That's
easily avoided if you opt for a single PWM per chip, since you can
simply manage the resources as part of the chip rather than relying on
the per-PWM chip data.

> +	if (!hrtimer)
> +		dev_warn(&pdev->dev, "unable to use High-Resolution timer,");
> +		dev_warn(&pdev->dev, "%s is restricted to low resolution.",
> +			 DRV_NAME);

You'll need curly braces around these two statements, otherwise you'll
always show the second part of the warning. Also in the above there is
no newline. It's also not going to produce the desired output, even if
you use curly braces because dev_warn() will cause KERN_WARNING prefix
to be output and mess up the output. You'd want pr_cont() here, but I
don't think that's necessary. Perhaps shorten this to something like:

	if (!hrtimer)
		dev_warn(&pdev->dev, "HR timer unavailable, restricting to "
			             "low resolution\n");

Also there's no need to include DRV_NAME in the message since it's
already part of the prefix added by dev_warn().

> +	platform_set_drvdata(pdev, gpio_chip);
> +
> +	dev_info(&pdev->dev, "%d gpio pwms loaded\n", npwm);

Please either drop this or make it debug level. No need to brag about
success. We're interested in seeing error messages. Also if you want to
keep this, please replace gpio by GPIO and pwms by PWMs.

> +static int gpio_pwm_remove(struct platform_device *pdev)
> +{
> +	struct gpio_pwm_chip *gpio_chip;
> +	int i;

unsigned int.

> +
> +	gpio_chip = platform_get_drvdata(pdev);

Any reason why you put this on the same line where you declare
gpio_chip?

> +	for (i = 0; i < gpio_chip->chip.npwm; i++) {
> +		struct gpio_pwm_data *gpio_data;
> +
> +		gpio_data = pwm_get_chip_data(&gpio_chip->chip.pwms[i]);
> +
> +		hrtimer_cancel(&gpio_data->timer);
> +	}
> +
> +	return pwmchip_remove(&gpio_chip->chip);
> +}
> +
> +static const struct of_device_id gpio_pwm_of_match[] = {
> +	{ .compatible = DRV_NAME, },
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, gpio_pwm_of_match);
> +
> +static struct platform_driver gpio_pwm_driver = {
> +	.probe = gpio_pwm_probe,
> +	.remove = gpio_pwm_remove,
> +	.driver = {
> +		.name = DRV_NAME,
> +		.of_match_table = of_match_ptr(gpio_pwm_of_match),

No need for this conditional. You're effectively OF-only because of the
call to of_gpio_named_count() in ->probe(). If you drop multiple-GPIO
support you'll get non-OF support for free as far as I can tell, so one
more reason to simplify.

But then if you do support non-OF, you'll need to protect the OF device
table with appropriate #ifdef guards, otherwise the compiler will
complain about it being unused.

Thierry

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

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Thierry Reding]

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

On Fri, Nov 06, 2015 at 04:46:54PM +0100, Olliver Schinagl wrote:
> Hey Thierry,
> 
> On 06-11-15 16:18, Thierry Reding wrote:
> >On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
> >>From: Olliver Schinagl <oliver@schinagl.nl>
> >>
> >>Some hardware PWM's have the possibility to only send out one (or more)
> >>pulses. This can be quite a useful feature in case one wants or needs
> >>only a single pulse, but at the exact width.
> >>
> >>Additionally, if multiple pulses are possible, outputting a fixed amount
> >>of pulses can be useful for various timing specific purposes.
> >I see how theoretically this would be nice to have. But I'm reluctant to
> >merge this feature if there aren't any users. What drivers in the kernel
> >would want to use this feature? Are there new drivers being worked on
> >that will need this?
> I should have brought this up as to why I added this, I'm working on a
> stepper driver framework (inspired by the pwm framework actually) and
> rotating moters by x degree's you do by sending pulses, using controlled
> pulses (timing wise) you can precisely move stepper motors. Yes we can do
> this reasonably accurate in software, but doing it in hardware is so much
> nicer.

So is this going to be a kernel framework for stepper motors? If you say
you rotate the motors by sending pulses, doesn't that mean that the PWM
framework with pulse support would be enough? Or are there dedicated
stepper chips that you plan to support, with PWM + pulses being the
fallback for when you don't have one of those chips?

> >>* pwm_pulse_done()	an internal function for drivers to call when
> >>			they have completed their pre-configured number
> >>			of pulses
> >>* pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
> >>			yielding a little more information than just
> >>			pwm_is_enabled()
> >Similarily, I'd think that once the PWM is done executing the series of
> >pulses that it was configured for it would be automatically disabled. A
> >consumer could then simply use pwm_is_enabled() and drivers could call
> >pwm_disable() on their PWM to mark them as disabled when they're done
> >pulsing.
> I agree, pulseating can be dropped too as we know that a) the pulse flag is
> set, b) we are enabled. But I'm not sure now if the flag is exported to
> sysfs, in any case, sysfs should just check the pulseating flag?

Can't you derive that information simply by looking at the enable and
pulses attributes? If enable == 1 and pulses > 0 you know the PWM is
pulsing. If enable == 1 and pulses == 0 you know it's in regular mode.

Thierry

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

Re: [PATCH 02/10] pwm: sunxi: fix whitespace issue

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:33PM +0100, Olliver Schinagl wrote:
> From: Olliver Schinagl <oliver@schinagl.nl>
> 
> This patch changes no code, it just fixes the whitespacing
> 
> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
> ---
>  drivers/pwm/pwm-sun4i.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, with a slightly modified commit message.

Thanks,
Thierry

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

Re: [PATCH 03/10] pwm: sunxi: Yield some time to the pwm-block to become ready

[Thierry Reding]

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

On Mon, Oct 26, 2015 at 10:32:34PM +0100, Olliver Schinagl wrote:
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
> 
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
> 
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
> 
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)

This looks okay to me (except for one minor thing I noticed, see below),
but I'd like an Acked-by from one of the sunxi people. Maxime, any
comments on this?

> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 58ff424..4d84d9d 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>  	u64 clk_rate, div = 0;
>  	unsigned int prescaler = 0;
>  	int err;
> +	int ret = 0;

Why not reuse err?

Thierry

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

Re: [PATCH 08/10] pwm: core: add pulse feature to the PWM framework

[Olliver Schinagl]

Hey Thierry,

On 06-11-15 17:05, Thierry Reding wrote:
> On Fri, Nov 06, 2015 at 04:46:54PM +0100, Olliver Schinagl wrote:
>> Hey Thierry,
>>
>> On 06-11-15 16:18, Thierry Reding wrote:
>>> On Mon, Oct 26, 2015 at 10:32:39PM +0100, Olliver Schinagl wrote:
>>>> From: Olliver Schinagl <oliver@schinagl.nl>
>>>>
>>>> Some hardware PWM's have the possibility to only send out one (or more)
>>>> pulses. This can be quite a useful feature in case one wants or needs
>>>> only a single pulse, but at the exact width.
>>>>
>>>> Additionally, if multiple pulses are possible, outputting a fixed amount
>>>> of pulses can be useful for various timing specific purposes.
>>> I see how theoretically this would be nice to have. But I'm reluctant to
>>> merge this feature if there aren't any users. What drivers in the kernel
>>> would want to use this feature? Are there new drivers being worked on
>>> that will need this?
>> I should have brought this up as to why I added this, I'm working on a
>> stepper driver framework (inspired by the pwm framework actually) and
>> rotating moters by x degree's you do by sending pulses, using controlled
>> pulses (timing wise) you can precisely move stepper motors. Yes we can do
>> this reasonably accurate in software, but doing it in hardware is so much
>> nicer.
> So is this going to be a kernel framework for stepper motors? If you say
> you rotate the motors by sending pulses, doesn't that mean that the PWM
> framework with pulse support would be enough? Or are there dedicated
> stepper chips that you plan to support, with PWM + pulses being the
> fallback for when you don't have one of those chips?
Well I'll have to investigate more into what other chips do, but 
generally speaking from what I know so far, that often you supply a 
stepper driver chip a variable voltage (via a regular pwm) to setup the 
current control, you have gpio's for direction, enable etc, and you 
'pulse' for each step you want the motor to take. There are of course 
some chips that have more logic, that work via i2c and spi interfaces.
>
>>>> * pwm_pulse_done()	an internal function for drivers to call when
>>>> 			they have completed their pre-configured number
>>>> 			of pulses
>>>> * pwm_is_pulsing()	tells the callers if the pwm is busy pulsing,
>>>> 			yielding a little more information than just
>>>> 			pwm_is_enabled()
>>> Similarily, I'd think that once the PWM is done executing the series of
>>> pulses that it was configured for it would be automatically disabled. A
>>> consumer could then simply use pwm_is_enabled() and drivers could call
>>> pwm_disable() on their PWM to mark them as disabled when they're done
>>> pulsing.
>> I agree, pulseating can be dropped too as we know that a) the pulse flag is
>> set, b) we are enabled. But I'm not sure now if the flag is exported to
>> sysfs, in any case, sysfs should just check the pulseating flag?
> Can't you derive that information simply by looking at the enable and
> pulses attributes? If enable == 1 and pulses > 0 you know the PWM is
> pulsing. If enable == 1 and pulses == 0 you know it's in regular mode.
oh right, yes you can :)

olliver
>
> Thierry

-- 
Met vriendelijke groeten, Kind regards, 与亲切的问候

Olliver Schinagl
Software Engineer
Research & Development
Ultimaker B.V.

Re: [PATCH 03/10] pwm: sunxi: Yield some time to the pwm-block to become ready

[Chen-Yu Tsai]

On Tue, Oct 27, 2015 at 5:32 AM, Olliver Schinagl
<o.schinagl@ultimaker.com> wrote:
> The pwm-block of some of the sunxi chips feature a 'ready' flag to
> indicate the software that it is ready for new commands.
>
> Right now, when we call pwm_config and set the period, we write the
> values to the registers, and turn off the clock to the IP. Because of
> this, the hardware does not have time to configure the hardware and set
> the 'ready' flag.
>
> By running the clock just before making new changes and before checking
> if the hardware is ready, the hardware has time to reconfigure itself
> and set the clear the flag appropriately.
>
> Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
> ---
>  drivers/pwm/pwm-sun4i.c | 43 +++++++++++++++++++++++++------------------
>  1 file changed, 25 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 58ff424..4d84d9d 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -104,6 +104,22 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         u64 clk_rate, div = 0;
>         unsigned int prescaler = 0;
>         int err;
> +       int ret = 0;
> +
> +       /* Let the PWM hardware run before making any changes. We do this to
> +        * allow the hardware to have some time to clear the 'ready' flag.
> +        */
> +       err = clk_prepare_enable(sun4i_pwm->clk);
> +       if (err) {
> +               dev_err(chip->dev, "failed to enable PWM clock\n");
> +               return err;
> +       }
> +       spin_lock(&sun4i_pwm->ctrl_lock);
> +       val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> +       clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       val |= BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +       spin_unlock(&sun4i_pwm->ctrl_lock);
>
>         clk_rate = clk_get_rate(sun4i_pwm->clk);
>
> @@ -136,7 +152,9 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>
>                 if (div - 1 > PWM_PRD_MASK) {
>                         dev_err(chip->dev, "period exceeds the maximum value\n");
> -                       return -EINVAL;
> +                       ret = -EINVAL;
> +                       spin_lock(&sun4i_pwm->ctrl_lock);
> +                       goto out;
>                 }
>         }
>
> @@ -145,26 +163,14 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         do_div(div, period_ns);
>         dty = div;
>
> -       err = clk_prepare_enable(sun4i_pwm->clk);
> -       if (err) {
> -               dev_err(chip->dev, "failed to enable PWM clock\n");
> -               return err;
> -       }
> -
>         spin_lock(&sun4i_pwm->ctrl_lock);
>         val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
> -
>         if (sun4i_pwm->data->has_rdy && (val & PWM_RDY(pwm->hwpwm))) {

Instead of moving the code around to try to give the hardware some unspecified
time to run, could we use a tight busy loop with a timeout to read the register
and check if it's been cleared? I think that works better with cpufreq as well.

Thanks.

ChenYu

> -               spin_unlock(&sun4i_pwm->ctrl_lock);
> -               clk_disable_unprepare(sun4i_pwm->clk);
> -               return -EBUSY;
> -       }
> -
> -       clk_gate = val & BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> -       if (clk_gate) {
> -               val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> -               sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
> +               ret = -EBUSY;
> +               goto out;
>         }
> +       val &= ~BIT_CH(PWM_CLK_GATING, pwm->hwpwm);
> +       sun4i_pwm_writel(sun4i_pwm, val, PWM_CTRL_REG);
>
>         val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>         val &= ~BIT_CH(PWM_PRESCAL_MASK, pwm->hwpwm);
> @@ -174,6 +180,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         val = (dty & PWM_DTY_MASK) | PWM_PRD(prd);
>         sun4i_pwm_writel(sun4i_pwm, val, PWM_CH_PRD(pwm->hwpwm));
>
> +out:
>         if (clk_gate) {
>                 val = sun4i_pwm_readl(sun4i_pwm, PWM_CTRL_REG);
>                 val |= clk_gate;
> @@ -183,7 +190,7 @@ static int sun4i_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>         spin_unlock(&sun4i_pwm->ctrl_lock);
>         clk_disable_unprepare(sun4i_pwm->clk);
>
> -       return 0;
> +       return ret;
>  }
>
>  static int sun4i_pwm_set_polarity(struct pwm_chip *chip, struct pwm_device *pwm,
> --
> 2.6.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/10] pwm: core: use bitops

[Andy Shevchenko]

On Fri, Nov 6, 2015 at 4:49 PM, Olliver Schinagl
<o.schinagl@ultimaker.com> wrote:
> Hey Thierry,
>
> but why have the bit macro at all then :)

For my opinion, it's good to use in new code, or when you have this
change as a continuation of bigger series.
Though, others might have a different one :-)

>
> But that choice I guess I leave to you, as it's your section, I know some
> submaintainers prefer it and want it to be used, so I guess it's something
> in general kernel wide that should be desided on, BIT() macro preferred or
> not.
>
> Olliver
>
>
> On 06-11-15 15:46, Thierry Reding wrote:
>>
>> On Mon, Oct 26, 2015 at 10:32:35PM +0100, Olliver Schinagl wrote:
>>>
>>> From: Olliver Schinagl <oliver@schinagl.nl>
>>>
>>> The pwm header defines bits manually while there is a nice bitops.h with
>>> a BIT() macro. Use the BIT() macro to set bits in pwm.h
>>>
>>> Signed-off-by: Olliver Schinagl <oliver@schinagl.nl>
>>> ---
>>>   include/linux/pwm.h | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> I don't think this is a useful change. The BIT() macro needs the same
>> number of characters to type at the expense of requiring an additional
>> include.
>>
>> Thierry
>
>
> --
> Met vriendelijke groeten, Kind regards, 与亲切的问候
>
> Olliver Schinagl
> Software Engineer
> Research & Development
> Ultimaker B.V.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/



-- 
With Best Regards,
Andy Shevchenko