Re: [PATCH v2 1/2] pwm: pwm-gpio: New driver
From: Uwe Kleine-König <hidden>
Date: 2021-01-17 13:05:38
Also in:
linux-pwm
Hello, [added the gpio people to Cc:] On Fri, Dec 11, 2020 at 06:04:31PM +0100, Nicola Di Lieto wrote:
quoted hunk
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 <redacted> --- 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.cdiff --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?
quoted hunk
+ PWM IR Transmitter M: Sean Young <sean@mess.org> L: linux-media@vger.kernel.orgdiff --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_TESTdiff --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.odiff --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.
quoted hunk
+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?
quoted hunk
+} + +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?
quoted hunk
+ + 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.
quoted hunk
+ .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
quoted hunk
+} + +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.
quoted hunk
+ + hrtimer_cancel(&pwm_gpio->timer); + + return 0; +}
Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachments
- signature.asc [application/pgp-signature] 488 bytes