Skip to contentSkip to conversation

dw_mmc: fixed regulator control

PATCH In review Jaehoon Chung 2 participants 5 messages Apr 29, 2011 – May 11, 2011

Subsystems (2)
MMC, SD and SDIO Synopsys DesignWare MMC/SD/SDIO driver
lore
Share & tools
lore

Jaehoon Chung

author maintainer
#0 lore raw

PATCH 1/1 dw_mmc: fixed regulator control

This patch fixed regulator control in dw_mmc.c
If we didn't set CONFIG_REGULATOR, always entered error condition. But that's not error..because we didn't use regulator framework.

So when we only used CONFIG_REGULATOR, i think that need to get regulator.

And In suspend function, regulator_enable() need not.. need that in resume function.
(i didn't know why located there...)

Thanks,
Jaehoon Chung

Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/mmc/host/dw_mmc.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc.c +5 -3
index 87e1f57..62b900f 100644
@@ -1441,12 +1441,14 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 	}
 #endif /* CONFIG_MMC_DW_IDMAC */
 
+#ifdef CONFIG_REGULATOR
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
 	if (IS_ERR(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
 		regulator_enable(host->vmmc);
+#endif /* CONFIG_REGULATOR */
 
 	if (dw_mci_get_cd(mmc))
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);

@@ -1769,9 +1771,6 @@ static int dw_mci_suspend(struct platform_device *pdev, pm_message_t mesg)
 	int i, ret;
 	struct dw_mci *host = platform_get_drvdata(pdev);
 
-	if (host->vmmc)
-		regulator_enable(host->vmmc);
-
 	for (i = 0; i < host->num_slots; i++) {
 		struct dw_mci_slot *slot = host->slot[i];
 		if (!slot)

@@ -1798,6 +1797,9 @@ static int dw_mci_resume(struct platform_device *pdev)
 	int i, ret;
 	struct dw_mci *host = platform_get_drvdata(pdev);
 
+	if (host->vmmc)
+		regulator_enable(host->vmmc);
+
 	if (host->dma_ops->init)
 		host->dma_ops->init(host);
#1 lore raw

Hi Jaehoon,

Quoted 5 lines · Jaehoon Chung

This patch fixed regulator control in dw_mmc.c
If we didn't set CONFIG_REGULATOR, always entered error condition. But that's not error..because we didn't use regulator framework.

So when we only used CONFIG_REGULATOR, i think that need to get regulator.

[..]

Quoted 9 lines
drivers/mmc/host/dw_mmc.c +1 -0
index 87e1f57..62b900f 100644
@@ -1441,12 +1441,14 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 	}
 #endif /* CONFIG_MMC_DW_IDMAC */
 
+#ifdef CONFIG_REGULATOR

drivers/mmc/host/dw_mmc.c static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id) · line 1454

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +1 -0
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
 	if (IS_ERR(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
 		regulator_enable(host->vmmc);
+#endif /* CONFIG_REGULATOR */
 
 	if (dw_mci_get_cd(mmc))
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);

[..]

As Lars pointed out, this doesn't make sense; without CONFIG_REGULATOR host->vmmc becomes NULL, which isn't IS_ERR.

- Chris.

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

Hi Chris..

Thanks for comments...
I think that could confuse that message when didn't set CONFIG_REGULATOR.

And i wonder how do you think about regulator control in suspend (dw_mmc.c)?

@@ -1769,9 +1771,6 @@ static int dw_mci_suspend(struct platform_device *pdev, pm_message_t mesg)
int i, ret;
struct dw_mci *host = platform_get_drvdata(pdev);

- if (host->vmmc)
- regulator_enable(host->vmmc);
-
for (i = 0; i < host->num_slots; i++) {
struct dw_mci_slot *slot = host->slot[i];
if (!slot)
@@ -1798,6 +1797,9 @@ static int dw_mci_resume(struct platform_device *pdev)
int i, ret;
struct dw_mci *host = platform_get_drvdata(pdev);

+ if (host->vmmc)
+ regulator_enable(host->vmmc);
+
if (host->dma_ops->init)
host->dma_ops->init(host);

Regards,
Jaehoon Chung

Quoted 34 lines · Chris Ball

Hi Jaehoon,

Quoted 5 lines · Jaehoon Chung

This patch fixed regulator control in dw_mmc.c
If we didn't set CONFIG_REGULATOR, always entered error condition. But that's not error..because we didn't use regulator framework.

So when we only used CONFIG_REGULATOR, i think that need to get regulator.

[..]

Quoted 9 lines
drivers/mmc/host/dw_mmc.c +1 -0
index 87e1f57..62b900f 100644
@@ -1441,12 +1441,14 @@ static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id)
 	}
 #endif /* CONFIG_MMC_DW_IDMAC */
 
+#ifdef CONFIG_REGULATOR

drivers/mmc/host/dw_mmc.c static int __init dw_mci_init_slot(struct dw_mci *host, unsigned int id) · line 1454

Quoted 10 lines
drivers/mmc/host/dw_mmc.c +1 -0
 	host->vmmc = regulator_get(mmc_dev(mmc), "vmmc");
 	if (IS_ERR(host->vmmc)) {
 		printk(KERN_INFO "%s: no vmmc regulator found\n", mmc_hostname(mmc));
 		host->vmmc = NULL;
 	} else
 		regulator_enable(host->vmmc);
+#endif /* CONFIG_REGULATOR */
 
 	if (dw_mci_get_cd(mmc))
 		set_bit(DW_MMC_CARD_PRESENT, &slot->flags);

[..]

As Lars pointed out, this doesn't make sense; without CONFIG_REGULATOR host->vmmc becomes NULL, which isn't IS_ERR.

- Chris.

#3 lore raw

Hi Jaehoon,

Quoted 4 lines · Jaehoon Chung

Hi Chris..

Thanks for comments...
I think that could confuse that message when didn't set CONFIG_REGULATOR.

The message *does not appear* when CONFIG_REGULATOR is unset, because NULL -- which the regulator subsystem returns via the regulator_get() stub -- is not an IS_ERR(). Do you agree?

Quoted 1 line

And i wonder how do you think about regulator control in suspend (dw_mmc.c)?

Yes, that patch looks correct, although I can't test it myself. Please send it separately.

- Chris.

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

Hi Jaehoon,

Quoted 4 lines · Jaehoon Chung

Hi Chris..

Thanks for comments...
I think that could confuse that message when didn't set CONFIG_REGULATOR.

The message *does not appear* when CONFIG_REGULATOR is unset, because NULL -- which the regulator subsystem returns via the regulator_get() stub -- is not an IS_ERR(). Do you agree?

Thanks, I agreed that.
I'm interesting mailing's discussion about "[PATCH] pxamci: remove an ifdef about CONFIG_REGULATOR " (in this case, i think similar case)

Quoted 5 lines
Quoted 2 lines

And i wonder how do you think about regulator control in suspend (dw_mmc.c)?

Yes, that patch looks correct, although I can't test it myself. Please send it separately.

Ok. i will send that separately patch.

Regards,
Jaehoon Chung