[PATCH 1/2] dt-bindings: pwm: Add pwm-gpio

[PATCH 1/2] dt-bindings: pwm: Add pwm-gpio

[Vincent Whitchurch]

Add bindings for the pwm-gpio driver.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
 .../devicetree/bindings/pwm/pwm-gpio.yaml     | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.yaml

diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
new file mode 100644
index 000000000000..51941cd03955
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
@@ -0,0 +1,29 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/pwm-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO-based PWM
+
+maintainers:
+  - Vincent Whitchurch <vincent.whitchurch@axis.com>
+
+properties:
+  compatible:
+    enum:
+      - pwm-gpio
+
+  "#pwm-cells":
+    const: 2
+
+  gpio:
+    maxItems: 1
+    description: GPIO to toggle.
+
+required:
+  - compatible
+  - "#pwm-cells"
+  - gpio
+
+additionalProperties: false
-- 
2.25.1

[PATCH 2/2] pwm: Add GPIO PWM driver

[Vincent Whitchurch]

Add a software PWM which toggles a GPIO from a high-resolution timer.

This will naturally not be as accurate or as efficient as a hardware
PWM, but it is useful in some cases.  I have for example used it for
evaluating LED brightness handling (via leds-pwm) on a board where the
LED was just hooked up to a GPIO, and for a simple verification of the
timer frequency on another platform.

Since high-resolution timers are used, sleeping gpio chips are not
supported and are rejected in the probe function.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
While preparing this driver for posting, I found a pwm-gpio driver posted to
the lists way back in 2015 by Olliver Schinagl:

 https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/

This driver was developed independently, but since both drivers are trivial
they are quite similar.  The main difference I see (apart from the usage of
newer APIs and DT schemas) is that this driver only supports one PWM per
instance, which makes for simpler code.  I also reject sleeping GPIO chips
explicitly while that driver uses gpio_set_value_cansleep() from a hrtimer,
which is a no-no.

 drivers/pwm/Kconfig    |  10 ++++
 drivers/pwm/Makefile   |   1 +
 drivers/pwm/pwm-gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 134 insertions(+)
 create mode 100644 drivers/pwm/pwm-gpio.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d33..20e4fda82e61 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -181,6 +181,16 @@ config PWM_FSL_FTM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-fsl-ftm.
 
+config PWM_GPIO
+	tristate "GPIO PWM support"
+	depends on OF && GPIOLIB
+	help
+	  Generic PWM framework driver for a software PWM toggling a GPIO pin
+	  from kernel high-resolution timers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-gpio.
+
 config PWM_HIBVT
 	tristate "HiSilicon BVT PWM support"
 	depends on ARCH_HISI || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 2c2ba0a03557..2e045f063cd1 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
 obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.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_HIBVT)		+= pwm-hibvt.o
 obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
new file mode 100644
index 000000000000..e579aca0f937
--- /dev/null
+++ b/drivers/pwm/pwm-gpio.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (C) 2020 Axis Communications AB */
+
+#include <linux/gpio/consumer.h>
+#include <linux/platform_device.h>
+#include <linux/hrtimer.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/pwm.h>
+#include <linux/err.h>
+#include <linux/of.h>
+
+struct pwm_gpio {
+	struct pwm_chip chip;
+	struct hrtimer hrtimer;
+	struct gpio_desc *gpio;
+	ktime_t on_interval;
+	ktime_t off_interval;
+	bool invert;
+	bool on;
+};
+
+static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *hrtimer)
+{
+	struct pwm_gpio *gpwm = container_of(hrtimer, struct pwm_gpio, hrtimer);
+	bool newon = !gpwm->on;
+
+	gpwm->on = newon;
+	gpiod_set_value(gpwm->gpio, newon ^ gpwm->invert);
+
+	hrtimer_forward_now(hrtimer, newon ? gpwm->on_interval : gpwm->off_interval);
+
+	return HRTIMER_RESTART;
+}
+
+static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			  const struct pwm_state *state)
+{
+	struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
+
+	hrtimer_cancel(&gpwm->hrtimer);
+
+	if (!state->enabled) {
+		gpiod_set_value(gpwm->gpio, 0);
+		return 0;
+	}
+
+	gpwm->on_interval = ns_to_ktime(state->duty_cycle);
+	gpwm->off_interval = ns_to_ktime(state->period - state->duty_cycle);
+	gpwm->invert = state->polarity == PWM_POLARITY_INVERSED;
+
+	gpwm->on = !!gpwm->on_interval;
+	gpiod_set_value(gpwm->gpio, gpwm->on ^ gpwm->invert);
+
+	if (gpwm->on_interval && gpwm->off_interval)
+		hrtimer_start(&gpwm->hrtimer, gpwm->on_interval, HRTIMER_MODE_REL);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_gpio_ops = {
+	.owner = THIS_MODULE,
+	.apply = pwm_gpio_apply,
+};
+
+static int pwm_gpio_probe(struct platform_device *pdev)
+{
+	struct pwm_gpio *gpwm;
+	int ret;
+
+	gpwm = devm_kzalloc(&pdev->dev, sizeof(*gpwm), GFP_KERNEL);
+	if (!gpwm)
+		return -ENOMEM;
+
+	gpwm->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
+	if (IS_ERR(gpwm->gpio))
+		return PTR_ERR(gpwm->gpio);
+
+	if (gpiod_cansleep(gpwm->gpio))
+		return -EINVAL;
+
+	gpwm->chip.dev = &pdev->dev;
+	gpwm->chip.ops = &pwm_gpio_ops;
+	gpwm->chip.base = pdev->id;
+	gpwm->chip.npwm = 1;
+
+	hrtimer_init(&gpwm->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
+	gpwm->hrtimer.function = pwm_gpio_timer;
+
+	ret = pwmchip_add(&gpwm->chip);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, gpwm);
+
+	return 0;
+}
+
+static int pwm_gpio_remove(struct platform_device *pdev)
+{
+	struct pwm_gpio *gpwm = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&gpwm->chip);
+}
+
+static const struct of_device_id pwm_gpio_dt_ids[] = {
+	{ .compatible = "pwm-gpio", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pwm_gpio_dt_ids);
+
+static struct platform_driver pwm_gpio_driver = {
+	.driver = {
+		.name = "pwm-gpio",
+		.of_match_table = pwm_gpio_dt_ids,
+	},
+	.probe = pwm_gpio_probe,
+	.remove = pwm_gpio_remove,
+};
+
+module_platform_driver(pwm_gpio_driver);
+
+MODULE_LICENSE("GPL v2");
-- 
2.25.1

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Uwe Kleine-König]

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

Hello,

On Fri, Aug 14, 2020 at 05:55:13PM +0200, Vincent Whitchurch wrote:
> Add a software PWM which toggles a GPIO from a high-resolution timer.
> 
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases.  I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
> 
> Since high-resolution timers are used, sleeping gpio chips are not
> supported and are rejected in the probe function.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>

Nice idea. IMHO this can serve as example about what we expect from pwm
drivers. There is some improvement necessary however to reach that
state.

> ---
> While preparing this driver for posting, I found a pwm-gpio driver posted to
> the lists way back in 2015 by Olliver Schinagl:
> 
>  https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/
> 
> This driver was developed independently, but since both drivers are trivial
> they are quite similar.  The main difference I see (apart from the usage of
> newer APIs and DT schemas) is that this driver only supports one PWM per
> instance, which makes for simpler code.  I also reject sleeping GPIO chips
> explicitly while that driver uses gpio_set_value_cansleep() from a hrtimer,
> which is a no-no.
> 
>  drivers/pwm/Kconfig    |  10 ++++
>  drivers/pwm/Makefile   |   1 +
>  drivers/pwm/pwm-gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/pwm/pwm-gpio.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..20e4fda82e61 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -181,6 +181,16 @@ config PWM_FSL_FTM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-fsl-ftm.
>  
> +config PWM_GPIO
> +	tristate "GPIO PWM support"
> +	depends on OF && GPIOLIB
> +	help
> +	  Generic PWM framework driver for a software PWM toggling a GPIO pin
> +	  from kernel high-resolution timers.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-gpio.
> +
>  config PWM_HIBVT
>  	tristate "HiSilicon BVT PWM support"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..2e045f063cd1 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.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_HIBVT)		+= pwm-hibvt.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 000000000000..e579aca0f937
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2020 Axis Communications AB */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/pwm.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +struct pwm_gpio {
> +	struct pwm_chip chip;
> +	struct hrtimer hrtimer;
> +	struct gpio_desc *gpio;
> +	ktime_t on_interval;
> +	ktime_t off_interval;
> +	bool invert;
> +	bool on;
> +};
> +
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *hrtimer)
> +{
> +	struct pwm_gpio *gpwm = container_of(hrtimer, struct pwm_gpio, hrtimer);
> +	bool newon = !gpwm->on;
> +
> +	gpwm->on = newon;
> +	gpiod_set_value(gpwm->gpio, newon ^ gpwm->invert);
> +
> +	hrtimer_forward_now(hrtimer, newon ? gpwm->on_interval : gpwm->off_interval);

If I understand correct this adds the new interval on top of "now" and
not on top of the previous timestamp where an edge was expected. Would
it make sense to change that?

> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> +
> +	hrtimer_cancel(&gpwm->hrtimer);
> +
> +	if (!state->enabled) {
> +		gpiod_set_value(gpwm->gpio, 0);
> +		return 0;
> +	}
> +
> +	gpwm->on_interval = ns_to_ktime(state->duty_cycle);
> +	gpwm->off_interval = ns_to_ktime(state->period - state->duty_cycle);
> +	gpwm->invert = state->polarity == PWM_POLARITY_INVERSED;
> +
> +	gpwm->on = !!gpwm->on_interval;
> +	gpiod_set_value(gpwm->gpio, gpwm->on ^ gpwm->invert);
> +
> +	if (gpwm->on_interval && gpwm->off_interval)
> +		hrtimer_start(&gpwm->hrtimer, gpwm->on_interval, HRTIMER_MODE_REL);

Ideally the currently running period is completed before a period with
the new configuration starts. Would be nice switch only on the next
period end if the current configuration is enabled.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = pwm_gpio_apply,

Usually a .get_state callback is nice. Does it make sense to do
something like:

	if (driver is up)
		report current setting
	else
		val = gpio_get_value()
		report(period=1, duty_cycle=val, enabled=val, polarity=NORMAL)

?

> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> +	struct pwm_gpio *gpwm;
> +	int ret;
> +
> +	gpwm = devm_kzalloc(&pdev->dev, sizeof(*gpwm), GFP_KERNEL);
> +	if (!gpwm)
> +		return -ENOMEM;
> +
> +	gpwm->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> +	if (IS_ERR(gpwm->gpio))
> +		return PTR_ERR(gpwm->gpio);
> +
> +	if (gpiod_cansleep(gpwm->gpio))
> +		return -EINVAL;
> +
> +	gpwm->chip.dev = &pdev->dev;
> +	gpwm->chip.ops = &pwm_gpio_ops;
> +	gpwm->chip.base = pdev->id;
> +	gpwm->chip.npwm = 1;
> +
> +	hrtimer_init(&gpwm->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	gpwm->hrtimer.function = pwm_gpio_timer;
> +
> +	ret = pwmchip_add(&gpwm->chip);
> +	if (ret < 0)
> +		return ret;

Error messages in the error paths please (preferably using
dev_err_probe).

Best regards
Uwe

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

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

Re: [PATCH 1/2] dt-bindings: pwm: Add pwm-gpio

[Rob Herring]

On Fri, Aug 14, 2020 at 05:55:12PM +0200, Vincent Whitchurch wrote:
> Add bindings for the pwm-gpio driver.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
>  .../devicetree/bindings/pwm/pwm-gpio.yaml     | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
> new file mode 100644
> index 000000000000..51941cd03955
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml
> @@ -0,0 +1,29 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pwm/pwm-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO-based PWM
> +
> +maintainers:
> +  - Vincent Whitchurch <vincent.whitchurch@axis.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - pwm-gpio
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +  gpio:

'gpios' is the preferred form even if singular.

> +    maxItems: 1
> +    description: GPIO to toggle.
> +
> +required:
> +  - compatible
> +  - "#pwm-cells"
> +  - gpio
> +
> +additionalProperties: false
> -- 
> 2.25.1
> 

Re: [PATCH 1/2] dt-bindings: pwm: Add pwm-gpio

[Vincent Whitchurch]

On Mon, Aug 17, 2020 at 09:38:25PM +0200, Rob Herring wrote:
> On Fri, Aug 14, 2020 at 05:55:12PM +0200, Vincent Whitchurch wrote:
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - pwm-gpio
> > +
> > +  "#pwm-cells":
> > +    const: 2
> > +
> > +  gpio:
> 
> 'gpios' is the preferred form even if singular.

Thanks, I've fixed this in v2.

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Vincent Whitchurch]

On Sat, Aug 15, 2020 at 09:50:32AM +0200, Uwe Kleine-König wrote:
> On Fri, Aug 14, 2020 at 05:55:13PM +0200, Vincent Whitchurch wrote:
> > Add a software PWM which toggles a GPIO from a high-resolution timer.
> > 
> > This will naturally not be as accurate or as efficient as a hardware
> > PWM, but it is useful in some cases.  I have for example used it for
> > evaluating LED brightness handling (via leds-pwm) on a board where the
> > LED was just hooked up to a GPIO, and for a simple verification of the
> > timer frequency on another platform.
> > 
> > Since high-resolution timers are used, sleeping gpio chips are not
> > supported and are rejected in the probe function.
> > 
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> 
> Nice idea. IMHO this can serve as example about what we expect from pwm
> drivers. There is some improvement necessary however to reach that
> state.

Thank you for the comments.  I think I've fixed all of them in v2.  Just
one point about this one:

> > +
> > +	return 0;
> > +}
> > +
> > +static const struct pwm_ops pwm_gpio_ops = {
> > +	.owner = THIS_MODULE,
> > +	.apply = pwm_gpio_apply,
> 
> Usually a .get_state callback is nice. Does it make sense to do
> something like:
> 
> 	if (driver is up)
> 		report current setting
> 	else
> 		val = gpio_get_value()
> 		report(period=1, duty_cycle=val, enabled=val, polarity=NORMAL)
> 
> ?

I implemented get_state() but I didn't do the gpio_get_value() thing
since the driver sets the gpio to a known value in probe().

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Uwe Kleine-König]

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

Hello Vincent,

On Wed, Sep 02, 2020 at 02:11:54PM +0200, Vincent Whitchurch wrote:
> On Sat, Aug 15, 2020 at 09:50:32AM +0200, Uwe Kleine-König wrote:
> > On Fri, Aug 14, 2020 at 05:55:13PM +0200, Vincent Whitchurch wrote:
> > > +static const struct pwm_ops pwm_gpio_ops = {
> > > +	.owner = THIS_MODULE,
> > > +	.apply = pwm_gpio_apply,
> > 
> > Usually a .get_state callback is nice. Does it make sense to do
> > something like:
> > 
> > 	if (driver is up)
> > 		report current setting
> > 	else
> > 		val = gpio_get_value()
> > 		report(period=1, duty_cycle=val, enabled=val, polarity=NORMAL)
> > 
> > ?
> 
> I implemented get_state() but I didn't do the gpio_get_value() thing
> since the driver sets the gpio to a known value in probe().

This however is wrong, .probe() is not supposed to modify the output.

Best regards
Uwe

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

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

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Olliver Schinagl]

Hey Vincent,

On 14-08-2020 17:55, Vincent Whitchurch wrote:
> Add a software PWM which toggles a GPIO from a high-resolution timer.
> 
> This will naturally not be as accurate or as efficient as a hardware
> PWM, but it is useful in some cases.  I have for example used it for
> evaluating LED brightness handling (via leds-pwm) on a board where the
> LED was just hooked up to a GPIO, and for a simple verification of the
> timer frequency on another platform.
> 
> Since high-resolution timers are used, sleeping gpio chips are not
> supported and are rejected in the probe function.
> 
> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> ---
> While preparing this driver for posting, I found a pwm-gpio driver posted to
> the lists way back in 2015 by Olliver Schinagl:
> 
>   https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/
> 
Thanks for reminding me there :) As I think I still use this driver, I 
don't mind migrating to this one (if merged) but how do you suggests to 
proceed with regards to multiple PWM's, as this is how I am using it 
currently. E.g. how do we merge them? I'm fine with 'taking the simpler 
code method' for a start point, but i guess I solved that part 
(somewhat) in 2015 :p

> This driver was developed independently, but since both drivers are trivial
> they are quite similar.  The main difference I see (apart from the usage of
> newer APIs and DT schemas) is that this driver only supports one PWM per
> instance, which makes for simpler code.  I also reject sleeping GPIO chips
> explicitly while that driver uses gpio_set_value_cansleep() from a hrtimer,
> which is a no-no.
That was indeed my bad :)

But I think we didn't get much feedback back then, and it was never 
merged sadly :(

Olliver

> 
>   drivers/pwm/Kconfig    |  10 ++++
>   drivers/pwm/Makefile   |   1 +
>   drivers/pwm/pwm-gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 134 insertions(+)
>   create mode 100644 drivers/pwm/pwm-gpio.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..20e4fda82e61 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -181,6 +181,16 @@ config PWM_FSL_FTM
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called pwm-fsl-ftm.
>   
> +config PWM_GPIO
> +	tristate "GPIO PWM support"
> +	depends on OF && GPIOLIB
> +	help
> +	  Generic PWM framework driver for a software PWM toggling a GPIO pin
> +	  from kernel high-resolution timers.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-gpio.
> +
>   config PWM_HIBVT
>   	tristate "HiSilicon BVT PWM support"
>   	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..2e045f063cd1 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>   obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.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_HIBVT)		+= pwm-hibvt.o
>   obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>   obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 000000000000..e579aca0f937
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2020 Axis Communications AB */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/pwm.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +struct pwm_gpio {
> +	struct pwm_chip chip;
> +	struct hrtimer hrtimer;
> +	struct gpio_desc *gpio;
> +	ktime_t on_interval;
> +	ktime_t off_interval;
> +	bool invert;
> +	bool on;
> +};
> +
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *hrtimer)
> +{
> +	struct pwm_gpio *gpwm = container_of(hrtimer, struct pwm_gpio, hrtimer);
> +	bool newon = !gpwm->on;
> +
> +	gpwm->on = newon;
> +	gpiod_set_value(gpwm->gpio, newon ^ gpwm->invert);
> +
> +	hrtimer_forward_now(hrtimer, newon ? gpwm->on_interval : gpwm->off_interval);
> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> +
> +	hrtimer_cancel(&gpwm->hrtimer);
> +
> +	if (!state->enabled) {
> +		gpiod_set_value(gpwm->gpio, 0);
> +		return 0;
> +	}
> +
> +	gpwm->on_interval = ns_to_ktime(state->duty_cycle);
> +	gpwm->off_interval = ns_to_ktime(state->period - state->duty_cycle);
> +	gpwm->invert = state->polarity == PWM_POLARITY_INVERSED;
> +
> +	gpwm->on = !!gpwm->on_interval;
> +	gpiod_set_value(gpwm->gpio, gpwm->on ^ gpwm->invert);
> +
> +	if (gpwm->on_interval && gpwm->off_interval)
> +		hrtimer_start(&gpwm->hrtimer, gpwm->on_interval, HRTIMER_MODE_REL);
> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = pwm_gpio_apply,
> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> +	struct pwm_gpio *gpwm;
> +	int ret;
> +
> +	gpwm = devm_kzalloc(&pdev->dev, sizeof(*gpwm), GFP_KERNEL);
> +	if (!gpwm)
> +		return -ENOMEM;
> +
> +	gpwm->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> +	if (IS_ERR(gpwm->gpio))
> +		return PTR_ERR(gpwm->gpio);
> +
> +	if (gpiod_cansleep(gpwm->gpio))
> +		return -EINVAL;
> +
> +	gpwm->chip.dev = &pdev->dev;
> +	gpwm->chip.ops = &pwm_gpio_ops;
> +	gpwm->chip.base = pdev->id;
> +	gpwm->chip.npwm = 1;
> +
> +	hrtimer_init(&gpwm->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	gpwm->hrtimer.function = pwm_gpio_timer;
> +
> +	ret = pwmchip_add(&gpwm->chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, gpwm);
> +
> +	return 0;
> +}
> +
> +static int pwm_gpio_remove(struct platform_device *pdev)
> +{
> +	struct pwm_gpio *gpwm = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&gpwm->chip);
> +}
> +
> +static const struct of_device_id pwm_gpio_dt_ids[] = {
> +	{ .compatible = "pwm-gpio", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pwm_gpio_dt_ids);
> +
> +static struct platform_driver pwm_gpio_driver = {
> +	.driver = {
> +		.name = "pwm-gpio",
> +		.of_match_table = pwm_gpio_dt_ids,
> +	},
> +	.probe = pwm_gpio_probe,
> +	.remove = pwm_gpio_remove,
> +};
> +
> +module_platform_driver(pwm_gpio_driver);
> +
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Vincent Whitchurch]

On Thu, Sep 03, 2020 at 11:15:31AM +0200, Olliver Schinagl wrote:
> On 14-08-2020 17:55, Vincent Whitchurch wrote:
> > Add a software PWM which toggles a GPIO from a high-resolution timer.
> > 
> > This will naturally not be as accurate or as efficient as a hardware
> > PWM, but it is useful in some cases.  I have for example used it for
> > evaluating LED brightness handling (via leds-pwm) on a board where the
> > LED was just hooked up to a GPIO, and for a simple verification of the
> > timer frequency on another platform.
> > 
> > Since high-resolution timers are used, sleeping gpio chips are not
> > supported and are rejected in the probe function.
> > 
> > Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
> > ---
> > While preparing this driver for posting, I found a pwm-gpio driver posted to
> > the lists way back in 2015 by Olliver Schinagl:
> > 
> >   https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/
> > 
> Thanks for reminding me there :) As I think I still use this driver, I 
> don't mind migrating to this one (if merged) but how do you suggests to 
> proceed with regards to multiple PWM's, as this is how I am using it 
> currently. E.g. how do we merge them? I'm fine with 'taking the simpler 
> code method' for a start point, but i guess I solved that part 
> (somewhat) in 2015 :p

Since this is just a software construct, the simplest way would just be
to create multiple instances in the device tree if you want multiple
PWMs, wouldn't it?

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Olliver Schinagl]

Hey Vincent,

On 15-09-2020 16:02, Vincent Whitchurch wrote:
> On Thu, Sep 03, 2020 at 11:15:31AM +0200, Olliver Schinagl wrote:
>> On 14-08-2020 17:55, Vincent Whitchurch wrote:
>>> Add a software PWM which toggles a GPIO from a high-resolution timer.
>>>
>>> This will naturally not be as accurate or as efficient as a hardware
>>> PWM, but it is useful in some cases.  I have for example used it for
>>> evaluating LED brightness handling (via leds-pwm) on a board where the
>>> LED was just hooked up to a GPIO, and for a simple verification of the
>>> timer frequency on another platform.
>>>
>>> Since high-resolution timers are used, sleeping gpio chips are not
>>> supported and are rejected in the probe function.
>>>
>>> Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>>> ---
>>> While preparing this driver for posting, I found a pwm-gpio driver posted to
>>> the lists way back in 2015 by Olliver Schinagl:
>>>
>>>    https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/
>>>
>> Thanks for reminding me there :) As I think I still use this driver, I
>> don't mind migrating to this one (if merged) but how do you suggests to
>> proceed with regards to multiple PWM's, as this is how I am using it
>> currently. E.g. how do we merge them? I'm fine with 'taking the simpler
>> code method' for a start point, but i guess I solved that part
>> (somewhat) in 2015 :p
> 
> Since this is just a software construct, the simplest way would just be
> to create multiple instances in the device tree if you want multiple
> PWMs, wouldn't it?
> 
Not entirely, as they are no longer 'logically grouped'?

Olliver

Re: [PATCH 2/2] pwm: Add GPIO PWM driver

[Uwe Kleine-König]

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

Hello Olliver,

On Fri, Sep 25, 2020 at 08:14:58PM +0200, Olliver Schinagl wrote:
> On 15-09-2020 16:02, Vincent Whitchurch wrote:
> > Since this is just a software construct, the simplest way would just be
> > to create multiple instances in the device tree if you want multiple
> > PWMs, wouldn't it?
>
> Not entirely, as they are no longer 'logically grouped'?

Why is it necessary for you that the PWMs are "logically grouped"?

Best regards
Uwe

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

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