[RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order

linux-pm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed

* [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order
@ 2019-10-04 15:07 Krzysztof Kozlowski
  2019-10-04 15:07 ` [RFT 2/3] power: supply: ab8500_fg: Do not free non-requested IRQs in probe's error path Krzysztof Kozlowski
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Krzysztof Kozlowski @ 2019-10-04 15:07 UTC (permalink / raw)
  To: Sebastian Reichel, Krzysztof Kozlowski, Gustavo A. R. Silva,
	Linus Walleij, linux-pm, linux-kernel

It is logical to cleanup in probe's error path in reverse order to
previous actions.  It also makes easier to add additional goto labels
within this error path.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Not marking as cc-stable as this was not reproduced and not tested.
---
 drivers/power/supply/ab8500_btemp.c | 4 ++--
 drivers/power/supply/ab8500_fg.c    | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/ab8500_btemp.c b/drivers/power/supply/ab8500_btemp.c
index 8fe81259bfd9..a167938e32f2 100644
--- a/drivers/power/supply/ab8500_btemp.c
+++ b/drivers/power/supply/ab8500_btemp.c
@@ -1104,13 +1104,13 @@ static int ab8500_btemp_probe(struct platform_device *pdev)
 	return ret;
 
 free_irq:
-	power_supply_unregister(di->btemp_psy);
-
 	/* We also have to free all successfully registered irqs */
 	for (i = i - 1; i >= 0; i--) {
 		irq = platform_get_irq_byname(pdev, ab8500_btemp_irq[i].name);
 		free_irq(irq, di);
 	}
+
+	power_supply_unregister(di->btemp_psy);
 free_btemp_wq:
 	destroy_workqueue(di->btemp_wq);
 	return ret;
diff --git a/drivers/power/supply/ab8500_fg.c b/drivers/power/supply/ab8500_fg.c
index 6fc4bc30644c..36bbb8ea05da 100644
--- a/drivers/power/supply/ab8500_fg.c
+++ b/drivers/power/supply/ab8500_fg.c
@@ -3212,15 +3212,15 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 	return ret;
 
 free_irq:
-	power_supply_unregister(di->fg_psy);
-
 	/* We also have to free all registered irqs */
+	irq = platform_get_irq_byname(pdev, ab8500_fg_irq_bh[0].name);
+	free_irq(irq, di);
 	for (i = 0; i < ARRAY_SIZE(ab8500_fg_irq_th); i++) {
 		irq = platform_get_irq_byname(pdev, ab8500_fg_irq_th[i].name);
 		free_irq(irq, di);
 	}
-	irq = platform_get_irq_byname(pdev, ab8500_fg_irq_bh[0].name);
-	free_irq(irq, di);
+
+	power_supply_unregister(di->fg_psy);
 free_inst_curr_wq:
 	destroy_workqueue(di->fg_wq);
 	return ret;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [RFT 2/3] power: supply: ab8500_fg: Do not free non-requested IRQs in probe's error path
  2019-10-04 15:07 [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Krzysztof Kozlowski
@ 2019-10-04 15:07 ` Krzysztof Kozlowski
  2019-10-04 15:07 ` [RFT 3/3] power: supply: ab8500: Handle invalid IRQ from platform_get_irq_byname() Krzysztof Kozlowski
  2019-10-16  8:33 ` [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Linus Walleij
  2 siblings, 0 replies; 5+ messages in thread
From: Krzysztof Kozlowski @ 2019-10-04 15:07 UTC (permalink / raw)
  To: Sebastian Reichel, Krzysztof Kozlowski, Gustavo A. R. Silva,
	Linus Walleij, linux-pm, linux-kernel

When requesting interrupt fails, free only interrupts already requested,
not all of them.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Not marking as cc-stable as this was not reproduced and not tested.
---
 drivers/power/supply/ab8500_fg.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/ab8500_fg.c b/drivers/power/supply/ab8500_fg.c
index 36bbb8ea05da..7d879589adc2 100644
--- a/drivers/power/supply/ab8500_fg.c
+++ b/drivers/power/supply/ab8500_fg.c
@@ -3158,7 +3158,7 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 		if (ret != 0) {
 			dev_err(di->dev, "failed to request %s IRQ %d: %d\n",
 				ab8500_fg_irq_th[i].name, irq, ret);
-			goto free_irq;
+			goto free_irq_th;
 		}
 		dev_dbg(di->dev, "Requested %s IRQ %d: %d\n",
 			ab8500_fg_irq_th[i].name, irq, ret);
@@ -3173,7 +3173,7 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 	if (ret != 0) {
 		dev_err(di->dev, "failed to request %s IRQ %d: %d\n",
 			ab8500_fg_irq_bh[0].name, irq, ret);
-		goto free_irq;
+		goto free_irq_th;
 	}
 	dev_dbg(di->dev, "Requested %s IRQ %d: %d\n",
 		ab8500_fg_irq_bh[0].name, irq, ret);
@@ -3215,7 +3215,9 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 	/* We also have to free all registered irqs */
 	irq = platform_get_irq_byname(pdev, ab8500_fg_irq_bh[0].name);
 	free_irq(irq, di);
-	for (i = 0; i < ARRAY_SIZE(ab8500_fg_irq_th); i++) {
+free_irq_th:
+	while (--i >= 0) {
+		/* Last assignment of i from primary interrupt handlers */
 		irq = platform_get_irq_byname(pdev, ab8500_fg_irq_th[i].name);
 		free_irq(irq, di);
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* [RFT 3/3] power: supply: ab8500: Handle invalid IRQ from platform_get_irq_byname()
  2019-10-04 15:07 [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Krzysztof Kozlowski
  2019-10-04 15:07 ` [RFT 2/3] power: supply: ab8500_fg: Do not free non-requested IRQs in probe's error path Krzysztof Kozlowski
@ 2019-10-04 15:07 ` Krzysztof Kozlowski
  2019-10-16  8:33 ` [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Linus Walleij
  2 siblings, 0 replies; 5+ messages in thread
From: Krzysztof Kozlowski @ 2019-10-04 15:07 UTC (permalink / raw)
  To: Sebastian Reichel, Krzysztof Kozlowski, Gustavo A. R. Silva,
	Linus Walleij, linux-pm, linux-kernel

platform_get_irq_byname() might return -errno which later would be
cast to an unsigned int and used in request_irq().

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Not marking as cc-stable as this was not reproduced and not tested.
---
 drivers/power/supply/ab8500_btemp.c   |  5 +++++
 drivers/power/supply/ab8500_charger.c |  5 +++++
 drivers/power/supply/ab8500_fg.c      | 10 ++++++++++
 3 files changed, 20 insertions(+)

diff --git a/drivers/power/supply/ab8500_btemp.c b/drivers/power/supply/ab8500_btemp.c
index a167938e32f2..cfd8152bf8fc 100644
--- a/drivers/power/supply/ab8500_btemp.c
+++ b/drivers/power/supply/ab8500_btemp.c
@@ -1082,6 +1082,11 @@ static int ab8500_btemp_probe(struct platform_device *pdev)
 	/* Register interrupts */
 	for (i = 0; i < ARRAY_SIZE(ab8500_btemp_irq); i++) {
 		irq = platform_get_irq_byname(pdev, ab8500_btemp_irq[i].name);
+		if (irq < 0) {
+			ret = irq;
+			goto free_irq;
+		}
+
 		ret = request_threaded_irq(irq, NULL, ab8500_btemp_irq[i].isr,
 			IRQF_SHARED | IRQF_NO_SUSPEND,
 			ab8500_btemp_irq[i].name, di);
diff --git a/drivers/power/supply/ab8500_charger.c b/drivers/power/supply/ab8500_charger.c
index e51d0e72beea..7ad23df6f75a 100644
--- a/drivers/power/supply/ab8500_charger.c
+++ b/drivers/power/supply/ab8500_charger.c
@@ -3556,6 +3556,11 @@ static int ab8500_charger_probe(struct platform_device *pdev)
 	/* Register interrupts */
 	for (i = 0; i < ARRAY_SIZE(ab8500_charger_irq); i++) {
 		irq = platform_get_irq_byname(pdev, ab8500_charger_irq[i].name);
+		if (irq < 0) {
+			ret = irq;
+			goto free_irq;
+		}
+
 		ret = request_threaded_irq(irq, NULL, ab8500_charger_irq[i].isr,
 			IRQF_SHARED | IRQF_NO_SUSPEND,
 			ab8500_charger_irq[i].name, di);
diff --git a/drivers/power/supply/ab8500_fg.c b/drivers/power/supply/ab8500_fg.c
index 7d879589adc2..01f3da103813 100644
--- a/drivers/power/supply/ab8500_fg.c
+++ b/drivers/power/supply/ab8500_fg.c
@@ -3151,6 +3151,11 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 	/* Register primary interrupt handlers */
 	for (i = 0; i < ARRAY_SIZE(ab8500_fg_irq_th); i++) {
 		irq = platform_get_irq_byname(pdev, ab8500_fg_irq_th[i].name);
+		if (irq < 0) {
+			ret = irq;
+			goto free_irq_th;
+		}
+
 		ret = request_irq(irq, ab8500_fg_irq_th[i].isr,
 				  IRQF_SHARED | IRQF_NO_SUSPEND,
 				  ab8500_fg_irq_th[i].name, di);
@@ -3166,6 +3171,11 @@ static int ab8500_fg_probe(struct platform_device *pdev)
 
 	/* Register threaded interrupt handler */
 	irq = platform_get_irq_byname(pdev, ab8500_fg_irq_bh[0].name);
+	if (irq < 0) {
+		ret = irq;
+		goto free_irq_th;
+	}
+
 	ret = request_threaded_irq(irq, NULL, ab8500_fg_irq_bh[0].isr,
 				IRQF_SHARED | IRQF_NO_SUSPEND | IRQF_ONESHOT,
 			ab8500_fg_irq_bh[0].name, di);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order
  2019-10-04 15:07 [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Krzysztof Kozlowski
  2019-10-04 15:07 ` [RFT 2/3] power: supply: ab8500_fg: Do not free non-requested IRQs in probe's error path Krzysztof Kozlowski
  2019-10-04 15:07 ` [RFT 3/3] power: supply: ab8500: Handle invalid IRQ from platform_get_irq_byname() Krzysztof Kozlowski
@ 2019-10-16  8:33 ` Linus Walleij
  2019-10-20 13:23   ` Sebastian Reichel
  2 siblings, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2019-10-16  8:33 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Sebastian Reichel, Gustavo A. R. Silva, Linux PM list, linux-kernel

On Fri, Oct 4, 2019 at 5:07 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:

> It is logical to cleanup in probe's error path in reverse order to
> previous actions.  It also makes easier to add additional goto labels
> within this error path.
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

For all 3 patches:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

The battery charging code is currently disabled on ux500 simply
because no platforms with batteries were available for testing
or supported by any device trees.

This is getting fix: PostmarketOS is brewing patches for enabling
all Ux500-based Samsung phones, all with batteries. So we will
soon be able to test and turn this on.

The patches are fine to merge, however notice that we are
refactoring all drivers using ADC through the IIO tree:
https://lore.kernel.org/linux-iio/20191011071805.5554-4-linus.walleij@linaro.org/
https://lore.kernel.org/linux-iio/20191011071805.5554-2-linus.walleij@linaro.org/
https://lore.kernel.org/linux-iio/20191011071805.5554-3-linus.walleij@linaro.org/

It would be nice if we could avoid colissions.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order
  2019-10-16  8:33 ` [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Linus Walleij
@ 2019-10-20 13:23   ` Sebastian Reichel
  0 siblings, 0 replies; 5+ messages in thread
From: Sebastian Reichel @ 2019-10-20 13:23 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Krzysztof Kozlowski, Gustavo A. R. Silva, Linux PM list, linux-kernel

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

Hi,

On Wed, Oct 16, 2019 at 10:33:12AM +0200, Linus Walleij wrote:
> On Fri, Oct 4, 2019 at 5:07 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> 
> > It is logical to cleanup in probe's error path in reverse order to
> > previous actions.  It also makes easier to add additional goto labels
> > within this error path.
> >
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> For all 3 patches:
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> 
> The battery charging code is currently disabled on ux500 simply
> because no platforms with batteries were available for testing
> or supported by any device trees.
> 
> This is getting fix: PostmarketOS is brewing patches for enabling
> all Ux500-based Samsung phones, all with batteries. So we will
> soon be able to test and turn this on.
> 
> The patches are fine to merge, however notice that we are
> refactoring all drivers using ADC through the IIO tree:
> https://lore.kernel.org/linux-iio/20191011071805.5554-4-linus.walleij@linaro.org/
> https://lore.kernel.org/linux-iio/20191011071805.5554-2-linus.walleij@linaro.org/
> https://lore.kernel.org/linux-iio/20191011071805.5554-3-linus.walleij@linaro.org/
> 
> It would be nice if we could avoid colissions.

Thanks, I merged your immutable branch for the ADC work
and this patchset.

-- Sebastian

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2019-10-20 13:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-04 15:07 [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Krzysztof Kozlowski
2019-10-04 15:07 ` [RFT 2/3] power: supply: ab8500_fg: Do not free non-requested IRQs in probe's error path Krzysztof Kozlowski
2019-10-04 15:07 ` [RFT 3/3] power: supply: ab8500: Handle invalid IRQ from platform_get_irq_byname() Krzysztof Kozlowski
2019-10-16  8:33 ` [RFT 1/3] power: supply: ab8500: Cleanup probe in reverse order Linus Walleij
2019-10-20 13:23   ` Sebastian Reichel

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).