[PATCH] dw_mmc: fixed regulator control

[PATCH] dw_mmc: fixed regulator control

[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.

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 <jh80.chung@samsung.com>
Signed-off-by: Kyungmin Park <kyumgmin.park@samsung.com>
---
 drivers/mmc/host/dw_mmc.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
index 87e1f57..62b900f 100644
--- a/drivers/mmc/host/dw_mmc.c
+++ b/drivers/mmc/host/dw_mmc.c
@@ -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);
 

Re: [PATCH] dw_mmc: fixed regulator control

[Chris Ball]

Hi Jaehoon,

On Fri, Apr 29 2011, Jaehoon Chung wrote:
> 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.
[..]
> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
> index 87e1f57..62b900f 100644
> --- a/drivers/mmc/host/dw_mmc.c
> +++ b/drivers/mmc/host/dw_mmc.c
> @@ -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);
[..]

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

- Chris.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] dw_mmc: fixed regulator control

[Jaehoon Chung]

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

Chris Ball wrote:
> Hi Jaehoon,
> 
> On Fri, Apr 29 2011, Jaehoon Chung wrote:
>> 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.
> [..]
>> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c
>> index 87e1f57..62b900f 100644
>> --- a/drivers/mmc/host/dw_mmc.c
>> +++ b/drivers/mmc/host/dw_mmc.c
>> @@ -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);
> [..]
> 
> As Lars pointed out, this doesn't make sense; without CONFIG_REGULATOR
> host->vmmc becomes NULL, which isn't IS_ERR.
> 
> - Chris.

Re: [PATCH] dw_mmc: fixed regulator control

[Chris Ball]

Hi Jaehoon,

On Mon, May 02 2011, Jaehoon Chung wrote:
> 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?

> 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.
-- 
Chris Ball   <cjb@laptop.org>   <http://printf.net/>
One Laptop Per Child

Re: [PATCH] dw_mmc: fixed regulator control

[Jaehoon Chung]

Chris Ball wrote:
> Hi Jaehoon,
> 
> On Mon, May 02 2011, Jaehoon Chung wrote:
>> 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)

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