diff mbox series

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

Message ID 20200814155513.31936-1-vincent.whitchurch@axis.com
State Changes Requested, archived
Headers show
Series [1/2] dt-bindings: pwm: Add pwm-gpio | expand

Checks

Context Check Description
robh/checkpatch success
robh/dt-meta-schema success

Commit Message

Vincent Whitchurch Aug. 14, 2020, 3:55 p.m. UTC
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

Comments

Uwe Kleine-König Aug. 15, 2020, 7:50 a.m. UTC | #1
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
Rob Herring (Arm) Aug. 17, 2020, 7:38 p.m. UTC | #2
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
>
Vincent Whitchurch Sept. 2, 2020, 12:08 p.m. UTC | #3
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.
Vincent Whitchurch Sept. 2, 2020, 12:11 p.m. UTC | #4
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().
Uwe Kleine-König Sept. 2, 2020, 6 p.m. UTC | #5
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
Olliver Schinagl Sept. 3, 2020, 9:15 a.m. UTC | #6
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");
>
Vincent Whitchurch Sept. 15, 2020, 2:02 p.m. UTC | #7
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?
Olliver Schinagl Sept. 25, 2020, 6:14 p.m. UTC | #8
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
Uwe Kleine-König Sept. 26, 2020, 1:24 p.m. UTC | #9
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
diff mbox series

Patch

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