Re: [PATCH v9 3/8] mmc: sdhci-of-k1: add regulator and pinctrl voltage switching support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Passing a NULL state pointer to pinctrl_select_state() causes a kernel panic.
- [High] Swallowing -EPROBE_DEFER from devm_pinctrl_get() permanently breaks UHS-I voltage switching.
- [Medium] Masking failures from pinctrl_select_state() can lead to data corruption or hardware damage.
- [Medium] Switching back to 3.3V executes the regulator switch before configuring the IO pads, risking overvoltage.
- [Low] The code uses "uhs" for the pinctrl state name instead of the standard "state_uhs" specified in the commit message.
- [Low] Unsupported signal voltages log a warning but falsely return success.
--

commit 54952d6d0a322262708a9ecada7a38d6245d7b6c
Author: Iker Pedrosa <ikerpedrosam@gmail.com>

mmc: sdhci-of-k1: add regulator and pinctrl voltage switching support

This commit adds voltage switching infrastructure for UHS-I modes by integrating
both the regulator framework for supply voltage control and pinctrl state
switching for pin drive strength optimization.

> - Add optional pinctrl state switching between "default" (3.3V) and
>   "state_uhs" (1.8V) configurations

This isn't a bug, but the commit message mentions "state_uhs" while the
code below looks for "uhs". Which one is expected by the device tree bindings?

> diff --git a/drivers/mmc/host/sdhci-of-k1.c b/drivers/mmc/host/sdhci-of-k1.c
> --- a/drivers/mmc/host/sdhci-of-k1.c
> +++ b/drivers/mmc/host/sdhci-of-k1.c

[ ... ]

> @@ -219,6 +223,46 @@ static void spacemit_sdhci_pre_hs400_to_hs200(struct mmc_host *mmc)
>  static int spacemit_sdhci_start_signal_voltage_switch(struct mmc_host *mmc,
>  						      struct mmc_ios *ios)
>  {
>  	struct sdhci_host *host = mmc_priv(mmc);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(pltfm_host);
>  	struct pinctrl_state *state;
>  	int ret;
>  
>  	ret = sdhci_start_signal_voltage_switch(mmc, ios);
>  	if (ret)
>  		return ret;

For transitions to 3.3V, sdhci_start_signal_voltage_switch() will increase
the vqmmc regulator voltage to 3.3V and wait 5ms for it to stabilize.

Because the pinctrl state is updated later in this function, the pads
will remain configured for 1.8V during this time.

Could this overvoltage stress the pads? Should the pinctrl state be
updated to "default" before raising the regulator voltage?

>  
>  	if (!sdhst->pinctrl)
>  		return 0;
>  
>  	/* Select appropriate pinctrl state based on signal voltage */
>  	switch (ios->signal_voltage) {
>  	case MMC_SIGNAL_VOLTAGE_330:
>  		state = sdhst->pinctrl_default;
>  		break;
>  	case MMC_SIGNAL_VOLTAGE_180:
>  		state = sdhst->pinctrl_uhs;
>  		break;
>  	default:
>  		dev_warn(mmc_dev(mmc), "unsupported voltage %d\n", ios->signal_voltage);
>  		return 0;

If the voltage is unsupported, should this return an error like -EINVAL
instead of 0? Returning 0 tells the mmc core the transition was successful.

>  	}
>  
>  	ret = pinctrl_select_state(sdhst->pinctrl, state);

spacemit_sdhci_get_pins() treats the pinctrl states as optional and leaves
them as null if they aren't found in the device tree.

Does pinctrl_select_state() handle a null state pointer safely?

Looking at the pinctrl subsystem, it appears to unconditionally dereference
the state pointer. Should there be a check like if (!state) before calling
pinctrl_select_state() to avoid a panic?

>  	if (ret) {
>  		dev_warn(mmc_dev(mmc), "failed to select pinctrl state: %d\n", ret);
>  		return 0;
>  	}

If selecting the pinctrl state fails, returning 0 masks the failure and
tells the mmc core the pad configuration succeeded.

If the controller switches to 1.8V but the pads remain at 3.3V, could
this lead to data corruption or electrical damage? Should this return the
negative error code instead?

>  	dev_dbg(mmc_dev(mmc), "switched to %s pinctrl state\n",
>  		ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180 ? "UHS" : "default");
>  
>  	return 0;
>  }

[ ... ]

> @@ -252,6 +296,30 @@ static inline int spacemit_sdhci_get_resets(struct device *dev)
>  static inline void spacemit_sdhci_get_pins(struct device *dev,
>  					   struct sdhci_pltfm_host *pltfm_host)
>  {
>  	struct spacemit_sdhci_host *sdhst = sdhci_pltfm_priv(pltfm_host);
>  
>  	sdhst->pinctrl = devm_pinctrl_get(dev);
>  	if (IS_ERR(sdhst->pinctrl)) {
>  		sdhst->pinctrl = NULL;
>  		dev_dbg(dev, "pinctrl not available, voltage switching will work without it\n");
>  		return;
>  	}

If the pin controller hasn't probed yet, devm_pinctrl_get() will return
-EPROBE_DEFER.

By swallowing this error and setting pinctrl to null, the host will
register without pinctrl support and UHS-I modes will permanently lack
correct pad configuration.

Should this propagate -EPROBE_DEFER back to spacemit_sdhci_probe() instead
of returning void?

>  
>  	sdhst->pinctrl_default = pinctrl_lookup_state(sdhst->pinctrl, "default");
>  	if (IS_ERR(sdhst->pinctrl_default))
>  		sdhst->pinctrl_default = NULL;
>  
>  	sdhst->pinctrl_uhs = pinctrl_lookup_state(sdhst->pinctrl, "uhs");

Does looking for "uhs" correctly match the expected device tree bindings?
The commit message states "state_uhs", and standard mmc bindings typically
use "state_uhs" as well.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-orangepi-sd-card-uhs-v9-0-ae48c0b2b2cf@gmail.com?part=3