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

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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)

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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),
};

/**

[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

Post by Olliver Schinagl
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
---
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 ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Andy Shevchenko]

On Fri, Nov 6, 2015 at 4:49 PM, Olliver Schinagl

Post by Olliver Schinagl
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 :-)

Post by Olliver Schinagl
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

Post by Olliver Schinagl
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
---
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
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Olliver Schinagl]

From: Olliver Schinagl <***@schinagl.nl>

This patch changes no code, it just fixes the whitespacing

Signed-off-by: Olliver Schinagl <***@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;

[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 <***@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,

[Chen-Yu Tsai]

On Tue, Oct 27, 2015 at 5:32 AM, Olliver Schinagl

Post by 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.
---
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

Post by Olliver Schinagl
- 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));
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
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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);

[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.

Post by Olliver Schinagl
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.
---
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.

[Olliver Schinagl]

Hey Ezequiel,

Post by 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.

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.

Post by Ezequiel Garcia
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

Post by Olliver Schinagl
The lpc18xx driver currently manipulates the pwm_device struct

directly

Post by Olliver Schinagl
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.

Post by Olliver Schinagl
---
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

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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;

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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))

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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);

[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 <***@pengutronix.de>
:::::: CC: Michael Turquette <***@linaro.org>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Olliver Schinagl]

Hey Thierry,

Post by Olliver Schinagl
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.

Post by Olliver Schinagl
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 :)

Post by Olliver Schinagl
* 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.

Post by Olliver Schinagl
* 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?

Post by Olliver Schinagl
---
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);
*
* 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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Olliver Schinagl]

This post might be inappropriate. Click to display it.

[Olliver Schinagl]

From: Olliver Schinagl <***@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 <***@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: ***@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 <***@schinagl.nl>
+S: Maintained
+F: drivers/pwm/pwm-gpio.c
+
GENERIC HDLC (WAN) DRIVERS
M: Krzysztof Halasa <***@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 <***@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 <***@schinagl.nl>");
+MODULE_DESCRIPTION("Generic GPIO bit-banged PWM driver");
+MODULE_LICENSE("GPL");

[Rob Herring]

On Mon, Oct 26, 2015 at 4:32 PM, Olliver Schinagl

Post by Olliver Schinagl
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.
---
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
+
+ - 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.

Post by Olliver Schinagl
+
+#include <dt-bindings/gpio/gpio.h>
+

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

Post by Olliver Schinagl
+ 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.

Post by Olliver Schinagl
+ };
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
+S: Maintained
+F: drivers/pwm/pwm-gpio.c

Can you add the binding doc too.

Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

[Olliver Schinagl]

Hey Rob,

Post by Rob Herring
On Mon, Oct 26, 2015 at 4:32 PM, Olliver Schinagl

Post by Olliver Schinagl
This patch adds a bit-banging gpio PWM driver. It makes use of

hrtimers,

Post by Olliver Schinagl
to allow nano-second resolution, though it obviously strongly depends

on

Post by Olliver Schinagl
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.
---
Documentation/devicetree/bindings/pwm/pwm-gpio.txt | 18 ++
MAINTAINERS | 5 +
drivers/pwm/Kconfig | 15 ++
drivers/pwm/Makefile | 1 +
drivers/pwm/pwm-gpio.c | 253

+++++++++++++++++++++

Post by Olliver Schinagl
5 files changed, 292 insertions(+)
create mode 100644

Documentation/devicetree/bindings/pwm/pwm-gpio.txt

Post by Olliver Schinagl
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

Post by Olliver Schinagl
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
+
+ - compatible: should be "pwm-gpio"
+ - #pwm-cells: should be 3, see pwm.txt in this directory for a

general

Post by Olliver Schinagl
+ description of the cells format.
+ - pwm-gpios: one or more gpios describing the used gpio, see the

gpio

Post by Olliver Schinagl
+ 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.

Post by Rob Herring

Post by Olliver Schinagl
+
+#include <dt-bindings/gpio/gpio.h>
+

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

Done

Post by Rob Herring

Post by Olliver Schinagl
+ 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.

Post by Rob Herring

Post by Olliver Schinagl
+ };
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
+S: Maintained
+F: drivers/pwm/pwm-gpio.c

Can you add the binding doc too.

Sure, done.

Post by Rob Herring
Rob

[Olliver Schinagl]

From: Olliver Schinagl <***@schinagl.nl>

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

Signed-off-by: Olliver Schinagl <***@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,

[a***@gmail.com]

Post by 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.

What is the status of pwm-gpio driver so far?