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
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 <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
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
This driver supports AC and USB chargers. Generic name is preferred.
ac_online -> online
prev_ac_online -> prev_online
Cc: Enric Balletbo i Serra <[email protected]>
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 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
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 <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Milo Kim <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> 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
>
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 <[email protected]>
> > Signed-off-by: Milo Kim <[email protected]>
> > ---
> > 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
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
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 <[email protected]>
> Signed-off-by: Milo Kim <[email protected]>
> ---
> 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
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