Skip to contentSkip to conversation

mmc: fix handling NULL returned from regulator_get()

PATCH New Antonio Ospite 4 participants 4 messages May 18, 2011 – May 19, 2011

Subsystems (6)
Arm i.MX and Layerscape MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver Synopsys DesignWare MMC/SD/SDIO driver +2 more subsystems
lore
Share & tools
lore

Antonio Ospite

author
#0 lore raw

PATCH 1/1 mmc: fix handling NULL returned from regulator_get()

When regulator_get() is stubbed down it returns NULL, handle this case when deciding whether the driver can use the regulator or not.

Remember: IS_ERR(NULL) is false, see also this discussion for more
insight: http://comments.gmane.org/gmane.linux.kernel.mmc/7934

Now that regulator_get() is handled correctly, the ifdef on CONFIG_REGULATOR in mmci.c can go away as well.

Signed-off-by Antonio Ospite <[email protected]>

---

 drivers/mmc/host/dw_mmc.c     |    2 +-
 drivers/mmc/host/mmci.c       |    4 +---
 drivers/mmc/host/mxcmmc.c     |    2 +-
 drivers/mmc/host/omap_hsmmc.c |    4 ++--
 drivers/mmc/host/sdhci.c      |    2 +-
 5 files changed, 6 insertions(+), 8 deletions(-)
drivers/mmc/host/dw_mmc.c +1 -1
index 87e1f57..5be1325 100644
@@ -1442,7 +1442,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 #endif /* CONFIG_MMC_DW_IDMAC */
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
drivers/mmc/host/mmci.c +1 -3
index b4a7e4f..6fac353 100644
@@ -1058,10 +1058,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
 		mmc->f_max = min(host->mclk, fmax);
 	dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
 
-#ifdef CONFIG_REGULATOR
 	/* If we're using the regulator framework, try to fetch a regulator */
 	host->vcc = regulator_get(&dev->dev, "vmmc");
-	if (IS_ERR(host->vcc))
+	if (IS_ERR_OR_NULL(host->vcc))
 		host->vcc = NULL;
 	else {
 		int mask = mmc_regulator_get_ocrmask(host->vcc);

@@ -1077,7 +1076,6 @@ static int __devinit mmci_probe(struct amba_device *dev,
 				 "(using regulator instead)\n");
 		}
 	}
-#endif
 	/* Fall back to platform data if no regulator is found */
 	if (host->vcc == NULL)
 		mmc->ocr_avail = plat->ocr_mask;
drivers/mmc/host/mxcmmc.c +1 -1
index cc20e02..a9152da 100644
@@ -155,7 +155,7 @@ static inline void mxcmci_init_ocr(struct mxcmci_host *host)
 {
 	host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
 
-	if (IS_ERR(host->vcc)) {
+	if (IS_ERR_OR_NULL(host->vcc)) {
 		host->vcc = NULL;
 	} else {
 		host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
drivers/mmc/host/omap_hsmmc.c +2 -2
index 259ece0..45fd4fe 100644
@@ -405,7 +405,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 	}
 
 	reg = regulator_get(host->dev, "vmmc");
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_dbg(host->dev, "vmmc regulator missing\n");
 		/*
 		* HACK: until fixed.c regulator is usable,

@@ -433,7 +433,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 
 		/* Allow an aux regulator */
 		reg = regulator_get(host->dev, "vmmc_aux");
-		host->vcc_aux = IS_ERR(reg) ? NULL : reg;
+		host->vcc_aux = IS_ERR_OR_NULL(reg) ? NULL : reg;
 
 		/*
 		* UGLY HACK:  workaround regulator framework bugs.
drivers/mmc/host/sdhci.c +1 -1
index 5d20661..d3585d4 100644
@@ -2004,7 +2004,7 @@ int sdhci_add_host(struct sdhci_host *host)
 		goto untasklet;
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else {
Signature · 1 line
1.7.5.1
#1 lore raw

PATCH 1/1 mmc: fix handling NULL returned from regulator_get()

Hi Antonio, thanks for doing this,

Quoted 96 lines · Antonio Ospite

When regulator_get() is stubbed down it returns NULL, handle this case when deciding whether the driver can use the regulator or not.

Remember: IS_ERR(NULL) is false, see also this discussion for more
insight: http://comments.gmane.org/gmane.linux.kernel.mmc/7934

Now that regulator_get() is handled correctly, the ifdef on CONFIG_REGULATOR in mmci.c can go away as well.

Signed-off-by Antonio Ospite <[email protected]>

---

 drivers/mmc/host/dw_mmc.c     |    2 +-
 drivers/mmc/host/mmci.c       |    4 +---
 drivers/mmc/host/mxcmmc.c     |    2 +-
 drivers/mmc/host/omap_hsmmc.c |    4 ++--
 drivers/mmc/host/sdhci.c      |    2 +-
 5 files changed, 6 insertions(+), 8 deletions(-)
drivers/mmc/host/dw_mmc.c +1 -1
index 87e1f57..5be1325 100644
@@ -1442,7 +1442,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 #endif /* CONFIG_MMC_DW_IDMAC */
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
drivers/mmc/host/mmci.c +1 -3
index b4a7e4f..6fac353 100644
@@ -1058,10 +1058,9 @@ static int __devinit mmci_probe(struct amba_device *dev,
 		mmc->f_max = min(host->mclk, fmax);
 	dev_dbg(mmc_dev(mmc), "clocking block at %u Hz\n", mmc->f_max);
 
-#ifdef CONFIG_REGULATOR
 	/* If we're using the regulator framework, try to fetch a regulator */
 	host->vcc = regulator_get(&dev->dev, "vmmc");
-	if (IS_ERR(host->vcc))
+	if (IS_ERR_OR_NULL(host->vcc))
 		host->vcc = NULL;
 	else {
 		int mask = mmc_regulator_get_ocrmask(host->vcc);

@@ -1077,7 +1076,6 @@ static int __devinit mmci_probe(struct amba_device *dev,
 				 "(using regulator instead)\n");
 		}
 	}
-#endif
 	/* Fall back to platform data if no regulator is found */
 	if (host->vcc == NULL)
 		mmc->ocr_avail = plat->ocr_mask;
drivers/mmc/host/mxcmmc.c +1 -1
index cc20e02..a9152da 100644
@@ -155,7 +155,7 @@ static inline void mxcmci_init_ocr(struct mxcmci_host *host)
 {
 	host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
 
-	if (IS_ERR(host->vcc)) {
+	if (IS_ERR_OR_NULL(host->vcc)) {
 		host->vcc = NULL;
 	} else {
 		host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
drivers/mmc/host/omap_hsmmc.c +2 -2
index 259ece0..45fd4fe 100644
@@ -405,7 +405,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 	}
 
 	reg = regulator_get(host->dev, "vmmc");
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_dbg(host->dev, "vmmc regulator missing\n");
 		/*
 		* HACK: until fixed.c regulator is usable,

@@ -433,7 +433,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 
 		/* Allow an aux regulator */
 		reg = regulator_get(host->dev, "vmmc_aux");
-		host->vcc_aux = IS_ERR(reg) ? NULL : reg;
+		host->vcc_aux = IS_ERR_OR_NULL(reg) ? NULL : reg;
 
 		/*
 		* UGLY HACK:  workaround regulator framework bugs.
drivers/mmc/host/sdhci.c +0 -0
index 5d20661..d3585d4 100644

drivers/mmc/host/sdhci.c int sdhci_add_host(struct sdhci_host *host) · line 2010

Quoted 8 lines
drivers/mmc/host/sdhci.c +1 -1
@@ -2004,7 +2004,7 @@ int sdhci_add_host(struct sdhci_host *host)
 		goto untasklet;
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else {

I think I like this change for every driver *except* sdhci -- users who compile with CONFIG_REGULATOR=n (i.e. most distros) will start seeing "mmc0: no vmmc regulator found" when they boot their x86 laptops, and printing a cryptic regulator error message when regulator support isn't even compiled in seems uncalled for.

So, I think my suggestion is to merge all but the last hunk of the patch. How do others feel about it?

Thanks,

- Chris.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
#2 lore raw

PATCH 1/1 mmc: fix handling NULL returned from regulator_get()

Quoted 56 lines · Chris Ball

Hi Antonio, thanks for doing this,

Quoted 45 lines · Antonio Ospite

When regulator_get() is stubbed down it returns NULL, handle this case when deciding whether the driver can use the regulator or not.

Remember: IS_ERR(NULL) is false, see also this discussion for more
insight: http://comments.gmane.org/gmane.linux.kernel.mmc/7934

Now that regulator_get() is handled correctly, the ifdef on CONFIG_REGULATOR in mmci.c can go away as well.

Signed-off-by Antonio Ospite <[email protected]>

---

 drivers/mmc/host/dw_mmc.c     |    2 +-
 drivers/mmc/host/mmci.c       |    4 +---
 drivers/mmc/host/mxcmmc.c     |    2 +-
 drivers/mmc/host/omap_hsmmc.c |    4 ++--
 drivers/mmc/host/sdhci.c      |    2 +-
 5 files changed, 6 insertions(+), 8 deletions(-)
drivers/mmc/host/dw_mmc.c +1 -1
index 87e1f57..5be1325 100644
@@ -1442,7 +1442,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 #endif /* CONFIG_MMC_DW_IDMAC */
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
drivers/mmc/host/omap_hsmmc.c +1 -1
index 259ece0..45fd4fe 100644
@@ -405,7 +405,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 	}
 
 	reg = regulator_get(host->dev, "vmmc");
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_dbg(host->dev, "vmmc regulator missing\n");
 		/*
 		* HACK: until fixed.c regulator is usable,
Quoted 2 lines

} else {
[...]

I think I like this change for every driver *except* sdhci -- users who compile with CONFIG_REGULATOR=n (i.e. most distros) will start seeing "mmc0: no vmmc regulator found" when they boot their x86 laptops, and printing a cryptic regulator error message when regulator support isn't even compiled in seems uncalled for.

Quoted 3 lines

So, I think my suggestion is to merge all but the last hunk of the patch. How do others feel about it?

dw_mmc and omap_hsmmc will also print error messages if the regulator framework is not build-in. I suggest to use something like:

if (IS_ERR(vmmc)) {
... do error handling
} else if(vmmc) {
... use regulator
} else {
... fallback code to initialize ocr_avail
}

- Lars

maintainer
#3 lore raw

PATCH 1/1 mmc: fix handling NULL returned from regulator_get()

Quoted 64 lines · Lars-Peter Clausen
Quoted 56 lines · Chris Ball

Hi Antonio, thanks for doing this,

Quoted 45 lines · Antonio Ospite

When regulator_get() is stubbed down it returns NULL, handle this case when deciding whether the driver can use the regulator or not.

Remember: IS_ERR(NULL) is false, see also this discussion for more
insight: http://comments.gmane.org/gmane.linux.kernel.mmc/7934

Now that regulator_get() is handled correctly, the ifdef on CONFIG_REGULATOR in mmci.c can go away as well.

Signed-off-by Antonio Ospite <[email protected]>

---

 drivers/mmc/host/dw_mmc.c     |    2 +-
 drivers/mmc/host/mmci.c       |    4 +---
 drivers/mmc/host/mxcmmc.c     |    2 +-
 drivers/mmc/host/omap_hsmmc.c |    4 ++--
 drivers/mmc/host/sdhci.c      |    2 +-
 5 files changed, 6 insertions(+), 8 deletions(-)
drivers/mmc/host/dw_mmc.c +1 -1
index 87e1f57..5be1325 100644
@@ -1442,7 +1442,7 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 #endif /* CONFIG_MMC_DW_IDMAC */
 
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
-	if (IS_ERR(host->vmmc)) {
+	if (IS_ERR_OR_NULL(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
drivers/mmc/host/omap_hsmmc.c +1 -1
index 259ece0..45fd4fe 100644
@@ -405,7 +405,7 @@ static int omap_hsmmc_reg_get(struct omap_hsmmc_host *host)
 	}
 
 	reg = regulator_get(host->dev, "vmmc");
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_dbg(host->dev, "vmmc regulator missing\n");
 		/*
 		* HACK: until fixed.c regulator is usable,
Quoted 2 lines

} else {
[...]

I think I like this change for every driver *except* sdhci -- users who compile with CONFIG_REGULATOR=n (i.e. most distros) will start seeing "mmc0: no vmmc regulator found" when they boot their x86 laptops, and printing a cryptic regulator error message when regulator support isn't even compiled in seems uncalled for.

Quoted 3 lines

So, I think my suggestion is to merge all but the last hunk of the patch. How do others feel about it?

dw_mmc and omap_hsmmc will also print error messages if the regulator framework is not build-in. I suggest to use something like:

Quoted 8 lines

if (IS_ERR(vmmc)) {
... do error handling
} else if(vmmc) {
... use regulator
} else {
... fallback code to initialize ocr_avail
}

if we use IS_ERR_OR_NULL(), i think that we can confuse sometimes. Because, When we set CONFIG_REGULATOR = n, we can find "mmc0: no vmmc regulator found". That message means "error or not use regulator framework".
But by that message, we can't know whether error or not use regulator framework.

So i think that Lars's suggestion is better than IS_ERR_OR_NULL.

Regards,
Jaeehoon Chung

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html