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

[Dhruva Gole <d-gole@xxxxxx>]

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>