| Message ID | 1304971869-20161-1-git-send-email-ospite@studenti.unina.it (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, May 09, 2011 at 10:11:09PM +0200, Antonio Ospite wrote: > Don't wrap regulator_get() inside an #ifdef CONFIG_REGULATOR anymore, as > now (since be1a50d) it correctly degenerates and returns NULL when the > regulator framework is disabled. Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com> But pleasse always include a plain text version of the commit description for those who didn't memorise the entire git log. -- 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
On Mon, May 09, 2011 at 10:11:09PM +0200, Antonio Ospite wrote: > Don't wrap regulator_get() inside an #ifdef CONFIG_REGULATOR anymore, as > now (since be1a50d) it correctly degenerates and returns NULL when the > regulator framework is disabled. > > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> NAK. This has been proposed before, and rejected. See the comments against the original proposal: http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6525/1 -- 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
On Mon, May 09, 2011 at 09:23:25PM +0100, Russell King - ARM Linux wrote: > NAK. This has been proposed before, and rejected. See the comments > against the original proposal: > http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6525/1 Hrm, this looks like the bodge we're doing for !REGULATOR isn't actually helping here unless we do a NULL || IS_ERR() in the error check. The ifdefs are certainly fail. -- 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
Hello. On 10-05-2011 0:11, Antonio Ospite wrote: > Don't wrap regulator_get() inside an #ifdef CONFIG_REGULATOR anymore, as > now (since be1a50d) Need the summary of that commit cited too -- for the human readers. > it correctly degenerates and returns NULL when the > regulator framework is disabled. > Signed-off-by: Antonio Ospite<ospite@studenti.unina.it> WBR, Sergei -- 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
On Mon, 9 May 2011 22:36:12 +0200 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Mon, May 09, 2011 at 09:23:25PM +0100, Russell King - ARM Linux wrote: > > > NAK. This has been proposed before, and rejected. See the comments > > against the original proposal: > > > http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=6525/1 > > Hrm, this looks like the bodge we're doing for !REGULATOR isn't actually > helping here unless we do a NULL || IS_ERR() in the error check. The > ifdefs are certainly fail. > [Adding Linus Walleij to CC as he was the author of a similar NAKed patch] I was blindly trusting code already in mainline again, and for that I apologize, I finally took the time to look at the implementation of IS_ERR() and test its use, and being IS_ERR(NULL) true it is not what we want indeed, see the attached test program. So, I am going to remove the ifdefs anyway but use IS_ERR_OR_NULL(); how does that sound? Am I still missing anything? Or changing the regulator_get() stub to return an error (-ENOSYS?) might be worth it? Thanks Russel for pointing out the issue BTW. Thanks, Antonio
On Tue, May 10, 2011 at 10:02:14PM +0200, Antonio Ospite wrote: > I was blindly trusting code already in mainline again, and for that I > apologize, I finally took the time to look at the implementation > of IS_ERR() and test its use, and being IS_ERR(NULL) true it is not what > we want indeed, see the attached test program. > > So, I am going to remove the ifdefs anyway but use IS_ERR_OR_NULL(); > how does that sound? Am I still missing anything? That sounds a lot better, and should avoid the issue which caused me to throw out the original patch. -- 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
On Tue, May 10, 2011 at 10:02:14PM +0200, Antonio Ospite wrote: > So, I am going to remove the ifdefs anyway but use IS_ERR_OR_NULL(); > how does that sound? Am I still missing anything? Looks good to me. > Or changing the regulator_get() stub to return an error (-ENOSYS?) might > be worth it? No, the whole point of stubbing out the API is that it allows most consumers which just do simple enables and disables to run without needing to worry if the regulator API is compiled in or not. -- 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
On Wed, May 11, 2011 at 12:19:05PM +0200, Antonio Ospite wrote: > Don't wrap regulator_get() inside an ifdef CONFIG_REGULATOR anymore, as > now it degenerates and returns NULL when the regulator framework is > disabled (since commit be1a50d: regulator: Let drivers know when they > use the stub API); and use IS_ERR_OR_NULL() to handle the stub function > correctly. > > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com> -- 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
Hi Antonio, On Wed, May 11 2011, Antonio Ospite wrote: > Don't wrap regulator_get() inside an ifdef CONFIG_REGULATOR anymore, as > now it degenerates and returns NULL when the regulator framework is > disabled (since commit be1a50d: regulator: Let drivers know when they > use the stub API); and use IS_ERR_OR_NULL() to handle the stub function > correctly. > > Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> > --- > > Changes since v1: > - Use IS_ERR_OR_NULL() to correctly handle the stub regulator_get() > - Put the short commit message of be1a50d so it is easier to see what it is > about, for the long commit and the actual code changes the git history can > be queried. > > Thanks, > Antonio Ospite > http://ao2.it > > drivers/mmc/host/pxamci.c | 5 ++--- > 1 files changed, 2 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c > index 7257738..3a89fb2 100644 > --- a/drivers/mmc/host/pxamci.c > +++ b/drivers/mmc/host/pxamci.c > @@ -79,10 +79,9 @@ struct pxamci_host { > > static inline void pxamci_init_ocr(struct pxamci_host *host) > { > -#ifdef CONFIG_REGULATOR > 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); > @@ -90,7 +89,7 @@ static inline void pxamci_init_ocr(struct pxamci_host *host) > dev_warn(mmc_dev(host->mmc), > "ocr_mask/setpower will not be used\n"); > } > -#endif > + > if (host->vcc == NULL) { > /* fall-back to platform data */ > host->mmc->ocr_avail = host->pdata ? Thanks, I've merged this and squashed the 2/2 punctuation patch into it; it didn't seem worth having a separate patch when both touch the same code. - Chris.
On Wed, 11 May 2011 10:41:43 -0400 Chris Ball <cjb@laptop.org> wrote: > On Wed, May 11 2011, Antonio Ospite wrote: [...] > > drivers/mmc/host/pxamci.c | 5 ++--- > > 1 files changed, 2 insertions(+), 3 deletions(-) [...] > > Thanks, I've merged this and squashed the 2/2 punctuation patch into it; > it didn't seem worth having a separate patch when both touch the same code. > That's perfectly fine with me. Thanks, Antonio
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c index 7257738..07e1f22 100644 --- a/drivers/mmc/host/pxamci.c +++ b/drivers/mmc/host/pxamci.c @@ -79,7 +79,6 @@ struct pxamci_host { static inline void pxamci_init_ocr(struct pxamci_host *host) { -#ifdef CONFIG_REGULATOR host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc"); if (IS_ERR(host->vcc)) @@ -90,7 +89,7 @@ static inline void pxamci_init_ocr(struct pxamci_host *host) dev_warn(mmc_dev(host->mmc), "ocr_mask/setpower will not be used\n"); } -#endif + if (host->vcc == NULL) { /* fall-back to platform data */ host->mmc->ocr_avail = host->pdata ?
Don't wrap regulator_get() inside an #ifdef CONFIG_REGULATOR anymore, as now (since be1a50d) it correctly degenerates and returns NULL when the regulator framework is disabled. Signed-off-by: Antonio Ospite <ospite@studenti.unina.it> --- This comes after the discussion at http://www.mail-archive.com/linux-mmc@vger.kernel.org/msg07443.html and following. Regards, Antonio Ospite http://ao2.it drivers/mmc/host/pxamci.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-)