dw_mmc: fixed wrong regulator_enable in suspend/resume

PATCH In review Jaehoon Chung 3 participants 6 messages May 11, 2011 – May 12, 2011

Subsystems (2)

MMC, SD and SDIO Synopsys DesignWare MMC/SD/SDIO driver

lore

Share & tools

lore

Jaehoon Chung

author maintainer

2011-05-11 17:02 UTC #0 lore raw

PATCH 1/1 dw_mmc: fixed wrong regulator_enable in suspend/resume

In suspend function, regulator_enable() need not. regulator_enable() should be locate in resume function.

Signed-off-by Jaehoon Chung <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/mmc/host/dw_mmc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +3 -3

index 87e1f57..66dcddb 100644
@@ -1769,9 +1769,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 +1795,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);

Chris Ball

2011-05-11 15:45 UTC #1 lore raw

Hi,

Quoted 25 lines · Jaehoon Chung

In suspend function, regulator_enable() need not. regulator_enable() should be locate in resume function.

Signed-off-by Jaehoon Chung <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/mmc/host/dw_mmc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +0 -3

index 87e1f57..66dcddb 100644
@@ -1769,9 +1769,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)

drivers/mmc/host/dw_mmc.c static int dw_mci_resume(struct platform_device *pdev) · line 1803

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +3 -0

@@ -1798,6 +1795,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);

Should the regulator be being disabled in the suspend function, then?

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Jaehoon Chung

author maintainer

2011-05-12 03:01 UTC #2 lore raw

Hi Chris..

I think you're confused something because my explanation.

My means..if we use the regulator, regulator is enabled when probing. Then regulator is enabling until we don't disable..

In order to power-save, disabled the regulator when entered suspend. Already in suspend function, located "regulator_disable()". but also located "regulator_enable()" in suspend function.

So i think right that "regulator_enable()" is located in resume function. not in suspend function.

Regards,
Jaehoon Chung

Quoted 41 lines · Chris Ball

Hi,
Quoted 25 lines · Jaehoon Chung
In suspend function, regulator_enable() need not. regulator_enable() should be locate in resume function.
Signed-off-by Jaehoon Chung <[email protected]>
Signed-off-by Kyungmin Park <[email protected]>
---
 drivers/mmc/host/dw_mmc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/mmc/host/dw_mmc.c +0 -3
index 87e1f57..66dcddb 100644
@@ -1769,9 +1769,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)
drivers/mmc/host/dw_mmc.c static int dw_mci_resume(struct platform_device *pdev) · line 1803
Quoted 9 lines
drivers/mmc/host/dw_mmc.c +3 -0
@@ -1798,6 +1795,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);
 
Should the regulator be being disabled in the suspend function, then?
- Chris.

Chris Ball

2011-05-12 03:43 UTC #3 lore raw

Hi,

Quoted 7 lines · Jaehoon Chung

Hi Chris..
I think you're confused something because my explanation.
My means..if we use the regulator, regulator is enabled when probing. Then regulator is enabling until we don't disable..

Quoted 6 lines

In order to power-save, disabled the regulator when entered suspend. Already in suspend function, located "regulator_disable()". but also located "regulator_enable()" in suspend function.
So i think right that "regulator_enable()" is located in resume function. not in suspend function.

Thanks, I agree. I hadn't looked at the suspend function, but now I see that it already has the disable.

Will, want to ACK this before I merge it?

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child

Will Newton

Acked-by

2011-05-12 09:01 UTC #4 lore raw

Quoted 14 lines · Chris Ball

Hi,
Quoted 11 lines · Jaehoon Chung
Hi Chris..
I think you're confused something because my explanation.
My means..if we use the regulator, regulator is enabled when probing. Then regulator is enabling until we don't disable..
In order to power-save, disabled the regulator when entered suspend. Already in suspend function, located "regulator_disable()". but also located "regulator_enable()" in suspend function.

Quoted 7 lines

Quoted 2 lines
So i think right that "regulator_enable()" is located in resume function. not in suspend function.
Thanks, I agree. I hadn't looked at the suspend function, but now I see that it already has the disable.
Will, want to ACK this before I merge it?

Looks ok to me, although I don't have suitable hardware to test it.

Acked-by Will Newton <[email protected]>

Chris Ball

2011-05-12 22:12 UTC #5 lore raw

In reply to Jaehoon Chung

Hi Jaehoon,

Quoted 24 lines · Jaehoon Chung

In suspend function, regulator_enable() need not. regulator_enable() should be locate in resume function.

Signed-off-by Jaehoon Chung <[email protected]>

Signed-off-by Kyungmin Park <[email protected]>

---
 drivers/mmc/host/dw_mmc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/mmc/host/dw_mmc.c +0 -3

index 87e1f57..66dcddb 100644
@@ -1769,9 +1769,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)

drivers/mmc/host/dw_mmc.c static int dw_mci_resume(struct platform_device *pdev) · line 1803

Quoted 9 lines

drivers/mmc/host/dw_mmc.c +3 -0

@@ -1798,6 +1795,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);

Thanks, pushed to mmc-next for .40 with Will's ACK.

- Chris.

Signature · 2 lines

Chris Ball   <[email protected]>   <http://printf.net/>
One Laptop Per Child