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

["Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 6683 bytes --]

Hello,

On Fri, Aug 14, 2020 at 05:55:13PM +0200, Vincent Whitchurch wrote:
> 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>

Nice idea. IMHO this can serve as example about what we expect from pwm
drivers. There is some improvement necessary however to reach that
state.

> ---
> While preparing this driver for posting, I found a pwm-gpio driver posted to
> the lists way back in 2015 by Olliver Schinagl:
> 
>  https://lore.kernel.org/linux-pwm/1445895161-2317-8-git-send-email-o.schinagl@ultimaker.com/
> 
> This driver was developed independently, but since both drivers are trivial
> they are quite similar.  The main difference I see (apart from the usage of
> newer APIs and DT schemas) is that this driver only supports one PWM per
> instance, which makes for simpler code.  I also reject sleeping GPIO chips
> explicitly while that driver uses gpio_set_value_cansleep() from a hrtimer,
> which is a no-no.
> 
>  drivers/pwm/Kconfig    |  10 ++++
>  drivers/pwm/Makefile   |   1 +
>  drivers/pwm/pwm-gpio.c | 123 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 134 insertions(+)
>  create mode 100644 drivers/pwm/pwm-gpio.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index 7dbcf6973d33..20e4fda82e61 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -181,6 +181,16 @@ 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 OF && GPIOLIB
> +	help
> +	  Generic PWM framework driver for a software PWM toggling a GPIO pin
> +	  from kernel high-resolution timers.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called pwm-gpio.
> +
>  config PWM_HIBVT
>  	tristate "HiSilicon BVT PWM support"
>  	depends on ARCH_HISI || COMPILE_TEST
> diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
> index 2c2ba0a03557..2e045f063cd1 100644
> --- a/drivers/pwm/Makefile
> +++ b/drivers/pwm/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PWM_CRC)		+= pwm-crc.o
>  obj-$(CONFIG_PWM_CROS_EC)	+= pwm-cros-ec.o
>  obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
>  obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
> +obj-$(CONFIG_PWM_GPIO)		+= pwm-gpio.o
>  obj-$(CONFIG_PWM_HIBVT)		+= pwm-hibvt.o
>  obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
>  obj-$(CONFIG_PWM_IMX1)		+= pwm-imx1.o
> diff --git a/drivers/pwm/pwm-gpio.c b/drivers/pwm/pwm-gpio.c
> new file mode 100644
> index 000000000000..e579aca0f937
> --- /dev/null
> +++ b/drivers/pwm/pwm-gpio.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (C) 2020 Axis Communications AB */
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/platform_device.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/pwm.h>
> +#include <linux/err.h>
> +#include <linux/of.h>
> +
> +struct pwm_gpio {
> +	struct pwm_chip chip;
> +	struct hrtimer hrtimer;
> +	struct gpio_desc *gpio;
> +	ktime_t on_interval;
> +	ktime_t off_interval;
> +	bool invert;
> +	bool on;
> +};
> +
> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *hrtimer)
> +{
> +	struct pwm_gpio *gpwm = container_of(hrtimer, struct pwm_gpio, hrtimer);
> +	bool newon = !gpwm->on;
> +
> +	gpwm->on = newon;
> +	gpiod_set_value(gpwm->gpio, newon ^ gpwm->invert);
> +
> +	hrtimer_forward_now(hrtimer, newon ? gpwm->on_interval : gpwm->off_interval);

If I understand correct this adds the new interval on top of "now" and
not on top of the previous timestamp where an edge was expected. Would
it make sense to change that?

> +
> +	return HRTIMER_RESTART;
> +}
> +
> +static int pwm_gpio_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  const struct pwm_state *state)
> +{
> +	struct pwm_gpio *gpwm = container_of(chip, struct pwm_gpio, chip);
> +
> +	hrtimer_cancel(&gpwm->hrtimer);
> +
> +	if (!state->enabled) {
> +		gpiod_set_value(gpwm->gpio, 0);
> +		return 0;
> +	}
> +
> +	gpwm->on_interval = ns_to_ktime(state->duty_cycle);
> +	gpwm->off_interval = ns_to_ktime(state->period - state->duty_cycle);
> +	gpwm->invert = state->polarity == PWM_POLARITY_INVERSED;
> +
> +	gpwm->on = !!gpwm->on_interval;
> +	gpiod_set_value(gpwm->gpio, gpwm->on ^ gpwm->invert);
> +
> +	if (gpwm->on_interval && gpwm->off_interval)
> +		hrtimer_start(&gpwm->hrtimer, gpwm->on_interval, HRTIMER_MODE_REL);

Ideally the currently running period is completed before a period with
the new configuration starts. Would be nice switch only on the next
period end if the current configuration is enabled.

> +
> +	return 0;
> +}
> +
> +static const struct pwm_ops pwm_gpio_ops = {
> +	.owner = THIS_MODULE,
> +	.apply = pwm_gpio_apply,

Usually a .get_state callback is nice. Does it make sense to do
something like:

	if (driver is up)
		report current setting
	else
		val = gpio_get_value()
		report(period=1, duty_cycle=val, enabled=val, polarity=NORMAL)

?

> +};
> +
> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> +	struct pwm_gpio *gpwm;
> +	int ret;
> +
> +	gpwm = devm_kzalloc(&pdev->dev, sizeof(*gpwm), GFP_KERNEL);
> +	if (!gpwm)
> +		return -ENOMEM;
> +
> +	gpwm->gpio = devm_gpiod_get(&pdev->dev, NULL, GPIOD_OUT_LOW);
> +	if (IS_ERR(gpwm->gpio))
> +		return PTR_ERR(gpwm->gpio);
> +
> +	if (gpiod_cansleep(gpwm->gpio))
> +		return -EINVAL;
> +
> +	gpwm->chip.dev = &pdev->dev;
> +	gpwm->chip.ops = &pwm_gpio_ops;
> +	gpwm->chip.base = pdev->id;
> +	gpwm->chip.npwm = 1;
> +
> +	hrtimer_init(&gpwm->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +	gpwm->hrtimer.function = pwm_gpio_timer;
> +
> +	ret = pwmchip_add(&gpwm->chip);
> +	if (ret < 0)
> +		return ret;

Error messages in the error paths please (preferably using
dev_err_probe).

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]