* [PATCH 1/2] pwm: pwm-gpio: New driver @ 2020-12-05 21:43 Nicola Di Lieto 2020-12-09 7:28 ` Uwe Kleine-König 0 siblings, 1 reply; 23+ messages in thread From: Nicola Di Lieto @ 2020-12-05 21:43 UTC (permalink / raw) To: linux-pwm; +Cc: Thierry Reding, Uwe Kleine-König, Lee Jones This new driver allows pulse width modulating any GPIOs using a high resolution timer. It is fully generic and can be useful in a variety of situations. As an example I used it to provide a pwm to the pwm-beeper driver so that my embedded system can produce tones through a piezo buzzer connected to a GPIO which unfortunately is not hardware PWM capable. Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> --- MAINTAINERS | 7 ++ drivers/pwm/Kconfig | 10 +++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-gpio.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 190 insertions(+) create mode 100644 drivers/pwm/pwm-gpio.c diff --git a/MAINTAINERS b/MAINTAINERS index ebe4829cdd4d..fe9b5b00ba94 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14215,6 +14215,13 @@ F: Documentation/devicetree/bindings/hwmon/pwm-fan.txt F: Documentation/hwmon/pwm-fan.rst F: drivers/hwmon/pwm-fan.c +PWM GPIO DRIVER +M: Nicola Di Lieto <nicola.dilieto@gmail.com> +L: linux-pwm@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/pwm/pwm-gpio.yaml +F: drivers/pwm/pwm-gpio.c + PWM IR Transmitter M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.org diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 63be5362fd3a..5432084c6276 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 "PWM GPIO support" + depends on GPIOLIB + depends on HIGH_RES_TIMERS + help + Generic PWM for software modulation of GPIOs + + 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 cbdcd55d69ee..eea0216215a7 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..6f425f2d02fe --- /dev/null +++ b/drivers/pwm/pwm-gpio.c @@ -0,0 +1,172 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Generic software PWM for modulating GPIOs + * + * Copyright 2020 Nicola Di Lieto + * + * Author: Nicola Di Lieto <nicola.dilieto@gmail.com> + */ + +#include <linux/atomic.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hrtimer.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/workqueue.h> + +struct pwm_gpio { + struct pwm_chip chip; + struct gpio_desc *desc; + struct work_struct work; + struct hrtimer timer; + atomic_t enabled; + u64 ton_ns; + u64 toff_ns; + enum pwm_polarity polarity; + bool output; +}; + +static void pwm_gpio_work(struct work_struct *work) +{ + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); + + gpiod_set_value_cansleep(pwm_gpio->desc, + (pwm_gpio->polarity == PWM_POLARITY_INVERSED) ^ pwm_gpio->output); +} + +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle) +{ + struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer); + u64 ns; + + if (!atomic_read(&pwm_gpio->enabled)) + return HRTIMER_NORESTART; + + if (pwm_gpio->output) { + ns = pwm_gpio->toff_ns; + pwm_gpio->output = false; + } else { + ns = pwm_gpio->ton_ns; + pwm_gpio->output = true; + } + + schedule_work(&pwm_gpio->work); + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); + + return HRTIMER_RESTART; +} + +static inline struct pwm_gpio *to_pwm_gpio(struct pwm_chip *_chip) +{ + return container_of(_chip, struct pwm_gpio, chip); +} + +static void pwm_gpio_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); + + cancel_work_sync(&pwm_gpio->work); + gpiod_set_value_cansleep(pwm_gpio->desc, + pwm_gpio->polarity == PWM_POLARITY_INVERSED); +} + +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); + u64 period = clamp(state->period, U64_C(50000), U64_C(1000000000)); + u64 duty_cycle = clamp(state->duty_cycle, U64_C(0), period); + + pwm_gpio->ton_ns = duty_cycle; + pwm_gpio->toff_ns = period - duty_cycle; + pwm_gpio->polarity = state->polarity; + + if (state->enabled && !atomic_read(&pwm_gpio->enabled)) { + atomic_set(&pwm_gpio->enabled, 1); + hrtimer_start(&pwm_gpio->timer, 0, HRTIMER_MODE_REL); + } else if (!state->enabled && atomic_read(&pwm_gpio->enabled)) { + atomic_set(&pwm_gpio->enabled, 0); + pwm_gpio->output = false; + schedule_work(&pwm_gpio->work); + } + return 0; +} + +static void pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); + + state->duty_cycle = pwm_gpio->ton_ns; + state->period = pwm_gpio->ton_ns + pwm_gpio->toff_ns; + state->polarity = pwm_gpio->polarity; + state->enabled = atomic_read(&pwm_gpio->enabled); +} + +static const struct pwm_ops pwm_gpio_ops = { + .free = pwm_gpio_free, + .apply = pwm_gpio_apply, + .get_state = pwm_gpio_get_state, + .owner = THIS_MODULE, +}; + +static int pwm_gpio_probe(struct platform_device *pdev) +{ + struct pwm_gpio *pwm_gpio; + + pwm_gpio = devm_kzalloc(&pdev->dev, sizeof(*pwm_gpio), GFP_KERNEL); + if (!pwm_gpio) + return -ENOMEM; + + pwm_gpio->desc = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW); + if (IS_ERR(pwm_gpio->desc)) + return PTR_ERR(pwm_gpio->desc); + + INIT_WORK(&pwm_gpio->work, pwm_gpio_work); + + hrtimer_init(&pwm_gpio->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + pwm_gpio->timer.function = pwm_gpio_do_timer; + pwm_gpio->chip.dev = &pdev->dev; + pwm_gpio->chip.ops = &pwm_gpio_ops; + pwm_gpio->chip.npwm = 1; + + platform_set_drvdata(pdev, pwm_gpio); + + return pwmchip_add(&pwm_gpio->chip); +} + +static int pwm_gpio_remove(struct platform_device *pdev) +{ + struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev); + + hrtimer_cancel(&pwm_gpio->timer); + + return pwmchip_remove(&pwm_gpio->chip); +} + +#ifdef CONFIG_OF +static const struct of_device_id pwm_gpio_of_match[] = { + { .compatible = "pwm-gpio", }, + { } +}; +MODULE_DEVICE_TABLE(of, pwm_gpio_of_match); +#endif + +static struct platform_driver pwm_gpio_driver = { + .probe = pwm_gpio_probe, + .remove = pwm_gpio_remove, + .driver = { + .name = "pwm-gpio", + .of_match_table = of_match_ptr(pwm_gpio_of_match), + }, +}; +module_platform_driver(pwm_gpio_driver); + +MODULE_DESCRIPTION("PWM GPIO driver"); +MODULE_ALIAS("platform:pwm-gpio"); +MODULE_AUTHOR("Nicola Di Lieto"); +MODULE_LICENSE("GPL"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH 1/2] pwm: pwm-gpio: New driver 2020-12-05 21:43 [PATCH 1/2] pwm: pwm-gpio: New driver Nicola Di Lieto @ 2020-12-09 7:28 ` Uwe Kleine-König 2020-12-11 17:04 ` [PATCH v2 0/2] " Nicola Di Lieto 0 siblings, 1 reply; 23+ messages in thread From: Uwe Kleine-König @ 2020-12-09 7:28 UTC (permalink / raw) To: Nicola Di Lieto; +Cc: linux-pwm, Thierry Reding, Lee Jones [-- Attachment #1: Type: text/plain, Size: 9479 bytes --] Hello Nicola, On Sat, Dec 05, 2020 at 10:43:53PM +0100, Nicola Di Lieto wrote: > This new driver allows pulse width modulating any GPIOs using > a high resolution timer. It is fully generic and can be useful > in a variety of situations. As an example I used it to provide > a pwm to the pwm-beeper driver so that my embedded system can > produce tones through a piezo buzzer connected to a GPIO which > unfortunately is not hardware PWM capable. > > Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> > --- > MAINTAINERS | 7 ++ > drivers/pwm/Kconfig | 10 +++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-gpio.c | 172 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 190 insertions(+) > create mode 100644 drivers/pwm/pwm-gpio.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index ebe4829cdd4d..fe9b5b00ba94 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14215,6 +14215,13 @@ F: Documentation/devicetree/bindings/hwmon/pwm-fan.txt > F: Documentation/hwmon/pwm-fan.rst > F: drivers/hwmon/pwm-fan.c > The patch is whitespace damaged. Also it would be good if the patch set was sent in a single mail thread. I suggest you look into git-send-email for the next round. > +PWM GPIO DRIVER > +M: Nicola Di Lieto <nicola.dilieto@gmail.com> > +L: linux-pwm@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/pwm/pwm-gpio.yaml > +F: drivers/pwm/pwm-gpio.c > + > PWM IR Transmitter > M: Sean Young <sean@mess.org> > L: linux-media@vger.kernel.org > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 63be5362fd3a..5432084c6276 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 "PWM GPIO support" > + depends on GPIOLIB > + depends on HIGH_RES_TIMERS > + help > + Generic PWM for software modulation of GPIOs > + > + 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 cbdcd55d69ee..eea0216215a7 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..6f425f2d02fe > --- /dev/null > +++ b/drivers/pwm/pwm-gpio.c > @@ -0,0 +1,172 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Generic software PWM for modulating GPIOs > + * > + * Copyright 2020 Nicola Di Lieto > + * > + * Author: Nicola Di Lieto <nicola.dilieto@gmail.com> > + */ > + > +#include <linux/atomic.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/hrtimer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/workqueue.h> > + > +struct pwm_gpio { > + struct pwm_chip chip; > + struct gpio_desc *desc; > + struct work_struct work; > + struct hrtimer timer; > + atomic_t enabled; > + u64 ton_ns; > + u64 toff_ns; > + enum pwm_polarity polarity; > + bool output; > +}; > + > +static void pwm_gpio_work(struct work_struct *work) > +{ > + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); > + > + gpiod_set_value_cansleep(pwm_gpio->desc, > + (pwm_gpio->polarity == PWM_POLARITY_INVERSED) ^ pwm_gpio->output); > +} > + > +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle) > +{ > + struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer); > + u64 ns; > + > + if (!atomic_read(&pwm_gpio->enabled)) > + return HRTIMER_NORESTART; > + > + if (pwm_gpio->output) { > + ns = pwm_gpio->toff_ns; > + pwm_gpio->output = false; > + } else { > + ns = pwm_gpio->ton_ns; > + pwm_gpio->output = true; > + } > + > + schedule_work(&pwm_gpio->work); > + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); > + > + return HRTIMER_RESTART; > +} > + > +static inline struct pwm_gpio *to_pwm_gpio(struct pwm_chip *_chip) If you rename this to "pwm_gpio_from_chip" it shares the prefix that all other functions and global variables in your driver use. > +{ > + return container_of(_chip, struct pwm_gpio, chip); > +} > + > +static void pwm_gpio_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); > + > + cancel_work_sync(&pwm_gpio->work); > + gpiod_set_value_cansleep(pwm_gpio->desc, > + pwm_gpio->polarity == PWM_POLARITY_INVERSED); > +} > + > +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); > + u64 period = clamp(state->period, U64_C(50000), U64_C(1000000000)); .apply is expected to implement the biggest period that is not bigger than the requested period. So clamping to a value > 50000 is wrong. Also the (arbitrary?) choice to not implement periods < 50 µs needs a comment. (I assume it is something like: the pwm-gpio driver suffers from latency in the timer and gpio frameworks, smaller periods are expected to be impossible to be implemented.) > + u64 duty_cycle = clamp(state->duty_cycle, U64_C(0), period); If you consider that a period < 50 µs is problematic, I wonder if you have similar concerns for a duty_cycle of 25 µs. > + pwm_gpio->ton_ns = duty_cycle; > + pwm_gpio->toff_ns = period - duty_cycle; If the timer triggers here it has an inconsistent view because ton and toff are already updated, but polarity isn't. > + pwm_gpio->polarity = state->polarity; > + > + if (state->enabled && !atomic_read(&pwm_gpio->enabled)) { > + atomic_set(&pwm_gpio->enabled, 1); > + hrtimer_start(&pwm_gpio->timer, 0, HRTIMER_MODE_REL); > + } else if (!state->enabled && atomic_read(&pwm_gpio->enabled)) { > + atomic_set(&pwm_gpio->enabled, 0); > + pwm_gpio->output = false; > + schedule_work(&pwm_gpio->work); > + } It's not obvious to me that this driver completes the currently running period before the new setting takes effect. If you think it is, please make the mechanism more obvious in a comment, if it isn't, please fix. > + return 0; > +} > + > +static void pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct pwm_gpio *pwm_gpio = to_pwm_gpio(chip); > + > + state->duty_cycle = pwm_gpio->ton_ns; > + state->period = pwm_gpio->ton_ns + pwm_gpio->toff_ns; > + state->polarity = pwm_gpio->polarity; > + state->enabled = atomic_read(&pwm_gpio->enabled); > +} > + > +static const struct pwm_ops pwm_gpio_ops = { > + .free = pwm_gpio_free, > + .apply = pwm_gpio_apply, > + .get_state = pwm_gpio_get_state, > + .owner = THIS_MODULE, > +}; > + > +static int pwm_gpio_probe(struct platform_device *pdev) > +{ > + struct pwm_gpio *pwm_gpio; > + > + pwm_gpio = devm_kzalloc(&pdev->dev, sizeof(*pwm_gpio), GFP_KERNEL); > + if (!pwm_gpio) > + return -ENOMEM; > + > + pwm_gpio->desc = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW); > + if (IS_ERR(pwm_gpio->desc)) > + return PTR_ERR(pwm_gpio->desc); > + > + INIT_WORK(&pwm_gpio->work, pwm_gpio_work); > + > + hrtimer_init(&pwm_gpio->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + pwm_gpio->timer.function = pwm_gpio_do_timer; > + pwm_gpio->chip.dev = &pdev->dev; > + pwm_gpio->chip.ops = &pwm_gpio_ops; > + pwm_gpio->chip.npwm = 1; pwm_gpio->base = -1; > + platform_set_drvdata(pdev, pwm_gpio); > + > + return pwmchip_add(&pwm_gpio->chip); > +} > + > +static int pwm_gpio_remove(struct platform_device *pdev) > +{ > + struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev); > + > + hrtimer_cancel(&pwm_gpio->timer); > + > + return pwmchip_remove(&pwm_gpio->chip); Please swap the order of hrtimer_cancel and pwmchip_remove to have it in reverse order to the allocation in .probe and to make the PWM unfunctional only after the pwmchip is unregistered. > +} > + > +#ifdef CONFIG_OF > +static const struct of_device_id pwm_gpio_of_match[] = { > + { .compatible = "pwm-gpio", }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, pwm_gpio_of_match); > +#endif > + > +static struct platform_driver pwm_gpio_driver = { > + .probe = pwm_gpio_probe, > + .remove = pwm_gpio_remove, > + .driver = { > + .name = "pwm-gpio", > + .of_match_table = of_match_ptr(pwm_gpio_of_match), > + }, > +}; > +module_platform_driver(pwm_gpio_driver); > + > +MODULE_DESCRIPTION("PWM GPIO driver"); > +MODULE_ALIAS("platform:pwm-gpio"); > +MODULE_AUTHOR("Nicola Di Lieto"); > +MODULE_LICENSE("GPL"); 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 0/2] pwm: pwm-gpio: New driver 2020-12-09 7:28 ` Uwe Kleine-König @ 2020-12-11 17:04 ` Nicola Di Lieto 2020-12-11 17:04 ` [PATCH v2 1/2] " Nicola Di Lieto 2020-12-11 17:04 ` [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings Nicola Di Lieto 0 siblings, 2 replies; 23+ messages in thread From: Nicola Di Lieto @ 2020-12-11 17:04 UTC (permalink / raw) To: linux-pwm; +Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Rob Herring New version, implementing Uwe's suggestions. Changes in v2: - Synchronize pwm variables to ensure consistency - Remove clamping of pwm period and duty cycle - Rename "to_pwm_gpio" as "pwm_gpio_from_chip" - Initialize pwm_gpio->chip.base to -1 - Swap order of hrtimer_cancel and pwmchip_remove - Increase #pwm-cells to 3 Nicola Di Lieto (2): pwm: pwm-gpio: New driver pwm: pwm-gpio: Add DT bindings .../devicetree/bindings/pwm/pwm-gpio.yaml | 42 +++++ MAINTAINERS | 7 + drivers/pwm/Kconfig | 10 ++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-gpio.c | 188 +++++++++++++++++++++ 5 files changed, 248 insertions(+) create mode 100644 Documentation/devicetree/bindings/pwm/pwm-gpio.yaml create mode 100644 drivers/pwm/pwm-gpio.c -- 2.11.0 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 1/2] pwm: pwm-gpio: New driver 2020-12-11 17:04 ` [PATCH v2 0/2] " Nicola Di Lieto @ 2020-12-11 17:04 ` Nicola Di Lieto 2021-01-17 13:04 ` Uwe Kleine-König 2023-02-13 23:36 ` andy.shevchenko 2020-12-11 17:04 ` [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings Nicola Di Lieto 1 sibling, 2 replies; 23+ messages in thread From: Nicola Di Lieto @ 2020-12-11 17:04 UTC (permalink / raw) To: linux-pwm; +Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Rob Herring This new driver allows pulse width modulating any GPIOs using a high resolution timer. It is fully generic and can be useful in a variety of situations. As an example I used it to provide a pwm to the pwm-beeper driver so that my embedded system can produce tones through a piezo buzzer connected to a GPIO which unfortunately is not hardware PWM capable. Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> --- MAINTAINERS | 7 ++ drivers/pwm/Kconfig | 10 +++ drivers/pwm/Makefile | 1 + drivers/pwm/pwm-gpio.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 206 insertions(+) create mode 100644 drivers/pwm/pwm-gpio.c diff --git a/MAINTAINERS b/MAINTAINERS index 52086876ce40..486a8857b47b 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14222,6 +14222,13 @@ F: Documentation/devicetree/bindings/hwmon/pwm-fan.txt F: Documentation/hwmon/pwm-fan.rst F: drivers/hwmon/pwm-fan.c +PWM GPIO DRIVER +M: Nicola Di Lieto <nicola.dilieto@gmail.com> +L: linux-pwm@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/pwm/pwm-gpio.yaml +F: drivers/pwm/pwm-gpio.c + PWM IR Transmitter M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.org diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig index 63be5362fd3a..5432084c6276 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 "PWM GPIO support" + depends on GPIOLIB + depends on HIGH_RES_TIMERS + help + Generic PWM for software modulation of GPIOs + + 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 cbdcd55d69ee..eea0216215a7 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..06b4ddee389d --- /dev/null +++ b/drivers/pwm/pwm-gpio.c @@ -0,0 +1,188 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Generic software PWM for modulating GPIOs + * + * Copyright 2020 Nicola Di Lieto + * + * Author: Nicola Di Lieto <nicola.dilieto@gmail.com> + */ + +#include <linux/atomic.h> +#include <linux/err.h> +#include <linux/gpio/consumer.h> +#include <linux/hrtimer.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pwm.h> +#include <linux/spinlock.h> +#include <linux/workqueue.h> + +struct pwm_gpio { + struct pwm_chip chip; + struct gpio_desc *desc; + struct work_struct work; + struct hrtimer timer; + atomic_t enabled; + spinlock_t lock; + struct { + u64 ton_ns; + u64 toff_ns; + bool invert; + } cur, new; + bool state; + bool output; +}; + +static void pwm_gpio_work(struct work_struct *work) +{ + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); + + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->output); +} + +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle) +{ + struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer); + u64 ns; + + if (!atomic_read(&pwm_gpio->enabled)) + return HRTIMER_NORESTART; + + if (pwm_gpio->state) { + ns = pwm_gpio->cur.toff_ns; + pwm_gpio->state = false; + } else { + if (spin_trylock(&pwm_gpio->lock)) { + pwm_gpio->cur = pwm_gpio->new; + spin_unlock(&pwm_gpio->lock); + } + ns = pwm_gpio->cur.ton_ns; + pwm_gpio->state = true; + } + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; + + schedule_work(&pwm_gpio->work); + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); + + return HRTIMER_RESTART; +} + +static inline struct pwm_gpio *pwm_gpio_from_chip(struct pwm_chip *_chip) +{ + return container_of(_chip, struct pwm_gpio, chip); +} + +static void pwm_gpio_free(struct pwm_chip *chip, struct pwm_device *pwm) +{ + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); + + cancel_work_sync(&pwm_gpio->work); + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->cur.invert); +} + +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm, + const struct pwm_state *state) +{ + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); + + spin_lock(&pwm_gpio->lock); + pwm_gpio->new.ton_ns = state->duty_cycle; + pwm_gpio->new.toff_ns = state->period - state->duty_cycle; + pwm_gpio->new.invert = (state->polarity == PWM_POLARITY_INVERSED); + spin_unlock(&pwm_gpio->lock); + + if (state->enabled && !atomic_cmpxchg(&pwm_gpio->enabled, 0, 1)) { + hrtimer_start(&pwm_gpio->timer, 0, HRTIMER_MODE_REL); + } else if (!state->enabled && atomic_cmpxchg(&pwm_gpio->enabled, 1, 0)) { + pwm_gpio->state = false; + pwm_gpio->output = (state->polarity == PWM_POLARITY_INVERSED); + schedule_work(&pwm_gpio->work); + } + return 0; +} + +static void pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm, + struct pwm_state *state) +{ + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); + + state->duty_cycle = pwm_gpio->new.ton_ns; + state->period = pwm_gpio->new.ton_ns + pwm_gpio->new.toff_ns; + state->polarity = pwm_gpio->new.invert ? PWM_POLARITY_INVERSED + : PWM_POLARITY_NORMAL; + state->enabled = atomic_read(&pwm_gpio->enabled); +} + +static const struct pwm_ops pwm_gpio_ops = { + .free = pwm_gpio_free, + .apply = pwm_gpio_apply, + .get_state = pwm_gpio_get_state, + .owner = THIS_MODULE, +}; + +static int pwm_gpio_probe(struct platform_device *pdev) +{ + struct pwm_gpio *pwm_gpio; + + pwm_gpio = devm_kzalloc(&pdev->dev, sizeof(*pwm_gpio), GFP_KERNEL); + if (!pwm_gpio) + return -ENOMEM; + + pwm_gpio->desc = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW); + if (IS_ERR(pwm_gpio->desc)) + return PTR_ERR(pwm_gpio->desc); + + INIT_WORK(&pwm_gpio->work, pwm_gpio_work); + + hrtimer_init(&pwm_gpio->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); + pwm_gpio->timer.function = pwm_gpio_do_timer; + pwm_gpio->chip.dev = &pdev->dev; + pwm_gpio->chip.ops = &pwm_gpio_ops; + pwm_gpio->chip.npwm = 1; + pwm_gpio->chip.base = -1; + + platform_set_drvdata(pdev, pwm_gpio); + + spin_lock_init(&pwm_gpio->lock); + + return pwmchip_add(&pwm_gpio->chip); +} + +static int pwm_gpio_remove(struct platform_device *pdev) +{ + int ret; + struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev); + + ret = pwmchip_remove(&pwm_gpio->chip); + if (ret) + return ret; + + hrtimer_cancel(&pwm_gpio->timer); + + return 0; +} + +#ifdef CONFIG_OF +static const struct of_device_id pwm_gpio_of_match[] = { + { .compatible = "pwm-gpio", }, + { } +}; +MODULE_DEVICE_TABLE(of, pwm_gpio_of_match); +#endif + +static struct platform_driver pwm_gpio_driver = { + .probe = pwm_gpio_probe, + .remove = pwm_gpio_remove, + .driver = { + .name = "pwm-gpio", + .of_match_table = of_match_ptr(pwm_gpio_of_match), + }, +}; +module_platform_driver(pwm_gpio_driver); + +MODULE_DESCRIPTION("PWM GPIO driver"); +MODULE_ALIAS("platform:pwm-gpio"); +MODULE_AUTHOR("Nicola Di Lieto"); +MODULE_LICENSE("GPL"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2020-12-11 17:04 ` [PATCH v2 1/2] " Nicola Di Lieto @ 2021-01-17 13:04 ` Uwe Kleine-König 2021-01-17 13:58 ` Nicola Di Lieto 2021-01-22 10:15 ` Linus Walleij 2023-02-13 23:36 ` andy.shevchenko 1 sibling, 2 replies; 23+ messages in thread From: Uwe Kleine-König @ 2021-01-17 13:04 UTC (permalink / raw) To: Nicola Di Lieto Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Linus Walleij, Bartosz Golaszewski, linux-gpio [-- Attachment #1: Type: text/plain, Size: 8907 bytes --] Hello, [added the gpio people to Cc:] On Fri, Dec 11, 2020 at 06:04:31PM +0100, Nicola Di Lieto wrote: > This new driver allows pulse width modulating any GPIOs using > a high resolution timer. It is fully generic and can be useful > in a variety of situations. As an example I used it to provide > a pwm to the pwm-beeper driver so that my embedded system can > produce tones through a piezo buzzer connected to a GPIO which > unfortunately is not hardware PWM capable. > > Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> > --- > MAINTAINERS | 7 ++ > drivers/pwm/Kconfig | 10 +++ > drivers/pwm/Makefile | 1 + > drivers/pwm/pwm-gpio.c | 188 +++++++++++++++++++++++++++++++++++++++++++++++++ > 4 files changed, 206 insertions(+) > create mode 100644 drivers/pwm/pwm-gpio.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 52086876ce40..486a8857b47b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -14222,6 +14222,13 @@ F: Documentation/devicetree/bindings/hwmon/pwm-fan.txt > F: Documentation/hwmon/pwm-fan.rst > F: drivers/hwmon/pwm-fan.c > > +PWM GPIO DRIVER > +M: Nicola Di Lieto <nicola.dilieto@gmail.com> > +L: linux-pwm@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/pwm/pwm-gpio.yaml > +F: drivers/pwm/pwm-gpio.c Maybe only add the line for the binding doc in the second patch? > + > PWM IR Transmitter > M: Sean Young <sean@mess.org> > L: linux-media@vger.kernel.org > diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig > index 63be5362fd3a..5432084c6276 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 "PWM GPIO support" > + depends on GPIOLIB > + depends on HIGH_RES_TIMERS > + help > + Generic PWM for software modulation of GPIOs > + > + 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 cbdcd55d69ee..eea0216215a7 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..06b4ddee389d > --- /dev/null > +++ b/drivers/pwm/pwm-gpio.c > @@ -0,0 +1,188 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Generic software PWM for modulating GPIOs > + * > + * Copyright 2020 Nicola Di Lieto > + * > + * Author: Nicola Di Lieto <nicola.dilieto@gmail.com> > + */ > + > +#include <linux/atomic.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/hrtimer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/spinlock.h> > +#include <linux/workqueue.h> > + > +struct pwm_gpio { > + struct pwm_chip chip; > + struct gpio_desc *desc; > + struct work_struct work; > + struct hrtimer timer; > + atomic_t enabled; > + spinlock_t lock; > + struct { > + u64 ton_ns; > + u64 toff_ns; > + bool invert; > + } cur, new; > + bool state; > + bool output; > +}; I would have called this struct pwm_gpio_ddata. Given that pwm_gpio is the common prefix of all variables and functions in this driver, pwm_gpio alone is a bit short. > +static void pwm_gpio_work(struct work_struct *work) > +{ > + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); > + > + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->output); Usually you want to check the return value of gpiod_set_value. @Linus + Bartosz: What do you think, does it need checking for an error here? > +} > + > +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle) > +{ > + struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer); > + u64 ns; > + > + if (!atomic_read(&pwm_gpio->enabled)) > + return HRTIMER_NORESTART; > + > + if (pwm_gpio->state) { > + ns = pwm_gpio->cur.toff_ns; > + pwm_gpio->state = false; > + } else { > + if (spin_trylock(&pwm_gpio->lock)) { > + pwm_gpio->cur = pwm_gpio->new; > + spin_unlock(&pwm_gpio->lock); > + } > + ns = pwm_gpio->cur.ton_ns; > + pwm_gpio->state = true; > + } > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; > + > + schedule_work(&pwm_gpio->work); > + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); This is hard to understand. I think we need more comments explaining (e.g.) the semantic of the members of struct pwm_gpio. Does it make sense to use the workqueue only if the GPIO is a sleeping one and for fast GPIOs call gpiod_set_value directly? > + > + return HRTIMER_RESTART; > +} > + > +static inline struct pwm_gpio *pwm_gpio_from_chip(struct pwm_chip *_chip) > +{ > + return container_of(_chip, struct pwm_gpio, chip); > +} > + > +static void pwm_gpio_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); > + > + cancel_work_sync(&pwm_gpio->work); > + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->cur.invert); > +} > + > +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm, > + const struct pwm_state *state) > +{ > + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); > + > + spin_lock(&pwm_gpio->lock); > + pwm_gpio->new.ton_ns = state->duty_cycle; > + pwm_gpio->new.toff_ns = state->period - state->duty_cycle; > + pwm_gpio->new.invert = (state->polarity == PWM_POLARITY_INVERSED); > + spin_unlock(&pwm_gpio->lock); > + > + if (state->enabled && !atomic_cmpxchg(&pwm_gpio->enabled, 0, 1)) { > + hrtimer_start(&pwm_gpio->timer, 0, HRTIMER_MODE_REL); > + } else if (!state->enabled && atomic_cmpxchg(&pwm_gpio->enabled, 1, 0)) { > + pwm_gpio->state = false; > + pwm_gpio->output = (state->polarity == PWM_POLARITY_INVERSED); > + schedule_work(&pwm_gpio->work); > + } > + return 0; > +} > + > +static void pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm, > + struct pwm_state *state) > +{ > + struct pwm_gpio *pwm_gpio = pwm_gpio_from_chip(chip); > + > + state->duty_cycle = pwm_gpio->new.ton_ns; > + state->period = pwm_gpio->new.ton_ns + pwm_gpio->new.toff_ns; > + state->polarity = pwm_gpio->new.invert ? PWM_POLARITY_INVERSED > + : PWM_POLARITY_NORMAL; > + state->enabled = atomic_read(&pwm_gpio->enabled); > +} > + > +static const struct pwm_ops pwm_gpio_ops = { > + .free = pwm_gpio_free, A free without request seems wrong. The callback stops the PWM, this is wrong, the PWM should continue to toggle. > + .apply = pwm_gpio_apply, > + .get_state = pwm_gpio_get_state, > + .owner = THIS_MODULE, > +}; > + > +static int pwm_gpio_probe(struct platform_device *pdev) > +{ > + struct pwm_gpio *pwm_gpio; > + > + pwm_gpio = devm_kzalloc(&pdev->dev, sizeof(*pwm_gpio), GFP_KERNEL); > + if (!pwm_gpio) > + return -ENOMEM; > + > + pwm_gpio->desc = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW); > + if (IS_ERR(pwm_gpio->desc)) > + return PTR_ERR(pwm_gpio->desc); > + > + INIT_WORK(&pwm_gpio->work, pwm_gpio_work); > + > + hrtimer_init(&pwm_gpio->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); > + pwm_gpio->timer.function = pwm_gpio_do_timer; > + pwm_gpio->chip.dev = &pdev->dev; > + pwm_gpio->chip.ops = &pwm_gpio_ops; > + pwm_gpio->chip.npwm = 1; > + pwm_gpio->chip.base = -1; > + > + platform_set_drvdata(pdev, pwm_gpio); > + > + spin_lock_init(&pwm_gpio->lock); > + > + return pwmchip_add(&pwm_gpio->chip); Error message please if pwmchip_add fails > +} > + > +static int pwm_gpio_remove(struct platform_device *pdev) > +{ > + int ret; > + struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev); > + > + ret = pwmchip_remove(&pwm_gpio->chip); > + if (ret) > + return ret; This exit path is bad. The return value of the remove callback is ignored and after pwm_gpio_remove() returns the gpio and *pwm_gpio are freed. > + > + hrtimer_cancel(&pwm_gpio->timer); > + > + return 0; > +} 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-17 13:04 ` Uwe Kleine-König @ 2021-01-17 13:58 ` Nicola Di Lieto 2021-01-17 18:45 ` Uwe Kleine-König 2021-01-22 10:15 ` Linus Walleij 1 sibling, 1 reply; 23+ messages in thread From: Nicola Di Lieto @ 2021-01-17 13:58 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Linus Walleij, Bartosz Golaszewski, linux-gpio Hello, thanks for reviewing this. On Sun, Jan 17, 2021 at 02:04:34PM +0100, Uwe Kleine-König wrote: >Maybe only add the line for the binding doc in the second patch? >I would have called this struct pwm_gpio_ddata. Given that pwm_gpio is >the common prefix of all variables and functions in this driver, >pwm_gpio alone is a bit short. I will change these as suggested. >Usually you want to check the return value of gpiod_set_value. @Linus + >Bartosz: What do you think, does it need checking for an error here? I will wait until they reply. >> + >> + if (!atomic_read(&pwm_gpio->enabled)) >> + return HRTIMER_NORESTART; >> + >> + if (pwm_gpio->state) { >> + ns = pwm_gpio->cur.toff_ns; >> + pwm_gpio->state = false; >> + } else { >> + if (spin_trylock(&pwm_gpio->lock)) { >> + pwm_gpio->cur = pwm_gpio->new; >> + spin_unlock(&pwm_gpio->lock); >> + } >> + ns = pwm_gpio->cur.ton_ns; >> + pwm_gpio->state = true; >> + } >> + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; >> + >> + schedule_work(&pwm_gpio->work); >> + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); > >This is hard to understand. I think we need more comments explaining >(e.g.) the semantic of the members of struct pwm_gpio. Would it be OK if I added the following comment in the code? pwm_gpio_apply writes the new settings to pwm_gpio->new, synchronized by the spinlock. At the beginning of every PWM cycle pwm_gpio->new is copied into pwm_gpio->cur, but only if the spinlock can be locked immediately (meaning the settings aren't being applied concurrently to the beginning of the period). By not waiting for the spinlock, no extra jitter in the PWM period is introduced. >Does it make sense to use the workqueue only if the GPIO is a sleeping >one and for fast GPIOs call gpiod_set_value directly? I thought about this but didn't implement it as it seemed overkill to me. The workqueue is needed anyway to cope with sleeping GPIOs, and it can deal with fast GPIOs with insignificant degradation compared to a direct implementation. >> +static const struct pwm_ops pwm_gpio_ops = { >> + .free = pwm_gpio_free, > >A free without request seems wrong. The callback stops the PWM, this is >wrong, the PWM should continue to toggle. > Would it be OK to remove the pwm_gpio_free callback altogether and move the cancel_work_sync() call to pwm_gpio_remove? >Error message please if pwmchip_add fails I will add this. >> +} >> + >> +static int pwm_gpio_remove(struct platform_device *pdev) >> +{ >> + int ret; >> + struct pwm_gpio *pwm_gpio = platform_get_drvdata(pdev); >> + >> + ret = pwmchip_remove(&pwm_gpio->chip); >> + if (ret) >> + return ret; > >This exit path is bad. The return value of the remove callback is >ignored and after pwm_gpio_remove() returns the gpio and *pwm_gpio are >freed. > >> + >> + hrtimer_cancel(&pwm_gpio->timer); >> + >> + return 0; >> +} Would it be ok to cancel the timer first and then "return pwmchip_remove(...)"? Best regards, Nicola ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-17 13:58 ` Nicola Di Lieto @ 2021-01-17 18:45 ` Uwe Kleine-König 2021-01-17 21:06 ` Nicola Di Lieto 0 siblings, 1 reply; 23+ messages in thread From: Uwe Kleine-König @ 2021-01-17 18:45 UTC (permalink / raw) To: Nicola Di Lieto Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Linus Walleij, Bartosz Golaszewski, linux-gpio [-- Attachment #1: Type: text/plain, Size: 2664 bytes --] Hello Nicola, On Sun, Jan 17, 2021 at 02:58:03PM +0100, Nicola Di Lieto wrote: > On Sun, Jan 17, 2021 at 02:04:34PM +0100, Uwe Kleine-König wrote: > > > + if (!atomic_read(&pwm_gpio->enabled)) > > > + return HRTIMER_NORESTART; > > > + > > > + if (pwm_gpio->state) { > > > + ns = pwm_gpio->cur.toff_ns; > > > + pwm_gpio->state = false; > > > + } else { > > > + if (spin_trylock(&pwm_gpio->lock)) { > > > + pwm_gpio->cur = pwm_gpio->new; > > > + spin_unlock(&pwm_gpio->lock); > > > + } > > > + ns = pwm_gpio->cur.ton_ns; > > > + pwm_gpio->state = true; > > > + } > > > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; > > > + > > > + schedule_work(&pwm_gpio->work); > > > + hrtimer_forward(handle, hrtimer_get_expires(handle), ns_to_ktime(ns)); > > > > This is hard to understand. I think we need more comments explaining > > (e.g.) the semantic of the members of struct pwm_gpio. > > Would it be OK if I added the following comment in the code? > > pwm_gpio_apply writes the new settings to pwm_gpio->new, synchronized by the > spinlock. At the beginning of every PWM cycle pwm_gpio->new is copied into > pwm_gpio->cur, but only if the spinlock can be locked immediately (meaning > the settings aren't being applied concurrently to the beginning of the > period). By not waiting for the spinlock, no extra jitter in the PWM period > is introduced. So far I understood also only comment. What wasn't obvious immediately is the state member. > > Does it make sense to use the workqueue only if the GPIO is a sleeping > > one and for fast GPIOs call gpiod_set_value directly? > > I thought about this but didn't implement it as it seemed overkill to me. > The workqueue is needed anyway to cope with sleeping GPIOs, and it can deal > with fast GPIOs with insignificant degradation compared to a direct > implementation. OK. If later the need arises this can be added then. > > > +static const struct pwm_ops pwm_gpio_ops = { > > > + .free = pwm_gpio_free, > > > > A free without request seems wrong. The callback stops the PWM, this is > > wrong, the PWM should continue to toggle. > > > > Would it be OK to remove the pwm_gpio_free callback altogether and move the > cancel_work_sync() call to pwm_gpio_remove? Yes, that sounds right. > Would it be ok to cancel the timer first and then "return > pwmchip_remove(...)"? No. The PWM must stay functional until pwmchip_remove() returns. 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-17 18:45 ` Uwe Kleine-König @ 2021-01-17 21:06 ` Nicola Di Lieto 2021-01-18 7:44 ` Uwe Kleine-König 0 siblings, 1 reply; 23+ messages in thread From: Nicola Di Lieto @ 2021-01-17 21:06 UTC (permalink / raw) To: Uwe Kleine-König Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Linus Walleij, Bartosz Golaszewski, linux-gpio Hello Uwe, On Sun, Jan 17, 2021 at 07:45:56PM +0100, Uwe Kleine-König wrote: >> > > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; > >So far I understood also only comment. What wasn't obvious immediately >is the state member. Would it become clear enough by adding: "state is the logical PWM output; the actual PIN output level is inverted by XORing with cur.invert when the latter is true" ? >> Would it be ok to cancel the timer first and then "return >> pwmchip_remove(...)"? > >No. The PWM must stay functional until pwmchip_remove() returns. > Could you please clarify what I should do when pwmchip_remove returns non-zero? In my original implementation - if pwmchip_remove returns a non-zero error code, I return it to the caller and do not cancel the timer. - if pwmchip_remove returns zero, I cancel the timer and return zero to the caller So under no circumstance I return a different code from what pwmchip_remove does... Best regards, Nicola ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-17 21:06 ` Nicola Di Lieto @ 2021-01-18 7:44 ` Uwe Kleine-König 0 siblings, 0 replies; 23+ messages in thread From: Uwe Kleine-König @ 2021-01-18 7:44 UTC (permalink / raw) To: Nicola Di Lieto Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Linus Walleij, Bartosz Golaszewski, linux-gpio [-- Attachment #1: Type: text/plain, Size: 2934 bytes --] Hello Nicola, On Sun, Jan 17, 2021 at 10:06:18PM +0100, Nicola Di Lieto wrote: > On Sun, Jan 17, 2021 at 07:45:56PM +0100, Uwe Kleine-König wrote: > > > > > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; > > > > So far I understood also only comment. What wasn't obvious immediately > > is the state member. > > Would it become clear enough by adding: "state is the logical PWM output; > the actual PIN output level is inverted by XORing with cur.invert when the > latter is true" ? This was at least good enough for me to understand it now. So iff state is true, the PWM is in the active phase of the current period. Maybe "currently_active" is a better name for this variable? Then the code could (with some comments added and a few more variables renamed) could look as follows: if (ddata->currently_active) { /* Enter the inactive part of the current period. */ ddata->currently_active = false; next_transistion = ddata->cur.toff_ns; } else { /* * Start a new period. First check if there is a new * configuration setting pending in ddata->new. */ ddata->currently_active = true; if (spin_trylock(&ddata->lock)) { ddata->cur = ddata->new; spin_unlock(&ddata->lock); } next_transition = ddata->cur.ton_ns; } ... which IMHO is easier to understand. I think there are still two problems with this approach: - The locking is hard to follow, .enabled is accessed using atomic accessors, .new is protected by the spinlock and the other members are not accessed concurrently, right? If pwm_apply(..., {.enabled = false}) and pwm_apply(.., {.enabled = true}) are called in quick sequence (e.g. faster than the first call triggers the work queue) there is trouble ahead, isn't there? - If .duty_cycle is equal to 0 (or .period) the output should be constant. I think this isn't what will happen. > > > Would it be ok to cancel the timer first and then "return > > > pwmchip_remove(...)"? > > > > No. The PWM must stay functional until pwmchip_remove() returns. > > > > Could you please clarify what I should do when pwmchip_remove returns > non-zero? In my original implementation > - if pwmchip_remove returns a non-zero error code, I return it to the > caller and do not cancel the timer. > - if pwmchip_remove returns zero, I cancel the timer and return zero to the > caller IMHO it's a bug that pwmchip_remove() can return an error code. I think the best you can do currently is: ret = pwmchip_remove(...) WARN_ON(ret); hrtimer_cancel(..); return 0; because whatever you do is wrong. To sort this out needs some thought and work in the framework and so is unrelated to this patch. 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-17 13:04 ` Uwe Kleine-König 2021-01-17 13:58 ` Nicola Di Lieto @ 2021-01-22 10:15 ` Linus Walleij 2023-02-10 21:54 ` Angelo Compagnucci 1 sibling, 1 reply; 23+ messages in thread From: Linus Walleij @ 2021-01-22 10:15 UTC (permalink / raw) To: Uwe Kleine-König Cc: Nicola Di Lieto, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Hi Nicola, Uwe, thanks for looping me in on this very interesting driver! This can be really helpful, I already see that it would be possible to replace the hopeless idiomatic driver for the NSLU2 ixp4xx beeper speaker with this driver. On Sun, Jan 17, 2021 at 2:04 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > +static void pwm_gpio_work(struct work_struct *work) > > +{ > > + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); > > + > > + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->output); > > Usually you want to check the return value of gpiod_set_value. @Linus + > Bartosz: What do you think, does it need checking for an error here? We have traditionally been quite relaxed on that. But since it is the cansleep variant, meaning this GPIO could be on a GPIO expander on I2C or SPI, it would be more important. However: in this specific case for PWM I wonder if we should stick with gpio_set_value() without _cansleep, and then we can definitely skip the error check. That means GPIO PWM can happen on on-chip GPIOs that are just a register write. Certainly no need to check the return value of these. Also we know it will satisfy timing. If we allow GPIO PWM on slow buses and expanders that can sleep I do not think the PWM will be very good or reliable. For example on an I2C expander, with the bus speed of max 400 kHz, what kind of PWM can we create on that? Certainly now above 200 kHz (Nyquist frequency) and probably less. And we have to way to actually check the frequency of the underlying bus, so I am a bit skeptic here. Would gpio_set_value() work for now or do we need to support expanders and _cansleep? Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2021-01-22 10:15 ` Linus Walleij @ 2023-02-10 21:54 ` Angelo Compagnucci 2023-02-22 12:51 ` andy.shevchenko 0 siblings, 1 reply; 23+ messages in thread From: Angelo Compagnucci @ 2023-02-10 21:54 UTC (permalink / raw) To: Linus Walleij Cc: Uwe Kleine-König, Nicola Di Lieto, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Il giorno ven 22 gen 2021 alle ore 11:20 Linus Walleij <linus.walleij@linaro.org> ha scritto: > > Hi Nicola, Uwe, > > thanks for looping me in on this very interesting driver! > > This can be really helpful, I already see that it would be possible to > replace the hopeless idiomatic driver for the NSLU2 ixp4xx > beeper speaker with this driver. > > On Sun, Jan 17, 2021 at 2:04 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > > > +static void pwm_gpio_work(struct work_struct *work) > > > +{ > > > + struct pwm_gpio *pwm_gpio = container_of(work, struct pwm_gpio, work); > > > + > > > + gpiod_set_value_cansleep(pwm_gpio->desc, pwm_gpio->output); > > > > Usually you want to check the return value of gpiod_set_value. @Linus + > > Bartosz: What do you think, does it need checking for an error here? > > We have traditionally been quite relaxed on that. But since it is > the cansleep variant, meaning this GPIO could be on a GPIO > expander on I2C or SPI, it would be more important. > > However: in this specific case for PWM I wonder if we should > stick with gpio_set_value() without _cansleep, and then we can > definitely skip the error check. > > That means GPIO PWM can happen on on-chip GPIOs that > are just a register write. Certainly no need to check the return > value of these. Also we know it will satisfy timing. > > If we allow GPIO PWM on slow buses and expanders that can > sleep I do not think the PWM will be very good or reliable. > > For example on an I2C expander, with the bus speed of > max 400 kHz, what kind of PWM can we create on that? > Certainly now above 200 kHz (Nyquist frequency) and > probably less. And we have to way to actually check the > frequency of the underlying bus, so I am a bit skeptic > here. > > Would gpio_set_value() work for now or do we need to > support expanders and _cansleep? More than a year passed from the last message, could we reopen the discussion? I'd like to have this upstream! Thanks! > > Yours, > Linus Walleij -- Profile: http://it.linkedin.com/in/compagnucciangelo ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2023-02-10 21:54 ` Angelo Compagnucci @ 2023-02-22 12:51 ` andy.shevchenko 2023-02-22 15:00 ` Nicola Di Lieto 0 siblings, 1 reply; 23+ messages in thread From: andy.shevchenko @ 2023-02-22 12:51 UTC (permalink / raw) To: Angelo Compagnucci Cc: Linus Walleij, Uwe Kleine-König, Nicola Di Lieto, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Fri, Feb 10, 2023 at 10:54:49PM +0100, Angelo Compagnucci kirjoitti: > Il giorno ven 22 gen 2021 alle ore 11:20 Linus Walleij > <linus.walleij@linaro.org> ha scritto: ... > More than a year passed from the last message, could we reopen the > discussion? I'd like to have this upstream! Seems not much interest neither from community nor from author. Maybe later people will look into this? P.S> FWIW, I have reviewed code more than a week ago. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2023-02-22 12:51 ` andy.shevchenko @ 2023-02-22 15:00 ` Nicola Di Lieto 2023-02-23 8:50 ` Linus Walleij 2024-01-12 13:38 ` Philip Howard 0 siblings, 2 replies; 23+ messages in thread From: Nicola Di Lieto @ 2023-02-22 15:00 UTC (permalink / raw) To: andy.shevchenko Cc: Angelo Compagnucci, Linus Walleij, Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM Hello Andy On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: > >Seems not much interest neither from community nor from author. Maybe later >people will look into this? > It's not lack of interest, but rather lack of time. I should be able to have a look at this sometime the week after next. Nicola ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2023-02-22 15:00 ` Nicola Di Lieto @ 2023-02-23 8:50 ` Linus Walleij 2024-01-12 13:38 ` Philip Howard 1 sibling, 0 replies; 23+ messages in thread From: Linus Walleij @ 2023-02-23 8:50 UTC (permalink / raw) To: Nicola Di Lieto Cc: andy.shevchenko, Angelo Compagnucci, Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Wed, Feb 22, 2023 at 4:00 PM Nicola Di Lieto <nicola.dilieto@gmail.com> wrote: > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: > > > >Seems not much interest neither from community nor from author. Maybe later > >people will look into this? > > > > It's not lack of interest, but rather lack of time. I should be able to > have a look at this sometime the week after next. Thanks Nicola, I am also interested in seeing this driver upstream. Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2023-02-22 15:00 ` Nicola Di Lieto 2023-02-23 8:50 ` Linus Walleij @ 2024-01-12 13:38 ` Philip Howard 2024-01-12 18:32 ` Andy Shevchenko 2024-01-24 19:37 ` Stefan Wahren 1 sibling, 2 replies; 23+ messages in thread From: Philip Howard @ 2024-01-12 13:38 UTC (permalink / raw) To: Nicola Di Lieto, andy.shevchenko Cc: Angelo Compagnucci, Linus Walleij, Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On 22/02/2023 15:00, Nicola Di Lieto wrote: > Hello Andy > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: >> >> Seems not much interest neither from community nor from author. Maybe >> later >> people will look into this? >> > > It's not lack of interest, but rather lack of time. I should be able to > have a look at this sometime the week after next. I'd love to know if you're still likely to look into this. As part of the shift away from memory-mapped GPIO and sysfs on Raspberry Pi, I have some legacy devices which need PWM on arbitrary pins and no canonical solution. As such- I am interested! > > Nicola ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2024-01-12 13:38 ` Philip Howard @ 2024-01-12 18:32 ` Andy Shevchenko 2024-01-16 14:15 ` Phil Howard 2024-01-24 19:37 ` Stefan Wahren 1 sibling, 1 reply; 23+ messages in thread From: Andy Shevchenko @ 2024-01-12 18:32 UTC (permalink / raw) To: Philip Howard Cc: Nicola Di Lieto, Angelo Compagnucci, Linus Walleij, Uwe Kleine-König, linux-pwm, Thierry Reding, Lee Jones, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote: > On 22/02/2023 15:00, Nicola Di Lieto wrote: > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: ... > I'd love to know if you're still likely to look into this. If you are asking me as well (since To refers to me), Cc for the new version and I might look at it. Currently I am OoO till the end of month and after that I probably will have more tasks to do, so no guarantee for this cycle, but just in case Cc and we will see. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2024-01-12 18:32 ` Andy Shevchenko @ 2024-01-16 14:15 ` Phil Howard 2024-01-16 20:13 ` Andy Shevchenko 0 siblings, 1 reply; 23+ messages in thread From: Phil Howard @ 2024-01-16 14:15 UTC (permalink / raw) To: Andy Shevchenko Cc: Nicola Di Lieto, Angelo Compagnucci, Linus Walleij, Uwe Kleine-König, linux-pwm, Thierry Reding, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Fri, 12 Jan 2024 at 18:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote: > > On 22/02/2023 15:00, Nicola Di Lieto wrote: > > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: > > ... > > > I'd love to know if you're still likely to look into this. > > If you are asking me as well (since To refers to me), Cc for the new > version and I might look at it. Currently I am OoO till the end of > month and after that I probably will have more tasks to do, so no > guarantee for this cycle, but just in case Cc and we will see. Thanks for your reply! I'm not sure I know how to "Cc for the new version," it took me about an hour to generate a (hopefully) suitably formatted email response to this patch. I've only just subbed to linux-pwm. But yes- my reply was cast out into the ether in the hopes that logging my interest might get things moving again, or at least let me know if I should try tackling it myself. I'm moving over to libgpiod on Raspberry Pi, losing software PWM ( from the library I was using) in the process. I don't want to use or contrive another not-invented-here solution. > > -- > With Best Regards, > Andy Shevchenko -- Philip Howard ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2024-01-16 14:15 ` Phil Howard @ 2024-01-16 20:13 ` Andy Shevchenko 0 siblings, 0 replies; 23+ messages in thread From: Andy Shevchenko @ 2024-01-16 20:13 UTC (permalink / raw) To: Phil Howard Cc: Nicola Di Lieto, Angelo Compagnucci, Linus Walleij, Uwe Kleine-König, linux-pwm, Thierry Reding, Rob Herring, Bartosz Golaszewski, open list:GPIO SUBSYSTEM On Tue, Jan 16, 2024 at 4:15 PM Phil Howard <phil@gadgetoid.com> wrote: > On Fri, 12 Jan 2024 at 18:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Jan 12, 2024 at 3:38 PM Philip Howard <phil@gadgetoid.com> wrote: > > > On 22/02/2023 15:00, Nicola Di Lieto wrote: > > > > On Wed, Feb 22, 2023 at 02:51:00PM +0200, andy.shevchenko@gmail.com wrote: ... > > > I'd love to know if you're still likely to look into this. > > > > If you are asking me as well (since To refers to me), Cc for the new > > version and I might look at it. Currently I am OoO till the end of > > month and after that I probably will have more tasks to do, so no > > guarantee for this cycle, but just in case Cc and we will see. > > Thanks for your reply! > > I'm not sure I know how to "Cc for the new version," it took me about > an hour to generate a (hopefully) suitably formatted email response > to this patch. I've only just subbed to linux-pwm. I use the script [1] that uses a heuristics for the Cc/To fields, additionally you can add --cc "Andy Shevchenko ..." at the end. > But yes- my reply was cast out into the ether in the hopes that logging > my interest might get things moving again, or at least let me know if I > should try tackling it myself. > > I'm moving over to libgpiod on Raspberry Pi, losing software PWM ( > from the library I was using) in the process. I don't want to use or > contrive another not-invented-here solution. Totally supporting this motivation! [1]: https://github.com/andy-shev/home-bin-tools/blob/master/ge2maintainer.sh -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2024-01-12 13:38 ` Philip Howard 2024-01-12 18:32 ` Andy Shevchenko @ 2024-01-24 19:37 ` Stefan Wahren 2024-01-28 0:38 ` Linus Walleij 1 sibling, 1 reply; 23+ messages in thread From: Stefan Wahren @ 2024-01-24 19:37 UTC (permalink / raw) To: Philip Howard, Uwe Kleine-König Cc: andy.shevchenko, Angelo Compagnucci, Linus Walleij, linux-pwm, Lee Jones, Rob Herring, Bartosz Golaszewski, linux-gpio, Nicola Di Lieto, Vincent Whitchurch, oliver Hi, i was working on this a little bit during the holiday. Personally i prefer Vincent's approach [1], which is easier to read and more consequent by rejecting sleeping GPIOs. So i prepared a WIP branch [2], which was tested on a Raspberry Pi 3 B Plus + a cheap Logic analyzer. So maybe you want to give it a try. I will try to send a proper series soon. Changes: - rebased Vincent's last patch series on top of Linux 6.7 - cherry picked some improvements from Nicola's series - tried to address Uwe's, Linus' and Andy's comments - tried to avoid glitches during probe Best regards [1] - https://lore.kernel.org/all/20200915135445.al75xmjxudj2rgcp@axis.com/T/ [2] - https://github.com/lategoodbye/rpi-zero/commits/v6.7-pwm-gpio/ ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2024-01-24 19:37 ` Stefan Wahren @ 2024-01-28 0:38 ` Linus Walleij 0 siblings, 0 replies; 23+ messages in thread From: Linus Walleij @ 2024-01-28 0:38 UTC (permalink / raw) To: Stefan Wahren Cc: Philip Howard, Uwe Kleine-König, andy.shevchenko, Angelo Compagnucci, linux-pwm, Lee Jones, Rob Herring, Bartosz Golaszewski, linux-gpio, Nicola Di Lieto, Vincent Whitchurch, oliver On Wed, Jan 24, 2024 at 8:37 PM Stefan Wahren <wahrenst@gmx.net> wrote: > i was working on this a little bit during the holiday. Personally i > prefer Vincent's approach [1], which is easier to read and more > consequent by rejecting sleeping GPIOs. So i prepared a WIP branch [2], > which was tested on a Raspberry Pi 3 B Plus + a cheap Logic analyzer. So > maybe you want to give it a try. I will try to send a proper series soon. > > Changes: > - rebased Vincent's last patch series on top of Linux 6.7 > - cherry picked some improvements from Nicola's series > - tried to address Uwe's, Linus' and Andy's comments > - tried to avoid glitches during probe This looks good, I guess you will just squash and send it Co-developed-by? Thanks for looking into this! Yours, Linus Walleij ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver 2020-12-11 17:04 ` [PATCH v2 1/2] " Nicola Di Lieto 2021-01-17 13:04 ` Uwe Kleine-König @ 2023-02-13 23:36 ` andy.shevchenko 1 sibling, 0 replies; 23+ messages in thread From: andy.shevchenko @ 2023-02-13 23:36 UTC (permalink / raw) To: Nicola Di Lieto Cc: linux-pwm, Uwe Kleine-König, Thierry Reding, Lee Jones, Rob Herring Fri, Dec 11, 2020 at 06:04:31PM +0100, Nicola Di Lieto kirjoitti: > This new driver allows pulse width modulating any GPIOs using > a high resolution timer. It is fully generic and can be useful > in a variety of situations. As an example I used it to provide > a pwm to the pwm-beeper driver so that my embedded system can > produce tones through a piezo buzzer connected to a GPIO which > unfortunately is not hardware PWM capable. ... > +#include <linux/atomic.h> > +#include <linux/err.h> > +#include <linux/gpio/consumer.h> > +#include <linux/hrtimer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of.h> No need (instead use mod_devicetable.h). See below. > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/spinlock.h> > +#include <linux/workqueue.h> ... > +struct pwm_gpio { > + struct pwm_chip chip; > + struct gpio_desc *desc; > + struct work_struct work; > + struct hrtimer timer; > + atomic_t enabled; > + spinlock_t lock; > + struct { > + u64 ton_ns; > + u64 toff_ns; > + bool invert; > + } cur, new; Can we instead have two struct pwm_state members? > + bool state; > + bool output; > +}; ... > +enum hrtimer_restart pwm_gpio_do_timer(struct hrtimer *handle) > +{ > + struct pwm_gpio *pwm_gpio = container_of(handle, struct pwm_gpio, timer); > + u64 ns; > + > + if (!atomic_read(&pwm_gpio->enabled)) > + return HRTIMER_NORESTART; > + > + if (pwm_gpio->state) { > + ns = pwm_gpio->cur.toff_ns; > + pwm_gpio->state = false; > + } else { > + if (spin_trylock(&pwm_gpio->lock)) { What does this mean? So, if we don't get a lock, it doesn't imply we won't get it locked below... > + pwm_gpio->cur = pwm_gpio->new; > + spin_unlock(&pwm_gpio->lock); > + } ...i.e. here... > + ns = pwm_gpio->cur.ton_ns; ...or here. > + pwm_gpio->state = true; So the trylock needs a good comment explaining why it's not a problem. > + } > + pwm_gpio->output = pwm_gpio->state ^ pwm_gpio->cur.invert; ... > +#ifdef CONFIG_OF Drop this ifdeffery. > +static const struct of_device_id pwm_gpio_of_match[] = { > + { .compatible = "pwm-gpio", }, Inner comma is not needed. > + { } > +}; > +MODULE_DEVICE_TABLE(of, pwm_gpio_of_match); > +#endif ... > + .of_match_table = of_match_ptr(pwm_gpio_of_match), No of_match_ptr(), please. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings 2020-12-11 17:04 ` [PATCH v2 0/2] " Nicola Di Lieto 2020-12-11 17:04 ` [PATCH v2 1/2] " Nicola Di Lieto @ 2020-12-11 17:04 ` Nicola Di Lieto 2021-01-17 12:34 ` Uwe Kleine-König 1 sibling, 1 reply; 23+ messages in thread From: Nicola Di Lieto @ 2020-12-11 17:04 UTC (permalink / raw) To: linux-pwm; +Cc: Uwe Kleine-König, Thierry Reding, Lee Jones, Rob Herring Added Documentation/devicetree/bindings/pwm/pwm-gpio.yaml Signed-off-by: Nicola Di Lieto <nicola.dilieto@gmail.com> --- .../devicetree/bindings/pwm/pwm-gpio.yaml | 42 ++++++++++++++++++++++ 1 file changed, 42 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..e681b2b1c229 --- /dev/null +++ b/Documentation/devicetree/bindings/pwm/pwm-gpio.yaml @@ -0,0 +1,42 @@ +# 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: Generic software PWM for modulating GPIOs + +maintainers: + - Nicola Di Lieto <nicola.dilieto@gmail.com> + +properties: + "#pwm-cells": + description: | + It should be 3. See pwm.yaml in this directory for a + description of the cells format. + const: 3 + + compatible: + const: pwm-gpio + + gpios: + description: + GPIO to be modulated + maxItems: 1 + +required: + - "#pwm-cells" + - compatible + - gpios + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + + pwm0 { + #pwm-cells = <3>; + compatible = "pwm-gpio"; + gpios = <&gpio 1 GPIO_ACTIVE_HIGH>; + }; -- 2.11.0 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings 2020-12-11 17:04 ` [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings Nicola Di Lieto @ 2021-01-17 12:34 ` Uwe Kleine-König 0 siblings, 0 replies; 23+ messages in thread From: Uwe Kleine-König @ 2021-01-17 12:34 UTC (permalink / raw) To: Nicola Di Lieto; +Cc: linux-pwm, Thierry Reding, Lee Jones, Rob Herring [-- Attachment #1: Type: text/plain, Size: 484 bytes --] Hello, On Fri, Dec 11, 2020 at 06:04:32PM +0100, Nicola Di Lieto wrote: > Added Documentation/devicetree/bindings/pwm/pwm-gpio.yaml this looks fine to me, but this patch should be sent to the dt mailing list (devicetree@vger.kernel.org) to catch the attention of the people who have to Ack it. 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 --] ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2024-01-28 0:38 UTC | newest] Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-05 21:43 [PATCH 1/2] pwm: pwm-gpio: New driver Nicola Di Lieto 2020-12-09 7:28 ` Uwe Kleine-König 2020-12-11 17:04 ` [PATCH v2 0/2] " Nicola Di Lieto 2020-12-11 17:04 ` [PATCH v2 1/2] " Nicola Di Lieto 2021-01-17 13:04 ` Uwe Kleine-König 2021-01-17 13:58 ` Nicola Di Lieto 2021-01-17 18:45 ` Uwe Kleine-König 2021-01-17 21:06 ` Nicola Di Lieto 2021-01-18 7:44 ` Uwe Kleine-König 2021-01-22 10:15 ` Linus Walleij 2023-02-10 21:54 ` Angelo Compagnucci 2023-02-22 12:51 ` andy.shevchenko 2023-02-22 15:00 ` Nicola Di Lieto 2023-02-23 8:50 ` Linus Walleij 2024-01-12 13:38 ` Philip Howard 2024-01-12 18:32 ` Andy Shevchenko 2024-01-16 14:15 ` Phil Howard 2024-01-16 20:13 ` Andy Shevchenko 2024-01-24 19:37 ` Stefan Wahren 2024-01-28 0:38 ` Linus Walleij 2023-02-13 23:36 ` andy.shevchenko 2020-12-11 17:04 ` [PATCH v2 2/2] pwm: pwm-gpio: Add DT bindings Nicola Di Lieto 2021-01-17 12:34 ` Uwe Kleine-König
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).