Thread (16 messages) 16 messages, 7 authors, 2024-01-28

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.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?
quoted hunk
+
 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.
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

Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help