RE: [PATCH v3 12/14] mmc: sdhci-esdhc-imx: restore pin state when resume back

[BOUGH CHEN <haibo.chen@nxp.com>]

> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org
> <linux-mmc-owner@vger.kernel.org> On Behalf Of Adrian Hunter
> Sent: 2020年2月18日 15:58
> To: BOUGH CHEN <haibo.chen@nxp.com>; ulf.hansson@linaro.org;
> linux-mmc@vger.kernel.org
> Cc: dl-linux-imx <linux-imx@nxp.com>; linus.walleij@linaro.org
> Subject: Re: [PATCH v3 12/14] mmc: sdhci-esdhc-imx: restore pin state when
> resume back
> 
> On 10/02/20 10:49 am, haibo.chen@nxp.com wrote:
> > From: Haibo Chen <haibo.chen@nxp.com>
> >
> > In some low power mode, SoC will lose the pin state, so need to
> > restore the pin state when resume back.
> >
> > Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c
> > b/drivers/mmc/host/sdhci-esdhc-imx.c
> > index 106097cbd0d4..dedc067cd0dd 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -1717,7 +1717,13 @@ static int sdhci_esdhc_suspend(struct device
> *dev)
> >  	if (host->tuning_mode != SDHCI_TUNING_MODE_3)
> >  		mmc_retune_needed(host->mmc);
> >
> > -	return sdhci_suspend_host(host);
> > +	ret = sdhci_suspend_host(host);
> > +	if (!ret)
> > +		if (pinctrl_pm_select_sleep_state(dev))
> > +			dev_warn(mmc_dev(host->mmc),
> > +			 "%s, failed to select sleep pin state!\n", __func__);
> 
> It looks to me like pinctrl_pm_select_sleep_state() prints an error anyway if
> it fails, so the warning here is redundant.

Okay, thanks for point out that!

> 
> Also a comment about why it is OK to ignore an error could be added.

Yes, I will add a comment like below, should it be okay?

/*
 * here ignore the error return, because current API sdhci_suspend_host always return zero,
 * When the API sdhci_suspend_host change the return policy, need to change at here too.
 */

Best Regards
Bough chen
> 
> > +
> > +	return ret;
> >  }
> >
> >  static int sdhci_esdhc_resume(struct device *dev) @@ -1725,6 +1731,10
> > @@ static int sdhci_esdhc_resume(struct device *dev)
> >  	struct sdhci_host *host = dev_get_drvdata(dev);
> >  	int ret;
> >
> > +	if (pinctrl_pm_select_default_state(dev))
> > +		dev_warn(mmc_dev(host->mmc),
> > +		 "%s, failed to select default pin state!\n", __func__);
> 
> Same as above
> 
> > +
> >  	/* re-initialize hw state in case it's lost in low power mode */
> >  	sdhci_esdhc_imx_hwinit(host);
> >
> >