On Thu, Jan 12, 2017 at 1:40 PM, Sebastian Reichel <sre@xxxxxxxxxx> wrote:
> Hi,
>
> On Thu, Jan 12, 2017 at 12:58:11PM -0800, Tony Lindgren wrote:
>> * Liam Breck <liam@xxxxxxxxxxxxxxxxx> [170112 12:25]:
>> > On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@xxxxxxxxxx> wrote:
>> > > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
>> > >> From: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
>> > >>
>> > >> The device specific data is not fully initialized after
>> > >> request_threaded_irq().
>> > >>
>> > >> This causes problems when the IRQ handler tries to reference them.
>> > >> Fix the issue by enabling IRQ only at the end of the probe.
>> > >>
>> > >> Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190
>> > >> Battery Charger")
>> > >> Cc: Mark A. Greer <mgreer@xxxxxxxxxxxxxxx>
>> > >> Cc: Matt Ranostay <matt@ranostay.consulting>
>> > >> Signed-off-by: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
>> > >> [tony@xxxxxxxxxxx: cleaned up patch description a bit]
>> > >> Signed-off-by: Tony Lindgren <tony@xxxxxxxxxxx>
>> > >> ---
>> > >> drivers/power/supply/bq24190_charger.c | 3 +++
>> > >> 1 file changed, 3 insertions(+)
>> > >>
>> > >> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> > >> --- a/drivers/power/supply/bq24190_charger.c
>> > >> +++ b/drivers/power/supply/bq24190_charger.c
>> > >> @@ -1392,6 +1392,7 @@ static int bq24190_probe(struct i2c_client *client,
>> > >> return -EINVAL;
>> > >> }
>> > >>
>> > >> + irq_set_status_flags(bdi->irq, IRQ_NOAUTOEN);
>> > >> ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
>> > >> bq24190_irq_handler_thread,
>> > >> IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> > >> @@ -1436,6 +1437,8 @@ static int bq24190_probe(struct i2c_client *client,
>> > >> goto out4;
>> > >> }
>> > >>
>> > >> + enable_irq(bdi->irq);
>> > >> +
>> > >> return 0;
>> > >>
>> > >> out4:
>> > >
>> > > Can't you just move the irq request towards the end of the probe?
>> > > That way it will also be released before the power-supply structure
>> > > is released.
>> >
>> > I did that in a first draft I showed Tony. He suggested this way.
>> > Tony, rationale?
>>
>> Both will work for me just fine as long as done in a single patch
>> with no other changes.
>>
>> The first option is less changes if needed as a fix, up to Sebastian
>> depending on what he prefers.
>
> Then please send the variant, which moves the block in the v2
> patchset, since it behaves correctly during driver removal and
> results in less lines of code.
OK.
>> > diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
>
> Also make sure, that you base your patches on power-supply's
> for-next branch. Your base is way too *old*, since the driver
> lives in drivers/power/supply/bq24190_charger.c nowadays.
Yep, that was from an old branch.
Any other feedback on patches 1-4 in this set?
Note #4 will become the following two patches:
From ea15efae6ecfa5045a95b4158a45a05544dcc259 Mon Sep 17 00:00:00 2001
From: Liam <github@xxxxxxxxxxxxxxxxx>
Date: Wed, 11 Jan 2017 23:20:14 -0800
Subject: [PATCH 1/2] power: bq24190_charger: Don't read fault register outside
irq_handle_thread()
Caching the fault register after a single I2C read may not keep an accurate
value.
Fix by doing two reads in irq_handle_thread() and using the cached value
elsewhere. If a safety timer fault later clears itself, we apparently don't get
an interrupt (INT), however other interrupts would refresh the register cache.
From the manual: "When a fault occurs, the charger device sends out INT
and keeps the fault state in REG09 until the host reads the fault register.
Before the host reads REG09 and all the faults are cleared, the charger
device would not send any INT upon new faults. In order to read the
current fault status, the host has to read REG09 two times consecutively.
The 1st reads fault register status from the last read [1] and the 2nd reads
the current fault register status."
[1] presumably a typo; should be "last fault"
Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Cc: Mark A. Greer <mgreer@xxxxxxxxxxxxxxx>
Cc: Tony Lindgren <tony@xxxxxxxxxxx>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
---
drivers/power/supply/bq24190_charger.c | 93 ++++++++++------------------------
1 file changed, 26 insertions(+), 67 deletions(-)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index ba5a5b2..a36788c 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -144,10 +144,7 @@
* so the first read after a fault returns the latched value and subsequent
* reads return the current value. In order to return the fault status
* to the user, have the interrupt handler save the reg's value and retrieve
- * it in the appropriate health/status routine. Each routine has its own
- * flag indicating whether it should use the value stored by the last run
- * of the interrupt handler or do an actual reg read. That way each routine
- * can report back whatever fault may have occured.
+ * it in the appropriate health/status routine.
*/
struct bq24190_dev_info {
struct i2c_client *client;
@@ -159,9 +156,6 @@ struct bq24190_dev_info {
unsigned int gpio_int;
unsigned int irq;
struct mutex f_reg_lock;
- bool charger_health_valid;
- bool battery_health_valid;
- bool battery_status_valid;
u8 f_reg;
u8 ss_reg;
u8 watchdog;
@@ -635,21 +629,11 @@ static int bq24190_charger_get_health(struct bq24190_dev_info *bdi,
union power_supply_propval *val)
{
u8 v;
- int health, ret;
+ int health;
mutex_lock(&bdi->f_reg_lock);
-
- if (bdi->charger_health_valid) {
- v = bdi->f_reg;
- bdi->charger_health_valid = false;
- mutex_unlock(&bdi->f_reg_lock);
- } else {
- mutex_unlock(&bdi->f_reg_lock);
-
- ret = bq24190_read(bdi, BQ24190_REG_F, &v);
- if (ret < 0)
- return ret;
- }
+ v = bdi->f_reg;
+ mutex_unlock(&bdi->f_reg_lock);
if (v & BQ24190_REG_F_BOOST_FAULT_MASK) {
/*
@@ -936,18 +920,8 @@ static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
int status, ret;
mutex_lock(&bdi->f_reg_lock);
-
- if (bdi->battery_status_valid) {
- chrg_fault = bdi->f_reg;
- bdi->battery_status_valid = false;
- mutex_unlock(&bdi->f_reg_lock);
- } else {
- mutex_unlock(&bdi->f_reg_lock);
-
- ret = bq24190_read(bdi, BQ24190_REG_F, &chrg_fault);
- if (ret < 0)
- return ret;
- }
+ chrg_fault = bdi->f_reg;
+ mutex_unlock(&bdi->f_reg_lock);
chrg_fault &= BQ24190_REG_F_CHRG_FAULT_MASK;
chrg_fault >>= BQ24190_REG_F_CHRG_FAULT_SHIFT;
@@ -995,21 +969,11 @@ static int bq24190_battery_get_health(struct bq24190_dev_info *bdi,
union power_supply_propval *val)
{
u8 v;
- int health, ret;
+ int health;
mutex_lock(&bdi->f_reg_lock);
-
- if (bdi->battery_health_valid) {
- v = bdi->f_reg;
- bdi->battery_health_valid = false;
- mutex_unlock(&bdi->f_reg_lock);
- } else {
- mutex_unlock(&bdi->f_reg_lock);
-
- ret = bq24190_read(bdi, BQ24190_REG_F, &v);
- if (ret < 0)
- return ret;
- }
+ v = bdi->f_reg;
+ mutex_unlock(&bdi->f_reg_lock);
if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
@@ -1201,7 +1165,7 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
| BQ24190_REG_F_NTC_FAULT_MASK;
bool alert_charger = false, alert_battery = false;
u8 ss_reg = 0, f_reg = 0;
- int ret;
+ int i, ret;
pm_runtime_get_sync(bdi->dev);
@@ -1231,33 +1195,34 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
alert_battery = true;
if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
alert_charger = true;
-
bdi->ss_reg = ss_reg;
}
- mutex_lock(&bdi->f_reg_lock);
-
- ret = bq24190_read(bdi, BQ24190_REG_F, &f_reg);
- if (ret < 0) {
- mutex_unlock(&bdi->f_reg_lock);
- dev_err(bdi->dev, "Can't read F reg: %d\n", ret);
- goto out;
- }
+ i = 0;
+ do {
+ ret = bq24190_read(bdi, BQ24190_REG_F, &f_reg);
+ if (ret < 0) {
+ dev_err(bdi->dev, "Can't read F reg: %d\n", ret);
+ goto out;
+ }
+ } while (f_reg && ++i < 2);
if (f_reg != bdi->f_reg) {
+ dev_info(bdi->dev, "Fault: boost %d, charge %d, battery %d, ntc %d\n",
+ !!(f_reg & BQ24190_REG_F_BOOST_FAULT_MASK),
+ !!(f_reg & BQ24190_REG_F_CHRG_FAULT_MASK),
+ !!(f_reg & BQ24190_REG_F_BAT_FAULT_MASK),
+ !!(f_reg & BQ24190_REG_F_NTC_FAULT_MASK));
+
+ mutex_lock(&bdi->f_reg_lock);
if ((bdi->f_reg & battery_mask_f) != (f_reg & battery_mask_f))
alert_battery = true;
if ((bdi->f_reg & ~battery_mask_f) != (f_reg & ~battery_mask_f))
alert_charger = true;
-
bdi->f_reg = f_reg;
- bdi->charger_health_valid = true;
- bdi->battery_health_valid = true;
- bdi->battery_status_valid = true;
+ mutex_unlock(&bdi->f_reg_lock);
}
- mutex_unlock(&bdi->f_reg_lock);
-
if (alert_charger)
power_supply_changed(bdi->charger);
if (alert_battery)
@@ -1377,9 +1342,6 @@ static int bq24190_probe(struct i2c_client *client,
mutex_init(&bdi->f_reg_lock);
bdi->f_reg = 0;
bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK;
- bdi->charger_health_valid = false;
- bdi->battery_health_valid = false;
- bdi->battery_status_valid = false;
i2c_set_clientdata(client, bdi);
@@ -1494,9 +1456,6 @@ static int bq24190_pm_resume(struct device *dev)
bdi->f_reg = 0;
bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK;
- bdi->charger_health_valid = false;
- bdi->battery_health_valid = false;
- bdi->battery_status_valid = false;
pm_runtime_get_sync(bdi->dev);
bq24190_register_reset(bdi);
From c65e77eb4a8d61d4eced8fad61305dd74ae42557 Mon Sep 17 00:00:00 2001
From: Liam <github@xxxxxxxxxxxxxxxxx>
Date: Wed, 11 Jan 2017 23:38:21 -0800
Subject: [PATCH 2/2] power: bq24190_charger: Handle fault before status on
interrupt
Reading both fault and status registers and logging any fault should
take priority over handling status register update.
Fix by moving the fault handling to earlier in interrupt routine.
Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Cc: Mark A. Greer <mgreer@xxxxxxxxxxxxxxx>
Cc: Tony Lindgren <tony@xxxxxxxxxxx>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@xxxxxxxxxxxxxxxxx>
---
drivers/power/supply/bq24190_charger.c | 46 +++++++++++++++++-----------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a36788c..1cb7d14 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1175,29 +1175,6 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
goto out;
}
- if (ss_reg != bdi->ss_reg) {
- /*
- * The device is in host mode so when PG_STAT goes from 1->0
- * (i.e., power removed) HIZ needs to be disabled.
- */
- if ((bdi->ss_reg & BQ24190_REG_SS_PG_STAT_MASK) &&
- !(ss_reg & BQ24190_REG_SS_PG_STAT_MASK)) {
- ret = bq24190_write_mask(bdi, BQ24190_REG_ISC,
- BQ24190_REG_ISC_EN_HIZ_MASK,
- BQ24190_REG_ISC_EN_HIZ_SHIFT,
- 0);
- if (ret < 0)
- dev_err(bdi->dev, "Can't access ISC reg: %d\n",
- ret);
- }
-
- if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
- alert_battery = true;
- if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
- alert_charger = true;
- bdi->ss_reg = ss_reg;
- }
-
i = 0;
do {
ret = bq24190_read(bdi, BQ24190_REG_F, &f_reg);
@@ -1223,6 +1200,29 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
mutex_unlock(&bdi->f_reg_lock);
}
+ if (ss_reg != bdi->ss_reg) {
+ /*
+ * The device is in host mode so when PG_STAT goes from 1->0
+ * (i.e., power removed) HIZ needs to be disabled.
+ */
+ if ((bdi->ss_reg & BQ24190_REG_SS_PG_STAT_MASK) &&
+ !(ss_reg & BQ24190_REG_SS_PG_STAT_MASK)) {
+ ret = bq24190_write_mask(bdi, BQ24190_REG_ISC,
+ BQ24190_REG_ISC_EN_HIZ_MASK,
+ BQ24190_REG_ISC_EN_HIZ_SHIFT,
+ 0);
+ if (ret < 0)
+ dev_err(bdi->dev, "Can't access ISC reg: %d\n",
+ ret);
+ }
+
+ if ((bdi->ss_reg & battery_mask_ss) != (ss_reg & battery_mask_ss))
+ alert_battery = true;
+ if ((bdi->ss_reg & ~battery_mask_ss) != (ss_reg & ~battery_mask_ss))
+ alert_charger = true;
+ bdi->ss_reg = ss_reg;
+ }
+
if (alert_charger)
power_supply_changed(bdi->charger);
if (alert_battery)
