[PATCH v6 0/4] More fixes for twl4030 charger

[PATCH v6 0/4] More fixes for twl4030 charger

[H. Nikolaus Schaller]

Changes V6:
* split up -EPROBE_DEFER and irq allocation patch into small steps:
	* convert iio to devm allocation (by Sebsatian Reichel)
	* fix potentially wrong initialization sequence (by Grygorii Strashko)
	* add -EPROBE_DEFER

2017-05-21 12:38:24: Changes V5:
* reworked max_current removal patch (comments by Sebastian Reichel)
* resubmit missing madc connection for AC power detection
* resubmit patch for irq allocation and -EPROBE_DEFER
* rebased on 4.12-rc1

Changes V4:
* resent commit (original one did contain material not upstream)

2017-04-14 21:29:39: 2017-04-14 20:26:00: Changes V3:
* worked in comments by Sebsatian Reichel
* clarifications of some commit messages
* rebased on v4.11rc-6

It took a while (18 months) until we propose an updated patch for upstream...

2015-11-02 12:27:39: 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. This patch set fixes some
issues.

While making twl4030 changes from 4.3 operable we have found some issues
during testing on GTA04 and OpenPandora.

H. Nikolaus Schaller (4):
  power: supply: twl4030-charger: allocate iio by devm_iio_channel_get()
    and fix error path
  power: supply: twl4030-charger: move allocation of iio channel to the
    beginning
  power: supply: twl4030-charger: move irq allocation to just before
    irqs are enabled
  power: supply: twl4030-charger: add deferred probing for phy and iio

 drivers/power/supply/twl4030_charger.c | 54 +++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 27 deletions(-)

-- 
2.12.2

[PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by devm_iio_channel_get() and fix error path

[H. Nikolaus Schaller]

Suggested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/supply/twl4030_charger.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 785a07bc4f39..9507c24495ba 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -1017,7 +1017,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
+	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
 	if (IS_ERR(bci->channel_vac)) {
 		bci->channel_vac = NULL;
 		dev_warn(&pdev->dev, "could not request vac iio channel");
@@ -1044,7 +1044,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 			       TWL4030_INTERRUPTS_BCIIMR1A);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
-		goto fail;
+		return ret;
 	}
 
 	reg = ~(u32)(TWL4030_VBATOV | TWL4030_VBUSOV | TWL4030_ACCHGOV);
@@ -1073,10 +1073,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		twl4030_charger_enable_backup(0, 0);
 
 	return 0;
-fail:
-	iio_channel_release(bci->channel_vac);
-
-	return ret;
 }
 
 static int twl4030_bci_remove(struct platform_device *pdev)
@@ -1087,8 +1083,6 @@ static int twl4030_bci_remove(struct platform_device *pdev)
 	twl4030_charger_enable_usb(bci, false);
 	twl4030_charger_enable_backup(0, 0);
 
-	iio_channel_release(bci->channel_vac);
-
 	device_remove_file(&bci->usb->dev, &dev_attr_mode);
 	device_remove_file(&bci->ac->dev, &dev_attr_mode);
 	/* mask interrupts */
-- 
2.12.2

Re: [PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by devm_iio_channel_get() and fix error path

[Sebastian Reichel]

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

Hi,

On Wed, Jun 14, 2017 at 11:25:53AM +0200, H. Nikolaus Schaller wrote:
> Suggested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/power/supply/twl4030_charger.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 785a07bc4f39..9507c24495ba 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -1017,7 +1017,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	bci->channel_vac = iio_channel_get(&pdev->dev, "vac");
> +	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
>  	if (IS_ERR(bci->channel_vac)) {
>  		bci->channel_vac = NULL;
>  		dev_warn(&pdev->dev, "could not request vac iio channel");
> @@ -1044,7 +1044,7 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  			       TWL4030_INTERRUPTS_BCIIMR1A);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "failed to unmask interrupts: %d\n", ret);
> -		goto fail;
> +		return ret;
>  	}
>  
>  	reg = ~(u32)(TWL4030_VBATOV | TWL4030_VBUSOV | TWL4030_ACCHGOV);
> @@ -1073,10 +1073,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		twl4030_charger_enable_backup(0, 0);
>  
>  	return 0;
> -fail:
> -	iio_channel_release(bci->channel_vac);
> -
> -	return ret;
>  }
>  
>  static int twl4030_bci_remove(struct platform_device *pdev)
> @@ -1087,8 +1083,6 @@ static int twl4030_bci_remove(struct platform_device *pdev)
>  	twl4030_charger_enable_usb(bci, false);
>  	twl4030_charger_enable_backup(0, 0);
>  
> -	iio_channel_release(bci->channel_vac);
> -
>  	device_remove_file(&bci->usb->dev, &dev_attr_mode);
>  	device_remove_file(&bci->ac->dev, &dev_attr_mode);
>  	/* mask interrupts */

Thanks, queued.

-- Sebastian

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

[PATCH v6 2/4] power: supply: twl4030-charger: move allocation of iio channel to the beginning

[H. Nikolaus Schaller]

This is in prepraration for EPROBE_DEFER handling because it is quite
likely that geting the (madc) iio channel is deferred more often than
later steps.

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

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 9507c24495ba..1fbbe0cc216a 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -984,6 +984,12 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bci);
 
+	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
+	if (IS_ERR(bci->channel_vac)) {
+		bci->channel_vac = NULL;
+		dev_warn(&pdev->dev, "could not request vac iio channel");
+	}
+
 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
 					     NULL);
 	if (IS_ERR(bci->ac)) {
@@ -1017,12 +1023,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
-	if (IS_ERR(bci->channel_vac)) {
-		bci->channel_vac = NULL;
-		dev_warn(&pdev->dev, "could not request vac iio channel");
-	}
-
 	INIT_WORK(&bci->work, twl4030_bci_usb_work);
 	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
 
-- 
2.12.2

Re: [PATCH v6 2/4] power: supply: twl4030-charger: move allocation of iio channel to the beginning

[Sebastian Reichel]

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

Hi,

On Wed, Jun 14, 2017 at 11:25:54AM +0200, H. Nikolaus Schaller wrote:
> This is in prepraration for EPROBE_DEFER handling because it is quite
> likely that geting the (madc) iio channel is deferred more often than
> later steps.
> 
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>

Thanks, queued.

-- Sebastian

>  drivers/power/supply/twl4030_charger.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 9507c24495ba..1fbbe0cc216a 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -984,6 +984,12 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, bci);
>  
> +	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
> +	if (IS_ERR(bci->channel_vac)) {
> +		bci->channel_vac = NULL;
> +		dev_warn(&pdev->dev, "could not request vac iio channel");
> +	}
> +
>  	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
>  					     NULL);
>  	if (IS_ERR(bci->ac)) {
> @@ -1017,12 +1023,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
> -	if (IS_ERR(bci->channel_vac)) {
> -		bci->channel_vac = NULL;
> -		dev_warn(&pdev->dev, "could not request vac iio channel");
> -	}
> -
>  	INIT_WORK(&bci->work, twl4030_bci_usb_work);
>  	INIT_DELAYED_WORK(&bci->current_worker, twl4030_current_worker);
>  
> -- 
> 2.12.2
> 

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

[PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled

[H. Nikolaus Schaller]

This avoids a potential race if irqs are enabled and triggered too early
before the worker is properly set up.

Suggested-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/power/supply/twl4030_charger.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 1fbbe0cc216a..3bebeecb4a1f 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -984,6 +984,16 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, bci);
 
+	if (bci->dev->of_node) {
+		struct device_node *phynode;
+
+		phynode = of_find_compatible_node(bci->dev->of_node->parent,
+						  NULL, "ti,twl4030-usb");
+		if (phynode)
+			bci->transceiver = devm_usb_get_phy_by_node(
+				bci->dev, phynode, &bci->usb_nb);
+	}
+
 	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
 	if (IS_ERR(bci->channel_vac)) {
 		bci->channel_vac = NULL;
@@ -1006,6 +1016,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	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;
 	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_chg, NULL,
 			twl4030_charger_interrupt, IRQF_ONESHOT, pdev->name,
 			bci);
@@ -1023,20 +1037,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	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) {
-		struct device_node *phynode;
-
-		phynode = of_find_compatible_node(bci->dev->of_node->parent,
-						  NULL, "ti,twl4030-usb");
-		if (phynode)
-			bci->transceiver = devm_usb_get_phy_by_node(
-				bci->dev, phynode, &bci->usb_nb);
-	}
-
 	/* Enable interrupts now. */
 	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
 		TWL4030_TBATOR1 | TWL4030_BATSTS);
-- 
2.12.2

Re: [PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled

[Sebastian Reichel]

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

Hi,

On Wed, Jun 14, 2017 at 11:25:55AM +0200, H. Nikolaus Schaller wrote:
> This avoids a potential race if irqs are enabled and triggered too early
> before the worker is properly set up.
> 
> Suggested-by: Grygorii Strashko <grygorii.strashko-l0cyMroinI0@public.gmane.org>
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
>  drivers/power/supply/twl4030_charger.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 1fbbe0cc216a..3bebeecb4a1f 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -984,6 +984,16 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, bci);
>  
> +	if (bci->dev->of_node) {
> +		struct device_node *phynode;
> +
> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> +						  NULL, "ti,twl4030-usb");
> +		if (phynode)
> +			bci->transceiver = devm_usb_get_phy_by_node(
> +				bci->dev, phynode, &bci->usb_nb);
> +	}
> +

The notifier is not that much different from an irq. I see
the call can access at least the iio channel (so iio channel
should be registered first). I suspect worker and power-supply
should also be initialized/registered first. Best location seems
to be directly before requesting the irqs.

>  	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
>  	if (IS_ERR(bci->channel_vac)) {
>  		bci->channel_vac = NULL;
> @@ -1006,6 +1016,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	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;

You should configure the notifier block *before* registering it.

>  	ret = devm_request_threaded_irq(&pdev->dev, bci->irq_chg, NULL,
>  			twl4030_charger_interrupt, IRQF_ONESHOT, pdev->name,
>  			bci);
> @@ -1023,20 +1037,6 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	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) {
> -		struct device_node *phynode;
> -
> -		phynode = of_find_compatible_node(bci->dev->of_node->parent,
> -						  NULL, "ti,twl4030-usb");
> -		if (phynode)
> -			bci->transceiver = devm_usb_get_phy_by_node(
> -				bci->dev, phynode, &bci->usb_nb);
> -	}
> -
>  	/* Enable interrupts now. */
>  	reg = ~(u32)(TWL4030_ICHGLOW | TWL4030_ICHGEOC | TWL4030_TBATOR2 |
>  		TWL4030_TBATOR1 | TWL4030_BATSTS);

-- Sebastian

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

Re: [PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled

[H. Nikolaus Schaller]

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

Hi Sebastian,

> Am 15.06.2017 um 13:47 schrieb Sebastian Reichel <sebastian.reichel@collabora.co.uk>:
> 
> Hi,
> 
> On Wed, Jun 14, 2017 at 11:25:55AM +0200, H. Nikolaus Schaller wrote:
>> This avoids a potential race if irqs are enabled and triggered too early
>> before the worker is properly set up.
>> 
>> Suggested-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/power/supply/twl4030_charger.c | 28 ++++++++++++++--------------
>> 1 file changed, 14 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>> index 1fbbe0cc216a..3bebeecb4a1f 100644
>> --- a/drivers/power/supply/twl4030_charger.c
>> +++ b/drivers/power/supply/twl4030_charger.c
>> @@ -984,6 +984,16 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>> 
>> 	platform_set_drvdata(pdev, bci);
>> 
>> +	if (bci->dev->of_node) {
>> +		struct device_node *phynode;
>> +
>> +		phynode = of_find_compatible_node(bci->dev->of_node->parent,
>> +						  NULL, "ti,twl4030-usb");
>> +		if (phynode)
>> +			bci->transceiver = devm_usb_get_phy_by_node(
>> +				bci->dev, phynode, &bci->usb_nb);
>> +	}
>> +
> 
> The notifier is not that much different from an irq. I see
> the call can access at least the iio channel (so iio channel
> should be registered first).

Hm. I only see that it can schedule the worker (which means the worker should be
initialized first) but does not access iio by itself.

Can the worker run before probe() succeeds?

> I suspect worker and power-supply
> should also be initialized/registered first. Best location seems
> to be directly before requesting the irqs.

I think they should be initialized even before devm_usb_get_phy_by_node().

> 
>> 	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
>> 	if (IS_ERR(bci->channel_vac)) {
>> 		bci->channel_vac = NULL;
>> @@ -1006,6 +1016,10 @@ static int twl4030_bci_probe(struct platform_device *pdev)
>> 		return ret;
>> 	}
>> 
>> +	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;
> 
> You should configure the notifier block *before* registering it.

Isn't the notifier block already configured by devm_usb_get_phy_by_node() - except for the pointer to the notifier_call?

So the only harmful thing I can see is that a notifier arrives with usb_nb.notifier_call still being uninitialized as NULL.

Is this harmful or not? I don't know.

I can only guess that it could equally well be set right after devm_usb_get_phy_by_node()
and I have reworked it that way.

It still works on GTA04 and OpenPandora with neither observing a regression nor an improvement.

So I will send a v7 with what I think should be most robust.

BR and thanks,
Nikolaus


[-- Attachment #2: Type: text/html, Size: 7228 bytes --]

[PATCH v6 4/4] power: supply: twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

This fixes an issue if both this twl4030_charger driver and
phy-twl4030-usb are compiled as modules and loaded in random order.

It has been observed on GTA04 and OpenPandora devices that in worst
case the boot process hangs and in best case the AC detection fails
with a warning.

Therefore we add deferred probing checks for the usb_phy and the
iio channel for AC detection.

Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
---
 drivers/power/supply/twl4030_charger.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
index 3bebeecb4a1f..6ac8816262bd 100644
--- a/drivers/power/supply/twl4030_charger.c
+++ b/drivers/power/supply/twl4030_charger.c
@@ -989,15 +989,21 @@ 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 */
+		}
 	}
 
 	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
 	if (IS_ERR(bci->channel_vac)) {
-		bci->channel_vac = NULL;
+		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;	/* iio not ready */
 		dev_warn(&pdev->dev, "could not request vac iio channel");
+		bci->channel_vac = NULL;
 	}
 
 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
-- 
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 4/4] power: supply: twl4030-charger: add deferred probing for phy and iio

[Sebastian Reichel]

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

Hi,

On Wed, Jun 14, 2017 at 11:25:56AM +0200, H. Nikolaus Schaller wrote:
> This fixes an issue if both this twl4030_charger driver and
> phy-twl4030-usb are compiled as modules and loaded in random order.
> 
> It has been observed on GTA04 and OpenPandora devices that in worst
> case the boot process hangs and in best case the AC detection fails
> with a warning.
> 
> Therefore we add deferred probing checks for the usb_phy and the
> iio channel for AC detection.
> 
> Signed-off-by: H. Nikolaus Schaller <hns-xXXSsgcRVICgSpxsJD1C4w@public.gmane.org>
> ---
>  drivers/power/supply/twl4030_charger.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
> index 3bebeecb4a1f..6ac8816262bd 100644
> --- a/drivers/power/supply/twl4030_charger.c
> +++ b/drivers/power/supply/twl4030_charger.c
> @@ -989,15 +989,21 @@ 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 */
> +		}
>  	}

Let's also set to NULL + dev_warn for other errors (like the iio
channel error handling).

>  	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
>  	if (IS_ERR(bci->channel_vac)) {
> -		bci->channel_vac = NULL;
> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;	/* iio not ready */
>  		dev_warn(&pdev->dev, "could not request vac iio channel");
> +		bci->channel_vac = NULL;
>  	}
>  
>  	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,

-- Sebastian

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

Re: [PATCH v6 4/4] power: supply: twl4030-charger: add deferred probing for phy and iio

[H. Nikolaus Schaller]

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


> Am 15.06.2017 um 13:57 schrieb Sebastian Reichel <sebastian.reichel@collabora.co.uk>:
> 
> Hi,
> 
> On Wed, Jun 14, 2017 at 11:25:56AM +0200, H. Nikolaus Schaller wrote:
>> This fixes an issue if both this twl4030_charger driver and
>> phy-twl4030-usb are compiled as modules and loaded in random order.
>> 
>> It has been observed on GTA04 and OpenPandora devices that in worst
>> case the boot process hangs and in best case the AC detection fails
>> with a warning.
>> 
>> Therefore we add deferred probing checks for the usb_phy and the
>> iio channel for AC detection.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>> drivers/power/supply/twl4030_charger.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/power/supply/twl4030_charger.c b/drivers/power/supply/twl4030_charger.c
>> index 3bebeecb4a1f..6ac8816262bd 100644
>> --- a/drivers/power/supply/twl4030_charger.c
>> +++ b/drivers/power/supply/twl4030_charger.c
>> @@ -989,15 +989,21 @@ 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 */
>> +		}
>> 	}
> 
> Let's also set to NULL + dev_warn for other errors (like the iio
> channel error handling).

Added to v7.

Also added a %d to print the error number to aid spotting the problems.

> 
>> 	bci->channel_vac = devm_iio_channel_get(&pdev->dev, "vac");
>> 	if (IS_ERR(bci->channel_vac)) {
>> -		bci->channel_vac = NULL;
>> +		if (PTR_ERR(bci->channel_vac) == -EPROBE_DEFER)
>> +			return -EPROBE_DEFER;	/* iio not ready */
>> 		dev_warn(&pdev->dev, "could not request vac iio channel");
>> +		bci->channel_vac = NULL;
>> 	}
>> 
>> 	bci->ac = devm_power_supply_register(&pdev->dev, &twl4030_bci_ac_desc,
> 
> -- Sebastian


[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v6 0/4] More fixes for twl4030 charger

[H. Nikolaus Schaller]

> Am 14.06.2017 um 11:25 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Changes V6:
> * split up -EPROBE_DEFER and irq allocation patch into small steps:
> 	* convert iio to devm allocation (by Sebsatian Reichel)
> 	* fix potentially wrong initialization sequence (by Grygorii Strashko)
> 	* add -EPROBE_DEFER
> 
> 2017-05-21 12:38:24: Changes V5:
> * reworked max_current removal patch (comments by Sebastian Reichel)
> * resubmit missing madc connection for AC power detection
> * resubmit patch for irq allocation and -EPROBE_DEFER
> * rebased on 4.12-rc1
> 
> Changes V4:
> * resent commit (original one did contain material not upstream)
> 
> 2017-04-14 21:29:39: 2017-04-14 20:26:00: Changes V3:
> * worked in comments by Sebsatian Reichel
> * clarifications of some commit messages
> * rebased on v4.11rc-6
> 
> It took a while (18 months) until we propose an updated patch for upstream...
> 
> 2015-11-02 12:27:39: 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. This patch set fixes some
> issues.
> 
> While making twl4030 changes from 4.3 operable we have found some issues
> during testing on GTA04 and OpenPandora.
> 
> H. Nikolaus Schaller (4):
>  power: supply: twl4030-charger: allocate iio by devm_iio_channel_get()
>    and fix error path
>  power: supply: twl4030-charger: move allocation of iio channel to the
>    beginning
>  power: supply: twl4030-charger: move irq allocation to just before
>    irqs are enabled
>  power: supply: twl4030-charger: add deferred probing for phy and iio

I should note that I have split up the original single commit into 4 separate patches.
If maintainers thing this is too fine granularity, please squash them.

The result is tested to work on GTA04.