Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <5e287ef8.1c69fb81.cacd.4c1b@mx.google.com>
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <1579617022-13031-1-git-send-email-sbhanu@codeaurora.org>
References: <1579617022-13031-1-git-send-email-sbhanu@codeaurora.org>
Cc:     asutoshd@codeaurora.org, stummala@codeaurora.org,
        vbadigan@codeaurora.org, sayalil@codeaurora.org,
        cang@codeaurora.org, rampraka@codeaurora.org,
        linux-mmc@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, agross@kernel.org,
        bjorn.andersson@linaro.org,
        Shaik Sajida Bhanu <sbhanu@codeaurora.org>
To:     Shaik Sajida Bhanu <sbhanu@codeaurora.org>,
        adrian.hunter@intel.com, ulf.hansson@linaro.org
Subject: Re: [PATCH V1] mmc: sdhci-msm: Add system suspend/resume callbacks
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.8.1
Date:   Wed, 22 Jan 2020 08:57:27 -0800
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Quoting Shaik Sajida Bhanu (2020-01-21 06:30:22)
> Add system suspend/resume callbacks to sdhci-msm platform driver.

Yes, but why? There are already suspend/resume callbacks so this is
replacing them too.

>=20
> Signed-off-by: Shaik Sajida Bhanu <sbhanu@codeaurora.org>
> ---
>  drivers/mmc/host/sdhci-msm.c | 47 ++++++++++++++++++++++++++++++++++++++=
++++--
>  1 file changed, 45 insertions(+), 2 deletions(-)
>=20
> diff --git a/drivers/mmc/host/sdhci-msm.c b/drivers/mmc/host/sdhci-msm.c
> index 71f29ba..4984857 100644
> --- a/drivers/mmc/host/sdhci-msm.c
> +++ b/drivers/mmc/host/sdhci-msm.c
> @@ -2028,9 +2028,52 @@ static __maybe_unused int sdhci_msm_runtime_resume=
(struct device *dev)
>         return 0;
>  }
> =20
> +static int sdhci_msm_suspend(struct device *dev)
> +{
> +       struct sdhci_host *host =3D dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host =3D sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host =3D sdhci_pltfm_priv(pltfm_host);
> +       int ret =3D 0;
> +
> +       if (host->mmc->caps2 & MMC_CAP2_CQE) {

It would be nice if this if check was rolled into cqhci_suspend so that
all the callers wouldn't have to check it.

> +               ret =3D cqhci_suspend(host->mmc);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       disable_irq(msm_host->pwr_irq);

Why is the irq disabled? Please add a comment.

> +       ret =3D sdhci_suspend_host(host);
> +       if (ret)
> +               return ret;
> +
> +       return sdhci_msm_runtime_suspend(dev);

pm_runtime_force_suspend() does different things than just call the
runtime suspend function for the driver. For example, it disables
runtime PM on the device. Can you explain in the commit text how this is
a correct conversion?

> +}
> +
> +static int sdhci_msm_resume(struct device *dev)
> +{
> +       struct sdhci_host *host =3D dev_get_drvdata(dev);
> +       struct sdhci_pltfm_host *pltfm_host =3D sdhci_priv(host);
> +       struct sdhci_msm_host *msm_host =3D sdhci_pltfm_priv(pltfm_host);
> +       int ret =3D 0;
> +
> +       ret =3D sdhci_msm_runtime_resume(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret =3D sdhci_resume_host(host);
> +       if (ret < 0)
> +               return ret;
> +       enable_irq(msm_host->pwr_irq);

Same question here about irq. Deserves a comment.

> +
> +       if (host->mmc->caps2 & MMC_CAP2_CQE)
> +               ret =3D cqhci_resume(host->mmc);
> +
> +       return ret;
> +}
> +
>  static const struct dev_pm_ops sdhci_msm_pm_ops =3D {
> -       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> -                               pm_runtime_force_resume)
> +       SET_SYSTEM_SLEEP_PM_OPS(sdhci_msm_suspend,
> +                               sdhci_msm_resume)
>         SET_RUNTIME_PM_OPS(sdhci_msm_runtime_suspend,
>                            sdhci_msm_runtime_resume,
>                            NULL)