Skip to contentSkip to conversation

Enhance twl4030_charger functionality. - V3

PATCH In review NeilBrown 7 participants 41 messages Jul 30, 2015 – Nov 24, 2015

Subsystems (3)
lore
Share & tools
lore

NeilBrown

author
#0 lore raw

cover Enhance twl4030_charger functionality. - V3

Following is most of my twl4030_charger patches, rebased against
git://git.infradead.org/battery-2.6

Since the previous set I have added the conversion to module_platform_driver so EPROBE_DEFER can be used, and fixed a few minor typos.

This does not include the changes to add extcon support, in part because extcon has seen some changes lately which leave me even more confused about how best to use it than before.
I need to sort that out before I can resolve the rest of my usb phy patches and then add a few more charger patches.

Thanks,
NeilBrown

---

NeilBrown (12):
twl4030_charger: use runtime_pm to keep usb phy active while charging.
twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node
twl4030_charger: trust phy to determine when USB power is available.
twl4030_charger: split uA calculation into a function.
twl4030_charger: allow fine control of charger current.
twl4030_charger: distinguish between USB current and 'AC' current
twl4030_charger: allow max_current to be managed via sysfs.
twl4030_charger: enable manual enable/disable of usb charging.
twl4030_charger: add software controlled linear charging mode.
twl4030_charger: add ac/mode to match usb/mode
twl4030_charger: Increase current carefully while watching voltage.
twl4030_charger: assume a 'charger' can supply maximum current.

Pavel Machek (1):
twl4030_charger: convert to module_platform_driver instead of ..._probe.

 .../ABI/testing/sysfs-class-power-twl4030          |   45 ++
 drivers/mfd/twl-core.c                             |    9 
 drivers/power/twl4030_charger.c                    |  541 ++++++++++++++++++--
 3 files changed, 531 insertions(+), 64 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-class-power-twl4030

--
Signature

author
#1 lore raw

PATCH 1/13 twl4030_charger: use runtime_pm to keep usb phy active while charging.

The twl4030 usb phy needs to be active while we are using the USB VBUS as a current source for charging.
In particular, the usb3v1 regulator must be enabled and the PHY_PWR_PHYPWD bit must be set to keep the phy powered.

commit ab37813f4093a5f59cb8e083cde277289dc72ed3
twl4030_charger: Allow charger to control the regulator that feeds it

gave the charger control over the regulator, but didn't resolve the PHY_PWR_PHYPWD issue.

Now that both of these are controlled by runtime_pm in phy-twl4030-usb, we can simply take a runtime_pm reference to the USB phy whenever the charger wants to use it as a current source.

So this patch reverts the above commit, and adds the necessary runtime_pm calls.

---
 drivers/mfd/twl-core.c          |    9 ++++-----
 drivers/power/twl4030_charger.c |   18 +++++-------------
 2 files changed, 9 insertions(+), 18 deletions(-)
drivers/mfd/twl-core.c +4 -5
index 489674a2497e..831696ee2472 100644
@@ -788,9 +788,8 @@ 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" },
-			{ .supply =	"bci3v1" },
+		static struct regulator_consumer_supply usb3v1 = {
+			.supply =	"usb3v1",
 		};
 
 	/* First add the regulators so that they can be used by transceiver */
@@ -818,7 +817,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, 2,
+						      &usb_fixed, &usb3v1, 1,
 						      features);
 			if (IS_ERR(child))
 				return PTR_ERR(child);
@@ -838,7 +837,7 @@ add_children(struct twl4030_platform_data *pdata, unsigned irq_base,
 		if (IS_ENABLED(CONFIG_REGULATOR_TWL4030) && child) {
 			usb1v5.dev_name = dev_name(child);
 			usb1v8.dev_name = dev_name(child);
-			usb3v1[0].dev_name = dev_name(child);
+			usb3v1.dev_name = dev_name(child);
 		}
 	}
 
drivers/power/twl4030_charger.c +5 -13
index 709d90dc75f1..fe71c61109f5 100644
@@ -22,7 +22,6 @@
 #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
@@ -94,7 +93,6 @@ struct twl4030_bci {
 	struct work_struct	work;
 	int			irq_chg;
 	int			irq_bci;
-	struct regulator	*usb_reg;
 	int			usb_enabled;
 
 	unsigned long		event;
@@ -208,7 +206,7 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 {
 	int ret;
 
-	if (enable) {
+	if (enable && !IS_ERR_OR_NULL(bci->transceiver)) {
 		/* Check for USB charger connected */
 		if (!twl4030_bci_have_vbus(bci))
 			return -ENODEV;
@@ -222,14 +220,9 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 			return -EACCES;
 		}
 
-		/* Need to keep regulator on */
+		/* Need to keep phy powered */
 		if (!bci->usb_enabled) {
-			ret = regulator_enable(bci->usb_reg);
-			if (ret) {
-				dev_err(bci->dev,
-					"Failed to enable regulator\n");
-				return ret;
-			}
+			pm_runtime_get_sync(bci->transceiver->dev);
 			bci->usb_enabled = 1;
 		}
 
@@ -244,7 +237,8 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 	} else {
 		ret = twl4030_clear_set_boot_bci(TWL4030_BCIAUTOUSB, 0);
 		if (bci->usb_enabled) {
-			regulator_disable(bci->usb_reg);
+			pm_runtime_mark_last_busy(bci->transceiver->dev);
+			pm_runtime_put_autosuspend(bci->transceiver->dev);
 			bci->usb_enabled = 0;
 		}
 	}
@@ -609,8 +603,6 @@ static int __init twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	bci->usb_reg = regulator_get(bci->dev, "bci3v1");
-
 	bci->usb = devm_power_supply_register(&pdev->dev, &twl4030_bci_usb_desc,
 					      NULL);
 	if (IS_ERR(bci->usb)) {
Acked-by on v1 1/13
#2 lore raw

Re: [PATCH 01/13] twl4030_charger: use runtime_pm to keep usb phy active while charging.

Quoted 15 lines · NeilBrown

The twl4030 usb phy needs to be active while we are using the USB VBUS as a current source for charging.
In particular, the usb3v1 regulator must be enabled and the PHY_PWR_PHYPWD bit must be set to keep the phy powered.

commit ab37813f4093a5f59cb8e083cde277289dc72ed3
twl4030_charger: Allow charger to control the regulator that feeds it

gave the charger control over the regulator, but didn't resolve the PHY_PWR_PHYPWD issue.

Now that both of these are controlled by runtime_pm in phy-twl4030-usb, we can simply take a runtime_pm reference to the USB phy whenever the charger wants to use it as a current source.

Quoted 5 lines

So this patch reverts the above commit, and adds the necessary runtime_pm calls.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#3 lore raw

PATCH 2/13 twl4030_charger: convert to module_platform_driver instead of ..._probe.

In reply to NeilBrown

From: Pavel Machek <[email protected]>

Drivers using module_platform_driver_probe cannot return EPROBE_DEFER from the probe function, which makes them rather useless these days...

Convert to module_platform_driver() so EPROBE_DEFER can be used.

---
 drivers/power/twl4030_charger.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/power/twl4030_charger.c +3 -3
index fe71c61109f5..045238370d3f 100644
@@ -568,7 +568,7 @@ static const struct power_supply_desc twl4030_bci_usb_desc = {
 	.get_property	= twl4030_bci_get_property,
 };
 
-static int __init twl4030_bci_probe(struct platform_device *pdev)
+static int twl4030_bci_probe(struct platform_device *pdev)
 {
 	struct twl4030_bci *bci;
 	const struct twl4030_bci_platform_data *pdata = pdev->dev.platform_data;
@@ -692,14 +692,14 @@ static const struct of_device_id twl_bci_of_match[] = {
 MODULE_DEVICE_TABLE(of, twl_bci_of_match);
 
 static struct platform_driver twl4030_bci_driver = {
+	.probe = twl4030_bci_probe,
 	.driver	= {
 		.name	= "twl4030_bci",
 		.of_match_table = of_match_ptr(twl_bci_of_match),
 	},
 	.remove	= __exit_p(twl4030_bci_remove),
 };
-
-module_platform_driver_probe(twl4030_bci_driver, twl4030_bci_probe);
+module_platform_driver(twl4030_bci_driver);
 
 MODULE_AUTHOR("Gražvydas Ignotas");
 MODULE_DESCRIPTION("TWL4030 Battery Charger Interface driver");
author
#4 lore raw

PATCH 3/13 twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

In reply to NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

---
 drivers/power/twl4030_charger.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/power/twl4030_charger.c +5 -1
index 045238370d3f..ffc123fb7158 100644
@@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 		phynode = of_find_compatible_node(bci->dev->of_node->parent,
 						  NULL, "ti,twl4030-usb");
-		if (phynode)
+		if (phynode) {
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+		}
 	}
 
 	/* Enable interrupts now. */
maintainer
#5 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 15 lines · NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

---
 drivers/power/twl4030_charger.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/power/twl4030_charger.c +0 -0
index 045238370d3f..ffc123fb7158 100644
@@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 		phynode = of_find_compatible_node(bci->dev->of_node->parent,

drivers/power/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 646

Quoted 10 lines
drivers/power/twl4030_charger.c +5 -1
 						  NULL, "ti,twl4030-usb");
-		if (phynode)
+		if (phynode) {
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+		}
 	}

Neil, the return with -EPROBE_DEFER here causes flakeyness booting for me somehow at least on my logicpd-torpedo-37xx-devkit using omap2plus_defconfig.

It seems that the twl4030_bci_probe keeps looping or something about 1/3 of the boots and that probably prevents the other twl modules from loading? Reverting this patch alone seems to fix the issue.

I don't think I have a battery wired on this board the USB is wired the same way as on beagle-xm. So I'd assume also flakeyness on beagle xm with this patch.

Regards,

Tony

author
#6 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 35 lines · On Tue, 18 Aug 2015 01:07:58 -0700 Tony Lindgren <[email protected]> wrote:
Quoted 15 lines · NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

---
 drivers/power/twl4030_charger.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/power/twl4030_charger.c +0 -0
index 045238370d3f..ffc123fb7158 100644
@@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 		phynode = of_find_compatible_node(bci->dev->of_node->parent,

drivers/power/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 646

Quoted 10 lines
drivers/power/twl4030_charger.c +5 -1
 						  NULL, "ti,twl4030-usb");
-		if (phynode)
+		if (phynode) {
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+		}
 	}

Neil, the return with -EPROBE_DEFER here causes flakeyness booting for me somehow at least on my logicpd-torpedo-37xx-devkit using omap2plus_defconfig.

It seems that the twl4030_bci_probe keeps looping or something about 1/3 of the boots and that probably prevents the other twl modules from loading? Reverting this patch alone seems to fix the issue.

Quoted 7 lines

I don't think I have a battery wired on this board the USB is wired the same way as on beagle-xm. So I'd assume also flakeyness on beagle xm with this patch.

Regards,

Tony

What dts file are you using?
I'm guessing that it doesn't have something like:

&usb_otg_hs {
        interface-type = <0>;
        usb-phy = <&usb2_phy>;
        phys = <&usb2_phy>;
        phy-names = "usb2-phy";
        mode = <3>;
        power = <50>;
};

? i.e. with a usb-phy=<&usb2_phy> ?

What if you add

&usb2_phy {
status = "disabled";
}

to your dts file?

Should the 'status' be disabled in twl4030.dtsi, and then marked OK in any dts that uses it? I'm not at all clear on how 'status' is meant to be used.

Thanks,
NeilBrown

maintainer
#7 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 45 lines · NeilBrown
Quoted 31 lines · On Tue, 18 Aug 2015 01:07:58 -0700 Tony Lindgren <[email protected]> wrote:
Quoted 15 lines · NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

---
 drivers/power/twl4030_charger.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/power/twl4030_charger.c +0 -0
index 045238370d3f..ffc123fb7158 100644
@@ -636,9 +636,13 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 		phynode = of_find_compatible_node(bci->dev->of_node->parent,

drivers/power/twl4030_charger.c static int twl4030_bci_probe(struct platform_device *pdev) · line 646

Quoted 10 lines
drivers/power/twl4030_charger.c +5 -1
 						  NULL, "ti,twl4030-usb");
-		if (phynode)
+		if (phynode) {
 			bci->transceiver = devm_usb_get_phy_by_node(
 				bci->dev, phynode, &bci->usb_nb);
+			if (IS_ERR(bci->transceiver) &&
+			    PTR_ERR(bci->transceiver) == -EPROBE_DEFER)
+				return -EPROBE_DEFER;
+		}
 	}

Neil, the return with -EPROBE_DEFER here causes flakeyness booting for me somehow at least on my logicpd-torpedo-37xx-devkit using omap2plus_defconfig.

Quoted 7 lines

It seems that the twl4030_bci_probe keeps looping or something about 1/3 of the boots and that probably prevents the other twl modules from loading? Reverting this patch alone seems to fix the issue.

I don't think I have a battery wired on this board the USB is wired the same way as on beagle-xm. So I'd assume also flakeyness on beagle xm with this patch.

What dts file are you using?
I'm guessing that it doesn't have something like:

Quoted 8 lines
&usb_otg_hs {
        interface-type = <0>;
        usb-phy = <&usb2_phy>;
        phys = <&usb2_phy>;
        phy-names = "usb2-phy";
        mode = <3>;
        power = <50>;
};

It's the logicpd-torpedo-37xx-devkit.dts like I mentioned above, and yes it has the same entry above like beagle xm.

Quoted 9 lines

? i.e. with a usb-phy=<&usb2_phy> ?

What if you add

&usb2_phy {
status = "disabled";
}

to your dts file?

Yes then the PHY driver won't get probed and modprobe of the charger won't hang with -EPROBE_DEFER.. The USB is working on this one like on beagle xm so that's not a solution.

Quoted 3 lines

Should the 'status' be disabled in twl4030.dtsi, and then marked OK in any dts that uses it? I'm not at all clear on how 'status' is meant to be used.

Well status = "disabled" makes kernel completely ignore the hardware, so the struct device is never created. I'd stay away from using that in general for proper runtime idling of devices.. The hardware is there for sure :)

Can you check if something like the the following allows you to reproduce the hang of modprobe twl4030_charger:

# rmmod phy-twl4030-usb
# modprobe twl4030_charger

So just don't probe phy-twl4030-usb before twl4030_charger and then modprobe of twl4030_charger hangs?

That is with proper usb2_phy configured in .dts file and not set to status = "disabled".

Regards,

Tony

#8 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

In reply to NeilBrown
Quoted 4 lines · NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

This patch has hit linux-next in the form of coommit 3fc3895e4fe1 (twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node) and kernelci.org found a regression on omap3-beagle-xm[1]. Bisecting[2] this boot failure pointed at this commit, and I verified that reverting it on top of next-20150827 gets the board booting again. I haven't debugged any further.

Kevin

[1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
[2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console

#9 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

ping... this boot failure has now landed in mainline

Quoted 17 lines · Kevin Hilman
Quoted 4 lines · NeilBrown

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

This patch has hit linux-next in the form of coommit 3fc3895e4fe1 (twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node) and kernelci.org found a regression on omap3-beagle-xm[1]. Bisecting[2] this boot failure pointed at this commit, and I verified that reverting it on top of next-20150827 gets the board booting again. I haven't debugged any further.

Kevin

[1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
[2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console

author
#10 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 1 line · Kevin Hilman

ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be able to look at this properly. I should be able to examine this issue before the end of the month but cannot promise sooner than that (though it is not impossible).

Maybe it would be best to just revert it until a proper analysis can be done.

NeilBrown

Quoted 19 lines · Kevin Hilman
Quoted 17 lines · NeilBrown
Quoted 4 lines

Now that twl4030_bci_probe can safely return -EPROBE_DEFER, do so when devm_usb_get_phy_by_node returns that error.

This patch has hit linux-next in the form of coommit 3fc3895e4fe1 (twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node) and kernelci.org found a regression on omap3-beagle-xm[1]. Bisecting[2] this boot failure pointed at this commit, and I verified that reverting it on top of next-20150827 gets the board booting again. I haven't debugged any further.

Kevin

[1] http://storage.kernelci.org/next/next-20150827/arm-omap2plus_defconfig/lab-khilman/boot-omap3-beagle-xm.html
[2] https://ci.linaro.org/view/people/job/tbaker-boot-bisect-bot/88/console

maintainer
#11 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 11 lines · Neil Brown
Quoted 1 line · Kevin Hilman

ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be able to look at this properly. I should be able to examine this issue before the end of the month but cannot promise sooner than that (though it is not impossible).

Maybe it would be best to just revert it until a proper analysis can be done.

OK yeah let's revert this one for now until we know what goes wrong.

Regards,

Tony

#12 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 10 lines · Tony Lindgren
Quoted 9 lines · Neil Brown
Quoted 1 line · Kevin Hilman

ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be able to look at this properly. I should be able to examine this issue before the end of the month but cannot promise sooner than that (though it is not impossible).

Quoted 4 lines
Quoted 2 lines

Maybe it would be best to just revert it until a proper analysis can be done.

OK yeah let's revert this one for now until we know what goes wrong.

Looks like this is still in mainline.

Tony, can you revert?

Kevin

maintainer
#13 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 11 lines · Kevin Hilman
Quoted 10 lines · Tony Lindgren
Quoted 9 lines · Neil Brown
Quoted 1 line · Kevin Hilman

ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be able to look at this properly. I should be able to examine this issue before the end of the month but cannot promise sooner than that (though it is not impossible).

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Maybe it would be best to just revert it until a proper analysis can be done.

OK yeah let's revert this one for now until we know what goes wrong.

Looks like this is still in mainline.

Tony, can you revert?

Probably best that Sebastian does it as there's another fix needed too. It seems the following are needed:

- Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node")

- Apply compile fix "[PATCH] twl4030_charger: fix another compile error"

Just in case Sebastian is travelling or something.. Sebastian, can you please confirm?

Regards,

Tony

#14 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Hi,

Quoted 24 lines · Tony Lindgren
Quoted 11 lines · Kevin Hilman
Quoted 10 lines · Tony Lindgren
Quoted 9 lines · Neil Brown
Quoted 1 line · Kevin Hilman

ping... this boot failure has now landed in mainline

sorry, I'm on leave at the moment and travelling so I'm unlikely to be able to look at this properly. I should be able to examine this issue before the end of the month but cannot promise sooner than that (though it is not impossible).

Quoted 8 lines
Quoted 4 lines
Quoted 2 lines

Maybe it would be best to just revert it until a proper analysis can be done.

OK yeah let's revert this one for now until we know what goes wrong.

Looks like this is still in mainline.

Tony, can you revert?

Probably best that Sebastian does it as there's another fix needed too. It seems the following are needed:

Quoted 7 lines

- Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node")

- Apply compile fix "[PATCH] twl4030_charger: fix another compile error"

Just in case Sebastian is travelling or something..

Right, I was on vacation without internet access until now.

Quoted 1 line

Sebastian, can you please confirm?

Sounds fine to me. I will take care of it once I've slept a bit.

-- Sebastian

#15 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Hi,

Quoted 8 lines · Sebastian Reichel
Quoted 8 lines

Probably best that Sebastian does it as there's another fix needed too. It seems the following are needed:

- Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node")

- Apply compile fix "[PATCH] twl4030_charger: fix another compile error"

I just added these to my next branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=aefc574bbbbe74bb891ba392d98f2d59a417c774
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=e11fc21e756e662e465cac3da6547d438d0b1446

-- Sebastian

maintainer
#16 lore raw

Re: [PATCH 03/13] twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node

Quoted 9 lines · Sebastian Reichel

Hi,

Quoted 6 lines · Sebastian Reichel
Quoted 6 lines

Probably best that Sebastian does it as there's another fix needed too. It seems the following are needed:

- Revert $subject patch 3fc3895e4fe1 ("twl4030_charger: correctly handle -EPROBE_DEFER from devm_usb_get_phy_by_node")

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines

- Apply compile fix "[PATCH] twl4030_charger: fix another compile error"

I just added these to my next branch:

https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=aefc574bbbbe74bb891ba392d98f2d59a417c774
https://git.kernel.org/cgit/linux/kernel/git/sre/linux-power-supply.git/commit/?h=next&id=e11fc21e756e662e465cac3da6547d438d0b1446

Thanks!

Tony

author
#17 lore raw

PATCH 4/13 twl4030_charger: trust phy to determine when USB power is available.

In reply to NeilBrown

The usb phy driver already determines when VBUS is available, so repeating the test in the charger driver is pointless duplication.

On probe, process the last event from the phy, and from then on, do whatever the phy tells us without double-checking.

---
 drivers/power/twl4030_charger.c |   33 ++++++---------------------------
 1 file changed, 6 insertions(+), 27 deletions(-)
drivers/power/twl4030_charger.c +6 -27
index ffc123fb7158..a075216d65ed 100644
@@ -178,28 +178,6 @@ static int twl4030_is_battery_present(struct twl4030_bci *bci)
 }
 
 /*
- * Check if VBUS power is present
- */
-static int twl4030_bci_have_vbus(struct twl4030_bci *bci)
-{
-	int ret;
-	u8 hwsts;
-
-	ret = twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &hwsts,
-			      TWL4030_PM_MASTER_STS_HW_CONDITIONS);
-	if (ret < 0)
-		return 0;
-
-	dev_dbg(bci->dev, "check_vbus: HW_CONDITIONS %02x\n", hwsts);
-
-	/* in case we also have STS_USB_ID, VBUS is driven by TWL itself */
-	if ((hwsts & TWL4030_STS_VBUS) && !(hwsts & TWL4030_STS_USB_ID))
-		return 1;
-
-	return 0;
-}
-
-/*
  * Enable/Disable USB Charge functionality.
  */
 static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
@@ -207,10 +185,6 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 	int ret;
 
 	if (enable && !IS_ERR_OR_NULL(bci->transceiver)) {
-		/* Check for USB charger connected */
-		if (!twl4030_bci_have_vbus(bci))
-			return -ENODEV;
-
 		/*
 		 * Until we can find out what current the device can provide,
 		 * require a module param to enable USB charging.
@@ -662,7 +636,12 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
 
 	twl4030_charger_enable_ac(true);
-	twl4030_charger_enable_usb(bci, true);
+	if (!IS_ERR_OR_NULL(bci->transceiver))
+		twl4030_bci_usb_ncb(&bci->usb_nb,
+				    bci->transceiver->last_event,
+				    NULL);
+	else
+		twl4030_charger_enable_usb(bci, false);
 	if (pdata)
 		twl4030_charger_enable_backup(pdata->bb_uvolt,
 					      pdata->bb_uamp);
Acked-by on v1 4/13
#18 lore raw

Re: [PATCH 04/13] twl4030_charger: trust phy to determine when USB power is available.

Quoted 7 lines · NeilBrown

The usb phy driver already determines when VBUS is available, so repeating the test in the charger driver is pointless duplication.

On probe, process the last event from the phy, and from then on, do whatever the phy tells us without double-checking.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
author
#19 lore raw

PATCH 5/13 twl4030_charger: split uA calculation into a function.

In reply to NeilBrown

We will need this calculation in other places, so create functions to map between register value and uA value.

---
 drivers/power/twl4030_charger.c |   48 ++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 13 deletions(-)
drivers/power/twl4030_charger.c +35 -13
index a075216d65ed..29984b263a35 100644
@@ -178,6 +178,40 @@ static int twl4030_is_battery_present(struct twl4030_bci *bci)
 }
 
 /*
+ * TI provided formulas:
+ * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 - 1) - 0.85
+ * CGAIN == 1: ICHG = (BCIICHG * 3.4) / (2^10 - 1) - 1.7
+ * Here we use integer approximation of:
+ * CGAIN == 0: val * 1.6618 - 0.85 * 1000
+ * CGAIN == 1: (val * 1.6618 - 0.85 * 1000) * 2
+ */
+/*
+ * convert twl register value for currents into uA
+ */
+static int regval2ua(int regval, bool cgain)
+{
+	if (cgain)
+		return (regval * 16618 - 8500 * 1000) / 5;
+	else
+		return (regval * 16618 - 8500 * 1000) / 10;
+}
+
+/*
+ * convert uA currents into twl register value
+ */
+static int ua2regval(int ua, bool cgain)
+{
+	int ret;
+	if (cgain)
+		ua /= 2;
+	ret = (ua * 10 + 8500 * 1000) / 16618;
+	/* rounding problems */
+	if (ret < 512)
+		ret = 512;
+	return ret;
+}
+
+/*
  * Enable/Disable USB Charge functionality.
  */
 static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
@@ -366,14 +400,6 @@ static int twl4030_bci_usb_ncb(struct notifier_block *nb, unsigned long val,
 	return NOTIFY_OK;
 }
 
-/*
- * TI provided formulas:
- * CGAIN == 0: ICHG = (BCIICHG * 1.7) / (2^10 - 1) - 0.85
- * CGAIN == 1: ICHG = (BCIICHG * 3.4) / (2^10 - 1) - 1.7
- * Here we use integer approximation of:
- * CGAIN == 0: val * 1.6618 - 0.85
- * CGAIN == 1: (val * 1.6618 - 0.85) * 2
- */
 static int twl4030_charger_get_current(void)
 {
 	int curr;
@@ -388,11 +414,7 @@ static int twl4030_charger_get_current(void)
 	if (ret)
 		return ret;
 
-	ret = (curr * 16618 - 850 * 10000) / 10;
-	if (bcictl1 & TWL4030_CGAIN)
-		ret *= 2;
-
-	return ret;
+	return regval2ua(curr, bcictl1 & TWL4030_CGAIN);
 }
 
 /*
author
#20 lore raw

PATCH 6/13 twl4030_charger: allow fine control of charger current.

In reply to NeilBrown

The twl4030 allows control of the incoming current. Part of this control is a 'CGAIN' setting which doubles the range for half the precision. This control affects several different current setting, so all need to be updated at once when CGAIN is changed.

With this patch, all of these current setting are managed by the driver, but most are left at their default settings.

The current drawn is set to 500mA if the allow_usb module parameter is set, and to 100mA otherwise.
More fine control will appear in later patches.

---
 drivers/power/twl4030_charger.c |  168 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 8 deletions(-)
drivers/power/twl4030_charger.c +160 -8
index 29984b263a35..3b7cc631bb8a 100644
@@ -31,6 +31,11 @@
 #define TWL4030_BCIMFSTS4	0x10
 #define TWL4030_BCICTL1		0x23
 #define TWL4030_BB_CFG		0x12
+#define TWL4030_BCIIREF1	0x27
+#define TWL4030_BCIIREF2	0x28
+#define TWL4030_BCIMFKEY	0x11
+#define TWL4030_BCIMFTH8	0x1d
+#define TWL4030_BCIMFTH9	0x1e
 
 #define TWL4030_BCIMFSTS1	0x01
 
@@ -95,6 +100,12 @@ struct twl4030_bci {
 	int			irq_bci;
 	int			usb_enabled;
 
+	/*
+	 * ichg values in uA. If any are 'large', we set CGAIN to
+	 * '1' which doubles the range for half the precision.
+	 */
+	unsigned int		ichg_eoc, ichg_lo, ichg_hi, cur;
+
 	unsigned long		event;
 };
 
@@ -211,6 +222,146 @@ static int ua2regval(int ua, bool cgain)
 	return ret;
 }
 
+static int twl4030_charger_update_current(struct twl4030_bci *bci)
+{
+	int status;
+	unsigned reg, cur_reg;
+	u8 bcictl1, oldreg, fullreg;
+	bool cgain = false;
+	u8 boot_bci;
+
+	/* First, check thresholds and see if cgain is needed */
+	if (bci->ichg_eoc >= 200000)
+		cgain = true;
+	if (bci->ichg_lo >= 400000)
+		cgain = true;
+	if (bci->ichg_hi >= 820000)
+		cgain = true;
+	if (bci->cur > 852000)
+		cgain = true;
+
+	status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
+	if (status < 0)
+		return status;
+	if (twl_i2c_read_u8(TWL_MODULE_PM_MASTER, &boot_bci,
+			    TWL4030_PM_MASTER_BOOT_BCI) < 0)
+		boot_bci = 0;
+	boot_bci &= 7;
+
+	if ((!!cgain) != !!(bcictl1 & TWL4030_CGAIN))
+		/* Need to turn for charging while we change the
+		 * CGAIN bit.  Leave it off while everything is
+		 * updated.
+		 */
+		twl4030_clear_set_boot_bci(boot_bci, 0);
+
+	/*
+	 * For ichg_eoc, the hardware only supports reg values matching
+	 * 100XXXX000, and requires the XXXX be stored in the high nibble
+	 * of TWL4030_BCIMFTH8.
+	 */
+	reg = ua2regval(bci->ichg_eoc, cgain);
+	if (reg > 0x278)
+		reg = 0x278;
+	if (reg < 0x200)
+		reg = 0x200;
+	reg = (reg >> 3) & 0xf;
+	fullreg = reg << 4;
+
+	/*
+	 * For ichg_lo, reg value must match 10XXXX0000.
+	 * XXXX is stored in low nibble of TWL4030_BCIMFTH8.
+	 */
+	reg = ua2regval(bci->ichg_lo, cgain);
+	if (reg > 0x2F0)
+		reg = 0x2F0;
+	if (reg < 0x200)
+		reg = 0x200;
+	reg = (reg >> 4) & 0xf;
+	fullreg |= reg;
+
+	/* ichg_eoc and ichg_lo live in same register */
+	status = twl4030_bci_read(TWL4030_BCIMFTH8, &oldreg);
+	if (status < 0)
+		return status;
+	if (oldreg != fullreg) {
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE, 0xF4,
+					  TWL4030_BCIMFKEY);
+		if (status < 0)
+			return status;
+		twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
+				 fullreg, TWL4030_BCIMFTH8);
+	}
+
+	/* ichg_hi threshold must be 1XXXX01100 (I think) */
+	reg = ua2regval(bci->ichg_hi, cgain);
+	if (reg > 0x3E0)
+		reg = 0x3E0;
+	if (reg < 0x200)
+		reg = 0x200;
+	fullreg = (reg >> 5) & 0xF;
+	fullreg <<= 4;
+	status = twl4030_bci_read(TWL4030_BCIMFTH9, &oldreg);
+	if (status < 0)
+		return status;
+	if ((oldreg & 0xF0) != fullreg) {
+		fullreg |= (oldreg & 0x0F);
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE, 0xE7,
+					  TWL4030_BCIMFKEY);
+		if (status < 0)
+			return status;
+		twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
+				 fullreg, TWL4030_BCIMFTH9);
+	}
+
+	/*
+	 * And finally, set the current.  This is stored in
+	 * two registers.
+	 */
+	reg = ua2regval(bci->cur, cgain);
+	/* we have only 10 bits */
+	if (reg > 0x3ff)
+		reg = 0x3ff;
+	status = twl4030_bci_read(TWL4030_BCIIREF1, &oldreg);
+	if (status < 0)
+		return status;
+	cur_reg = oldreg;
+	status = twl4030_bci_read(TWL4030_BCIIREF2, &oldreg);
+	if (status < 0)
+		return status;
+	cur_reg |= oldreg << 8;
+	if (reg != oldreg) {
+		/* disable write protection for one write access for
+		 * BCIIREF */
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE, 0xE7,
+					  TWL4030_BCIMFKEY);
+		if (status < 0)
+			return status;
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
+					  (reg & 0x100) ? 3 : 2,
+					  TWL4030_BCIIREF2);
+		if (status < 0)
+			return status;
+		/* disable write protection for one write access for
+		 * BCIIREF */
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE, 0xE7,
+					  TWL4030_BCIMFKEY);
+		if (status < 0)
+			return status;
+		status = twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
+					  reg & 0xff,
+					  TWL4030_BCIIREF1);
+	}
+	if ((!!cgain) != !!(bcictl1 & TWL4030_CGAIN)) {
+		/* Flip CGAIN and re-enable charging */
+		bcictl1 ^= TWL4030_CGAIN;
+		twl_i2c_write_u8(TWL_MODULE_MAIN_CHARGE,
+				 bcictl1, TWL4030_BCICTL1);
+		twl4030_clear_set_boot_bci(0, boot_bci);
+	}
+	return 0;
+}
+
 /*
  * Enable/Disable USB Charge functionality.
  */
@@ -219,14 +370,6 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 	int ret;
 
 	if (enable && !IS_ERR_OR_NULL(bci->transceiver)) {
-		/*
-		 * Until we can find out what current the device can provide,
-		 * require a module param to enable USB charging.
-		 */
-		if (!allow_usb) {
-			dev_warn(bci->dev, "USB charging is disabled.\n");
-			return -EACCES;
-		}
 
 		/* Need to keep phy powered */
 		if (!bci->usb_enabled) {
@@ -578,6 +721,14 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	if (!pdata)
 		pdata = twl4030_bci_parse_dt(&pdev->dev);
 
+	bci->ichg_eoc = 80100; /* Stop charging when current drops to here */
+	bci->ichg_lo = 241000; /* Low threshold */
+	bci->ichg_hi = 500000; /* High threshold */
+	if (allow_usb)
+		bci->cur = 500000;  /* 500mA */
+	else
+		bci->cur = 100000;  /* 100mA */
+
 	bci->dev = &pdev->dev;
 	bci->irq_chg = platform_get_irq(pdev, 0);
 	bci->irq_bci = platform_get_irq(pdev, 1);
@@ -657,6 +808,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	if (ret < 0)
 		dev_warn(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
 
+	twl4030_charger_update_current(bci);
 	twl4030_charger_enable_ac(true);
 	if (!IS_ERR_OR_NULL(bci->transceiver))
 		twl4030_bci_usb_ncb(&bci->usb_nb,
author
#21 lore raw

PATCH 7/13 twl4030_charger: distinguish between USB current and 'AC' current

In reply to NeilBrown

The twl4030 charger has two current sources, 'USB' and 'AC' (presumably "Accessory Charger" because it isn't Alternating Current).

If 'AC' is providing current, we should set the current limit differently to when it isn't (and so USB is used).
So split 'cur' into 'usb_cur' and 'ac_cur' and use accordingly.

Now we must review the current setting on any interrupt or USB event which might indicate that the charger-source has changed.

---
 drivers/power/twl4030_charger.c |   36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)
drivers/power/twl4030_charger.c +29 -7
index 3b7cc631bb8a..982675df21b7 100644
@@ -22,6 +22,7 @@
 #include <linux/power_supply.h>
 #include <linux/notifier.h>
 #include <linux/usb/otg.h>
+#include <linux/i2c/twl4030-madc.h>
 
 #define TWL4030_BCIMSTATEC	0x02
 #define TWL4030_BCIICHG		0x08
@@ -101,10 +102,13 @@ struct twl4030_bci {
 	int			usb_enabled;
 
 	/*
-	 * ichg values in uA. If any are 'large', we set CGAIN to
-	 * '1' which doubles the range for half the precision.
+	 * ichg_* and *_cur values in uA. If any are 'large', we set
+	 * CGAIN to '1' which doubles the range for half the
+	 * precision.
 	 */
-	unsigned int		ichg_eoc, ichg_lo, ichg_hi, cur;
+	unsigned int		ichg_eoc, ichg_lo, ichg_hi;
+	unsigned int		usb_cur, ac_cur;
+	bool			ac_is_active;
 
 	unsigned long		event;
 };
@@ -225,11 +229,24 @@ static int ua2regval(int ua, bool cgain)
 static int twl4030_charger_update_current(struct twl4030_bci *bci)
 {
 	int status;
+	int cur;
 	unsigned reg, cur_reg;
 	u8 bcictl1, oldreg, fullreg;
 	bool cgain = false;
 	u8 boot_bci;
 
+	/*
+	 * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
+	 * and AC is enabled, set current for 'ac'
+	 */
+	if (twl4030_get_madc_conversion(11) > 4500) {
+		cur = bci->ac_cur;
+		bci->ac_is_active = true;
+	} else {
+		cur = bci->usb_cur;
+		bci->ac_is_active = false;
+	}
+
 	/* First, check thresholds and see if cgain is needed */
 	if (bci->ichg_eoc >= 200000)
 		cgain = true;
@@ -237,7 +254,7 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 		cgain = true;
 	if (bci->ichg_hi >= 820000)
 		cgain = true;
-	if (bci->cur > 852000)
+	if (cur > 852000)
 		cgain = true;
 
 	status = twl4030_bci_read(TWL4030_BCICTL1, &bcictl1);
@@ -318,7 +335,7 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 	 * And finally, set the current.  This is stored in
 	 * two registers.
 	 */
-	reg = ua2regval(bci->cur, cgain);
+	reg = ua2regval(cur, cgain);
 	/* we have only 10 bits */
 	if (reg > 0x3ff)
 		reg = 0x3ff;
@@ -371,6 +388,8 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 
 	if (enable && !IS_ERR_OR_NULL(bci->transceiver)) {
 
+		twl4030_charger_update_current(bci);
+
 		/* Need to keep phy powered */
 		if (!bci->usb_enabled) {
 			pm_runtime_get_sync(bci->transceiver->dev);
@@ -463,6 +482,7 @@ static irqreturn_t twl4030_charger_interrupt(int irq, void *arg)
 	struct twl4030_bci *bci = arg;
 
 	dev_dbg(bci->dev, "CHG_PRES irq\n");
+	twl4030_charger_update_current(bci);
 	power_supply_changed(bci->ac);
 	power_supply_changed(bci->usb);
 
@@ -495,6 +515,7 @@ static irqreturn_t twl4030_bci_interrupt(int irq, void *arg)
 		power_supply_changed(bci->ac);
 		power_supply_changed(bci->usb);
 	}
+	twl4030_charger_update_current(bci);
 
 	/* various monitoring events, for now we just log them here */
 	if (irqs1 & (TWL4030_TBATOR2 | TWL4030_TBATOR1))
@@ -724,10 +745,11 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	bci->ichg_eoc = 80100; /* Stop charging when current drops to here */
 	bci->ichg_lo = 241000; /* Low threshold */
 	bci->ichg_hi = 500000; /* High threshold */
+	bci->ac_cur = 500000; /* 500mA */
 	if (allow_usb)
-		bci->cur = 500000;  /* 500mA */
+		bci->usb_cur = 500000;  /* 500mA */
 	else
-		bci->cur = 100000;  /* 100mA */
+		bci->usb_cur = 100000;  /* 100mA */
 
 	bci->dev = &pdev->dev;
 	bci->irq_chg = platform_get_irq(pdev, 0);
maintainer
#22 lore raw

Re: [PATCH 07/13] twl4030_charger: distinguish between USB current and 'AC' current

Quoted 16 lines · NeilBrown
drivers/power/twl4030_charger.c +6 -0
 static int twl4030_charger_update_current(struct twl4030_bci *bci)
 {
 	int status;
+	int cur;
 	unsigned reg, cur_reg;
 	u8 bcictl1, oldreg, fullreg;
 	bool cgain = false;
 	u8 boot_bci;
 
+	/*
+	 * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
+	 * and AC is enabled, set current for 'ac'
+	 */
+	if (twl4030_get_madc_conversion(11) > 4500) {

drivers/power/twl4030_charger.c static int ua2regval(int ua, bool cgain) · line 252

Quoted 10 lines
drivers/power/twl4030_charger.c +7 -0
+		cur = bci->ac_cur;
+		bci->ac_is_active = true;
+	} else {
+		cur = bci->usb_cur;
+		bci->ac_is_active = false;
+	}
+
 	/* First, check thresholds and see if cgain is needed */
 	if (bci->ichg_eoc >= 200000)
 		cgain = true;

Neil, you need a stub or something for twl4030_get_madc_conversion if madc is not selected. Now at least omap2plus_defconfig and ARM allmodconfig fails in Linux next.

Regards,

Tony

author
#23 lore raw

Re: [PATCH 07/13] twl4030_charger: distinguish between USB current and 'AC' current

Quoted 32 lines · On Thu, 6 Aug 2015 20:11:16 -0700 Tony Lindgren <[email protected]> wrote:
Quoted 16 lines · NeilBrown
drivers/power/twl4030_charger.c +6 -0
 static int twl4030_charger_update_current(struct twl4030_bci *bci)
 {
 	int status;
+	int cur;
 	unsigned reg, cur_reg;
 	u8 bcictl1, oldreg, fullreg;
 	bool cgain = false;
 	u8 boot_bci;
 
+	/*
+	 * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
+	 * and AC is enabled, set current for 'ac'
+	 */
+	if (twl4030_get_madc_conversion(11) > 4500) {

drivers/power/twl4030_charger.c

Quoted 10 lines
drivers/power/twl4030_charger.c +7 -0
+		cur = bci->ac_cur;
+		bci->ac_is_active = true;
+	} else {
+		cur = bci->usb_cur;
+		bci->ac_is_active = false;
+	}
+
 	/* First, check thresholds and see if cgain is needed */
 	if (bci->ichg_eoc >= 200000)
 		cgain = true;

Neil, you need a stub or something for twl4030_get_madc_conversion if madc is not selected. Now at least omap2plus_defconfig and ARM allmodconfig fails in Linux next.

Quoted 3 lines

Regards,

Tony

Thanks, I did get notified about that by Fengguang's test robot, but it's still on my list....

I guess making CHARGER_TWL4030 auto-select TWL4030_MADC would not be acceptable? That would pull in IIO (it didn't use to...).

If this OK?

Thanks,
NeilBrown

From: NeilBrown <[email protected]>
Date: Fri, 7 Aug 2015 13:44:37 +1000
Subject: [PATCH] twl4030_charger: fix compile error when TWL4030_MADC not
available.

We can only use the madc to check for 'ac' availability if the madc has been compiled in.
If not: assume always using USB.

drivers/power/twl4030_charger.c +16 -1
index c7432f532a83..265fd236f4c0 100644
@@ -91,6 +91,21 @@
 #define TWL4030_MSTATEC_COMPLETE1	0x0b
 #define TWL4030_MSTATEC_COMPLETE4	0x0e
 
+#if IS_ENABLED(CONFIG_TWL4030_MADC)
+/*
+ * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
+ * then AC is available.
+ */
+static inline int ac_available(void)
+{
+	return twl4030_get_madc_conversion(11) > 4500;
+}
+#else
+static inline int ac_available(void)
+{
+	return 0;
+}
+#endif
 static bool allow_usb;
 module_param(allow_usb, bool, 0644);
 MODULE_PARM_DESC(allow_usb, "Allow USB charge drawing default current");
@@ -263,7 +278,7 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 	 * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
 	 * and AC is enabled, set current for 'ac'
 	 */
-	if (twl4030_get_madc_conversion(11) > 4500) {
+	if (ac_available()) {
 		cur = bci->ac_cur;
 		bci->ac_is_active = true;
 	} else {
maintainer Acked-by
#24 lore raw

Re: [PATCH 07/13] twl4030_charger: distinguish between USB current and 'AC' current

Quoted 8 lines · NeilBrown

Thanks, I did get notified about that by Fengguang's test robot, but it's still on my list....

I guess making CHARGER_TWL4030 auto-select TWL4030_MADC would not be acceptable? That would pull in IIO (it didn't use to...).

If this OK?

Looks OK to me thanks:

#25 lore raw

Re: [PATCH 07/13] twl4030_charger: distinguish between USB current and 'AC' current

In reply to NeilBrown

Hi,

This actually slipped through my review. IMHO madc should be accessed through IIO, as already done for twl4030-madc-battery and rx51-battery. That way the custom API can be removed at some point.

Anyway, I queued the below patch with Tony's ACK to fix the build issue in next.

Quoted 39 lines · NeilBrown

From: NeilBrown <[email protected]>
Date: Fri, 7 Aug 2015 13:44:37 +1000
Subject: [PATCH] twl4030_charger: fix compile error when TWL4030_MADC not
available.

We can only use the madc to check for 'ac' availability if the madc has been compiled in.
If not: assume always using USB.

drivers/power/twl4030_charger.c +15 -0
index c7432f532a83..265fd236f4c0 100644
@@ -91,6 +91,21 @@
 #define TWL4030_MSTATEC_COMPLETE1	0x0b
 #define TWL4030_MSTATEC_COMPLETE4	0x0e
 
+#if IS_ENABLED(CONFIG_TWL4030_MADC)
+/*
+ * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
+ * then AC is available.
+ */
+static inline int ac_available(void)
+{
+	return twl4030_get_madc_conversion(11) > 4500;
+}
+#else
+static inline int ac_available(void)
+{
+	return 0;
+}
+#endif
 static bool allow_usb;
 module_param(allow_usb, bool, 0644);
 MODULE_PARM_DESC(allow_usb, "Allow USB charge drawing default current");

drivers/power/twl4030_charger.c static int twl4030_charger_update_current(struct twl4030_bci *bci) · line 284

Quoted 8 lines
drivers/power/twl4030_charger.c +1 -1
@@ -263,7 +278,7 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 	 * If AC (Accessory Charger) voltage exceeds 4.5V (MADC 11)
 	 * and AC is enabled, set current for 'ac'
 	 */
-	if (twl4030_get_madc_conversion(11) > 4500) {
+	if (ac_available()) {
 		cur = bci->ac_cur;
 		bci->ac_is_active = true;
 	} else {

-- Sebastian

author
#26 lore raw

Re: [PATCH 07/13] twl4030_charger: distinguish between USB current and 'AC' current

Quoted 10 lines · On Fri, 7 Aug 2015 07:13:09 +0200 Sebastian Reichel <[email protected]> wrote:

Hi,

This actually slipped through my review. IMHO madc should be accessed through IIO, as already done for twl4030-madc-battery and rx51-battery. That way the custom API can be removed at some point.

Anyway, I queued the below patch with Tony's ACK to fix the build issue in next.

OK, thanks.

I'll try to figure out are more proper approach ... might be a week or so though.

Thanks,
NeilBrown

author
#27 lore raw

PATCH 8/13 twl4030_charger: allow max_current to be managed via sysfs.

In reply to NeilBrown

'max_current' sysfs attributes are created which allow the max to be set.
Whenever a current source changes, the default is restored. This will be followed by a uevent, so user-space can decide to update again.

---
 .../ABI/testing/sysfs-class-power-twl4030          |   15 ++++
 drivers/power/twl4030_charger.c                    |   72 ++++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-power-twl4030
Documentation/ABI/testing/sysfs-class-power-twl4030 new file +15 -0
new file mode 100644
index 000000000000..0331bba4605d
@@ -0,0 +1,15 @@
+What: /sys/class/power_supply/twl4030_ac/max_current
+      /sys/class/power_supply/twl4030_usb/max_current
+Description:
+	Read/Write limit on current which may
+	be drawn from the ac (Accessory Charger) or
+	USB port.
+
+	Value is in micro-Amps.
+
+	Value is set automatically to an appropriate
+	value when a cable is plugged or unplugged.
+
+	Value can the set by writing to the attribute.
+	The change will only persist until the next
+	plug event.  These event are reported via udev.
drivers/power/twl4030_charger.c +72 -0

102 lines not on this page · raw message

author
#28 lore raw

PATCH 9/13 twl4030_charger: enable manual enable/disable of usb charging.

In reply to NeilBrown

'off' or 'auto' to

/sys/class/power/twl4030_usb/mode

will now enable or disable charging from USB port. Normally this is enabled on 'plug' and disabled on 'unplug'.
Unplug will still disable charging. 'plug' will only enable it if 'auto' if selected.

---
 .../ABI/testing/sysfs-class-power-twl4030          |   11 ++++
 drivers/power/twl4030_charger.c                    |   59 ++++++++++++++++++++
 2 files changed, 70 insertions(+)
Documentation/ABI/testing/sysfs-class-power-twl4030 +11 -0
index 0331bba4605d..40e0f919cbde 100644
@@ -13,3 +13,14 @@ Description:
 	Value can the set by writing to the attribute.
 	The change will only persist until the next
 	plug event.  These event are reported via udev.
+
+
+What: /sys/class/power_supply/twl4030_usb/mode
+Description:
+	Changing mode for USB port.
+	Writing to this can disable charging.
+
+	Possible values are:
+		"auto" - draw power as appropriate for detected
+			 power source and battery status.
+		"off"  - do not draw any power.
drivers/power/twl4030_charger.c +59 -0

99 lines not on this page · raw message

author
#29 lore raw

PATCH 10/13 twl4030_charger: add software controlled linear charging mode.

In reply to NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

---
 .../ABI/testing/sysfs-class-power-twl4030          |    9 +++
 drivers/power/twl4030_charger.c                    |   55 ++++++++++++++++++--
 2 files changed, 59 insertions(+), 5 deletions(-)
Documentation/ABI/testing/sysfs-class-power-twl4030 +9 -0
index 40e0f919cbde..e8baa36aaa2b 100644
@@ -24,3 +24,12 @@ Description:
 		"auto" - draw power as appropriate for detected
 			 power source and battery status.
 		"off"  - do not draw any power.
+		"continuous"
+		       - activate mode described as "linear" in
+		         TWL data sheets.  This uses whatever
+			 current is available and doesn't switch off
+			 when voltage drops.
+
+			 This is useful for unstable power sources
+			 such as bicycle dynamo, but care should
+			 be taken that battery is not over-charged.
drivers/power/twl4030_charger.c +50 -5

109 lines not on this page · raw message

#30 lore raw

Re: [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

Quoted 4 lines
drivers/power/twl4030_charger.c +0 -0
@@ -750,6 +784,17 @@ static int twl4030_bci_get_property(struct power_supply *psy,
 		is_charging = state & TWL4030_MSTATEC_USB;
 	else
 		is_charging = state & TWL4030_MSTATEC_AC;

drivers/power/twl4030_charger.c static int twl4030_bci_get_property(struct power_supply *psy, · line 796

Quoted 10 lines
drivers/power/twl4030_charger.c +10 -0
+	if (!is_charging) {
+		u8 s;
+		twl4030_bci_read(TWL4030_BCIMDEN, &s);
+		if (psy->desc->type == POWER_SUPPLY_TYPE_USB)
+			is_charging = s & 1;
+		else
+			is_charging = s & 2;
+		if (is_charging)
+			/* A little white lie */
+			state = TWL4030_MSTATEC_QUICK1;

I'm not sure... can't this white lie turn into black smoke?

Like.. normally, when battery is below something (like 3.5V) it must not be quick-charged (because something is very wrong with it). Are you just forcing the quick charge here?

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#31 lore raw

Re: [Gta04-owner] [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Hi all,

Quoted 10 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

Just my $0.02, I assumed (whoops!) that Neil mean that the _code_ should take care not to over-charge... but not sure how he intended to to that.

Anyway, that was my interpretation of the (indeed, it could be explained in different ways...) comment.

Christ van Willegen

author
#32 lore raw

Re: [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

In reply to Pavel Machek
Quoted 10 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

The following text reads:

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

so: take a break from cycling occasionally.

Quoted 18 lines
Quoted 5 lines
drivers/power/twl4030_charger.c +0 -0
@@ -750,6 +784,17 @@ static int twl4030_bci_get_property(struct power_supply *psy,
 		is_charging = state & TWL4030_MSTATEC_USB;
 	else
 		is_charging = state & TWL4030_MSTATEC_AC;

drivers/power/twl4030_charger.c static int twl4030_bci_get_property(struct power_supply *psy, · line 796

Quoted 10 lines
drivers/power/twl4030_charger.c +10 -0
+	if (!is_charging) {
+		u8 s;
+		twl4030_bci_read(TWL4030_BCIMDEN, &s);
+		if (psy->desc->type == POWER_SUPPLY_TYPE_USB)
+			is_charging = s & 1;
+		else
+			is_charging = s & 2;
+		if (is_charging)
+			/* A little white lie */
+			state = TWL4030_MSTATEC_QUICK1;

I'm not sure... can't this white lie turn into black smoke?

Quoted 3 lines

Like.. normally, when battery is below something (like 3.5V) it must not be quick-charged (because something is very wrong with it). Are you just forcing the quick charge here?

No. That value is only used for reporting a status via sysfs. That 'lie' gets translated to "POWER_SUPPLY_STATUS_CHARGING" which is not a lie.

Thanks,
NeilBrown

Quoted 5 lines

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
#33 lore raw

Re: [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Hi!

Quoted 10 lines
Quoted 9 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Quoted 8 lines
Quoted 1 line

Can you explain how the user can "care not to over-charge"?

The following text reads:

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

so: take a break from cycling occasionally.

If the charger does not exceed 4.2V, I'd not call it overcharge. (Yes, some clever chargers actually let the battery drop below 4.2V when charge is done, but...)

If the charger _does_ exceed 4.2V, then the battery will explode. Don't do that. Don't offer that to the user.

On a related note... I've just killed USB charger by overloading it. They are not protected.

I believe your automatically-pull-max-power really should stick to the well-known charging currents (.5A, 1A, 1.7A), at the very minimum.

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#34 lore raw

Re: [Gta04-owner] [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Quoted 16 lines · On Tue, 6 Oct 2015 16:34:07 +0200 Pavel Machek <[email protected]> wrote:

Hi!

Quoted 14 lines
Quoted 10 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

The following text reads:

Quoted 8 lines
Quoted 4 lines

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

so: take a break from cycling occasionally.

If the charger does not exceed 4.2V, I'd not call it overcharge. (Yes, some clever chargers actually let the battery drop below 4.2V when charge is done, but...)

Yes, that is the case. Perhaps it is not to be called overcharge but it is said that lithium battery charging has to stop if in CV mode the current drops too low. In automatic mode the charger does exactly that.
I would not let a battery for days at 4.2V CV.mode although a lot of cheap chargers

Quoted 8 lines

If the charger _does_ exceed 4.2V, then the battery will explode. Don't do that. Don't offer that to the user.

On a related note... I've just killed USB charger by overloading it. They are not protected.

I believe your automatically-pull-max-power really should stick to the well-known charging currents (.5A, 1A, 1.7A), at the very minimum.

The main reason for the patch was to prevent switching off charging when Vbus drops low. The reason was not to get out extremely much current out of the charger.
The electrical characteristics of a bicycle as a power source are.
- the amount of current available changes
- 500mA at around 17km/h
- you cannot destroy it by electrically overloading

If the current is set to e.g. 500mA and that linear charging mode is enabled, the battery gets the maximum current available (upto 500mA) regardless of the speed which is often changing.

Regards,
Andreas Kemnade

#35 lore raw

Re: [Gta04-owner] [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Hi!

Quoted 22 lines
Quoted 14 lines
Quoted 14 lines
Quoted 10 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

The following text reads:

Quoted 8 lines
Quoted 4 lines

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

so: take a break from cycling occasionally.

If the charger does not exceed 4.2V, I'd not call it overcharge. (Yes, some clever chargers actually let the battery drop below 4.2V when charge is done, but...)

Quoted 6 lines

Yes, that is the case. Perhaps it is not to be called overcharge but it is said that lithium battery charging has to stop if in CV mode the current drops too low. In automatic mode the charger does exactly that.
I would not let a battery for days at 4.2V CV.mode although a lot of cheap chargers

Well, I agree that keeping battery at 4.2V constant voltage mode is bad, but I'd not call it overcharge. If someone can fix the comment, that would be nice.

Quoted 16 lines
Quoted 8 lines

If the charger _does_ exceed 4.2V, then the battery will explode. Don't do that. Don't offer that to the user.

On a related note... I've just killed USB charger by overloading it. They are not protected.

I believe your automatically-pull-max-power really should stick to the well-known charging currents (.5A, 1A, 1.7A), at the very minimum.

The main reason for the patch was to prevent switching off charging when Vbus drops low. The reason was not to get out extremely much current out of the charger.
The electrical characteristics of a bicycle as a power source are.
- the amount of current available changes
- 500mA at around 17km/h
- you cannot destroy it by electrically overloading

Quoted 3 lines

If the current is set to e.g. 500mA and that linear charging mode is enabled, the battery gets the maximum current available (upto 500mA) regardless of the speed which is often changing.

Yes... I guess that makes sense for you, but I wonder if we should be doing this by default. It seems a lot of cheap chargers can be easily destroyed if you overload them.

Pavel

Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
maintainer
#36 lore raw

Re: [Gta04-owner] [PATCH 10/13] twl4030_charger: add software controlled linear charging mode.

Quoted 31 lines · On Sat, 14 Nov 2015 19:12:16 +0100 Pavel Machek <[email protected]> wrote:

Hi!

Quoted 22 lines
Quoted 14 lines
Quoted 14 lines
Quoted 10 lines · Pavel Machek
Quoted 7 lines · NeilBrown

Add a 'continuous' option for usb charging which enables the "linear" charging mode of the twl4030.

Linear charging does a good job with not-so-reliable power sources. Auto mode does not work well as it switches off when voltage drops momentarily. Care must be taken not to over-charge.

Can you explain how the user can "care not to over-charge"?

The following text reads:

Quoted 8 lines
Quoted 4 lines

It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break.

so: take a break from cycling occasionally.

If the charger does not exceed 4.2V, I'd not call it overcharge. (Yes, some clever chargers actually let the battery drop below 4.2V when charge is done, but...)

Quoted 6 lines

Yes, that is the case. Perhaps it is not to be called overcharge but it is said that lithium battery charging has to stop if in CV mode the current drops too low. In automatic mode the charger does exactly that.
I would not let a battery for days at 4.2V CV.mode although a lot of cheap chargers

Quoted 4 lines

Well, I agree that keeping battery at 4.2V constant voltage mode is bad, but I'd not call it overcharge. If someone can fix the comment, that would be nice.

here is my original comment ("on" was replaced by continuous "now"):

twl4030_charger: add software controlled linear charging mode.

adds a sysfs control node to achieve that. It can be set to
auto: normal automatic charging is enabled (default)
off: charging is off
on: charing is on (software controlled)
CC/CV mode is still automatically done, but end of charge due to low current not.

Note: If linear charging mode is used there should be some method of stopping charging automatically. It is not a so time-critical, but it is the wrong setting for leaving a charger connected for several
days since Lithium batteries should not be kept at 100% for longer periods. Linear charging does a good job with not so reliable power sources, since several voltage controlling is then often too intelligent. It was used with a bike hub dynamo since a year or so. In that case there are automatically charging stops when the cyclist needs a break. Signed-off-by: Andreas Kemnade <[email protected]>

Quoted 16 lines
Quoted 16 lines
Quoted 8 lines

If the charger _does_ exceed 4.2V, then the battery will explode. Don't do that. Don't offer that to the user.

On a related note... I've just killed USB charger by overloading it. They are not protected.

I believe your automatically-pull-max-power really should stick to the well-known charging currents (.5A, 1A, 1.7A), at the very minimum.

The main reason for the patch was to prevent switching off charging when Vbus drops low. The reason was not to get out extremely much current out of the charger.
The electrical characteristics of a bicycle as a power source are.
- the amount of current available changes
- 500mA at around 17km/h
- you cannot destroy it by electrically overloading

Quoted 8 lines
Quoted 3 lines

If the current is set to e.g. 500mA and that linear charging mode is enabled, the battery gets the maximum current available (upto 500mA) regardless of the speed which is often changing.

Yes... I guess that makes sense for you, but I wonder if we should be doing this by default. It seems a lot of cheap chargers can be easily destroyed if you overload them.

Hmm, I guess the twl4030_charger would not be the only one destroying such chargers. I have seen such hub dynamo-friendly behaviour on every device I had connected to it before (an ipaq h2200, openmoko gta02). I have checked all usb wall plug chargers I have seen and I found none which has a lower current then 500mA. Only one has 500mA. The rest has 1A or even 2A.

But I think the non-ending cv stuff is a reason enough so that it is not the default charge method. I use it only at bootup when battery is low to have some time to fix charging issues manually and when cycling. Cycling is detected by acceleration values and I get some feedback if that charge mode is enabled or disabled.

Regards.
Andreas Kemnade

author
#37 lore raw

PATCH 11/13 twl4030_charger: add ac/mode to match usb/mode

In reply to NeilBrown

This allows AC charging to be turned off, much like usb charging. "continuous" mode is not available though.

---
 .../ABI/testing/sysfs-class-power-twl4030          |   10 ++++++
 drivers/power/twl4030_charger.c                    |   35 +++++++++++++++-----
 2 files changed, 37 insertions(+), 8 deletions(-)
Documentation/ABI/testing/sysfs-class-power-twl4030 +10 -0
index e8baa36aaa2b..be26af0f1895 100644
@@ -33,3 +33,13 @@ Description:
 			 This is useful for unstable power sources
 			 such as bicycle dynamo, but care should
 			 be taken that battery is not over-charged.
+
+What: /sys/class/power_supply/twl4030_ac/mode
+Description:
+	Changing mode for 'ac' port.
+	Writing to this can disable charging.
+
+	Possible values are:
+		"auto" - draw power as appropriate for detected
+			 power source and battery status.
+		"off"  - do not draw any power.
drivers/power/twl4030_charger.c +27 -8
index de5430deaf23..68117ad23564 100644
@@ -114,7 +114,7 @@ struct twl4030_bci {
 	unsigned int		ichg_eoc, ichg_lo, ichg_hi;
 	unsigned int		usb_cur, ac_cur;
 	bool			ac_is_active;
-	int			usb_mode; /* charging mode requested */
+	int			usb_mode, ac_mode; /* charging mode requested */
 #define	CHARGE_OFF	0
 #define	CHARGE_AUTO	1
 #define	CHARGE_LINEAR	2
@@ -459,10 +459,13 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 /*
  * Enable/Disable AC Charge funtionality.
  */
-static int twl4030_charger_enable_ac(bool enable)
+static int twl4030_charger_enable_ac(struct twl4030_bci *bci, bool enable)
 {
 	int ret;
 
+	if (bci->ac_mode == CHARGE_OFF)
+		enable = false;
+
 	if (enable)
 		ret = twl4030_clear_set_boot_bci(0, TWL4030_BCIAUTOAC);
 	else
@@ -688,9 +691,17 @@ twl4030_bci_mode_store(struct device *dev, struct device_attribute *attr,
 		mode = 2;
 	else
 		return -EINVAL;
-	twl4030_charger_enable_usb(bci, false);
-	bci->usb_mode = mode;
-	status = twl4030_charger_enable_usb(bci, true);
+	if (dev == &bci->ac->dev) {
+		if (mode == 2)
+			return -EINVAL;
+		twl4030_charger_enable_ac(bci, false);
+		bci->ac_mode = mode;
+		status = twl4030_charger_enable_ac(bci, true);
+	} else {
+		twl4030_charger_enable_usb(bci, false);
+		bci->usb_mode = mode;
+		status = twl4030_charger_enable_usb(bci, true);
+	}
 	return (status == 0) ? n : status;
 }
 
@@ -704,9 +715,13 @@ twl4030_bci_mode_show(struct device *dev,
 	struct twl4030_bci *bci = dev_get_drvdata(dev->parent);
 	int len = 0;
 	int i;
+	int mode = bci->usb_mode;
+
+	if (dev == &bci->ac->dev)
+		mode = bci->ac_mode;
 
 	for (i = 0; i < ARRAY_SIZE(modes); i++)
-		if (bci->usb_mode == i)
+		if (mode == i)
 			len += snprintf(buf+len, PAGE_SIZE-len,
 					"[%s] ", modes[i]);
 		else
@@ -916,6 +931,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	else
 		bci->usb_cur = 100000;  /* 100mA */
 	bci->usb_mode = CHARGE_AUTO;
+	bci->ac_mode = CHARGE_AUTO;
 
 	bci->dev = &pdev->dev;
 	bci->irq_chg = platform_get_irq(pdev, 0);
@@ -1001,10 +1017,12 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 	if (device_create_file(&bci->usb->dev, &dev_attr_mode))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
+	if (device_create_file(&bci->ac->dev, &dev_attr_mode))
+		dev_warn(&pdev->dev, "could not create sysfs file\n");
 	if (device_create_file(&bci->ac->dev, &dev_attr_max_current))
 		dev_warn(&pdev->dev, "could not create sysfs file\n");
 
-	twl4030_charger_enable_ac(true);
+	twl4030_charger_enable_ac(bci, true);
 	if (!IS_ERR_OR_NULL(bci->transceiver))
 		twl4030_bci_usb_ncb(&bci->usb_nb,
 				    bci->transceiver->last_event,
@@ -1024,13 +1042,14 @@ static int __exit twl4030_bci_remove(struct platform_device *pdev)
 {
 	struct twl4030_bci *bci = platform_get_drvdata(pdev);
 
-	twl4030_charger_enable_ac(false);
+	twl4030_charger_enable_ac(bci, false);
 	twl4030_charger_enable_usb(bci, false);
 	twl4030_charger_enable_backup(0, 0);
 
 	device_remove_file(&bci->usb->dev, &dev_attr_max_current);
 	device_remove_file(&bci->usb->dev, &dev_attr_mode);
 	device_remove_file(&bci->ac->dev, &dev_attr_max_current);
+	device_remove_file(&bci->ac->dev, &dev_attr_mode);
 	/* mask interrupts */
 	twl_i2c_write_u8(TWL4030_MODULE_INTERRUPTS, 0xff,
 			 TWL4030_INTERRUPTS_BCIIMR1A);
author
#38 lore raw

PATCH 12/13 twl4030_charger: Increase current carefully while watching voltage.

In reply to NeilBrown

The USB Battery Charging spec (BC1.2) suggests a dedicated charging port can deliver from 0.5 to 5.0A at between 4.75 and 5.25 volts.

To choose the "correct" current voltage setting requires a trial and error approach: try to draw current and see if the voltage drops too low.

Even with a configured Standard Downstream Port, it may not be possible to reliably pull 500mA - depending on cable quality and source quality I have reports of charging failure due to the voltage dropping too low.

To address both these concerns, this patch introduce incremental current setting.
The current pull from VBUS is increased in steps of 20mA every 100ms until the target is reached or until the measure voltage drops below 4.75V. If the voltage does go too low, the target current is reduced by 20mA and kept there.

This applies to currents selected automatically, or to values set via sysfs. So setting a large value will cause the maximum available to be used - up to the limit of 1.7A imposed by the hardware.

---
 drivers/power/twl4030_charger.c |   67 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 61 insertions(+), 6 deletions(-)
drivers/power/twl4030_charger.c +61 -6
index 68117ad23564..2c537ee11bbe 100644
@@ -119,6 +119,18 @@ struct twl4030_bci {
 #define	CHARGE_AUTO	1
 #define	CHARGE_LINEAR	2
 
+	/* When setting the USB current we slowly increase the
+	 * requested current until target is reached or the voltage
+	 * drops below 4.75V.  In the latter case we step back one
+	 * step.
+	 */
+	unsigned int		usb_cur_target;
+	struct delayed_work	current_worker;
+#define	USB_CUR_STEP	20000	/* 20mA at a time */
+#define	USB_MIN_VOLT	4750000	/* 4.75V */
+#define	USB_CUR_DELAY	msecs_to_jiffies(100)
+#define	USB_MAX_CURRENT	1700000 /* TWL4030 caps at 1.7A */
+
 	unsigned long		event;
 };
 
@@ -257,6 +269,12 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 	} else {
 		cur = bci->usb_cur;
 		bci->ac_is_active = false;
+		if (cur > bci->usb_cur_target) {
+			cur = bci->usb_cur_target;
+			bci->usb_cur = cur;
+		}
+		if (cur < bci->usb_cur_target)
+			schedule_delayed_work(&bci->current_worker, USB_CUR_DELAY);
 	}
 
 	/* First, check thresholds and see if cgain is needed */
@@ -391,6 +409,41 @@ static int twl4030_charger_update_current(struct twl4030_bci *bci)
 	return 0;
 }
 
+static int twl4030_charger_get_current(void);
+
+static void twl4030_current_worker(struct work_struct *data)
+{
+	int v, curr;
+	int res;
+	struct twl4030_bci *bci = container_of(data, struct twl4030_bci,
+					       current_worker.work);
+
+	res = twl4030bci_read_adc_val(TWL4030_BCIVBUS);
+	if (res < 0)
+		v = 0;
+	else
+		/* BCIVBUS uses ADCIN8, 7/1023 V/step */
+		v = res * 6843;
+	curr = twl4030_charger_get_current();
+
+	dev_dbg(bci->dev, "v=%d cur=%d limit=%d target=%d\n", v, curr,
+		bci->usb_cur, bci->usb_cur_target);
+
+	if (v < USB_MIN_VOLT) {
+		/* Back up and stop adjusting. */
+		bci->usb_cur -= USB_CUR_STEP;
+		bci->usb_cur_target = bci->usb_cur;
+	} else if (bci->usb_cur >= bci->usb_cur_target ||
+		   bci->usb_cur + USB_CUR_STEP > USB_MAX_CURRENT) {
+		/* Reached target and voltage is OK - stop */
+		return;
+	} else {
+		bci->usb_cur += USB_CUR_STEP;
+		schedule_delayed_work(&bci->current_worker, USB_CUR_DELAY);
+	}
+	twl4030_charger_update_current(bci);
+}
+
 /*
  * Enable/Disable USB Charge functionality.
  */
@@ -451,6 +504,7 @@ static int twl4030_charger_enable_usb(struct twl4030_bci *bci, bool enable)
 			pm_runtime_put_autosuspend(bci->transceiver->dev);
 			bci->usb_enabled = 0;
 		}
+		bci->usb_cur = 0;
 	}
 
 	return ret;
@@ -599,7 +653,7 @@ twl4030_bci_max_current_store(struct device *dev, struct device_attribute *attr,
 	if (dev == &bci->ac->dev)
 		bci->ac_cur = cur;
 	else
-		bci->usb_cur = cur;
+		bci->usb_cur_target = cur;
 
 	twl4030_charger_update_current(bci);
 	return n;
@@ -621,7 +675,7 @@ static ssize_t twl4030_bci_max_current_show(struct device *dev,
 			cur = bci->ac_cur;
 	} else {
 		if (bci->ac_is_active)
-			cur = bci->usb_cur;
+			cur = bci->usb_cur_target;
 	}
 	if (cur < 0) {
 		cur = twl4030bci_read_adc_val(TWL4030_BCIIREF1);
@@ -662,9 +716,9 @@ static int twl4030_bci_usb_ncb(struct notifier_block *nb, unsigned long val,
 
 	/* reset current on each 'plug' event */
 	if (allow_usb)
-		bci->usb_cur = 500000;
+		bci->usb_cur_target = 500000;
 	else
-		bci->usb_cur = 100000;
+		bci->usb_cur_target = 100000;
 
 	bci->event = val;
 	schedule_work(&bci->work);
@@ -927,9 +981,9 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	bci->ichg_hi = 500000; /* High threshold */
 	bci->ac_cur = 500000; /* 500mA */
 	if (allow_usb)
-		bci->usb_cur = 500000;  /* 500mA */
+		bci->usb_cur_target = 500000;  /* 500mA */
 	else
-		bci->usb_cur = 100000;  /* 100mA */
+		bci->usb_cur_target = 100000;  /* 100mA */
 	bci->usb_mode = CHARGE_AUTO;
 	bci->ac_mode = CHARGE_AUTO;
 
@@ -980,6 +1034,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	}
 
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
+	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
 
 	bci->usb_nb.notifier_call = twl4030_bci_usb_ncb;
 	if (bci->dev->of_node) {
author
#39 lore raw

PATCH 13/13 twl4030_charger: assume a 'charger' can supply maximum current.

In reply to NeilBrown

If it cannot, we will stop pulling more current when voltage drops.

---
 drivers/power/twl4030_charger.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/power/twl4030_charger.c +3 -1
index 2c537ee11bbe..c7432f532a83 100644
@@ -697,8 +697,10 @@ static void twl4030_bci_usb_work(struct work_struct *data)
 	struct twl4030_bci *bci = container_of(data, struct twl4030_bci, work);
 
 	switch (bci->event) {
-	case USB_EVENT_VBUS:
 	case USB_EVENT_CHARGER:
+		bci->usb_cur = USB_MAX_CURRENT;
+		/* FALL THROUGH */
+	case USB_EVENT_VBUS:
 		twl4030_charger_enable_usb(bci, true);
 		break;
 	case USB_EVENT_NONE:
#40 lore raw
In reply to NeilBrown

Hi,

Quoted 7 lines · NeilBrown

Following is most of my twl4030_charger patches, rebased against
git://git.infradead.org/battery-2.6

Since the previous set I have added the conversion to module_platform_driver so EPROBE_DEFER can be used, and fixed a few minor typos.

Quoted 5 lines

This does not include the changes to add extcon support, in part because extcon has seen some changes lately which leave me even more confused about how best to use it than before.
I need to sort that out before I can resolve the rest of my usb phy patches and then add a few more charger patches.

Thanks, queued.

-- Sebastian