| Message ID | 200902081053.00255.david-b@pacbell.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Felipe Balbi |
| Headers | show |
On Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > when the USB cable is unplugged, to eliminate that source of power > waste. (Enabled LDOs consume power at all times.) Please put this one on hold. A bug has been discovered here. If the twl-regulator is not compiled in, the twl4030-usb fails while trying to access the reg-fw interfaces (and not checking error values). Also it's bit confusing how we get different error values if reg_fw is not compiled (no error) vs. reg_fw is compiled, but twl_regulator is not... (error) Also problems with boot order were discovered. I'll post a set that fixes this soon. - Kalle > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > --- > Depends on the twl4030 regulator driver, so I'm suggesting this > be merged (with that driver) through the regulator patch queue > to simplify things. > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > --- a/drivers/usb/otg/twl4030-usb.c > +++ b/drivers/usb/otg/twl4030-usb.c > @@ -34,6 +34,7 @@ > #include <linux/delay.h> > #include <linux/usb/otg.h> > #include <linux/i2c/twl4030.h> > +#include <linux/regulator/consumer.h> > > > /* Register defines */ > @@ -246,6 +247,11 @@ struct twl4030_usb { > struct otg_transceiver otg; > struct device *dev; > > + /* TWL4030 internal USB regulator supplies */ > + struct regulator *usb1v5; > + struct regulator *usb1v8; > + struct regulator *usb3v1; > + > /* for vbus reporting with irqs disabled */ > spinlock_t lock; > > @@ -434,6 +440,9 @@ static void twl4030_phy_power(struct twl > > pwr = twl4030_usb_read(twl, PHY_PWR_CTRL); > if (on) { > + regulator_enable(twl->usb1v5); > + regulator_enable(twl->usb1v8); > + regulator_enable(twl->usb3v1); > pwr &= ~PHY_PWR_PHYPWD; > WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); > twl4030_usb_write(twl, PHY_CLK_CTRL, > @@ -443,6 +452,9 @@ static void twl4030_phy_power(struct twl > } else { > pwr |= PHY_PWR_PHYPWD; > WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); > + regulator_disable(twl->usb1v5); > + regulator_disable(twl->usb1v8); > + regulator_disable(twl->usb3v1); > } > } > > @@ -480,16 +492,19 @@ static void twl4030_usb_ldo_init(struct > /* input to VUSB3V1 LDO is from VBAT, not VBUS */ > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x14, VUSB_DEDICATED1); > > - /* turn on 3.1V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB3V1_DEV_GRP); > + /* Initialize 3.1V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_DEV_GRP); > + twl->usb3v1 = regulator_get(twl->dev, "usb3v1"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_TYPE); > > - /* turn on 1.5V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V5_DEV_GRP); > + /* Initialize 1.5V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_DEV_GRP); > + twl->usb1v5 = regulator_get(twl->dev, "usb1v5"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_TYPE); > > - /* turn on 1.8V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V8_DEV_GRP); > + /* Initialize 1.8V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_DEV_GRP); > + twl->usb1v8 = regulator_get(twl->dev, "usb1v8"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_TYPE); > > /* disable access to power configuration registers */ > @@ -688,6 +703,9 @@ static int __exit twl4030_usb_remove(str > twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); > > twl4030_phy_power(twl, 0); > + regulator_put(twl->usb1v5); > + regulator_put(twl->usb1v8); > + regulator_put(twl->usb3v1); > > kfree(twl); > -- 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 Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > when the USB cable is unplugged, to eliminate that source of power > waste. (Enabled LDOs consume power at all times.) Please put this on hold for a while. This won't work if we don't have TWL regulator support enabled. Also it's missing error value checking. I'll post new patch(es) soon. - Kalle > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > --- > Depends on the twl4030 regulator driver, so I'm suggesting this > be merged (with that driver) through the regulator patch queue > to simplify things. > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > --- a/drivers/usb/otg/twl4030-usb.c > +++ b/drivers/usb/otg/twl4030-usb.c > @@ -34,6 +34,7 @@ > #include <linux/delay.h> > #include <linux/usb/otg.h> > #include <linux/i2c/twl4030.h> > +#include <linux/regulator/consumer.h> > > > /* Register defines */ > @@ -246,6 +247,11 @@ struct twl4030_usb { > struct otg_transceiver otg; > struct device *dev; > > + /* TWL4030 internal USB regulator supplies */ > + struct regulator *usb1v5; > + struct regulator *usb1v8; > + struct regulator *usb3v1; > + > /* for vbus reporting with irqs disabled */ > spinlock_t lock; > > @@ -434,6 +440,9 @@ static void twl4030_phy_power(struct twl > > pwr = twl4030_usb_read(twl, PHY_PWR_CTRL); > if (on) { > + regulator_enable(twl->usb1v5); > + regulator_enable(twl->usb1v8); > + regulator_enable(twl->usb3v1); > pwr &= ~PHY_PWR_PHYPWD; > WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); > twl4030_usb_write(twl, PHY_CLK_CTRL, > @@ -443,6 +452,9 @@ static void twl4030_phy_power(struct twl > } else { > pwr |= PHY_PWR_PHYPWD; > WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); > + regulator_disable(twl->usb1v5); > + regulator_disable(twl->usb1v8); > + regulator_disable(twl->usb3v1); > } > } > > @@ -480,16 +492,19 @@ static void twl4030_usb_ldo_init(struct > /* input to VUSB3V1 LDO is from VBAT, not VBUS */ > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x14, VUSB_DEDICATED1); > > - /* turn on 3.1V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB3V1_DEV_GRP); > + /* Initialize 3.1V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_DEV_GRP); > + twl->usb3v1 = regulator_get(twl->dev, "usb3v1"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_TYPE); > > - /* turn on 1.5V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V5_DEV_GRP); > + /* Initialize 1.5V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_DEV_GRP); > + twl->usb1v5 = regulator_get(twl->dev, "usb1v5"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_TYPE); > > - /* turn on 1.8V regulator */ > - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V8_DEV_GRP); > + /* Initialize 1.8V regulator */ > + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_DEV_GRP); > + twl->usb1v8 = regulator_get(twl->dev, "usb1v8"); > twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_TYPE); > > /* disable access to power configuration registers */ > @@ -688,6 +703,9 @@ static int __exit twl4030_usb_remove(str > twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); > > twl4030_phy_power(twl, 0); > + regulator_put(twl->usb1v5); > + regulator_put(twl->usb1v8); > + regulator_put(twl->usb3v1); > > kfree(twl); > -- 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 Tue, Feb 10, 2009 at 03:08:14PM +0200, Kalle Jokiniemi wrote: > A bug has been discovered here. If the twl-regulator is not compiled in, > the twl4030-usb fails while trying to access the reg-fw interfaces (and reg-fw is the regulator framework? > not checking error values). Also it's bit confusing how we get different > error values if reg_fw is not compiled (no error) vs. reg_fw is > compiled, but twl_regulator is not... (error) The general model is that if you've got regulator API support enabled then you'll have the relevant regulator drivers enabled. The support for compiling out the core is intended to allow configurations that don't use regulators at all. This is a bit restrictive but works well for most users. -- 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 10 February 2009, Kalle Jokiniemi wrote: > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > > when the USB cable is unplugged, to eliminate that source of power > > waste. (Enabled LDOs consume power at all times.) > > Please put this on hold for a while. This won't work if we don't have > TWL regulator support enabled. So Kconfig should have TWL4030_USB depend on REGULATOR_TWL4030? > Also it's missing error value checking. You mean like in the ldo_init() stuff? Fair enough. - Dave > I'll post new patch(es) soon. > -- 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 Tue, 2009-02-10 at 11:52 -0800, David Brownell wrote: > On Tuesday 10 February 2009, Kalle Jokiniemi wrote: > > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > > > when the USB cable is unplugged, to eliminate that source of power > > > waste. (Enabled LDOs consume power at all times.) > > > > Please put this on hold for a while. This won't work if we don't have > > TWL regulator support enabled. > > So Kconfig should have TWL4030_USB depend on REGULATOR_TWL4030? Yes. > > > > Also it's missing error value checking. > > You mean like in the ldo_init() stuff? Fair enough. I mean, if regulator_get fails. - Kalle > > - Dave > > > > I'll post new patch(es) soon. > > > -- 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 Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > when the USB cable is unplugged, to eliminate that source of power > waste. (Enabled LDOs consume power at all times.) > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > --- > Depends on the twl4030 regulator driver, so I'm suggesting this > be merged (with that driver) through the regulator patch queue > to simplify things. > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) Applied. Thanks Liam -- 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 Thursday 26 February 2009, Liam Girdwood wrote: > On Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > > when the USB cable is unplugged, to eliminate that source of power > > waste. (Enabled LDOs consume power at all times.) > > > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > > --- > > Depends on the twl4030 regulator driver, so I'm suggesting this > > be merged (with that driver) through the regulator patch queue > > to simplify things. > > > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > > 1 file changed, 24 insertions(+), 6 deletions(-) > > Applied. Better suggestion: grab Kalle's updated patch from Greg's USB queue: http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-04-usb/ patch name usb-twl-disable-vusb-regulators-when-cable-unplugged.patch - 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 Thu, 2009-02-26 at 14:40 -0800, David Brownell wrote: > On Thursday 26 February 2009, Liam Girdwood wrote: > > On Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > > > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > > > > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > > > when the USB cable is unplugged, to eliminate that source of power > > > waste. (Enabled LDOs consume power at all times.) > > > > > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > > > --- > > > Depends on the twl4030 regulator driver, so I'm suggesting this > > > be merged (with that driver) through the regulator patch queue > > > to simplify things. > > > > > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > Applied. > > Better suggestion: grab Kalle's updated patch from Greg's USB > queue: > > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-04-usb/ > > patch name usb-twl-disable-vusb-regulators-when-cable-unplugged.patch Yep, this previous one lacked some error checks and a build dependency to TWL_REGULATOR. - Kalle > > - 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, 2009-02-27 at 08:19 +0200, Kalle Jokiniemi wrote: > On Thu, 2009-02-26 at 14:40 -0800, David Brownell wrote: > > On Thursday 26 February 2009, Liam Girdwood wrote: > > > On Sun, 2009-02-08 at 10:52 -0800, David Brownell wrote: > > > > From: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > > > > > > > This patch disables LDO regulators VUSB1V5, VUSB1V8, and VUSB3V1 > > > > when the USB cable is unplugged, to eliminate that source of power > > > > waste. (Enabled LDOs consume power at all times.) > > > > > > > > Signed-off-by: Kalle Jokiniemi <kalle.jokiniemi@digia.com> > > > > Signed-off-by: David Brownell <dbrownell@users.sourceforge.net> > > > > --- > > > > Depends on the twl4030 regulator driver, so I'm suggesting this > > > > be merged (with that driver) through the regulator patch queue > > > > to simplify things. > > > > > > > > drivers/usb/otg/twl4030-usb.c | 30 ++++++++++++++++++++++++------ > > > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > > > Applied. > > > > Better suggestion: grab Kalle's updated patch from Greg's USB > > queue: > > > > http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-04-usb/ > > > > patch name usb-twl-disable-vusb-regulators-when-cable-unplugged.patch > > > Yep, this previous one lacked some error checks and a build dependency > to TWL_REGULATOR. I've now removed this patch from the regulator tree. I guess Greg will take care of it via the USB tree now. Thanks Liam -- 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/drivers/usb/otg/twl4030-usb.c +++ b/drivers/usb/otg/twl4030-usb.c @@ -34,6 +34,7 @@ #include <linux/delay.h> #include <linux/usb/otg.h> #include <linux/i2c/twl4030.h> +#include <linux/regulator/consumer.h> /* Register defines */ @@ -246,6 +247,11 @@ struct twl4030_usb { struct otg_transceiver otg; struct device *dev; + /* TWL4030 internal USB regulator supplies */ + struct regulator *usb1v5; + struct regulator *usb1v8; + struct regulator *usb3v1; + /* for vbus reporting with irqs disabled */ spinlock_t lock; @@ -434,6 +440,9 @@ static void twl4030_phy_power(struct twl pwr = twl4030_usb_read(twl, PHY_PWR_CTRL); if (on) { + regulator_enable(twl->usb1v5); + regulator_enable(twl->usb1v8); + regulator_enable(twl->usb3v1); pwr &= ~PHY_PWR_PHYPWD; WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); twl4030_usb_write(twl, PHY_CLK_CTRL, @@ -443,6 +452,9 @@ static void twl4030_phy_power(struct twl } else { pwr |= PHY_PWR_PHYPWD; WARN_ON(twl4030_usb_write_verify(twl, PHY_PWR_CTRL, pwr) < 0); + regulator_disable(twl->usb1v5); + regulator_disable(twl->usb1v8); + regulator_disable(twl->usb3v1); } } @@ -480,16 +492,19 @@ static void twl4030_usb_ldo_init(struct /* input to VUSB3V1 LDO is from VBAT, not VBUS */ twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x14, VUSB_DEDICATED1); - /* turn on 3.1V regulator */ - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB3V1_DEV_GRP); + /* Initialize 3.1V regulator */ + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_DEV_GRP); + twl->usb3v1 = regulator_get(twl->dev, "usb3v1"); twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB3V1_TYPE); - /* turn on 1.5V regulator */ - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V5_DEV_GRP); + /* Initialize 1.5V regulator */ + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_DEV_GRP); + twl->usb1v5 = regulator_get(twl->dev, "usb1v5"); twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V5_TYPE); - /* turn on 1.8V regulator */ - twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0x20, VUSB1V8_DEV_GRP); + /* Initialize 1.8V regulator */ + twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_DEV_GRP); + twl->usb1v8 = regulator_get(twl->dev, "usb1v8"); twl4030_i2c_write_u8(TWL4030_MODULE_PM_RECEIVER, 0, VUSB1V8_TYPE); /* disable access to power configuration registers */ @@ -688,6 +703,9 @@ static int __exit twl4030_usb_remove(str twl4030_usb_clear_bits(twl, POWER_CTRL, POWER_CTRL_OTG_ENAB); twl4030_phy_power(twl, 0); + regulator_put(twl->usb1v5); + regulator_put(twl->usb1v8); + regulator_put(twl->usb3v1); kfree(twl);