[patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Resolve longstanding issue noted by Adrian Hunter:  confusion
between settting VSEL=0 (which is 1.8V on MMC1) and poweroff.

Also, leave VSEL alone if we're just powering the regulator off.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
NOTE: won't apply to current mainline GIT.

 arch/arm/mach-omap2/mmc-twl4030.c |   18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

--- a/arch/arm/mach-omap2/mmc-twl4030.c
+++ b/arch/arm/mach-omap2/mmc-twl4030.c
@@ -178,7 +178,10 @@ static int twl_mmc_resume(struct device 
 static int twl_mmc_set_voltage(struct twl_mmc_controller *c, int vdd)
 {
 	int ret;
-	u8 vmmc, dev_grp_val;
+	u8 vmmc = 0, dev_grp_val;
+
+	if (!vdd)
+		goto doit;
 
 	if (c->twl_vmmc_dev_grp == VMMC1_DEV_GRP) {
 		/* VMMC1:  max 220 mA.  And for 8-bit mode,
@@ -203,8 +206,7 @@ static int twl_mmc_set_voltage(struct tw
 			/* error if VSIM needed */
 			break;
 		default:
-			vmmc = 0;
-			break;
+			return -EINVAL;
 		}
 	} else if (c->twl_vmmc_dev_grp == VMMC2_DEV_GRP) {
 		/* VMMC2:  max 100 mA */
@@ -230,21 +232,21 @@ static int twl_mmc_set_voltage(struct tw
 			vmmc = VMMC2_315V;
 			break;
 		default:
-			vmmc = 0;
-			break;
+			return -EINVAL;
 		}
 	} else {
-		return 0;
+		return -EINVAL;
 	}
 
-	if (vmmc)
+doit:
+	if (vdd)
 		dev_grp_val = VMMC_DEV_GRP_P1;	/* Power up */
 	else
 		dev_grp_val = LDO_CLR;		/* Power down */
 
 	ret = twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER,
 					dev_grp_val, c->twl_vmmc_dev_grp);
-	if (ret)
+	if (ret || !vdd)
 		return ret;
 
 	ret = twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER,

Re: [patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[Adrian Hunter]

David Brownell wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
> 
> Resolve longstanding issue noted by Adrian Hunter:  confusion
> between settting VSEL=0 (which is 1.8V on MMC1) and poweroff.
> 
> Also, leave VSEL alone if we're just powering the regulator off.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---

Looks fine to me, but I wonder if you know why the regulator is
turned on (DEV_GRP) and then the voltage is selected (DEDICATED).
Wouldn't it make more sense the other way around?  Doesn't
the new regulator framework do it that way?

Re: [patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[David Brownell]

On Tuesday 24 February 2009, Adrian Hunter wrote:
> David Brownell wrote:
> > From: David Brownell <dbrownell@users.sourceforge.net>
> > 
> > Resolve longstanding issue noted by Adrian Hunter:  confusion
> > between settting VSEL=0 (which is 1.8V on MMC1) and poweroff.
> > 
> > Also, leave VSEL alone if we're just powering the regulator off.
> > 
> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> > ---
> 
> Looks fine to me, but I wonder if you know why the regulator is
> turned on (DEV_GRP) and then the voltage is selected (DEDICATED).

History, I'd guess.  I seem to remember some sequencing
constraint, but couldn't find it when last I sought it
out in the reference manual.


> Wouldn't it make more sense the other way around?

I had the same thought.  Eventually I'd like this code
to mostly wither away ... using the regulator framework
for the hsmmc driver will eliminate most of the remaining
bits of twl4030-specific code.  So I'd just leave it be
for the moment.


> Doesn't the new regulator framework do it that way?

No; setting voltage and enabling are distinct operations.

Re: [patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[Adrian Hunter]

David Brownell wrote:
> On Tuesday 24 February 2009, Adrian Hunter wrote:
>> David Brownell wrote:
>>> From: David Brownell <dbrownell@users.sourceforge.net>
>>>
>>> Resolve longstanding issue noted by Adrian Hunter:  confusion
>>> between settting VSEL=0 (which is 1.8V on MMC1) and poweroff.
>>>
>>> Also, leave VSEL alone if we're just powering the regulator off.
>>>
>>> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
>>> ---
>> Looks fine to me, but I wonder if you know why the regulator is
>> turned on (DEV_GRP) and then the voltage is selected (DEDICATED).
> 
> History, I'd guess.  I seem to remember some sequencing
> constraint, but couldn't find it when last I sought it
> out in the reference manual.

What about section 5.5.1 item 6?

>> Wouldn't it make more sense the other way around?
> 
> I had the same thought.  Eventually I'd like this code
> to mostly wither away ... using the regulator framework
> for the hsmmc driver will eliminate most of the remaining
> bits of twl4030-specific code.  So I'd just leave it be
> for the moment.
> 
> 
>> Doesn't the new regulator framework do it that way?
> 
> No; setting voltage and enabling are distinct operations.

Perhaps, but there are features like regulator constraints,
or the Virtual regulator that do set the voltage and then
enable.

Re: [patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[David Brownell]

> >> Looks fine to me, but I wonder if you know why the regulator is
> >> turned on (DEV_GRP) and then the voltage is selected (DEDICATED).
> > 
> > History, I'd guess.  I seem to remember some sequencing
> > constraint, but couldn't find it when last I sought it
> > out in the reference manual.
> 
> What about section 5.5.1 item 6?

That's an example.  I'm thinking the constraint had
to do with one or more of the USB regulators.  It just
doesn't make sense to have a *general* rule that the
resource needs to be enabled before tweaking.  Maybe
it was an erratum or something.


> >> Wouldn't it make more sense the other way around?
> > 
> > I had the same thought.  Eventually I'd like this code
> > to mostly wither away ... using the regulator framework
> > for the hsmmc driver will eliminate most of the remaining
> > bits of twl4030-specific code.  So I'd just leave it be
> > for the moment.
> > 
> > 
> >> Doesn't the new regulator framework do it that way?
> > 
> > No; setting voltage and enabling are distinct operations.
> 
> Perhaps, but there are features like regulator constraints,
> or the Virtual regulator that do set the voltage and then
> enable.

Right, and I have most certainly configured regulators
with regulator_init_data.constraints.apply_uV so that
the VSEL gets set to a sane value as Linux sets up,
since u-boot wasn't touching it.

- Dave

[APPLIED] [patch 2.6.29-rc5-omap-git] mach-omap2/mmc-twl4030: fix for vmmc = 0

[Tony Lindgren]

This patch has been applied to the linux-omap
by youw fwiendly patch wobot.

Commit: b69837a59461d6f7ca557d978d02734bcdb60f35

PatchWorks
http://patchwork.kernel.org/patch/8443/

Git
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=b69837a59461d6f7ca557d978d02734bcdb60f35