| Message ID | 20201211170432.6113-2-nicola.dilieto@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | pwm: pwm-gpio: New driver | expand |
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
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
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
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
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
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
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
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.
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.
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
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
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
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.
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
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
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/
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
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");
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