RE: [PATCH v6 7/8] mmc: sdhci-esdhc-imx: make non-fatal errors non-blocking in suspend

> -----Original Message-----
> From: Frank Li (OSS) <[email protected]>
> Sent: Monday, July 13, 2026 10:53 PM
> To: Luke Wang (OSS) <[email protected]>
> Cc: [email protected]; [email protected]; Bough Chen
> <[email protected]>; Frank Li <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; dl-S32 <[email protected]>;
> [email protected]; [email protected]
> Subject: Re: [PATCH v6 7/8] mmc: sdhci-esdhc-imx: make non-fatal errors non-
> blocking in suspend
> 
> On Mon, Jul 13, 2026 at 12:39:03PM +0800, [email protected] wrote:
> > From: Luke Wang <[email protected]>
> >
> > Make pinctrl_pm_select_sleep_state() and mmc_gpio_set_cd_wake() failures
> > non-fatal in the suspend path. These failures only mean slightly higher
> > power consumption or missing CD wakeup capability, but should not block
> > system suspend.
> >
> > Also change the function to always return 0 on the success path instead
> > of propagating non-fatal warning return values.
> >
> > Fixes: 676a83855614 ("mmc: host: sdhci-esdhc-imx: refactor the system PM
> logic")
> > Signed-off-by: Luke Wang <[email protected]>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> > index 7818f351a58a..6cb84690fde4 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -2097,10 +2097,12 @@ static int sdhci_esdhc_suspend(struct device
> *dev)
> >  		 */
> >  		ret = pinctrl_pm_select_sleep_state(dev);
> >  		if (ret)
> > -			return ret;
> > +			dev_warn(dev, "Failed to select sleep pinctrl state\n");
> >  	}
> >
> >  	ret = mmc_gpio_set_cd_wake(host->mmc, true);
> > +	if (ret)
> > +		dev_warn(dev, "Failed to enable cd wake\n");
> >
> >  	/*
> >  	 * Make sure invoke runtime_suspend to gate off clock.
> > @@ -2108,7 +2110,7 @@ static int sdhci_esdhc_suspend(struct device *dev)
> >  	 */
> >  	pm_runtime_force_suspend(dev);
> >
> > -	return ret;
> > +	return 0;
> 
> if other error path set ret as not zero, the real error may missed, I suggest
> don't set it let
> 
> 	if (pinctrl_pm_select_sleep_state(dev))
> 		dev_warn(...)
> 
> 	still leave ret here.

Thanks for the review.

The fatal error path (pm_runtime_resume_and_get()) already returns
early, so by the time we reach the end of the function, ret can only
hold a non-fatal (warn-only) value. Keeping "return ret" would turn
these intentionally non-fatal errors back into fatal ones and block
system suspend, which is exactly what this patch tries to avoid.

To address your concern about ret being polluted, I'll drop the
assignment for the non-fatal calls so they no longer touch ret:

	if (!sdhci_enable_irq_wakeups(host))
		dev_warn(dev, "Failed to enable irq wakeup\n");

	if (pinctrl_pm_select_sleep_state(dev))
		dev_warn(dev, "Failed to select sleep pinctrl state\n");

	if (mmc_gpio_set_cd_wake(host->mmc, true))
		dev_warn(dev, "Failed to enable cd wake\n");

	pm_runtime_force_suspend(dev);
	return 0;

This keeps the semantics clear. Will send v7 patch.

Thanks, 
Luke

> 
> Frank
> 
> >  }
> >
> >  static int sdhci_esdhc_resume(struct device *dev)
> > --
> > 2.34.1
> >
> >