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;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Tue, 11 Feb 2020 20:08:02 +0530
From:   sbhanu@codeaurora.org
To:     Stephen Boyd <swboyd@chromium.org>
Cc:     adrian.hunter@intel.com, mka@chromium.org, robh+dt@kernel.org,
        ulf.hansson@linaro.org, asutoshd@codeaurora.org,
        stummala@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, devicetree@vger.kernel.org,
        agross@kernel.org, bjorn.andersson@linaro.org,
        devicetree-owner@vger.kernel.org
Subject: Re: [PATCH V2] mmc: sdhci-msm: Update system suspend/resume callbacks
 of sdhci-msm platform driver.
In-Reply-To: <158136367603.121156.1867941302835915258@swboyd.mtv.corp.google.com>
References: <1581081650-22228-1-git-send-email-sbhanu@codeaurora.org>
 <158136367603.121156.1867941302835915258@swboyd.mtv.corp.google.com>
Message-ID: <1328455c7256f41f3f3bd9b96fb21d8d@codeaurora.org>
User-Agent: Roundcube Webmail/1.3.9
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@codeaurora.org>
>> 
>> 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);
>> +}
>> +