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

[Andy Shevchenko <andy.shevchenko@xxxxxxxxx>]

On Wed, Jul 5, 2023 at 10:01 PM Andy Shevchenko
<andy.shevchenko@xxxxxxxxx> wrote:
> On Wed, Jul 5, 2023 at 6:37 PM Shenwei Wang <shenwei.wang@xxxxxxx> wrote:
> >
> > Adds runtime PM support and allow the GPIO controller to enter

Add

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

...

> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +       err = pm_runtime_resume_and_get(&pdev->dev);
> > +       if (err < 0)
> > +               goto out_pm_dis;
>
> So, after this if an error happens, you will have PM left enabled and
> the next probe won't work as expected, right?

Just noticed that this patch completely screwed up the error handling
WRT clock and PM.

...

> >         platform_set_drvdata(pdev, port);
> > +       pm_runtime_put_autosuspend(&pdev->dev);
> >
> >         return 0;
>
> ...something here is missing?
>
> > +out_pm_dis:
> > +       pm_runtime_disable(&pdev->dev);
> > +       clk_disable_unprepare(port->clk);

This is definitely in the wrong position to begin with.

> >  out_irqdomain_remove:
> >         irq_domain_remove(port->domain);
> >  out_bgio:

-- 
With Best Regards,
Andy Shevchenko