* [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
@ 2011-05-10 20:02 ` Antonio Ospite
0 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-10 20:02 UTC (permalink / raw)
To: linux-arm-kernel
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
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: IS_ERR_regulator_get.c
Type: text/x-csrc
Size: 1590 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110510/12e659f9/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110510/12e659f9/attachment.sig>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
2011-05-10 20:02 ` Antonio Ospite
@ 2011-05-10 20:32 ` Russell King - ARM Linux
-1 siblings, 0 replies; 28+ messages in thread
From: Russell King - ARM Linux @ 2011-05-10 20:32 UTC (permalink / raw)
To: Antonio Ospite
Cc: Mark Brown, linux-mmc, openezx-devel, Chris Ball,
linux-arm-kernel, Linus Walleij
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
@ 2011-05-10 20:32 ` Russell King - ARM Linux
0 siblings, 0 replies; 28+ messages in thread
From: Russell King - ARM Linux @ 2011-05-10 20:32 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
2011-05-10 20:02 ` Antonio Ospite
@ 2011-05-10 20:36 ` Mark Brown
-1 siblings, 0 replies; 28+ messages in thread
From: Mark Brown @ 2011-05-10 20:36 UTC (permalink / raw)
To: Antonio Ospite
Cc: Russell King - ARM Linux, linux-mmc, openezx-devel, Chris Ball,
linux-arm-kernel, Linus Walleij
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
@ 2011-05-10 20:36 ` Mark Brown
0 siblings, 0 replies; 28+ messages in thread
From: Mark Brown @ 2011-05-10 20:36 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
2011-05-10 20:36 ` Mark Brown
@ 2011-05-11 10:19 ` Antonio Ospite
-1 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 10:19 UTC (permalink / raw)
To: linux-mmc
Cc: openezx-devel, Chris Ball, Antonio Ospite, linux-arm-kernel, Mark Brown
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 ?
--
1.7.5.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
@ 2011-05-11 10:19 ` Antonio Ospite
0 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 10:19 UTC (permalink / raw)
To: linux-arm-kernel
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 ?
--
1.7.5.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
2011-05-11 10:19 ` Antonio Ospite
@ 2011-05-11 13:07 ` Mark Brown
-1 siblings, 0 replies; 28+ messages in thread
From: Mark Brown @ 2011-05-11 13:07 UTC (permalink / raw)
To: Antonio Ospite; +Cc: openezx-devel, Chris Ball, linux-mmc, linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
@ 2011-05-11 13:07 ` Mark Brown
0 siblings, 0 replies; 28+ messages in thread
From: Mark Brown @ 2011-05-11 13:07 UTC (permalink / raw)
To: linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
2011-05-11 10:19 ` Antonio Ospite
@ 2011-05-11 14:41 ` Chris Ball
-1 siblings, 0 replies; 28+ messages in thread
From: Chris Ball @ 2011-05-11 14:41 UTC (permalink / raw)
To: Antonio Ospite; +Cc: openezx-devel, linux-mmc, linux-arm-kernel, Mark Brown
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.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
@ 2011-05-11 14:41 ` Chris Ball
0 siblings, 0 replies; 28+ messages in thread
From: Chris Ball @ 2011-05-11 14:41 UTC (permalink / raw)
To: linux-arm-kernel
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.
--
Chris Ball <cjb@laptop.org> <http://printf.net/>
One Laptop Per Child
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR
2011-05-11 14:41 ` Chris Ball
@ 2011-05-11 20:19 ` Antonio Ospite
-1 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 20:19 UTC (permalink / raw)
To: Chris Ball; +Cc: linux-mmc, openezx-devel, linux-arm-kernel, Mark Brown
[-- Attachment #1: Type: text/plain, Size: 690 bytes --]
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
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/2] pxamci: fix coding style for multi statement conditionals
2011-05-10 20:36 ` Mark Brown
@ 2011-05-11 10:19 ` Antonio Ospite
-1 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 10:19 UTC (permalink / raw)
To: linux-mmc
Cc: openezx-devel, Chris Ball, Antonio Ospite, linux-arm-kernel, Mark Brown
Put parentheses around single-statement 'if' branches when the else
branch is not a single statement as suggested in
Documentation/CodingStyle Chapter 3.
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
drivers/mmc/host/pxamci.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 3a89fb2..fbdb21e 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -81,9 +81,9 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
{
host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
- if (IS_ERR_OR_NULL(host->vcc))
+ if (IS_ERR_OR_NULL(host->vcc)) {
host->vcc = NULL;
- else {
+ } else {
host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
if (host->pdata && host->pdata->ocr_mask)
dev_warn(mmc_dev(host->mmc),
--
1.7.5.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/2] pxamci: fix coding style for multi statement conditionals
@ 2011-05-11 10:19 ` Antonio Ospite
0 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 10:19 UTC (permalink / raw)
To: linux-arm-kernel
Put parentheses around single-statement 'if' branches when the else
branch is not a single statement as suggested in
Documentation/CodingStyle Chapter 3.
Signed-off-by: Antonio Ospite <ospite@studenti.unina.it>
---
drivers/mmc/host/pxamci.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 3a89fb2..fbdb21e 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -81,9 +81,9 @@ static inline void pxamci_init_ocr(struct pxamci_host *host)
{
host->vcc = regulator_get(mmc_dev(host->mmc), "vmmc");
- if (IS_ERR_OR_NULL(host->vcc))
+ if (IS_ERR_OR_NULL(host->vcc)) {
host->vcc = NULL;
- else {
+ } else {
host->mmc->ocr_avail = mmc_regulator_get_ocrmask(host->vcc);
if (host->pdata && host->pdata->ocr_mask)
dev_warn(mmc_dev(host->mmc),
--
1.7.5.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR
2011-05-10 20:02 ` Antonio Ospite
@ 2011-05-11 10:26 ` Antonio Ospite
-1 siblings, 0 replies; 28+ messages in thread
From: Antonio Ospite @ 2011-05-11 10:26 UTC (permalink / raw)
To: Mark Brown
Cc: openezx-devel, Russell King - ARM Linux, Linus Walleij,
linux-mmc, Chris Ball, linux-arm-kernel
[-- Attachment #1: Type: text/plain, Size: 714 bytes --]
On Tue, 10 May 2011 22:02:14 +0200
Antonio Ospite <ospite@studenti.unina.it> 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.
>
Just for the records, I meant to say "being IS_ERR(NULL) _false_" but
you got it anyways.
Regards,
Antonio
--
Antonio Ospite
http://ao2.it
PGP public key ID: 0x4553B001
A: Because it messes up the order in which people normally read text.
See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?
[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 28+ messages in thread