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>
