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

[Shenwei Wang <shenwei.wang@xxxxxxx>]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> Sent: Friday, June 30, 2023 4:19 AM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>; Bartosz Golaszewski
> <brgl@xxxxxxxx>; Andy Shevchenko <andy@xxxxxxxxxx>; linux-
> gpio@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>
> Subject: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support
> > +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.

Seems we shouldn't check the return value here and simply return 0.
Or should it be changed to " pm_runtime_resume_and_get" ?

> 
> > +       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.
> 

Yes, that's the purpose. Once a GPIO line is requested, the GPIO controller
should be in active state. 

> 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?
> 

Yes, the controller supports wake from IRQ. This patch has been
Verified with the SDCARD plug-in/out use case which use a GPIO line as CD PIN.

> ...
> 
> > +       err = pm_runtime_get_sync(&pdev->dev);
> > +       if (err < 0)
> 
> reference count leak here.

Change to pm_runtime_resume_and_get?

> 
> > +               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()?

Yes, dev_get_drvdata is better.

> 
> > +       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?
> 

While putting the GPIO module itself into power saving mode may not have an obvious impact 
on current dissipation, the function is necessary because the GPIO module disables its clock when idle. 
This enables the system an opportunity to power off the parent subsystem, and this conserves more 
power. The typical i.MX8 SoC features up to 8 GPIO controllers, but most of the controllers often 
remain unused.

Thanks,
Shenwei

> --
> With Best Regards,
> Andy Shevchenko