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
