Re: [PATCH] gpio: mxc: add runtime pm support

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Thu, Jun 29, 2023 at 10:19 PM Shenwei Wang <shenwei.wang@xxxxxxx> wrote:
>
> Adds runtime PM support and allow the GPIO controller to enter
> into runtime suspend automatically when not in use to save power.
> However, it will automatically resume and enable clocks when a
> GPIO or IRQ is requested.

...

> +static int mxc_gpio_request(struct gpio_chip *chip, unsigned int offset)
> +{
> +       int ret;
> +
> +       ret = gpiochip_generic_request(chip, offset);
> +       if (ret)
> +               return ret;
> +
> +       ret = pm_runtime_get_sync(chip->parent);

reference count disbalance here.

> +       return ret < 0 ? ret : 0;
> +}
> +
> +static void mxc_gpio_free(struct gpio_chip *chip, unsigned int offset)
> +{
> +       gpiochip_generic_free(chip, offset);
> +       pm_runtime_put(chip->parent);
> +}

So, you want to have this to track the amount of GPIO lines requested, right?
Calling PM runtime after the first request makes little sense.

But here is the question: does your controller support wake from IRQ?

Have you tried to see if the lines that are used for IRQ with
gpiod_to_irq() really work with this?

...

> +       err = pm_runtime_get_sync(&pdev->dev);
> +       if (err < 0)

reference count leak here.

> +               goto out_pm_dis;


> +static int __maybe_unused mxc_gpio_runtime_suspend(struct device *dev)

Please, no __maybe_unused. Use new PM macros for that.

> +{
> +       struct platform_device *pdev = to_platform_device(dev);
> +       struct mxc_gpio_port *port = platform_get_drvdata(pdev);

What's wrong with dev_get_drvdata()?

> +       mxc_gpio_save_regs(port);
> +       clk_disable_unprepare(port->clk);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused mxc_gpio_runtime_resume(struct device *dev)
> +{

Same comments as per above function.

> +}

...

Personal view on this is that it makes little sense to do and is prone
to subtle bugs with wake sources or other, not so obvious, uses of the
GPIO lines. Can you provide the numbers of the current dissipation if
the controller is on and no line is requested? Also is there any real
example of hardware (existing DTS) that has no GPIO in use?

-- 
With Best Regards,
Andy Shevchenko