Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks of sdhci-msm platform driver.

[sbhanu]

On 2020-02-11 01:11, Stephen Boyd wrote:
> Quoting Shaik Sajida Bhanu (2020-02-07 05:20:50)
> > The existing suspend/resume callbacks of sdhci-msm driver are just
> > gating/un-gating the clocks. During suspend cycle more can be done
> > like disabling controller, interrupts and card detection.
> >
> > So updating the system pm callbacks for performing these extra
> > actions besides controlling the clocks.
> >
> > Signed-off-by: Shaik Sajida Bhanu <sbhanu@xxxxxxxxxxxxxx>
> >
> > Changes since V1:
> >         Addressed review comments
>
> Please don't write this. Instead, describe what's actually different so
> the reader doesn't have to go figure out what the review comments were.

sure

> > ---
> >  drivers/mmc/host/sdhci-msm.c | 50 
> > ++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 48 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-msm.c 
> > b/drivers/mmc/host/sdhci-msm.c
> > index c3a160c..e30c8a3 100644
> > --- a/drivers/mmc/host/sdhci-msm.c
> > +++ b/drivers/mmc/host/sdhci-msm.c
> > @@ -2159,9 +2159,55 @@ static __maybe_unused int 
> > sdhci_msm_runtime_resume(struct device *dev)
> >         return 0;
> >  }
> >
> > +static int sdhci_msm_suspend(struct device *dev)
> > +{
> > +       struct sdhci_host *host = dev_get_drvdata(dev);
> > +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +       struct sdhci_msm_host *msm_host = 
> > sdhci_pltfm_priv(pltfm_host);
> > +       int ret;
> > +
> > +       if (host->mmc->caps2 & MMC_CAP2_CQE) {
> > +               ret = cqhci_suspend(host->mmc);
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       ret = sdhci_suspend_host(host);
> > +       if (ret)
> > +               return ret;
> > +       /* Disable pwr-irq since SDHC would be inactive */
> > +       disable_irq(msm_host->pwr_irq);
>
> Why do we need to do this? If it's inactive then the irq won't be 
> raised
> by the inactive hardware. Given that we're going to suspend the device,
> the irq won't matter unless it's marked for wakeup. Please remove this
> irq enable/disable logic, or explain why it's really needed.

You are right. This is not needed.
We have checked more on this and interrupt are getting disabled in 
suspend_device_irqs().
Will remove this.

> > +
> > +       return pm_runtime_force_suspend(dev);
> > +}
> > +