| Message ID | 200902231014.26352.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | b69837a59461d6f7ca557d978d02734bcdb60f35 |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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,