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
IRQ number is only used on requesting the interrupt, so no need to keep
it inside the driver data.
Signed-off-by: Milo Kim <[email protected]>
---
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
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 <[email protected]>
---
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
This driver supports AC and USB chargers. Generic name is preferred.
Replace 'ac_online' with 'online'.
Signed-off-by: Milo Kim <[email protected]>
---
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
Replace 'ac_props' with 'charger_props'.
Signed-off-by: Milo Kim <[email protected]>
---
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
"tps65217-charger" is more appropriate name because the driver supports
not only AC but also USB charger.
Signed-off-by: Milo Kim <[email protected]>
---
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
Rename it as tps65217_charger_get_property().
Signed-off-by: Milo Kim <[email protected]>
---
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
Replace 'ac' of tps65217_charger structure with 'psy'.
Signed-off-by: Milo Kim <[email protected]>
---
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
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 <[email protected]>
---
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
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
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