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 12:29 PM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>; Bartosz Golaszewski
> <brgl@xxxxxxxx>; linux-gpio@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx; dl-linux-imx
> <linux-imx@xxxxxxx>
> Subject: Re: [EXT] Re: [PATCH] gpio: mxc: add runtime pm support
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report this
> email' button
> 
> 
> On Fri, Jun 30, 2023 at 05:01:10PM +0000, Shenwei Wang wrote:
> > > From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> > > Sent: Friday, June 30, 2023 4:19 AM
> 
> ...
> 
> > > > +       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" ?
> 
> It depends. I don't know the goal and what the case will be if PM fails and you
> still go with that.
> 
> > > > +       return ret < 0 ? ret : 0;
> 
> ...
> 
> > > 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.
> 
> Yes, but in that case it has been probably requested. What I'm telling is when the
> GPIO IRQ is went via IRQ chip and hence never been requested by GPIO.
> 

Just did a quick testing, and it still works even without requesting the GPIO line.
This is because we have specified the gpio controller as the pm_dev for the irq_chip in 
the probe function.

+	irq_domain_set_pm_device(port->domain, &pdev->dev);
+

Thanks,
Shenwei

> ...
> 
> > > > +       err = pm_runtime_get_sync(&pdev->dev);
> > > > +       if (err < 0)
> > >
> > > reference count leak here.
> >
> > Change to pm_runtime_resume_and_get?
> 
> No idea. You know it better than me. See above.
> 
> > > > +               goto out_pm_dis;
> 
> ...
> 
> > > 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.
> 
> Maybe you need to summarize above in the commit message to improve your
> selling point.
> 
> --
> With Best Regards,
> Andy Shevchenko
>