* Ulf Hansson <ulf.hansson@xxxxxxxxxx> [211012 15:17]:
> On Tue, 12 Oct 2021 at 12:38, Tony Lindgren <tony@xxxxxxxxxxx> 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
