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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sun, Feb 04, 2024 at 11:08:51PM +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

GPIO

> supported and are rejected in the probe function.

Overall LGTM, but I have a few comments below.

...

+ container_of.h

> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/hrtimer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwm.h>
> +#include <linux/spinlock.h>

+ time.h
+ types.h

...

> +static enum hrtimer_restart pwm_gpio_timer(struct hrtimer *gpio_timer)
> +{
> +	struct pwm_gpio *gpwm = container_of(gpio_timer, struct pwm_gpio,
> +					     gpio_timer);
> +	unsigned long next_toggle;
> +	unsigned long flags;
> +	bool new_level;

> +	spin_lock_irqsave(&gpwm->lock, flags);

Can we use cleanup.h from day 1?

> +	/* Apply new state at end of current period */
> +	if (!gpwm->level && gpwm->changing) {
> +		gpwm->changing = false;
> +		gpwm->state = gpwm->next_state;
> +		new_level = !!gpwm->state.duty_cycle;
> +	} else {
> +		new_level = !gpwm->level;
> +	}
> +
> +	next_toggle = pwm_gpio_toggle(gpwm, new_level);
> +	if (next_toggle) {
> +		hrtimer_forward(gpio_timer, hrtimer_get_expires(gpio_timer),
> +				ns_to_ktime(next_toggle));
> +	}
> +
> +	spin_unlock_irqrestore(&gpwm->lock, flags);
> +
> +	return next_toggle ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}

...

> +		/*
> +		 * This just enables the output, but pwm_gpio_toggle()
> +		 * really starts the duty cycle.
> +		 */
> +		int ret = gpiod_direction_output(gpwm->gpio, invert);
> +
> +		if (ret)
> +			return ret;

Better to have it written as

		int ret;

		/*
		 * This just enables the output, but pwm_gpio_toggle()
		 * really starts the duty cycle.
		 */
		ret = gpiod_direction_output(gpwm->gpio, invert);
		if (ret)
			return ret;

...

> +	gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> +	if (IS_ERR(gpwm->gpio)) {
> +		return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> +				     "could not get gpio\n");
> +	}

{} are not needed.

...

> +	if (gpiod_cansleep(gpwm->gpio)) {
> +		return dev_err_probe(dev, -EINVAL,
> +				     "sleeping GPIO %d not supported\n",

> +				     desc_to_gpio(gpwm->gpio));

Do not use plain GPIO numbers.

> +	}

{} are not needed.

-- 
With Best Regards,
Andy Shevchenko