| Message ID | 1238440539-2394-1-git-send-email-broonie@sirena.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e8acebfbfd355bc52a6a18c173c400f15cb8e4a9 |
| Headers | show |
On Monday 30 March 2009, Mark Brown wrote: > The approach that's being taken by the mmc-twl4030 driver to disabling > regulators is a normal and supported one So when are you going to fix the regulator docs to report that: ALL regulator consumers must start by enabling and then disabling the regulator. The regulator framework is still broken in this respect. (Not that you want to hear that, but it's still true.) -- 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 Mon, Mar 30, 2009 at 01:53:43PM -0700, David Brownell wrote: > So when are you going to fix the regulator docs to report that: > ALL regulator consumers must start by enabling and > then disabling the regulator. The documention should not be changed to say that since only consumers that need the regulator to be off at startup should do this, and then probably only if they find that it is already enabled. Other consumers do not need to do this. Consumers that want to enable a regulator at startup can do so directly. Consumers that don't need a specific state (for example, because they are able to share the regulator and don't need it enabling) should just leave it alone. -- 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. Initial commit ID (Likely to change): e8acebfbfd355bc52a6a18c173c400f15cb8e4a9 PatchWorks http://patchwork.kernel.org/patch/15244/ Git http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap-2.6.git;a=commit;h=e8acebfbfd355bc52a6a18c173c400f15cb8e4a9 -- 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
* Mark Brown <broonie@sirena.org.uk> [090331 14:00]: > On Mon, Mar 30, 2009 at 01:53:43PM -0700, David Brownell wrote: > > > So when are you going to fix the regulator docs to report that: > > > ALL regulator consumers must start by enabling and > > then disabling the regulator. > > The documention should not be changed to say that since only consumers > that need the regulator to be off at startup should do this, and then > probably only if they find that it is already enabled. > > Other consumers do not need to do this. Consumers that want to enable a > regulator at startup can do so directly. Consumers that don't need a > specific state (for example, because they are able to share the > regulator and don't need it enabling) should just leave it alone. Maybe you guys have gone back and forth on this option too.. But what if we have something in regulator_init_data that would tell the regulator to reset the regulator on init? That could be then be then disabled with some cmdline option if needed for debugging or while booting from other operating systems etc. I've applied Mark's patch to l-o, I guess now I need to start sorting out what we need to mainline for twl4030-mmc.c for this -rc :) Regards, Tony -- 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 Fri, Apr 03, 2009 at 05:26:52PM -0700, Tony Lindgren wrote: > Maybe you guys have gone back and forth on this option too.. But what > if we have something in regulator_init_data that would tell the > regulator to reset the regulator on init? That could be then be > then disabled with some cmdline option if needed for debugging or > while booting from other operating systems etc. Yup, that works perfectly well - in fact, Jonathan Cameron has a patch adding a new constraint doing exactly that which managed ought to have been merged but ended up falling through the cracks for 2.6.30. I'm expecting him to do the required refresh early on in this cycle. Since it's an explicit constraint we can just go ahead and apply it without needing an ability to override it. There's also support for powering off any unused regulators in a late_initcall() which did get merged but that doesn't help with things like MMC since they'll be probing before late_initcall(). -- 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 31 March 2009, Mark Brown wrote: > On Mon, Mar 30, 2009 at 01:53:43PM -0700, David Brownell wrote: > > > So when are you going to fix the regulator docs to report that: > > > ALL regulator consumers must start by enabling and > > then disabling the regulator. > > The documention should not be changed to say that since only consumers > that need the regulator to be off at startup should do this, and then > probably only if they find that it is already enabled. "Probably" shows you still don't have a good answer... I see that mainline now has ca7255614e0861e36480103f4a402a115803d7b5 which is a variant of the first late-fixup patch I sent. The isssue with that approach is that it leaves a huge window between regulator init and its late fixup ... during which driver probe() calls suffer the bad effects of the current self-inconsistent initialization. So it doesn't really address the MMC cases. On Friday 03 April 2009, Tony Lindgren wrote: > ... what > if we have something in regulator_init_data that would tell the > regulator to reset the regulator on init? That would address the MMC scenario, where the stack always wants to start with regulator "off" if it's possible. And in fact, that's exactly what I think folk should be doing with the recent additions to twl4030-power supporting explicit init of all the power resources ... as done with e.g. Beagle. The benefit of doing it that way is that it can cover the regulators which are not actually used on the board (like VAUX3 on Beagle, which u-boot enables but is not wired to anything) plus non-regulator resources like CLKREQ etc. So there's an OMAP-specific workaround now in place, albeit not yet headed towards mainline. - 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
On Fri, Apr 03, 2009 at 06:03:33PM -0700, David Brownell wrote: > On Tuesday 31 March 2009, Mark Brown wrote: > > The documention should not be changed to say that since only consumers > > that need the regulator to be off at startup should do this, and then > > probably only if they find that it is already enabled. > "Probably" shows you still don't have a good answer... No, it means that I can't think of any reason why they'd want to bounce the supply but I'm open to the possibility that there could be one. > I see that mainline now has ca7255614e0861e36480103f4a402a115803d7b5 > which is a variant of the first late-fixup patch I sent. The isssue > with that approach is that it leaves a huge window between regulator > init and its late fixup ... during which driver probe() calls suffer > the bad effects of the current self-inconsistent initialization. So > it doesn't really address the MMC cases. That change was not intended to do anything for MMC, it's there for other users like shared regulators and situations where no consumer driver instantiates. It is really important that you engage with the feedback you are getting. At this point we are all quite familiar with your views and we do understand them, restating them is not what's needed. Changes have to be made with an understanding of the existing interface and how it supports the use cases the API has, including users other than MMC. Changes also need to be made with consideration given to merge issues. > And in fact, that's exactly what I think folk should be doing with > the recent additions to twl4030-power supporting explicit init of > all the power resources ... as done with e.g. Beagle. The benefit ... > So there's an OMAP-specific workaround now in place, albeit not > yet headed towards mainline. With Jonathan's patch there will be support in the regulator API for doing this with all regulators. -- 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
diff --git a/arch/arm/mach-omap2/mmc-twl4030.c b/arch/arm/mach-omap2/mmc-twl4030.c index 8db1f29..b76a31a 100644 --- a/arch/arm/mach-omap2/mmc-twl4030.c +++ b/arch/arm/mach-omap2/mmc-twl4030.c @@ -136,14 +136,11 @@ static int twl_mmc_late_init(struct device *dev) * (which is safe for MMC, but not in general). */ if (regulator_is_enabled(hsmmc[i].vcc) > 0) { - dev_warn(dev, "APPLY REGULATOR HACK for vmmc\n"); regulator_enable(hsmmc[i].vcc); regulator_disable(hsmmc[i].vcc); } if (hsmmc[i].vcc_aux) { if (regulator_is_enabled(reg) > 0) { - dev_warn(dev, "APPLY REGULATOR HACK " - "for vmmc_aux\n"); regulator_enable(reg); regulator_disable(reg); }
The approach that's being taken by the mmc-twl4030 driver to disabling regulators is a normal and supported one so there is no need to print messages on the console warning about this - their system is functioning normally. Signed-off-by: Mark Brown <broonie@sirena.org.uk> --- arch/arm/mach-omap2/mmc-twl4030.c | 3 --- 1 files changed, 0 insertions(+), 3 deletions(-)