[PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

[PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

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

Patch 1: Main patch
Patch 2, 3: Clean up for charger driver data
Patch 4 ~ 8: Naming changes for generic power supply class structure

v2:
  Regenerate the patchset for better code review

Milo Kim (8):
  power: supply: tps65217: Support USB charger interrupt
  power: supply: tps65217: Use 'poll_task' on unloading the module
  power: supply: tps65217: Remove IRQ data from driver data
  power: supply: tps65217: Use generic name for charger online
  power: supply: tps65217: Use generic name for power supply structure
  power: supply: tps65217: Use generic name for power supply property
  power: supply: tps65217: Use generic name for get_property()
  power: supply: tps65217: Use generic charger name

 drivers/power/supply/tps65217_charger.c | 99 ++++++++++++++++++---------------
 1 file changed, 53 insertions(+), 46 deletions(-)

-- 
2.9.3

[PATCH v2 1/8] power: supply: tps65217: Support USB charger interrupt

[Milo Kim]

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

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

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

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 47 +++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 9fd019f..2000e59 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -35,6 +35,8 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/tps65217.h>
 
+#define CHARGER_STATUS_PRESENT	(TPS65217_STATUS_ACPWR | TPS65217_STATUS_USBPWR)
+#define NUM_CHARGER_IRQS	2
 #define POLL_INTERVAL		(HZ * 2)
 
 struct tps65217_charger {
@@ -144,8 +146,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,
@@ -200,8 +202,9 @@ 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 irq[NUM_CHARGER_IRQS];
 	int ret;
+	int i;
 
 	dev_dbg(&pdev->dev, "%s\n", __func__);
 
@@ -224,10 +227,8 @@ 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;
+	irq[0] = platform_get_irq_byname(pdev, "USB");
+	irq[1] = platform_get_irq_byname(pdev, "AC");
 
 	ret = tps65217_config_charger(charger);
 	if (ret < 0) {
@@ -235,29 +236,35 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (irq != -ENXIO) {
-		ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+	/* Create a polling thread if an interrupt is invalid */
+	if (irq[0] < 0 || irq[1] < 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 ret;
+		}
+
+		return 0;
+	}
+
+	/* 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,
+				"Unable to register irq %d err %d\n", irq[i],
 				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;
-		}
+		tps65217_charger_irq(-1, charger);
 	}
 
 	return 0;
-- 
2.9.3

[PATCH v2 2/8] power: supply: tps65217: Use 'poll_task' on unloading the module

[Milo Kim]

TPS65217 has two interrupt numbers so checking single IRQ number is not
appropriate when the module is removed.
Use the task_struct variable for running polling thread. If polling task
is activated, then use it to stop running thread.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 2000e59..55371d6 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -202,6 +202,7 @@ 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 = {};
+	struct task_struct *poll_task;
 	int irq[NUM_CHARGER_IRQS];
 	int ret;
 	int i;
@@ -238,15 +239,16 @@ static int tps65217_charger_probe(struct platform_device *pdev)
 
 	/* Create a polling thread if an interrupt is invalid */
 	if (irq[0] < 0 || irq[1] < 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;
 	}
 
@@ -274,7 +276,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

[PATCH v2 3/8] power: supply: tps65217: Remove IRQ data from driver data

[Milo Kim]

IRQ number is only used on requesting the interrupt, so no need to keep
it inside the driver data.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 55371d6..482ee9f 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -48,8 +48,6 @@ struct tps65217_charger {
 	int	prev_ac_online;
 
 	struct task_struct	*poll_task;
-
-	int	irq;
 };
 
 static enum power_supply_property tps65217_ac_props[] = {
-- 
2.9.3

[PATCH v2 4/8] power: supply: tps65217: Use generic name for charger online

[Milo Kim]

This driver supports AC and USB chargers. Generic name is preferred.
Replace 'ac_online' with 'online'.

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 482ee9f..424a6d3 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

[PATCH v2 5/8] power: supply: tps65217: Use generic name for power supply structure

[Milo Kim]

Replace 'ac' of tps65217_charger structure with 'psy'.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 424a6d3..5daf361 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;
@@ -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) {
@@ -218,12 +218,12 @@ 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);
 	}
 
 	irq[0] = platform_get_irq_byname(pdev, "USB");
-- 
2.9.3

[PATCH v2 6/8] power: supply: tps65217: Use generic name for power supply property

[Milo Kim]

Replace 'ac_props' with 'charger_props'.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 5daf361..79afeca 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -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,
 };
 
@@ -191,8 +191,8 @@ static const struct power_supply_desc tps65217_charger_desc = {
 	.name			= "tps65217-ac",
 	.type			= POWER_SUPPLY_TYPE_MAINS,
 	.get_property		= tps65217_ac_get_property,
-	.properties		= tps65217_ac_props,
-	.num_properties		= ARRAY_SIZE(tps65217_ac_props),
+	.properties		= tps65217_charger_props,
+	.num_properties		= ARRAY_SIZE(tps65217_charger_props),
 };
 
 static int tps65217_charger_probe(struct platform_device *pdev)
-- 
2.9.3

[PATCH v2 7/8] power: supply: tps65217: Use generic name for get_property()

[Milo Kim]

Rename it as tps65217_charger_get_property().

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 79afeca..63c5556 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -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);
 
@@ -190,7 +190,7 @@ static int tps65217_charger_poll_task(void *data)
 static const struct power_supply_desc tps65217_charger_desc = {
 	.name			= "tps65217-ac",
 	.type			= POWER_SUPPLY_TYPE_MAINS,
-	.get_property		= tps65217_ac_get_property,
+	.get_property		= tps65217_charger_get_property,
 	.properties		= tps65217_charger_props,
 	.num_properties		= ARRAY_SIZE(tps65217_charger_props),
 };
-- 
2.9.3

[PATCH v2 8/8] power: supply: tps65217: Use generic charger name

[Milo Kim]

"tps65217-charger" is more appropriate name because the driver supports
not only AC but also USB charger.

Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 drivers/power/supply/tps65217_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/tps65217_charger.c b/drivers/power/supply/tps65217_charger.c
index 63c5556..29b61e8 100644
--- a/drivers/power/supply/tps65217_charger.c
+++ b/drivers/power/supply/tps65217_charger.c
@@ -188,7 +188,7 @@ 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_charger_get_property,
 	.properties		= tps65217_charger_props,
-- 
2.9.3

Re: [PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

[Sebastian Reichel]

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

Hi,

On Fri, Dec 09, 2016 at 04:48:58PM +0900, Milo Kim wrote:
> 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.
> 
> Patch 1: Main patch
> Patch 2, 3: Clean up for charger driver data
> Patch 4 ~ 8: Naming changes for generic power supply class structure
> 
> v2:
>   Regenerate the patchset for better code review
> 
> Milo Kim (8):
>   power: supply: tps65217: Support USB charger interrupt
>   power: supply: tps65217: Use 'poll_task' on unloading the module

patches look fine, but these two patches must be reordered to fix
bisectability. Otherwise after patch 1 the thread is not properly
killed during driver removal.

-- Sebastian

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

Re: [PATCH v2 0/8] power: supply: tps65217: Support USB charger feature

[Milo Kim]

On 12/18/2016 01:24 AM, Sebastian Reichel wrote:
> patches look fine, but these two patches must be reordered to fix
> bisectability. Otherwise after patch 1 the thread is not properly
> killed during driver removal.

Thanks for your guidelines. The v3 patchset has been sent.

Best regards,
Milo