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@xxxxxxxxx>
> ---
> 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@xxxxxxxxx>
> +L: linux-pwm@xxxxxxxxxxxxxxx
> +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@xxxxxxxx>
> L: linux-media@xxxxxxxxxxxxxxx
> 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@xxxxxxxxx>
> + */
> +
> +#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:
signature.asc
Description: PGP signature
