[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Antonio Ospite]

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 at 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(-)

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 ?
-- 
1.7.5.1

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Mark Brown]

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.

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Russell King - ARM Linux]

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

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Mark Brown]

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.

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Antonio Ospite]

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>

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Russell King - ARM Linux]

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.

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Mark Brown]

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.

[PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR

[Antonio Ospite]

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

[PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR

[Mark Brown]

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>

[PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR

[Chris Ball]

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

[PATCH v2 1/2] pxamci: remove the ifdef CONFIG_REGULATOR

[Antonio Ospite]

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?
-------------- 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/20110511/260c91c5/attachment.sig>

[PATCH 2/2] pxamci: fix coding style for multi statement conditionals

[Antonio Ospite]

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

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Antonio Ospite]

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?
-------------- 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/20110511/8314eb4a/attachment.sig>

[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR

[Sergei Shtylyov]

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