Re: [PATCH V4 2/2] pwm: Add GPIO PWM driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dhruva,

[drop Vincent's address because it bounces]

Am 05.02.24 um 07:11 schrieb Dhruva Gole:
On Feb 04, 2024 at 23:08:51 +0100, Stefan Wahren wrote:
From: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>

Add a software PWM which toggles a GPIO from a high-resolution timer.

This will naturally not be as accurate or as efficient as a hardware
PWM, but it is useful in some cases.  I have for example used it for
evaluating LED brightness handling (via leds-pwm) on a board where the
LED was just hooked up to a GPIO, and for a simple verification of the
timer frequency on another platform.

Since high-resolution timers are used, sleeping gpio chips are not
supported and are rejected in the probe function.

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@xxxxxxxx>
Co-developed-by: Stefan Wahren <wahrenst@xxxxxxx>
Signed-off-by: Stefan Wahren <wahrenst@xxxxxxx>
---
  drivers/pwm/Kconfig    |  11 ++
  drivers/pwm/Makefile   |   1 +
  drivers/pwm/pwm-gpio.c | 228 +++++++++++++++++++++++++++++++++++++++++
  3 files changed, 240 insertions(+)
  create mode 100644 drivers/pwm/pwm-gpio.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 4b956d661755..7cfda2cde130 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -227,6 +227,17 @@ 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 "GPIO PWM support"
+	depends on GPIOLIB
+	depends on HIGH_RES_TIMERS
+	help
+	  Generic PWM framework driver for a software PWM toggling a GPIO pin
a software PWM
Nit: remove the "a", as it's not required.
thanks will fix this.

+	  from kernel high-resolution timers.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-gpio.
+
[..snip..]
+
+static int pwm_gpio_get_state(struct pwm_chip *chip, struct pwm_device *pwm,
+			       struct pwm_state *state)
+{
+	struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
+	unsigned long flags;
+
+	spin_lock_irqsave(&gpwm->lock, flags);
+
+	if (gpwm->changing)
+		*state = gpwm->next_state;
+	else
+		*state = gpwm->state;
+
+	spin_unlock_irqrestore(&gpwm->lock, flags);
+
+	return 0;
+}
+
+static const struct pwm_ops pwm_gpio_ops = {
+	.apply = pwm_gpio_apply,
Kinda looks like this is setting state? Can we be consistent with the
naming then, like pwm_gpio_set_state?
The pwm_op is called apply, so the expected suffix would be _apply like
all the other PWM driver does.

grep ".apply =" drivers/pwm/*
drivers/pwm/pwm-ab8500.c:    .apply = ab8500_pwm_apply,
drivers/pwm/pwm-apple.c:    .apply = apple_pwm_apply,
drivers/pwm/pwm-atmel.c:    .apply = atmel_pwm_apply,
...

This will help those contributing to the driver or referring to it to
understand better what each function is doing exactly.
Using ...set_state here would confuse all the experienced kernel developer.

+	.get_state = pwm_gpio_get_state,
+};
Otherwise, driver looks good to me,
Reviewed-by: Dhruva Gole <d-gole@xxxxxx>







[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux