[PATCH v2 0/3] Fixes for twl4030 charger

[PATCH v2 0/3] Fixes for twl4030 charger

[H. Nikolaus Schaller]

Changes V2:
* worked in comments by Nishanth Menon <nm@ti.com>
* added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)

V1:
4.3-rc1 introduced a new charger driver for the twl4030.

While making it operable and testing on GTA04 and OpenPandora
we have found some issues.


H. Nikolaus Schaller (3):
  drivers:power:twl4030-charger: fix problem with EPROBE_DEFER
  drivers:power:twl4030-charger: don't return after allocating irq
  drivers:power:twl4030-charger: don't check if battery is present

 drivers/power/twl4030_charger.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

-- 
2.5.1

[PATCH v2 1/3] drivers:power:twl4030-charger: fix problem with EPROBE_DEFER

[H. Nikolaus Schaller]

If the phy-twol4030-usb driver is compiled as a module,
devm_usb_get_phy_by_node() may return -EPROBE_DEFER in which
case we should also defer probing of the twl4030 charger instead of
turning USB charging off (forever or until next reboot) further down in
code.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/twl4030_charger.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 74f2d3f..00697e9 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -1057,9 +1057,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;	/* PHY not ready */
+		}
 	}
 
 	/* Enable interrupts now. */
-- 
2.5.1

[PATCH v2 2/3] drivers:power:twl4030-charger: don't return after allocating irq

[H. Nikolaus Schaller]

It appears that simply returning with error status (especially
-EPROBE_DEFER) is very dangerous *after* allocating devm managed
interrupts.

See discussion of potential issues: https://lkml.org/lkml/2013/2/22/65

The result is that the boot process hangs if both, phy-twl4030-usb and this
twl4030_charger driver are compiled as modules. This has been observed
on gta04 and openpandora boards.

So we move the search and check that the twl4030-phy transceiver link is
available before we request to set up the interrupts.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/twl4030_charger.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 00697e9..05d693d 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -1031,23 +1031,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_chg, NULL,
-			twl4030_charger_interrupt, IRQF_ONESHOT, pdev->name,
-			bci);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
-			bci->irq_chg, ret);
-		return ret;
-	}
-
-	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_bci, NULL,
-			twl4030_bci_interrupt, IRQF_ONESHOT, pdev->name, bci);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
-			bci->irq_bci, ret);
-		return ret;
-	}
-
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
 
@@ -1066,6 +1049,23 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_chg, NULL,
+			twl4030_charger_interrupt, IRQF_ONESHOT, pdev->name,
+			bci);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
+			bci->irq_chg, ret);
+		return ret;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_bci, NULL,
+			twl4030_bci_interrupt, IRQF_ONESHOT, pdev->name, bci);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not request irq %d, status %d\n",
+			bci->irq_bci, ret);
+		return ret;
+	}
+
 	/* Enable interrupts now. */
 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
 		TWL4030_TBATOR1 | TWL4030_BATSTS);
-- 
2.5.1

[PATCH v2 3/3] drivers:power:twl4030-charger: don't check if battery is present

[H. Nikolaus Schaller]

We can't assume that the battery is present after probing (it can
sometimes be removed/hot swapped by the user while device
(e.g. GTA04 or OpenPanodra) is operated through external AC
or USB power).

So it makes no sense to check for this situation (only) during probe.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/twl4030_charger.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/power/twl4030_charger.c b/drivers/power/twl4030_charger.c
index 05d693d..6fbdf24 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -1006,13 +1006,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 	bci->irq_chg = platform_get_irq(pdev, 0);
 	bci->irq_bci = platform_get_irq(pdev, 1);
 
-	/* Only proceed further *IF* battery is physically present */
-	ret = twl4030_is_battery_present(bci);
-	if  (ret) {
-		dev_crit(&pdev->dev, "Battery was not detected:%d\n", ret);
-		return ret;
-	}
-
 	platform_set_drvdata(pdev, bci);
 
 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
-- 
2.5.1

Re: [PATCH v2 0/3] Fixes for twl4030 charger

[H. Nikolaus Schaller]

ping.

Am 02.11.2015 um 12:27 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> Changes V2:
> * worked in comments by Nishanth Menon <nm@ti.com>
> * added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)
> 
> V1:
> 4.3-rc1 introduced a new charger driver for the twl4030.
> 
> While making it operable and testing on GTA04 and OpenPandora
> we have found some issues.
> 
> 
> H. Nikolaus Schaller (3):
>  drivers:power:twl4030-charger: fix problem with EPROBE_DEFER
>  drivers:power:twl4030-charger: don't return after allocating irq
>  drivers:power:twl4030-charger: don't check if battery is present
> 
> drivers/power/twl4030_charger.c | 39 ++++++++++++++++++---------------------
> 1 file changed, 18 insertions(+), 21 deletions(-)
> 
> -- 
> 2.5.1
> 

Re: [Gta04-owner] [PATCH v2 0/3] Fixes for twl4030 charger

[H. Nikolaus Schaller]

ping.

Am 13.11.2015 um 11:28 schrieb H. Nikolaus Schaller <hns@goldelico.com>:

> ping.
> 
> Am 02.11.2015 um 12:27 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
>> Changes V2:
>> * worked in comments by Nishanth Menon <nm@ti.com>
>> * added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)
>> 
>> V1:
>> 4.3-rc1 introduced a new charger driver for the twl4030.
>> 
>> While making it operable and testing on GTA04 and OpenPandora
>> we have found some issues.
>> 
>> 
>> H. Nikolaus Schaller (3):
>> drivers:power:twl4030-charger: fix problem with EPROBE_DEFER
>> drivers:power:twl4030-charger: don't return after allocating irq
>> drivers:power:twl4030-charger: don't check if battery is present
>> 
>> drivers/power/twl4030_charger.c | 39 ++++++++++++++++++---------------------
>> 1 file changed, 18 insertions(+), 21 deletions(-)
>> 
>> -- 
>> 2.5.1
>> 
> 
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Re: [PATCH v2 0/3] Fixes for twl4030 charger

[Sebastian Reichel]

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

Hi Nikolaus,

On Mon, Nov 02, 2015 at 12:27:33PM +0100, H. Nikolaus Schaller wrote:
> Changes V2:
> * worked in comments by Nishanth Menon <nm@ti.com>
> * added another patch which solves a probing/boot stall problem (irq allocation vs. -EPROBE_DEFER)
> 
> V1:
> 4.3-rc1 introduced a new charger driver for the twl4030.
> 
> While making it operable and testing on GTA04 and OpenPandora
> we have found some issues.
> 
> 
> H. Nikolaus Schaller (3):
>   drivers:power:twl4030-charger: fix problem with EPROBE_DEFER
>   drivers:power:twl4030-charger: don't return after allocating irq
>   drivers:power:twl4030-charger: don't check if battery is present

PATCH 1 and 2 should be rebased to my current next branch.
They definetly do not take 2202e1fc5a into account. Also
take into account, that iio_channel_get() is not managed,
so you will have to free the channel, if your return an
error after it.

PATCH 3 introduces a new warning:

drivers/power/twl4030_charger.c:211:12: warning: ‘twl4030_is_battery_present’ defined but not used [-Wunused-function]

-- Sebastian

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