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

[Stefan Wahren <wahrenst@gmx.net>]

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@axis.com>
>>
>> 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@axis.com>
>> Co-developed-by: Stefan Wahren <wahrenst@gmx.net>
>> Signed-off-by: Stefan Wahren <wahrenst@gmx.net>
>> ---
>>   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@ti.com>
>