[PATCH 0/2] Fixes for twl4030 charger

[PATCH 0/2] Fixes for twl4030 charger

[H. Nikolaus Schaller]

4.3-rc1 introduced new charger driver for the twl4030.

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


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

 drivers/power/twl4030_charger.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

-- 
2.5.1

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

[H. Nikolaus Schaller]

devm_usb_get_phy_by_node() may return -EPROBE_DEFER in which
case we should also defer probing of the twl4030 charger
instead of turing USB charging off (forever).

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 423e886..859991f 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -1059,9 +1059,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 2/2] 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
usually be removed while device is operated through external AC
or USB power). So it makes no sense to check for it 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 859991f..e232453 100644
--- a/drivers/power/twl4030_charger.c
+++ b/drivers/power/twl4030_charger.c
@@ -1008,13 +1008,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 2/2] drivers:power:twl4030-charger: don't check if battery is present

[Nishanth Menon]

On 10/29/2015 12:01 PM, H. Nikolaus Schaller wrote:
> We can't assume that the battery is present after probing (it can
> usually be removed while device is operated through external AC
> or USB power). So it makes no sense to check for it during probe.


Do you mean hot plug battery? you sure twl4030 is capable of dealing
with that :) ? we ran into all kinds of issues with LDP3430 trying to
make that logic work, finally came to the conclusion that the TWL4030
as it stands on LDP cannot just do that.

> 
> 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 859991f..e232453 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -1008,13 +1008,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,
> 


-- 
Regards,
Nishanth Menon

Re: [PATCH 1/2] drivers:power:twl4030-charger: fix problem with EPROBE_DEFER

[Nishanth Menon]

On 10/29/2015 12:01 PM, H. Nikolaus Schaller wrote:
> devm_usb_get_phy_by_node() may return -EPROBE_DEFER in which
> case we should also defer probing of the twl4030 charger
> instead of turing USB charging off (forever).

s/turing/turning

> 
> 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 423e886..859991f 100644
> --- a/drivers/power/twl4030_charger.c
> +++ b/drivers/power/twl4030_charger.c
> @@ -1059,9 +1059,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)
I might align this with the IS_ERR

> +				return -EPROBE_DEFER;	/* PHY not ready */
> +		}
>  	}
>  
>  	/* Enable interrupts now. */
> 

Otherwise it looks good to me.

-- 
Regards,
Nishanth Menon

Re: [PATCH 1/2] drivers:power:twl4030-charger: fix problem with EPROBE_DEFER

[H. Nikolaus Schaller]

Am 29.10.2015 um 20:52 schrieb Nishanth Menon <nm@ti.com>:

> On 10/29/2015 12:01 PM, H. Nikolaus Schaller wrote:
>> devm_usb_get_phy_by_node() may return -EPROBE_DEFER in which
>> case we should also defer probing of the twl4030 charger
>> instead of turing USB charging off (forever).
> 
> s/turing/turning

Thanks for noting this.

> 
>> 
>> 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 423e886..859991f 100644
>> --- a/drivers/power/twl4030_charger.c
>> +++ b/drivers/power/twl4030_charger.c
>> @@ -1059,9 +1059,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)
> I might align this with the IS_ERR

Ok.

> 
>> +				return -EPROBE_DEFER;	/* PHY not ready */
>> +		}
>> 	}
>> 
>> 	/* Enable interrupts now. */
>> 
> 
> Otherwise it looks good to me.

I will update for v2.

BR,
Nikolaus Schaller
igitt

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

[H. Nikolaus Schaller]

Am 29.10.2015 um 20:50 schrieb Nishanth Menon <nm@ti.com>:

> On 10/29/2015 12:01 PM, H. Nikolaus Schaller wrote:
>> We can't assume that the battery is present after probing (it can
>> usually be removed while device is operated through external AC
>> or USB power). So it makes no sense to check for it during probe.
> 
> 
> Do you mean hot plug battery?

Yes.

> you sure twl4030 is capable of dealing
> with that :) ?

I am not sure about the twl4030 but with the tps65950 used in the GTA04,
it worked always in the past years with the old drivers (before 4.3-rc1 because
we have not tested the new ones well enough).

> we ran into all kinds of issues with LDP3430 trying to
> make that logic work, finally came to the conclusion that the TWL4030
> as it stands on LDP cannot just do that.

One issue is of course that AC or USB must provide enough current to
operate the board stand-alone. I.e. charging with high enough max_current
must be enabled.

A typical scenario I was using is to enable e.g. 800 mA max_current for
USB, connect the device to some laptop or USB wall charger and then
replace the battery.

This works with full and empty replacement batteries. Well, it should not
be too empty so that the inrush-current is not too much for the USB supply.

What we can't do is to boot from power-off without battery. In that case
the BCI isn't enabled for high enough current to operate the OMAP. But
as soon as the BCI enables enough current, it worked.

My main observation is that you can't make sure by software that the
user is not trying to hot-swap the battery. I.e. it can still happen after
probe time. This is why I think this check doesn't solve a problem.

So if there are problems with battery removed, we must find a solution
in the driver to handle that situation. And not only check once at boot time.

BR,
NIkolaus Schaller

> 
>> 
>> 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 859991f..e232453 100644
>> --- a/drivers/power/twl4030_charger.c
>> +++ b/drivers/power/twl4030_charger.c
>> @@ -1008,13 +1008,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,
>> 
> 
> 
> -- 
> Regards,
> Nishanth Menon