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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Sun, Jun 2, 2024 at 11:33 PM Linus Walleij <linus.walleij@xxxxxxxxxx> 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.


Reviewed-by: Andy Shevchenko <andy@xxxxxxxxxx>
See one question below.

...

> +static int pwm_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct pwm_chip *chip;
> +       struct pwm_gpio *gpwm;
> +       int ret;
> +
> +       chip = devm_pwmchip_alloc(dev, 1, sizeof(*gpwm));
> +       if (IS_ERR(chip))
> +               return PTR_ERR(chip);
> +
> +       gpwm = pwmchip_get_drvdata(chip);
> +
> +       spin_lock_init(&gpwm->lock);
> +
> +       gpwm->gpio = devm_gpiod_get(dev, NULL, GPIOD_ASIS);
> +       if (IS_ERR(gpwm->gpio))
> +               return dev_err_probe(dev, PTR_ERR(gpwm->gpio),
> +                                    "%pfw: could not get gpio\n",
> +                                    dev_fwnode(dev));
> +
> +       if (gpiod_cansleep(gpwm->gpio))
> +               return dev_err_probe(dev, -EINVAL,
> +                                    "%pfw: sleeping GPIO not supported\n",
> +                                    dev_fwnode(dev));
> +
> +       chip->ops = &pwm_gpio_ops;
> +       chip->atomic = true;
> +
> +       hrtimer_init(&gpwm->gpio_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> +       ret = devm_add_action_or_reset(dev, pwm_gpio_disable_hrtimer, gpwm);
> +       if (ret)
> +               return ret;
> +
> +       gpwm->gpio_timer.function = pwm_gpio_timer;
> +
> +       ret = pwmchip_add(chip);
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "could not add pwmchip\n");

> +       platform_set_drvdata(pdev, gpwm);

Do we still need this?

> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko