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

* Ulf Hansson [off-list ref] [211012 15:17]:

On Tue, 12 Oct 2021 at 12:38, Tony Lindgren [off-list ref] wrote:

quoted

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

quoted

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

quoted

+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