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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

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.

...

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