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.
> + 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?
This will help those contributing to the driver or referring to it to
understand better what each function is doing exactly.
> + .get_state = pwm_gpio_get_state,
> +};
Otherwise, driver looks good to me,
Reviewed-by: Dhruva Gole <d-gole@xxxxxx>
--
Best regards,
Dhruva Gole <d-gole@xxxxxx>
