Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1665613 > unrolled thread

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

Started by"H. Nikolaus Schaller" <hns@goldelico.com>
First post2017-06-14 11:30 +0200
Last post2017-06-14 11:40 +0200
Articles 8 — 3 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 0/4] More fixes for twl4030 charger "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-14 11:30 +0200
    [PATCH v6 2/4] power: supply: twl4030-charger: move allocation of iio channel to the beginning "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-14 11:30 +0200
      Re: [PATCH v6 2/4] power: supply: twl4030-charger: move allocation  of iio channel to the beginning Sebastian Reichel <sre@kernel.org> - 2017-06-14 22:20 +0200
    [PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by devm_iio_channel_get() and fix error path "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-14 11:30 +0200
      Re: [PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by  devm_iio_channel_get() and fix error path Sebastian Reichel <sre@kernel.org> - 2017-06-14 22:20 +0200
    [PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-14 11:30 +0200
      Re: [PATCH v6 3/4] power: supply: twl4030-charger: move irq  allocation to just before irqs are enabled Sebastian Reichel <sebastian.reichel@collabora.co.uk> - 2017-06-15 13:50 +0200
    Re: [PATCH v6 0/4] More fixes for twl4030 charger "H. Nikolaus Schaller" <hns@goldelico.com> - 2017-06-14 11:40 +0200

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

From"H. Nikolaus Schaller" <hns@goldelico.com>
Date2017-06-14 11:30 +0200
Subject[PATCH v6 0/4] More fixes for twl4030 charger
Message-ID<tScMF-3nJ-5@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

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

From"H. Nikolaus Schaller" <hns@goldelico.com>
Date2017-06-14 11:30 +0200
Subject[PATCH v6 2/4] power: supply: twl4030-charger: move allocation of iio channel to the beginning
Message-ID<tScMG-3nJ-17@gated-at.bofh.it>
In reply to#1665613
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

[toc] | [prev] | [next] | [standalone]

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

FromSebastian Reichel <sre@kernel.org>
Date2017-06-14 22:20 +0200
SubjectRe: [PATCH v6 2/4] power: supply: twl4030-charger: move allocation of iio channel to the beginning
Message-ID<tSmVH-1k9-11@gated-at.bofh.it>
In reply to#1665617

[Multipart message — attachments visible in raw view] — view raw

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@goldelico.com>

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
>

[toc] | [prev] | [next] | [standalone]

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

From"H. Nikolaus Schaller" <hns@goldelico.com>
Date2017-06-14 11:30 +0200
Subject[PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by devm_iio_channel_get() and fix error path
Message-ID<tScMG-3nJ-25@gated-at.bofh.it>
In reply to#1665613
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

[toc] | [prev] | [next] | [standalone]

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

FromSebastian Reichel <sre@kernel.org>
Date2017-06-14 22:20 +0200
SubjectRe: [PATCH v6 1/4] power: supply: twl4030-charger: allocate iio by devm_iio_channel_get() and fix error path
Message-ID<tSmVI-1k9-27@gated-at.bofh.it>
In reply to#1665621

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

From"H. Nikolaus Schaller" <hns@goldelico.com>
Date2017-06-14 11:30 +0200
Subject[PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled
Message-ID<tScMG-3nJ-21@gated-at.bofh.it>
In reply to#1665613
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

[toc] | [prev] | [next] | [standalone]

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

FromSebastian Reichel <sebastian.reichel@collabora.co.uk>
Date2017-06-15 13:50 +0200
SubjectRe: [PATCH v6 3/4] power: supply: twl4030-charger: move irq allocation to just before irqs are enabled
Message-ID<tSBrH-1XL-9@gated-at.bofh.it>
In reply to#1665625

[Multipart message — attachments visible in raw view] — view raw

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). 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

[toc] | [prev] | [next] | [standalone]

#1665632

From"H. Nikolaus Schaller" <hns@goldelico.com>
Date2017-06-14 11:40 +0200
Message-ID<tScWm-3r6-7@gated-at.bofh.it>
In reply to#1665613
> 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web