| Message ID | 1305754664-2917-1-git-send-email-ospite@studenti.unina.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 {
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(-)