Re: [PATCH 4/6] mmc: sdhci-omap: Implement PM runtime functions

[Tony Lindgren <tony@atomide.com>]

* Ulf Hansson <ulf.hansson@linaro.org> [211012 15:17]:
> On Tue, 12 Oct 2021 at 12:38, Tony Lindgren <tony@atomide.com> wrote:
> > @@ -1350,15 +1355,21 @@ static int sdhci_omap_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto err_cleanup_host;
> >
> > +       sdhci_omap_context_save(omap_host);
> 
> Calling sdhci_omap_context_save() here looks unnecessary. The device
> is already runtime resumed at this point.
> 
> In other words, sdhci_omap_context_save() will be called from the
> ->runtime_suspend() callback, next time the device becomes runtime
> suspended. That should be sufficient, right?

Yup this can be now dropped with omap_host->con initialized to
-EINVAL earlier.

> > @@ -1371,8 +1382,12 @@ static int sdhci_omap_remove(struct platform_device *pdev)
> >         struct device *dev = &pdev->dev;
> >         struct sdhci_host *host = platform_get_drvdata(pdev);
> >
> > +       pm_runtime_get_sync(dev);
> >         sdhci_remove_host(host, true);
> > +       pm_runtime_dont_use_autosuspend(dev);
> >         pm_runtime_put_sync(dev);
> > +       /* Ensure device gets idled despite userspace sysfs config */
> > +       pm_runtime_force_suspend(dev);
> >         pm_runtime_disable(dev);
> 
> The call to pm_runtime_disable() can be removed, as that is taken care
> of in pm_runtime_force_suspend().

OK

> > +static int __maybe_unused sdhci_omap_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       int err;
> > +
> > +       /* Enable for configuring wakeups, paired in resume */
> > +       err = pm_runtime_resume_and_get(dev);
> > +       if (err < 0)
> > +               return err;
> > +
> > +       sdhci_suspend_host(host);
> 
> As far as I can tell, sdhci_suspend_host() doesn't really make sense
> for the omap variant. What you need, is to put the device into the
> same low power state as "runtime suspend", that should be sufficient.
> 
> The system wakeup will be armed (and later then disarmed) by the PM
> core, when it calls device_wakeup_arm_wake_irqs() from the
> dpm_suspend_noirq() phase.
> 
> In other words, pointing the system suspend/resume callbacks to
> pm_runtime_force_suspend|resume() should work fine, I think.

OK sounds good to me.

Regards,

Tony