All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/5] power: supply: tps65217: Support USB charger feature
@ 2016-11-15 13:18 Milo Kim
  2016-11-15 13:18 ` [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function Milo Kim
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

TPS65217 device supports two charger inputs - AC and USB.
Currently, only AC charger is supported. This patch-set adds USB charger 
feature. Tested on Beaglebone black.

Milo Kim (5):
  power: supply: tps65217: Move IRQ related operation into single
    function
  power: supply: tps65217: Remove IRQ data from driver data
  power: supply: tps65217: Support USB charger interrupt
  power: supply: tps65217: Use generic name for charger online
  power: supply: tps65217: Use generic name for power supply structures

 drivers/power/supply/tps65217_charger.c | 133 ++++++++++++++++++--------------
 1 file changed, 73 insertions(+), 60 deletions(-)

-- 
2.9.3

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

* [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function
  2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
@ 2016-11-15 13:18 ` Milo Kim
  2016-11-22 16:49   ` Sebastian Reichel
  2016-11-15 13:18 ` [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data Milo Kim
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

TPS65217 charger driver handles the charger interrupt through the IRQ or
polling. Both cases can be requested in single function.

Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 70 ++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 32 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 9fd019f..04f8322 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -195,12 +195,48 @@ static const struct power_supply_desc tps65217_charger_desc = {
 	.num_properties		= ARRAY_SIZE(tps65217_ac_props),
 };
 
+static int tps65217_charger_request_interrupt(struct platform_device *pdev)
+{
+	struct tps65217_charger *charger = platform_get_drvdata(pdev);
+	int irq;
+	int ret;
+
+	irq = platform_get_irq_byname(pdev, "AC");
+	if (irq < 0)
+		irq = -ENXIO;
+
+	charger->irq = irq;
+
+	if (irq != -ENXIO) {
+		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+						tps65217_charger_irq, 0,
+						"tps65217-charger", charger);
+		if (ret) {
+			dev_err(charger->dev,
+				"Unable to register irq %d err %d\n", irq, ret);
+			return ret;
+		}
+
+		/* Check current state */
+		tps65217_charger_irq(irq, charger);
+		return 0;
+	}
+
+	charger->poll_task = kthread_run(tps65217_charger_poll_task, charger,
+					 "ktps65217charger");
+	if (IS_ERR(charger->poll_task)) {
+		ret = PTR_ERR(charger->poll_task);
+		dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
+	}
+
+	return 0;
+}
+
 static int tps65217_charger_probe(struct platform_device *pdev)
 {
 	struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
 	struct tps65217_charger *charger;
 	struct power_supply_config cfg = {};
-	int irq;
 	int ret;
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
@@ -224,43 +260,13 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 		return PTR_ERR(charger->ac);
 	}
 
-	irq = platform_get_irq_byname(pdev, "AC");
-	if (irq < 0)
-		irq = -ENXIO;
-	charger->irq = irq;
-
 	ret = tps65217_config_charger(charger);
 	if (ret < 0) {
 		dev_err(charger->dev, "charger config failed, err %d\n", ret);
 		return ret;
 	}
 
-	if (irq != -ENXIO) {
-		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
-						tps65217_charger_irq,
-						0, "tps65217-charger",
-						charger);
-		if (ret) {
-			dev_err(charger->dev,
-				"Unable to register irq %d err %d\n", irq,
-				ret);
-			return ret;
-		}
-
-		/* Check current state */
-		tps65217_charger_irq(irq, charger);
-	} else {
-		charger->poll_task = kthread_run(tps65217_charger_poll_task,
-						charger, "ktps65217charger");
-		if (IS_ERR(charger->poll_task)) {
-			ret = PTR_ERR(charger->poll_task);
-			dev_err(charger->dev,
-				"Unable to run kthread err %d\n", ret);
-			return ret;
-		}
-	}
-
-	return 0;
+	return tps65217_charger_request_interrupt(pdev);
 }
 
 static int tps65217_charger_remove(struct platform_device *pdev)
-- 
2.9.3

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

* [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data
  2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
  2016-11-15 13:18 ` [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function Milo Kim
@ 2016-11-15 13:18 ` Milo Kim
  2016-11-22 16:51   ` Sebastian Reichel
  2016-11-15 13:18 ` [PATCH 3/5] power: supply: tps65217: Support USB charger interrupt Milo Kim
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

IRQ number is only used on requesting the interrupt, so no need to keep
it inside the driver data.
In case of polling, poll_task is valid only when polling thread is
activated.

Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 04f8322..55a4f34 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -46,8 +46,6 @@ struct tps65217_charger {
 	int	prev_ac_online;
 
 	struct task_struct	*poll_task;
-
-	int	irq;
 };
 
 static enum power_supply_property tps65217_ac_props[] = {
@@ -198,6 +196,7 @@ static const struct power_supply_desc tps65217_charger_desc = {
 static int tps65217_charger_request_interrupt(struct platform_device *pdev)
 {
 	struct tps65217_charger *charger = platform_get_drvdata(pdev);
+	struct task_struct *poll_task;
 	int irq;
 	int ret;
 
@@ -205,8 +204,6 @@ static int tps65217_charger_request_interrupt(struct platform_device *pdev)
 	if (irq < 0)
 		irq = -ENXIO;
 
-	charger->irq = irq;
-
 	if (irq != -ENXIO) {
 		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
 						tps65217_charger_irq, 0,
@@ -222,13 +219,16 @@ static int tps65217_charger_request_interrupt(struct platform_device *pdev)
 		return 0;
 	}
 
-	charger->poll_task = kthread_run(tps65217_charger_poll_task, charger,
-					 "ktps65217charger");
-	if (IS_ERR(charger->poll_task)) {
-		ret = PTR_ERR(charger->poll_task);
+	poll_task = kthread_run(tps65217_charger_poll_task, charger,
+				"ktps65217charger");
+	if (IS_ERR(poll_task)) {
+		ret = PTR_ERR(poll_task);
 		dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
+		return ret;
 	}
 
+	charger->poll_task = poll_task;
+
 	return 0;
 }
 
@@ -273,7 +273,7 @@ static int tps65217_charger_remove(struct platform_device *pdev)
 {
 	struct tps65217_charger *charger = platform_get_drvdata(pdev);
 
-	if (charger->irq == -ENXIO)
+	if (charger->poll_task)
 		kthread_stop(charger->poll_task);
 
 	return 0;
-- 
2.9.3

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

* [PATCH 3/5] power: supply: tps65217: Support USB charger interrupt
  2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
  2016-11-15 13:18 ` [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function Milo Kim
  2016-11-15 13:18 ` [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data Milo Kim
@ 2016-11-15 13:18 ` Milo Kim
  2016-11-15 13:18 ` [PATCH 4/5] power: supply: tps65217: Use generic name for charger online Milo Kim
  2016-11-15 13:18 ` [PATCH 5/5] power: supply: tps65217: Use generic name for power supply structures Milo Kim
  4 siblings, 0 replies; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

TPS65217 has two charger interrupts - AC or USB power status change.

Interrupt request:
  If an interrupt number is not defined, then use legacy polling thread.
  Otherwise, create IRQ threads to handle AC or USB charger event.

Interrupt handler:
  Check not only AC but also USB charger status.
  In both cases, enable charging operation.

Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 52 +++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 55a4f34..2894132 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -35,7 +35,9 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps65217.h>
 
+#define CHARGER_STATUS_PRESENT	(TPS65217_STATUS_ACPWR | TPS65217_STATUS_USBPWR)
 #define POLL_INTERVAL		(HZ * 2)
+#define NUM_CHARGER_IRQS	2
 
 struct tps65217_charger {
 	struct tps65217 *tps;
@@ -142,8 +144,8 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 
 	dev_dbg(charger->dev, "%s: 0x%x\n", __func__, val);
 
-	/* check for AC status bit */
-	if (val & TPS65217_STATUS_ACPWR) {
+	/* check for charger status bit */
+	if (val & CHARGER_STATUS_PRESENT) {
 		ret = tps65217_enable_charging(charger);
 		if (ret) {
 			dev_err(charger->dev,
@@ -197,37 +199,43 @@ static int tps65217_charger_request_interrupt(struct platform_device *pdev)
 {
 	struct tps65217_charger *charger = platform_get_drvdata(pdev);
 	struct task_struct *poll_task;
-	int irq;
+	int irq[NUM_CHARGER_IRQS];
 	int ret;
+	int i;
+
+	irq[0] = platform_get_irq_byname(pdev, "AC");
+	irq[1] = platform_get_irq_byname(pdev, "USB");
+
+	/* Create a polling thread if an interrupt is invalid */
+	if (irq[0] < 0 || irq[1] < 0) {
+		poll_task = kthread_run(tps65217_charger_poll_task, charger,
+					"ktps65217charger");
+		if (IS_ERR(poll_task)) {
+			ret = PTR_ERR(poll_task);
+			dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
+			return ret;
+		}
+
+		charger->poll_task = poll_task;
 
-	irq = platform_get_irq_byname(pdev, "AC");
-	if (irq < 0)
-		irq = -ENXIO;
+		return 0;
+	}
 
-	if (irq != -ENXIO) {
-		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+	/* Create IRQ threads for charger interrupts */
+	for (i = 0; i < NUM_CHARGER_IRQS; i++) {
+		ret = devm_request_threaded_irq(&pdev->dev, irq[i], NULL,
 						tps65217_charger_irq, 0,
 						"tps65217-charger", charger);
 		if (ret) {
 			dev_err(charger->dev,
-				"Unable to register irq %d err %d\n", irq, ret);
+				"Unable to register irq %d err %d\n", irq[i],
+				ret);
 			return ret;
 		}
-
-		/* Check current state */
-		tps65217_charger_irq(irq, charger);
-		return 0;
-	}
-
-	poll_task = kthread_run(tps65217_charger_poll_task, charger,
-				"ktps65217charger");
-	if (IS_ERR(poll_task)) {
-		ret = PTR_ERR(poll_task);
-		dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
-		return ret;
 	}
 
-	charger->poll_task = poll_task;
+	/* Check current state */
+	tps65217_charger_irq(-1, charger);
 
 	return 0;
 }
-- 
2.9.3

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

* [PATCH 4/5] power: supply: tps65217: Use generic name for charger online
  2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
                   ` (2 preceding siblings ...)
  2016-11-15 13:18 ` [PATCH 3/5] power: supply: tps65217: Support USB charger interrupt Milo Kim
@ 2016-11-15 13:18 ` Milo Kim
  2016-11-15 13:18 ` [PATCH 5/5] power: supply: tps65217: Use generic name for power supply structures Milo Kim
  4 siblings, 0 replies; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

This driver supports AC and USB chargers. Generic name is preferred.

  ac_online -> online
  prev_ac_online -> prev_online

Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 2894132..8b7ce61 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -44,8 +44,8 @@ struct tps65217_charger {
 	struct device *dev;
 	struct power_supply *ac;
 
-	int	ac_online;
-	int	prev_ac_online;
+	int	online;
+	int	prev_online;
 
 	struct task_struct	*poll_task;
 };
@@ -95,7 +95,7 @@ static int tps65217_enable_charging(struct tps65217_charger *charger)
 	int ret;
 
 	/* charger already enabled */
-	if (charger->ac_online)
+	if (charger->online)
 		return 0;
 
 	dev_dbg(charger->dev, "%s: enable charging\n", __func__);
@@ -110,7 +110,7 @@ static int tps65217_enable_charging(struct tps65217_charger *charger)
 		return ret;
 	}
 
-	charger->ac_online = 1;
+	charger->online = 1;
 
 	return 0;
 }
@@ -122,7 +122,7 @@ static int tps65217_ac_get_property(struct power_supply *psy,
 	struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
 	if (psp == POWER_SUPPLY_PROP_ONLINE) {
-		val->intval = charger->ac_online;
+		val->intval = charger->online;
 		return 0;
 	}
 	return -EINVAL;
@@ -133,7 +133,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 	int ret, val;
 	struct tps65217_charger *charger = dev;
 
-	charger->prev_ac_online = charger->ac_online;
+	charger->prev_online = charger->online;
 
 	ret = tps65217_reg_read(charger->tps, TPS65217_REG_STATUS, &val);
 	if (ret < 0) {
@@ -153,10 +153,10 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 			return IRQ_HANDLED;
 		}
 	} else {
-		charger->ac_online = 0;
+		charger->online = 0;
 	}
 
-	if (charger->prev_ac_online != charger->ac_online)
+	if (charger->prev_online != charger->online)
 		power_supply_changed(charger->ac);
 
 	ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, &val);
-- 
2.9.3

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

* [PATCH 5/5] power: supply: tps65217: Use generic name for power supply structures
  2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
                   ` (3 preceding siblings ...)
  2016-11-15 13:18 ` [PATCH 4/5] power: supply: tps65217: Use generic name for charger online Milo Kim
@ 2016-11-15 13:18 ` Milo Kim
  4 siblings, 0 replies; 11+ messages in thread
From: Milo Kim @ 2016-11-15 13:18 UTC (permalink / raw)
  To: Sebastian Reichel
  Cc: Enric Balletbo i Serra, linux-pm, linux-kernel, Milo Kim

AC and USB charger are supported, so generic names are preferred.

  ac in tps65217_charger -> psy
  tps65217_ac_props -> tps65217_charger_props
  tps65217_ac_get_property -> tps65217_charger_get_property
  tps65217-ac -> tps65217-charger

Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 8b7ce61..4e3f5cb 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -42,7 +42,7 @@
 struct tps65217_charger {
 	struct tps65217 *tps;
 	struct device *dev;
-	struct power_supply *ac;
+	struct power_supply *psy;
 
 	int	online;
 	int	prev_online;
@@ -50,7 +50,7 @@ struct tps65217_charger {
 	struct task_struct	*poll_task;
 };
 
-static enum power_supply_property tps65217_ac_props[] = {
+static enum power_supply_property tps65217_charger_props[] = {
 	POWER_SUPPLY_PROP_ONLINE,
 };
 
@@ -115,9 +115,9 @@ static int tps65217_enable_charging(struct tps65217_charger *charger)
 	return 0;
 }
 
-static int tps65217_ac_get_property(struct power_supply *psy,
-			enum power_supply_property psp,
-			union power_supply_propval *val)
+static int tps65217_charger_get_property(struct power_supply *psy,
+					 enum power_supply_property psp,
+					 union power_supply_propval *val)
 {
 	struct tps65217_charger *charger = power_supply_get_drvdata(psy);
 
@@ -157,7 +157,7 @@ static irqreturn_t tps65217_charger_irq(int irq, void *dev)
 	}
 
 	if (charger->prev_online != charger->online)
-		power_supply_changed(charger->ac);
+		power_supply_changed(charger->psy);
 
 	ret = tps65217_reg_read(charger->tps, TPS65217_REG_CHGCONFIG0, &val);
 	if (ret < 0) {
@@ -188,11 +188,11 @@ static int tps65217_charger_poll_task(void *data)
 }
 
 static const struct power_supply_desc tps65217_charger_desc = {
-	.name			= "tps65217-ac",
+	.name			= "tps65217-charger",
 	.type			= POWER_SUPPLY_TYPE_MAINS,
-	.get_property		= tps65217_ac_get_property,
-	.properties		= tps65217_ac_props,
-	.num_properties		= ARRAY_SIZE(tps65217_ac_props),
+	.get_property		= tps65217_charger_get_property,
+	.properties		= tps65217_charger_props,
+	.num_properties		= ARRAY_SIZE(tps65217_charger_props),
 };
 
 static int tps65217_charger_request_interrupt(struct platform_device *pdev)
@@ -260,12 +260,11 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 	cfg.of_node = pdev->dev.of_node;
 	cfg.drv_data = charger;
 
-	charger->ac = devm_power_supply_register(&pdev->dev,
-						 &tps65217_charger_desc,
-						 &cfg);
-	if (IS_ERR(charger->ac)) {
+	charger->psy = devm_power_supply_register(&pdev->dev,
+						  &tps65217_charger_desc, &cfg);
+	if (IS_ERR(charger->psy)) {
 		dev_err(&pdev->dev, "failed: power supply register\n");
-		return PTR_ERR(charger->ac);
+		return PTR_ERR(charger->psy);
 	}
 
 	ret = tps65217_config_charger(charger);
-- 
2.9.3

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

* Re: [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function
  2016-11-15 13:18 ` [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function Milo Kim
@ 2016-11-22 16:49   ` Sebastian Reichel
  2016-11-22 16:56     ` Sebastian Reichel
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Reichel @ 2016-11-22 16:49 UTC (permalink / raw)
  To: Milo Kim; +Cc: Enric Balletbo i Serra, linux-pm, linux-kernel

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

Hi,

On Tue, Nov 15, 2016 at 10:18:51PM +0900, Milo Kim wrote:
> TPS65217 charger driver handles the charger interrupt through the IRQ or
> polling. Both cases can be requested in single function.
> 
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  drivers/power/supply/tps65217_charger.c | 70 ++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 32 deletions(-)

I don't see the advantage of this. It's more lines of codes than
before and does not really increase readability.

-- Sebastian

> 
> diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
> index 9fd019f..04f8322 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -195,12 +195,48 @@ static const struct power_supply_desc tps65217_charger_desc = {
>  	.num_properties		= ARRAY_SIZE(tps65217_ac_props),
>  };
>  
> +static int tps65217_charger_request_interrupt(struct platform_device *pdev)
> +{
> +	struct tps65217_charger *charger = platform_get_drvdata(pdev);
> +	int irq;
> +	int ret;
> +
> +	irq = platform_get_irq_byname(pdev, "AC");
> +	if (irq < 0)
> +		irq = -ENXIO;
> +
> +	charger->irq = irq;
> +
> +	if (irq != -ENXIO) {
> +		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +						tps65217_charger_irq, 0,
> +						"tps65217-charger", charger);
> +		if (ret) {
> +			dev_err(charger->dev,
> +				"Unable to register irq %d err %d\n", irq, ret);
> +			return ret;
> +		}
> +
> +		/* Check current state */
> +		tps65217_charger_irq(irq, charger);
> +		return 0;
> +	}
> +
> +	charger->poll_task = kthread_run(tps65217_charger_poll_task, charger,
> +					 "ktps65217charger");
> +	if (IS_ERR(charger->poll_task)) {
> +		ret = PTR_ERR(charger->poll_task);
> +		dev_err(charger->dev, "Unable to run kthread err %d\n", ret);
> +	}
> +
> +	return 0;
> +}
> +
>  static int tps65217_charger_probe(struct platform_device *pdev)
>  {
>  	struct tps65217 *tps = dev_get_drvdata(pdev->dev.parent);
>  	struct tps65217_charger *charger;
>  	struct power_supply_config cfg = {};
> -	int irq;
>  	int ret;
>  
>  	dev_dbg(&pdev->dev, "%s\n", __func__);
> @@ -224,43 +260,13 @@ static int tps65217_charger_probe(struct platform_device *pdev)
>  		return PTR_ERR(charger->ac);
>  	}
>  
> -	irq = platform_get_irq_byname(pdev, "AC");
> -	if (irq < 0)
> -		irq = -ENXIO;
> -	charger->irq = irq;
> -
>  	ret = tps65217_config_charger(charger);
>  	if (ret < 0) {
>  		dev_err(charger->dev, "charger config failed, err %d\n", ret);
>  		return ret;
>  	}
>  
> -	if (irq != -ENXIO) {
> -		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> -						tps65217_charger_irq,
> -						0, "tps65217-charger",
> -						charger);
> -		if (ret) {
> -			dev_err(charger->dev,
> -				"Unable to register irq %d err %d\n", irq,
> -				ret);
> -			return ret;
> -		}
> -
> -		/* Check current state */
> -		tps65217_charger_irq(irq, charger);
> -	} else {
> -		charger->poll_task = kthread_run(tps65217_charger_poll_task,
> -						charger, "ktps65217charger");
> -		if (IS_ERR(charger->poll_task)) {
> -			ret = PTR_ERR(charger->poll_task);
> -			dev_err(charger->dev,
> -				"Unable to run kthread err %d\n", ret);
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> +	return tps65217_charger_request_interrupt(pdev);
>  }
>  
>  static int tps65217_charger_remove(struct platform_device *pdev)
> -- 
> 2.9.3
> 

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

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

* Re: [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data
  2016-11-15 13:18 ` [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data Milo Kim
@ 2016-11-22 16:51   ` Sebastian Reichel
  2016-11-22 16:58     ` Sebastian Reichel
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Reichel @ 2016-11-22 16:51 UTC (permalink / raw)
  To: Milo Kim; +Cc: Enric Balletbo i Serra, linux-pm, linux-kernel

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

Hi,

On Tue, Nov 15, 2016 at 10:18:52PM +0900, Milo Kim wrote:
> IRQ number is only used on requesting the interrupt, so no need to keep
> it inside the driver data.
> In case of polling, poll_task is valid only when polling thread is
> activated.
> 
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  drivers/power/supply/tps65217_charger.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
> index 04f8322..55a4f34 100644
> --- a/drivers/power/supply/tps65217_charger.c
> +++ b/drivers/power/supply/tps65217_charger.c
> @@ -46,8 +46,6 @@ struct tps65217_charger {
>  	int	prev_ac_online;
>  
>  	struct task_struct	*poll_task;
> -
> -	int	irq;

Removal of this (and related changes) look ok. The poll_task changes
are actually only needed due to Patch 1 (which should be dropped imho).

-- Sebastian

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

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

* Re: [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function
  2016-11-22 16:49   ` Sebastian Reichel
@ 2016-11-22 16:56     ` Sebastian Reichel
  0 siblings, 0 replies; 11+ messages in thread
From: Sebastian Reichel @ 2016-11-22 16:56 UTC (permalink / raw)
  To: Milo Kim; +Cc: Enric Balletbo i Serra, linux-pm, linux-kernel

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

Hi,

On Tue, Nov 22, 2016 at 05:49:11PM +0100, Sebastian Reichel wrote:
> On Tue, Nov 15, 2016 at 10:18:51PM +0900, Milo Kim wrote:
> > TPS65217 charger driver handles the charger interrupt through the IRQ or
> > polling. Both cases can be requested in single function.
> > 
> > Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> > Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> > ---
> >  drivers/power/supply/tps65217_charger.c | 70 ++++++++++++++++++---------------
> >  1 file changed, 38 insertions(+), 32 deletions(-)
> 
> I don't see the advantage of this. It's more lines of codes than
> before and does not really increase readability.

Ok it makes sense with changes from PATCH 3. Please add a note to
the commit description, that this prep-work for introducing more
irqs.

-- Sebastian

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

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

* Re: [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data
  2016-11-22 16:51   ` Sebastian Reichel
@ 2016-11-22 16:58     ` Sebastian Reichel
  2016-11-23 11:47       ` Milo Kim
  0 siblings, 1 reply; 11+ messages in thread
From: Sebastian Reichel @ 2016-11-22 16:58 UTC (permalink / raw)
  To: Milo Kim; +Cc: Enric Balletbo i Serra, linux-pm, linux-kernel

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

Hi,

[it's me again after reading Patch 3]

On Tue, Nov 22, 2016 at 05:51:07PM +0100, Sebastian Reichel wrote:
> On Tue, Nov 15, 2016 at 10:18:52PM +0900, Milo Kim wrote:
> > IRQ number is only used on requesting the interrupt, so no need to keep
> > it inside the driver data.

Please move simple cleanup patches like this one to the
begin of the patch series.

> > In case of polling, poll_task is valid only when polling thread is
> > activated.

And merge this change into the patch were you introduced the
problem...

-- Sebastian

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

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

* Re: [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data
  2016-11-22 16:58     ` Sebastian Reichel
@ 2016-11-23 11:47       ` Milo Kim
  0 siblings, 0 replies; 11+ messages in thread
From: Milo Kim @ 2016-11-23 11:47 UTC (permalink / raw)
  To: Sebastian Reichel; +Cc: Enric Balletbo i Serra, linux-pm, linux-kernel

On 11/23/2016 01:58 AM, Sebastian Reichel wrote:
> On Tue, Nov 22, 2016 at 05:51:07PM +0100, Sebastian Reichel wrote:
>> > On Tue, Nov 15, 2016 at 10:18:52PM +0900, Milo Kim wrote:
>>> > > IRQ number is only used on requesting the interrupt, so no need to keep
>>> > > it inside the driver data.
> Please move simple cleanup patches like this one to the
> begin of the patch series.
>
>>> > > In case of polling, poll_task is valid only when polling thread is
>>> > > activated.
> And merge this change into the patch were you introduced the
> problem...

Thanks for your feedback. Let me regenerate the patch-set for better review.

Best regards,
Milo

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

end of thread, other threads:[~2016-11-23 11:47 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-15 13:18 [PATCH 0/5] power: supply: tps65217: Support USB charger feature Milo Kim
2016-11-15 13:18 ` [PATCH 1/5] power: supply: tps65217: Move IRQ related operation into single function Milo Kim
2016-11-22 16:49   ` Sebastian Reichel
2016-11-22 16:56     ` Sebastian Reichel
2016-11-15 13:18 ` [PATCH 2/5] power: supply: tps65217: Remove IRQ data from driver data Milo Kim
2016-11-22 16:51   ` Sebastian Reichel
2016-11-22 16:58     ` Sebastian Reichel
2016-11-23 11:47       ` Milo Kim
2016-11-15 13:18 ` [PATCH 3/5] power: supply: tps65217: Support USB charger interrupt Milo Kim
2016-11-15 13:18 ` [PATCH 4/5] power: supply: tps65217: Use generic name for charger online Milo Kim
2016-11-15 13:18 ` [PATCH 5/5] power: supply: tps65217: Use generic name for power supply structures Milo Kim

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.