Skip to contentSkip to conversation

pxamci: remove an ifdef about CONFIG_REGULATOR

PATCH New Antonio Ospite 5 participants 14 messages May 9, 2011 – May 11, 2011 MMC, SD and SDIO

lore
Share & tools
lore

Antonio Ospite

author
#0 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

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 <[email protected]>

---

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(-)
drivers/mmc/host/pxamci.c +1 -2
index 7257738..07e1f22 100644
@@ -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 ?
Signature · 1 line
1.7.5.1
#1 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

Quoted 3 lines · 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.

Acked-by Mark Brown <[email protected]>

But pleasse always include a plain text version of the commit description for those who didn't memorise the entire git log.

#2 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

In reply to Antonio Ospite
Quoted 5 lines · 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 <[email protected]>

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

#3 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

Quoted 4 lines · Russell King - ARM Linux

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.

author
#4 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

Quoted 11 lines · On Mon, 9 May 2011 22:36:12 +0200 Mark Brown…
Quoted 4 lines · Russell King - ARM Linux

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

Signature · 22 lines
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>
#5 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

Quoted 7 lines · Antonio Ospite

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.

#6 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

In reply to Antonio Ospite
Quoted 2 lines · Antonio Ospite

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.

Quoted 2 lines

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.

author
#7 lore raw

PATCH v2 1/2 pxamci: remove the ifdef CONFIG_REGULATOR

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 <[email protected]>

---

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(-)
drivers/mmc/host/pxamci.c +2 -3
index 7257738..3a89fb2 100644
@@ -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 ?
Signature · 1 line
1.7.5.1
#8 lore raw

PATCH v2 1/2 pxamci: remove the ifdef CONFIG_REGULATOR

Quoted 7 lines · 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 <[email protected]>
Reviewed-by Mark Brown <[email protected]>
#9 lore raw

PATCH v2 1/2 pxamci: remove the ifdef CONFIG_REGULATOR

In reply to Antonio Ospite

Hi Antonio,

Quoted 39 lines · 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 <[email protected]>

---

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(-)
drivers/mmc/host/pxamci.c +1 -2
index 7257738..3a89fb2 100644
@@ -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);

drivers/mmc/host/pxamci.c static inline void pxamci_init_ocr(struct pxamci_host *host) · line 95

Quoted 8 lines
drivers/mmc/host/pxamci.c +1 -1
@@ -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.

Signature · 2 lines
Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child
author
#10 lore raw

PATCH v2 1/2 pxamci: remove the ifdef CONFIG_REGULATOR

Quoted 1 line · On Wed, 11 May 2011 10:41:43 -0400 Chris Ball <[email protected]> wrote:

On Wed, May 11 2011, Antonio Ospite wrote:

[...]

Quoted 2 lines
Quoted 2 lines
 drivers/mmc/host/pxamci.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

[...]

Quoted 4 lines

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

Signature · 15 lines
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>
author
#11 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/mmc/host/pxamci.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)
drivers/mmc/host/pxamci.c +2 -2
index 3a89fb2..fbdb21e 100644
@@ -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),
Signature · 1 line
1.7.5.1
author
#12 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

In reply to Antonio Ospite
Quoted 5 lines · On Tue, 10 May 2011 22:02:14 +0200 Antonio Ospite <[email protected]>…

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

Signature · 15 lines
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>
#13 lore raw

PATCH 1/1 pxamci: remove an ifdef about CONFIG_REGULATOR

In reply to Antonio Ospite

Hello.

Quoted 2 lines · Antonio Ospite

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.

Quoted 4 lines

it correctly degenerates and returns NULL when the regulator framework is disabled.

Signed-off-by Antonio Ospite<[email protected]>

WBR, Sergei