[PATCH v2 1/2] mmc: sdhci-omap: properly check regulator_is_supported_voltage() return

[PATCH v2 1/2] mmc: sdhci-omap: properly check regulator_is_supported_voltage() return

[Alexander Stein]

From: Matthias Schiffer <matthias.schiffer@tq-group.com>

regulator_is_supported_voltage() returns negative values on errors.

Note that this patch might result in regressions on existing boards that
should support voltage switching, but don't have their regulators set up
correctly.

Fixes: de5ccd2af71f ("mmc: sdhci-omap: Handle voltages to add support omap4")
Signed-off-by: Matthias Schiffer <matthias.schiffer@tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
New in v2

 drivers/mmc/host/sdhci-omap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index b5d7c1a80a92f..08d5a82b7d01b 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -949,11 +949,11 @@ static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
 	if (IS_ERR(reg))
 		return ~0U;
 
-	if (regulator_is_supported_voltage(reg, 1700000, 1950000))
+	if (regulator_is_supported_voltage(reg, 1700000, 1950000) > 0)
 		caps |= SDHCI_CAN_VDD_180;
-	if (regulator_is_supported_voltage(reg, 2700000, 3150000))
+	if (regulator_is_supported_voltage(reg, 2700000, 3150000) > 0)
 		caps |= SDHCI_CAN_VDD_300;
-	if (regulator_is_supported_voltage(reg, 3150000, 3600000))
+	if (regulator_is_supported_voltage(reg, 3150000, 3600000) > 0)
 		caps |= SDHCI_CAN_VDD_330;
 
 	regulator_put(reg);
-- 
2.43.0

[PATCH v2 2/2] mmc: sdhci-omap: use regulator_get_optional() and reuse pbias in sdhci_omap_regulator_get_caps()

[Alexander Stein]

From: Matthias Schiffer <matthias.schiffer@tq-group.com>

We actually want to get an error return instead of a dummy regulator
when a supply is not set. Change regulator_get() to
regulator_get_optional() for the vqmmc supply and reuse omap_host->pbias,
which is already initialized at this point.

This change also avoids warning messages:

    sdhci-omap 48060000.mmc: supply pbias not found, using dummy regulator
    sdhci-omap 48060000.mmc: supply vqmmc not found, using dummy regulator

Fixes: de5ccd2af71f ("mmc: sdhci-omap: Handle voltages to add support omap4")
Signed-off-by: Matthias Schiffer <matthias.schiffer@tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/mmc/host/sdhci-omap.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
index 08d5a82b7d01b..4623781adba7b 100644
--- a/drivers/mmc/host/sdhci-omap.c
+++ b/drivers/mmc/host/sdhci-omap.c
@@ -939,16 +939,10 @@ static const struct sdhci_ops sdhci_omap_ops = {
 	.set_timeout = sdhci_omap_set_timeout,
 };
 
-static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
-						  const char *name)
+static unsigned int sdhci_omap_regulator_get_caps(struct regulator *reg)
 {
-	struct regulator *reg;
 	unsigned int caps = 0;
 
-	reg = regulator_get(dev, name);
-	if (IS_ERR(reg))
-		return ~0U;
-
 	if (regulator_is_supported_voltage(reg, 1700000, 1950000) > 0)
 		caps |= SDHCI_CAN_VDD_180;
 	if (regulator_is_supported_voltage(reg, 2700000, 3150000) > 0)
@@ -956,8 +950,6 @@ static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
 	if (regulator_is_supported_voltage(reg, 3150000, 3600000) > 0)
 		caps |= SDHCI_CAN_VDD_330;
 
-	regulator_put(reg);
-
 	return caps;
 }
 
@@ -967,11 +959,20 @@ static int sdhci_omap_set_capabilities(struct sdhci_host *host)
 	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
 	struct device *dev = omap_host->dev;
 	const u32 mask = SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_300 | SDHCI_CAN_VDD_330;
-	unsigned int pbias, vqmmc, caps = 0;
+	unsigned int pbias = ~0U, vqmmc = ~0U, caps = 0;
+	struct	regulator *reg_vqmmc;
 	u32 reg;
 
-	pbias = sdhci_omap_regulator_get_caps(dev, "pbias");
-	vqmmc = sdhci_omap_regulator_get_caps(dev, "vqmmc");
+	if (!IS_ERR(omap_host->pbias))
+		pbias = sdhci_omap_regulator_get_caps(omap_host->pbias);
+
+	/* mmc->supply.vqmmc is not initialized yet */
+	reg_vqmmc = regulator_get_optional(dev, "vqmmc");
+	if (!IS_ERR(reg_vqmmc)) {
+		vqmmc = sdhci_omap_regulator_get_caps(reg_vqmmc);
+		regulator_put(reg_vqmmc);
+	}
+
 	caps = pbias & vqmmc;
 
 	if (pbias != ~0U && vqmmc == ~0U)
-- 
2.43.0

Re: [PATCH v2 2/2] mmc: sdhci-omap: use regulator_get_optional() and reuse pbias in sdhci_omap_regulator_get_caps()

[Adrian Hunter]

On 25/08/2025 15:21, Alexander Stein wrote:
> From: Matthias Schiffer <matthias.schiffer@tq-group.com>
> 
> We actually want to get an error return instead of a dummy regulator
> when a supply is not set.

Please explain why here.

> Change regulator_get() to
> regulator_get_optional() for the vqmmc supply and reuse omap_host->pbias,
> which is already initialized at this point.

Probably better to make "reuse omap_host->pbias" a separate patch

> 
> This change also avoids warning messages:
> 
>     sdhci-omap 48060000.mmc: supply pbias not found, using dummy regulator
>     sdhci-omap 48060000.mmc: supply vqmmc not found, using dummy regulator
> 
> Fixes: de5ccd2af71f ("mmc: sdhci-omap: Handle voltages to add support omap4")
> Signed-off-by: Matthias Schiffer <matthias.schiffer@tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/mmc/host/sdhci-omap.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index 08d5a82b7d01b..4623781adba7b 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -939,16 +939,10 @@ static const struct sdhci_ops sdhci_omap_ops = {
>  	.set_timeout = sdhci_omap_set_timeout,
>  };
>  
> -static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
> -						  const char *name)
> +static unsigned int sdhci_omap_regulator_get_caps(struct regulator *reg)
>  {
> -	struct regulator *reg;
>  	unsigned int caps = 0;
>  
> -	reg = regulator_get(dev, name);
> -	if (IS_ERR(reg))
> -		return ~0U;
> -
>  	if (regulator_is_supported_voltage(reg, 1700000, 1950000) > 0)
>  		caps |= SDHCI_CAN_VDD_180;
>  	if (regulator_is_supported_voltage(reg, 2700000, 3150000) > 0)
> @@ -956,8 +950,6 @@ static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
>  	if (regulator_is_supported_voltage(reg, 3150000, 3600000) > 0)
>  		caps |= SDHCI_CAN_VDD_330;
>  
> -	regulator_put(reg);
> -
>  	return caps;
>  }
>  
> @@ -967,11 +959,20 @@ static int sdhci_omap_set_capabilities(struct sdhci_host *host)
>  	struct sdhci_omap_host *omap_host = sdhci_pltfm_priv(pltfm_host);
>  	struct device *dev = omap_host->dev;
>  	const u32 mask = SDHCI_CAN_VDD_180 | SDHCI_CAN_VDD_300 | SDHCI_CAN_VDD_330;
> -	unsigned int pbias, vqmmc, caps = 0;
> +	unsigned int pbias = ~0U, vqmmc = ~0U, caps = 0;
> +	struct	regulator *reg_vqmmc;
>  	u32 reg;
>  
> -	pbias = sdhci_omap_regulator_get_caps(dev, "pbias");
> -	vqmmc = sdhci_omap_regulator_get_caps(dev, "vqmmc");
> +	if (!IS_ERR(omap_host->pbias))
> +		pbias = sdhci_omap_regulator_get_caps(omap_host->pbias);
> +
> +	/* mmc->supply.vqmmc is not initialized yet */
> +	reg_vqmmc = regulator_get_optional(dev, "vqmmc");
> +	if (!IS_ERR(reg_vqmmc)) {
> +		vqmmc = sdhci_omap_regulator_get_caps(reg_vqmmc);
> +		regulator_put(reg_vqmmc);
> +	}
> +
>  	caps = pbias & vqmmc;
>  
>  	if (pbias != ~0U && vqmmc == ~0U)

Re: [PATCH v2 1/2] mmc: sdhci-omap: properly check regulator_is_supported_voltage() return

[Adrian Hunter]

On 25/08/2025 15:21, Alexander Stein wrote:
> From: Matthias Schiffer <matthias.schiffer@tq-group.com>
> 
> regulator_is_supported_voltage() returns negative values on errors.
> 
> Note that this patch might result in regressions on existing boards that
> should support voltage switching, but don't have their regulators set up
> correctly.
> 
> Fixes: de5ccd2af71f ("mmc: sdhci-omap: Handle voltages to add support omap4")

It is not very clear what is being fixed nor why.

It changes from "setting a capability if there is an error" to
"not setting a capability if there is an error".  Both seem less
than ideal.

Also "might result in regressions" is really not suitable for
stable, which is probably where it will end up if it has a Fixes tag.

> Signed-off-by: Matthias Schiffer <matthias.schiffer@tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> New in v2
> 
>  drivers/mmc/host/sdhci-omap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-omap.c b/drivers/mmc/host/sdhci-omap.c
> index b5d7c1a80a92f..08d5a82b7d01b 100644
> --- a/drivers/mmc/host/sdhci-omap.c
> +++ b/drivers/mmc/host/sdhci-omap.c
> @@ -949,11 +949,11 @@ static unsigned int sdhci_omap_regulator_get_caps(struct device *dev,
>  	if (IS_ERR(reg))
>  		return ~0U;
>  
> -	if (regulator_is_supported_voltage(reg, 1700000, 1950000))
> +	if (regulator_is_supported_voltage(reg, 1700000, 1950000) > 0)

Why is the error ignored?

>  		caps |= SDHCI_CAN_VDD_180;
> -	if (regulator_is_supported_voltage(reg, 2700000, 3150000))
> +	if (regulator_is_supported_voltage(reg, 2700000, 3150000) > 0)
>  		caps |= SDHCI_CAN_VDD_300;
> -	if (regulator_is_supported_voltage(reg, 3150000, 3600000))
> +	if (regulator_is_supported_voltage(reg, 3150000, 3600000) > 0)
>  		caps |= SDHCI_CAN_VDD_330;
>  
>  	regulator_put(reg);