RE: [PATCH v5 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

> -----Original Message-----
> From: Frank Li (OSS) <[email protected]>
> Sent: Wednesday, July 8, 2026 3:02 AM
> 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 v5 3/8] mmc: sdhci-esdhc-imx: fix
> esdhc_change_pinstate() to allow default state restore
> 
> On Tue, Jul 07, 2026 at 07:02:58PM +0800, [email protected] wrote:
> > From: Luke Wang <[email protected]>
> >
> > esdhc_change_pinstate() checks for pins_100mhz and pins_200mhz at the
> > top of the function and returns -EINVAL if either is not defined. This
> > prevents the default case from ever being reached, which means devices
> > with a sleep pinctrl state but without high-speed pin states (100mhz/
> > 200mhz) can never restore their default pin configuration.
> >
> > Move the IS_ERR checks for pins_100mhz and pins_200mhz into their
> > respective switch cases.
> >
> > Signed-off-by: Luke Wang <[email protected]>
> > ---
> >  drivers/mmc/host/sdhci-esdhc-imx.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-
> esdhc-imx.c
> > index 7230d70e02ae..ead4685d621a 100644
> > --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> > +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> > @@ -1326,19 +1326,21 @@ static int esdhc_change_pinstate(struct
> sdhci_host *host,
> >
> >  	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n",
> uhs);
> >
> > -	if (IS_ERR(imx_data->pinctrl) ||
> > -		IS_ERR(imx_data->pins_100mhz) ||
> > -		IS_ERR(imx_data->pins_200mhz))
> > +	if (IS_ERR(imx_data->pinctrl))
> >  		return -EINVAL;
> 
> Maybe off topic,  I suggest set pins_100mhz and pins_200mhz to
> default pinctrl if not exist.
> 
> Currently driver use pinctrl_200mhz/100mhz judge if support SDR50 and
> HS200.
> Actually it is hidden ABI.
> 
> You can save max support speed to variable after check pinctrl_200mhz
> and pinctl_100mhz. then make these two variable to fallback to low
> frequency
> one.
> 
> So you can reduce these check in code.
> 
> Is it simpler?

Good point - using the presence of the 100MHz/200MHz pinctrl states to gate
the UHS/HS200 caps is a hidden ABI, and making it explicit would be cleaner.

I'd prefer to keep this out of the current series, though - these eight are
suspend/resume fixes. I'll send it as a separate patch on top once this
series lands.

Thanks,
Luke

> 
> Frank
> 
> >
> >  	switch (uhs) {
> >  	case MMC_TIMING_UHS_SDR50:
> >  	case MMC_TIMING_UHS_DDR50:
> > +		if (IS_ERR(imx_data->pins_100mhz))
> > +			return -EINVAL;
> >  		pinctrl = imx_data->pins_100mhz;
> >  		break;
> >  	case MMC_TIMING_UHS_SDR104:
> >  	case MMC_TIMING_MMC_HS200:
> >  	case MMC_TIMING_MMC_HS400:
> > +		if (IS_ERR(imx_data->pins_200mhz))
> > +			return -EINVAL;
> >  		pinctrl = imx_data->pins_200mhz;
> >  		break;
> >  	default:
> > --
> > 2.34.1
> >
> >