* [PATCH] mmc: fix handling NULL returned from regulator_get()
@ 2011-05-18 21:37 Antonio Ospite
2011-05-18 21:47 ` Chris Ball
0 siblings, 1 reply; 4+ messages in thread
From: Antonio Ospite @ 2011-05-18 21:37 UTC (permalink / raw)
To: linux-arm-kernel
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 <ospite@studenti.unina.it>
---
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(-)
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 87e1f57..5be1325 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -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
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index b4a7e4f..6fac353 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -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;
diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
index cc20e02..a9152da 100644
--- a/drivers/mmc/host/mxcmmc.c
+++ b/drivers/mmc/host/mxcmmc.c
@@ -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);
diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
index 259ece0..45fd4fe 100644
--- a/drivers/mmc/host/omap_hsmmc.c
+++ b/drivers/mmc/host/omap_hsmmc.c
@@ -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.
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5d20661..d3585d4 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -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 {
--
1.7.5.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH] mmc: fix handling NULL returned from regulator_get()
2011-05-18 21:37 [PATCH] mmc: fix handling NULL returned from regulator_get() Antonio Ospite
@ 2011-05-18 21:47 ` Chris Ball
2011-05-19 2:10 ` Lars-Peter Clausen
0 siblings, 1 reply; 4+ messages in thread
From: Chris Ball @ 2011-05-18 21:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Antonio, thanks for doing this,
On Wed, May 18 2011, Antonio Ospite wrote:
> 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 <ospite@studenti.unina.it>
> ---
>
> 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(-)
>
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 87e1f57..5be1325 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -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
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index b4a7e4f..6fac353 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -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;
> diff --git a/drivers/mmc/host/mxcmmc.c b/drivers/mmc/host/mxcmmc.c
> index cc20e02..a9152da 100644
> --- a/drivers/mmc/host/mxcmmc.c
> +++ b/drivers/mmc/host/mxcmmc.c
> @@ -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);
> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
> index 259ece0..45fd4fe 100644
> --- a/drivers/mmc/host/omap_hsmmc.c
> +++ b/drivers/mmc/host/omap_hsmmc.c
> @@ -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.
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 5d20661..d3585d4 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -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.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] mmc: fix handling NULL returned from regulator_get()
2011-05-18 21:47 ` Chris Ball
@ 2011-05-19 2:10 ` Lars-Peter Clausen
2011-05-19 5:14 ` Jaehoon Chung
0 siblings, 1 reply; 4+ messages in thread
From: Lars-Peter Clausen @ 2011-05-19 2:10 UTC (permalink / raw)
To: linux-arm-kernel
On 05/18/2011 02:47 PM, Chris Ball wrote:
> Hi Antonio, thanks for doing this,
>
> On Wed, May 18 2011, Antonio Ospite wrote:
>> 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 <ospite@studenti.unina.it>
>> ---
>>
>> 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(-)
>>
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 87e1f57..5be1325 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -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
>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>> index 259ece0..45fd4fe 100644
>> --- a/drivers/mmc/host/omap_hsmmc.c
>> +++ b/drivers/mmc/host/omap_hsmmc.c
>> @@ -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,
>> } 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?
>
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
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH] mmc: fix handling NULL returned from regulator_get()
2011-05-19 2:10 ` Lars-Peter Clausen
@ 2011-05-19 5:14 ` Jaehoon Chung
0 siblings, 0 replies; 4+ messages in thread
From: Jaehoon Chung @ 2011-05-19 5:14 UTC (permalink / raw)
To: linux-arm-kernel
Lars-Peter Clausen wrote:
> On 05/18/2011 02:47 PM, Chris Ball wrote:
>> Hi Antonio, thanks for doing this,
>>
>> On Wed, May 18 2011, Antonio Ospite wrote:
>>> 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 <ospite@studenti.unina.it>
>>> ---
>>>
>>> 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(-)
>>>
>>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>>> index 87e1f57..5be1325 100644
>>> --- a/drivers/mmc/host/dw_mmc.c
>>> +++ b/drivers/mmc/host/dw_mmc.c
>>> @@ -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
>>> diff --git a/drivers/mmc/host/omap_hsmmc.c b/drivers/mmc/host/omap_hsmmc.c
>>> index 259ece0..45fd4fe 100644
>>> --- a/drivers/mmc/host/omap_hsmmc.c
>>> +++ b/drivers/mmc/host/omap_hsmmc.c
>>> @@ -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,
>>> } 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?
>>
>
> 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
> }
>
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
> --
> 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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-05-19 5:14 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-05-18 21:37 [PATCH] mmc: fix handling NULL returned from regulator_get() Antonio Ospite
2011-05-18 21:47 ` Chris Ball
2011-05-19 2:10 ` Lars-Peter Clausen
2011-05-19 5:14 ` Jaehoon Chung
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).