[PATCH 0/4] twl4030_charger improvements.

[PATCH 0/4] twl4030_charger improvements.

[NeilBrown]

Hi,
 this series is a resend of a previous submission with some
 contentious patches removed and extra maintainers added.

 They allow the twl4030 to charge the backup battery (voltage and
 current configured in board file) and allow the charger to continue
 charging while the USB PHY is otherwise idle (no gadget module
 loaded, or host in suspend-to-RAM).

 Anton: could you take the first three?  Do I need to split the third
   patch into an 'MFD' part for Samuel and a power_supply part for you?

 Felipe: could you take the usb/otg patch?  It can be applied quite
   independently of the others, but won't be effective until they are
   also present.

Thanks,
NeilBrown

---

NeilBrown (4):
      usb: otg: twl4030-usb: Don't power down phy when it is in-use by charger.
      power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.
      power_supply: twl4030_charger: add backup-battery charging.
      power_supply: twl4030_charger: Fix some typos


 drivers/mfd/twl-core.c          |    9 ++--
 drivers/power/twl4030_charger.c |   80 ++++++++++++++++++++++++++++++++++++++-
 drivers/usb/otg/twl4030-usb.c   |    8 +++-
 include/linux/i2c/twl.h         |    2 +
 4 files changed, 91 insertions(+), 8 deletions(-)

-- 
Signature

[PATCH 1/4] power_supply: twl4030_charger: Fix some typos

[NeilBrown]

Signed-off-by:  NeilBrown <neilb@suse.de>
---

 drivers/power/twl4030_charger.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index fdad850..3e6e991 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -103,7 +103,7 @@ static int twl4030_bci_read(u8 reg, u8 *val)
 
 static int twl4030_clear_set_boot_bci(u8 clear, u8 set)
 {
-	return twl4030_clear_set(TWL4030_MODULE_PM_MASTER, 0,
+	return twl4030_clear_set(TWL4030_MODULE_PM_MASTER, clear,
 			TWL4030_CONFIG_DONE | TWL4030_BCIAUTOWEN | set,
 			TWL4030_PM_MASTER_BOOT_BCI);
 }
@@ -151,14 +151,14 @@ static int twl4030_bci_have_vbus(struct twl4030_bci *bci)
 }
 
 /*
- * Enable/Disable USB Charge funtionality.
+ * Enable/Disable USB Charge functionality.
  */
 static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 {
 	int ret;
 
 	if (enable) {
-		/* Check for USB charger conneted */
+		/* Check for USB charger connected */
 		if (!twl4030_bci_have_vbus(bci))
 			return -ENODEV;
 

[PATCH 2/4] power_supply: twl4030_charger: add backup-battery charging.

[NeilBrown]

This allows a voltage and current (bb_uvolts and bb_uamps)
to be specified in the platform_data, and charging of the backup
battery will be enabled with those specification.

As it is not possible to monitor the backup battery at all
there is no new device created to represent it.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/power/twl4030_charger.c |   59 +++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/twl.h         |    2 +
 2 files changed, 61 insertions(+)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 3e6e991..0511610 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -28,6 +28,7 @@
 #define TWL4030_BCIVBUS		0x0c
 #define TWL4030_BCIMFSTS4	0x10
 #define TWL4030_BCICTL1		0x23
+#define TWL4030_BB_CFG		0x12
 
 #define TWL4030_BCIAUTOWEN	BIT(5)
 #define TWL4030_CONFIG_DONE	BIT(4)
@@ -37,6 +38,17 @@
 #define TWL4030_USBFASTMCHG	BIT(2)
 #define TWL4030_STS_VBUS	BIT(7)
 #define TWL4030_STS_USB_ID	BIT(2)
+#define TWL4030_BBCHEN		BIT(4)
+#define TWL4030_BBSEL_MASK	0b1100
+#define TWL4030_BBSEL_2V5	0b0000
+#define TWL4030_BBSEL_3V0	0b0100
+#define TWL4030_BBSEL_3V1	0b1000
+#define TWL4030_BBSEL_3V2	0b1100
+#define TWL4030_BBISEL_MASK	0b11
+#define TWL4030_BBISEL_25uA	0b00
+#define TWL4030_BBISEL_150uA	0b01
+#define TWL4030_BBISEL_500uA	0b10
+#define TWL4030_BBISEL_1000uA	0b11
 
 /* BCI interrupts */
 #define TWL4030_WOVF		BIT(0) /* Watchdog overflow */
@@ -202,6 +214,49 @@ static int twl4030_charger_enable_ac(bool enable)
 }
 
 /*
+ * Enable/Disable charging of Backup Battery.
+ */
+static int twl4030_charger_enable_backup(int uvolt, int uamp)
+{
+	int ret;
+	u8 flags;
+
+	if (uvolt < 2500000 ||
+	    uamp < 25) {
+		/* disable charging of backup battery */
+		ret = twl4030_clear_set(TWL4030_MODULE_PM_RECEIVER,
+					TWL4030_BBCHEN, 0, TWL4030_BB_CFG);
+		return ret;
+	}
+
+	flags = TWL4030_BBCHEN;
+	if (uvolt >= 3200000)
+		flags |= TWL4030_BBSEL_3V2;
+	else if (uvolt >= 3100000)
+		flags |= TWL4030_BBSEL_3V1;
+	else if (uvolt >= 3000000)
+		flags |= TWL4030_BBSEL_3V0;
+	else
+		flags |= TWL4030_BBSEL_2V5;
+
+	if (uamp >= 1000)
+		flags |= TWL4030_BBISEL_1000uA;
+	else if (uamp >= 500)
+		flags |= TWL4030_BBISEL_500uA;
+	else if (uamp >= 150)
+		flags |= TWL4030_BBISEL_150uA;
+	else
+		flags |= TWL4030_BBISEL_25uA;
+
+	ret = twl4030_clear_set(TWL4030_MODULE_PM_RECEIVER,
+				TWL4030_BBSEL_MASK | TWL4030_BBISEL_MASK,
+				flags,
+				TWL4030_BB_CFG);
+
+	return ret;
+}
+
+/*
  * TWL4030 CHG_PRES (AC charger presence) events
  */
 static irqreturn_t twl4030_charger_interrupt(int irq, void *arg)
@@ -424,6 +479,7 @@ static enum power_supply_property twl4030_charger_props[] = {
 static int __init twl4030_bci_probe(struct platform_device *pdev)
 {
 	struct twl4030_bci *bci;
+	struct twl4030_bci_platform_data *pdata = pdev->dev.platform_data;
 	int ret;
 	u32 reg;
 
@@ -503,6 +559,8 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 
 	twl4030_charger_enable_ac(true);
 	twl4030_charger_enable_usb(bci, true);
+	twl4030_charger_enable_backup(pdata->bb_uvolt,
+				      pdata->bb_uamp);
 
 	return 0;
 
@@ -531,6 +589,7 @@ static int __exit twl4030_bci_remove(struct platform_device *pdev)
 
 	twl4030_charger_enable_ac(false);
 	twl4030_charger_enable_usb(bci, false);
+	twl4030_charger_enable_backup(0, 0);
 
 	/* mask interrupts */
 	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 1f90de0..b526031 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -557,6 +557,8 @@ struct twl4030_clock_init_data {
 struct twl4030_bci_platform_data {
 	int *battery_tmp_tbl;
 	unsigned int tblsize;
+	int	bb_uvolt;	/* voltage to charge backup battery */
+	int	bb_uamp;	/* current for backup battery charging */
 };
 
 /* TWL4030_GPIO_MAX (18) GPIOs, with interrupts */

[PATCH 4/4] usb: otg: twl4030-usb: Don't power down phy when it is in-use by charger.

[NeilBrown]

The USB phy is used both for data transfer and to charge the battery.
If the charger is active it will hold a reference to
usb3v1.  In that case we don't want to power-down the phy.

This allows charging to continue while the device is suspended.

Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/usb/otg/twl4030-usb.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/otg/twl4030-usb.c b/drivers/usb/otg/twl4030-usb.c
index c4a86da..bd8fe9b 100644
--- a/drivers/usb/otg/twl4030-usb.c
+++ b/drivers/usb/otg/twl4030-usb.c
@@ -388,10 +388,16 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
 					(PHY_CLK_CTRL_CLOCKGATING_EN |
 						PHY_CLK_CTRL_CLK32K_EN));
 	} else {
-		__twl4030_phy_power(twl, 0);
 		regulator_disable(twl->usb1v5);
 		regulator_disable(twl->usb1v8);
 		regulator_disable(twl->usb3v1);
+		if (!regulator_is_enabled(twl->usb3v1))
+			/* no-one else is requesting this
+			 * so it is OK to power-down the
+			 * phy.  Sometimes a charger might
+			 * hold the regulator active.
+			 */
+			__twl4030_phy_power(twl, 0);
 	}
 }
 

Re: [PATCH 4/4] usb: otg: twl4030-usb: Don't power down phy when it is in-use by charger.

[Andi Shyti]

Hi,

On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> @@ -388,10 +388,16 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
>  					(PHY_CLK_CTRL_CLOCKGATING_EN |
>  						PHY_CLK_CTRL_CLK32K_EN));
>  	} else {
> -		__twl4030_phy_power(twl, 0);
>  		regulator_disable(twl->usb1v5);
>  		regulator_disable(twl->usb1v8);
>  		regulator_disable(twl->usb3v1);
> +		if (!regulator_is_enabled(twl->usb3v1))
> +			/* no-one else is requesting this
> +			 * so it is OK to power-down the
> +			 * phy.  Sometimes a charger might
> +			 * hold the regulator active.
> +			 */
> +			__twl4030_phy_power(twl, 0);
>  	}

Usually a regulator line is shared by more than one device and
regulator_is_enable() returns true if at least one of these
devices is holding the regulator. This means that here the check
will not work if this is your case.

Andi

Re: [PATCH 4/4] usb: otg: twl4030-usb: Don't power down phy when it is in-use by charger.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1541 bytes --]

On Sun, 13 May 2012 20:14:09 +0200 Andi Shyti <andi.shyti@gmail.com> wrote:

> Hi,
> 
> On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> > @@ -388,10 +388,16 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> >  					(PHY_CLK_CTRL_CLOCKGATING_EN |
> >  						PHY_CLK_CTRL_CLK32K_EN));
> >  	} else {
> > -		__twl4030_phy_power(twl, 0);
> >  		regulator_disable(twl->usb1v5);
> >  		regulator_disable(twl->usb1v8);
> >  		regulator_disable(twl->usb3v1);
> > +		if (!regulator_is_enabled(twl->usb3v1))
> > +			/* no-one else is requesting this
> > +			 * so it is OK to power-down the
> > +			 * phy.  Sometimes a charger might
> > +			 * hold the regulator active.
> > +			 */
> > +			__twl4030_phy_power(twl, 0);
> >  	}
> 
> Usually a regulator line is shared by more than one device and
> regulator_is_enable() returns true if at least one of these
> devices is holding the regulator. This means that here the check
> will not work if this is your case.
> 
> Andi

This regulator is inside an MFD and it only feeds a very limited number of
devices within that MFD.  So I don't think there is much room for confusion.

However is it a somewhat indirect method of signalling.  I want the charger
to be able to tell the USB controller that it is using the PHY so please
don't turn it off.  Doing that through the regulator seems simple and
effective.
Maybe there is a better way, but it isn't immediately clear what that would
be.
Suggestions welcome.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 4/4] usb: otg: twl4030-usb: Don't power down phy when it is in-use by charger.

[Andi Shyti]

Hi,

On Fri, May 18, 2012 at 12:50:10PM +1000, NeilBrown wrote:
> On Sun, 13 May 2012 20:14:09 +0200 Andi Shyti <andi.shyti@gmail.com> wrote:
> > On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> > > @@ -388,10 +388,16 @@ static void twl4030_phy_power(struct twl4030_usb *twl, int on)
> > >  					(PHY_CLK_CTRL_CLOCKGATING_EN |
> > >  						PHY_CLK_CTRL_CLK32K_EN));
> > >  	} else {
> > > -		__twl4030_phy_power(twl, 0);
> > >  		regulator_disable(twl->usb1v5);
> > >  		regulator_disable(twl->usb1v8);
> > >  		regulator_disable(twl->usb3v1);
> > > +		if (!regulator_is_enabled(twl->usb3v1))
> > > +			/* no-one else is requesting this
> > > +			 * so it is OK to power-down the
> > > +			 * phy.  Sometimes a charger might
> > > +			 * hold the regulator active.
> > > +			 */
> > > +			__twl4030_phy_power(twl, 0);
> > >  	}
> > 
> > Usually a regulator line is shared by more than one device and
> > regulator_is_enable() returns true if at least one of these
> > devices is holding the regulator. This means that here the check
> > will not work if this is your case.
> > 
> > Andi
> 
> This regulator is inside an MFD and it only feeds a very limited number of
> devices within that MFD.  So I don't think there is much room for confusion.
> 
> However is it a somewhat indirect method of signalling.  I want the charger
> to be able to tell the USB controller that it is using the PHY so please
> don't turn it off.  Doing that through the regulator seems simple and
> effective.
> Maybe there is a better way, but it isn't immediately clear what that would
> be.
> Suggestions welcome.

When using regulators I would keep track of his status internally
in the driver... 'true' if on or 'false' if of, in this way there
is no room for confusion.
But the mine is just a suggestion, you know the environment
better

Andi

[PATCH 3/4] power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.

[NeilBrown]

The charger needs usb3v1 to be running, so add a new consumer to
keep it running.

This allows the charger to draw current even when the USB driver has
powered down.

Acked-by: Tero Kristo <t-kristo@ti.com>
Signed-off-by: NeilBrown <neilb@suse.de>
---

 drivers/mfd/twl-core.c          |    9 +++++----
 drivers/power/twl4030_charger.c |   15 +++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 7c2267e..4cbf285 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -723,8 +723,9 @@ add_children(struct twl4030_platform_data *pdata, unsigned irq_base,
 		static struct regulator_consumer_supply usb1v8 = {
 			.supply =	"usb1v8",
 		};
-		static struct regulator_consumer_supply usb3v1 = {
-			.supply =	"usb3v1",
+		static struct regulator_consumer_supply usb3v1[] = {
+			{ .supply =	"usb3v1" },
+			{ .supply =	"bci3v1" },
 		};
 
 	/* First add the regulators so that they can be used by transceiver */
@@ -752,7 +753,7 @@ add_children(struct twl4030_platform_data *pdata, unsigned irq_base,
 				return PTR_ERR(child);
 
 			child = add_regulator_linked(TWL4030_REG_VUSB3V1,
-						      &usb_fixed, &usb3v1, 1,
+						      &usb_fixed, usb3v1, 2,
 						      features);
 			if (IS_ERR(child))
 				return PTR_ERR(child);
@@ -773,7 +774,7 @@ add_children(struct twl4030_platform_data *pdata, unsigned irq_base,
 		if (twl_has_regulator() && child) {
 			usb1v5.dev_name = dev_name(child);
 			usb1v8.dev_name = dev_name(child);
-			usb3v1.dev_name = dev_name(child);
+			usb3v1[0].dev_name = dev_name(child);
 		}
 	}
 	if (twl_has_usb() && pdata->usb && twl_class_is_6030()) {
diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 0511610..bbda083 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -21,6 +21,7 @@
 #include <linux/power_supply.h>
 #include <linux/notifier.h>
 #include <linux/usb/otg.h>
+#include <linux/regulator/machine.h>
 
 #define TWL4030_BCIMSTATEC	0x02
 #define TWL4030_BCIICHG		0x08
@@ -86,6 +87,8 @@ struct twl4030_bci {
 	struct work_struct	work;
 	int			irq_chg;
 	int			irq_bci;
+	struct regulator	*usb_reg;
+	int			usb_enabled;
 
 	unsigned long		event;
 };
@@ -183,6 +186,12 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 			return -EACCES;
 		}
 
+		/* Need to keep regulator on */
+		if (!bci->usb_enabled) {
+			regulator_enable(bci->usb_reg);
+			bci->usb_enabled = 1;
+		}
+
 		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
 		ret = twl4030_clear_set_boot_bci(0, TWL4030_BCIAUTOUSB);
 		if (ret < 0)
@@ -193,6 +202,10 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 			TWL4030_USBFASTMCHG, TWL4030_BCIMFSTS4);
 	} else {
 		ret = twl4030_clear_set_boot_bci(TWL4030_BCIAUTOUSB, 0);
+		if (bci->usb_enabled) {
+			regulator_disable(bci->usb_reg);
+			bci->usb_enabled = 0;
+		}
 	}
 
 	return ret;
@@ -511,6 +524,8 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 	bci->usb.num_properties = ARRAY_SIZE(twl4030_charger_props);
 	bci->usb.get_property = twl4030_bci_get_property;
 
+	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
+
 	ret = power_supply_register(&pdev->dev, &bci->usb);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register usb: %d\n", ret);

Re: [PATCH 3/4] power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.

[Samuel Ortiz]

Hi Neil,

On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> The charger needs usb3v1 to be running, so add a new consumer to
> keep it running.
> 
> This allows the charger to draw current even when the USB driver has
> powered down.
> 
> Acked-by: Tero Kristo <t-kristo@ti.com>
> Signed-off-by: NeilBrown <neilb@suse.de>
Acked-by: Samuel Ortiz <sameo@linux.intel.com>

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 3/4] power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.

[Andi Shyti]

Hi Neil,

On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> @@ -183,6 +186,12 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
>  			return -EACCES;
>  		}
>  
> +		/* Need to keep regulator on */
> +		if (!bci->usb_enabled) {
> +			regulator_enable(bci->usb_reg);

Should you consider here a failure case?

> +			bci->usb_enabled = 1;
> +		}
> +
>  		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
>  		ret = twl4030_clear_set_boot_bci(0, TWL4030_BCIAUTOUSB);
>  		if (ret < 0)
> @@ -511,6 +524,8 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
>  	bci->usb.num_properties = ARRAY_SIZE(twl4030_charger_props);
>  	bci->usb.get_property = twl4030_bci_get_property;
>  
> +	bci->usb_reg = regulator_get(bci->dev, "bci3v1");

... and here.

Andi

Re: [PATCH 3/4] power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

On Sun, 13 May 2012 20:12:08 +0200 Andi Shyti <andi.shyti@gmail.com> wrote:

> Hi Neil,
> 
> On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> > @@ -183,6 +186,12 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
> >  			return -EACCES;
> >  		}
> >  
> > +		/* Need to keep regulator on */
> > +		if (!bci->usb_enabled) {
> > +			regulator_enable(bci->usb_reg);
> 
> Should you consider here a failure case?

Probably.  Something like this?

+		/* Need to keep regulator on */
+		if (!bci->usb_enabled &&
+		    bci->usb_reg &&
+		    regulator_enable(bci->usb_reg) == 0)
+			bci->usb_enabled = 1;
+

> 
> > +			bci->usb_enabled = 1;
> > +		}
> > +
> >  		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
> >  		ret = twl4030_clear_set_boot_bci(0, TWL4030_BCIAUTOUSB);
> >  		if (ret < 0)
> > @@ -511,6 +524,8 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
> >  	bci->usb.num_properties = ARRAY_SIZE(twl4030_charger_props);
> >  	bci->usb.get_property = twl4030_bci_get_property;
> >  
> > +	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
> 
> ... and here.

In what circumstances can that fail, I wonder.
Still, maybe this is best:


+	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
+	if (IS_ERR(bci->usb_reg)) {
+		dev_warn(&bci->dev, "regulator get bci3v1 failed\n");
+		bci->usb_reg = NULL;
+	}
+

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 3/4] power_supply/MFD: twl4030_charger: Allow charger to control the regulator that feeds it.

[Andi Shyti]

Hi Neil,

On Fri, May 18, 2012 at 12:46:51PM +1000, NeilBrown wrote:
> On Sun, 13 May 2012 20:12:08 +0200 Andi Shyti <andi.shyti@gmail.com> wrote:
> 
> > Hi Neil,
> > 
> > On Wed, May 09, 2012 at 07:40:40AM +1000, NeilBrown wrote:
> > > @@ -183,6 +186,12 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
> > >  			return -EACCES;
> > >  		}
> > >  
> > > +		/* Need to keep regulator on */
> > > +		if (!bci->usb_enabled) {
> > > +			regulator_enable(bci->usb_reg);
> > 
> > Should you consider here a failure case?
> 
> Probably.  Something like this?
> 
> +		/* Need to keep regulator on */
> +		if (!bci->usb_enabled &&
> +		    bci->usb_reg &&
> +		    regulator_enable(bci->usb_reg) == 0)
> +			bci->usb_enabled = 1;
> +

maybe you could write it a bit nicer :)

> 
> > 
> > > +			bci->usb_enabled = 1;
> > > +		}
> > > +
> > >  		/* forcing the field BCIAUTOUSB (BOOT_BCI[1]) to 1 */
> > >  		ret = twl4030_clear_set_boot_bci(0, TWL4030_BCIAUTOUSB);
> > >  		if (ret < 0)
> > > @@ -511,6 +524,8 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
> > >  	bci->usb.num_properties = ARRAY_SIZE(twl4030_charger_props);
> > >  	bci->usb.get_property = twl4030_bci_get_property;
> > >  
> > > +	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
> > 
> > ... and here.
> 
> In what circumstances can that fail, I wonder.
> Still, maybe this is best:
> 
> 
> +	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
> +	if (IS_ERR(bci->usb_reg)) {
> +		dev_warn(&bci->dev, "regulator get bci3v1 failed\n");
> +		bci->usb_reg = NULL;
> +	}
> +

Yep!

Andi

Re: [PATCH 0/4] twl4030_charger improvements.

[Samuel Ortiz]

Hi Neil,

On Wed, May 09, 2012 at 07:40:39AM +1000, NeilBrown wrote:
> Hi,
>  this series is a resend of a previous submission with some
>  contentious patches removed and extra maintainers added.
> 
>  They allow the twl4030 to charge the backup battery (voltage and
>  current configured in board file) and allow the charger to continue
>  charging while the USB PHY is otherwise idle (no gadget module
>  loaded, or host in suspend-to-RAM).
> 
>  Anton: could you take the first three?  Do I need to split the third
>    patch into an 'MFD' part for Samuel and a power_supply part for you?
I'm fine with Anton taking patch #3 as it is.

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

Re: [PATCH 0/4] twl4030_charger improvements.

[Anton Vorontsov]

On Wed, May 09, 2012 at 07:40:39AM +1000, NeilBrown wrote:
> Hi,
>  this series is a resend of a previous submission with some
>  contentious patches removed and extra maintainers added.
> 
>  They allow the twl4030 to charge the backup battery (voltage and
>  current configured in board file) and allow the charger to continue
>  charging while the USB PHY is otherwise idle (no gadget module
>  loaded, or host in suspend-to-RAM).
> 
>  Anton: could you take the first three?  Do I need to split the third
>    patch into an 'MFD' part for Samuel and a power_supply part for you?

Applied the three patches (w/ Samuel's Ack).

Though, please consider addressing Andi's comments (as follow-up
patches).

Thank you guys!

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com