[PATCH v3 0/6] BQ24190 charger fixes

[PATCH v3 0/6] BQ24190 charger fixes

[Liam Breck]

These are misc fixes, tested on a custom OMAP3 board.

Changes in v3

#1 fixes figure ID to 37
#5 fixes line over 80 chars
#* adds Acked-by Tony & Mark

Changes in v2

#1 reorders:            Fix irq triggering to IRQF_TRIGGER_FALLING
#2 factors out part of: Call power_supply_changed() only for relevant component
#3 replaces:            Call enable_irq() only at the end of probe()
#4 reorders:            Call power_supply_changed() only for relevant component
#5 unscrambles:         Don't read fault register outside irq_handle_thread()
#6 factors out part of: Don't read fault register outside irq_handle_thread()

These are dropped; Tony will resubmit them in new patchset:
  Check the interrupt status on resume
  Use PM runtime autosuspend

Initial patchset v1

Mark, I missed Tony's 0/6 preamble last time. It was...

Liam Breck (4):
  power: bq24190_charger: Call enable_irq() only at the end of probe()
  power: bq24190_charger: Fix irq triggering to IRQF_TRIGGER_FALLING
  power: bq24190_charger: Call power_supply_changed() only for
relevant component
  power: bq24190_charger: Don't read fault register outside irq_handle_thread()

Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

Re: [PATCH v3 0/6] BQ24190 charger fixes

[Sebastian Reichel]

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

Hi,

On Mon, Jan 16, 2017 at 06:28:28PM -0800, Liam Breck wrote:
> These are misc fixes, tested on a custom OMAP3 board.
> 
> Changes in v3
> 
> #1 fixes figure ID to 37
> #5 fixes line over 80 chars
> #* adds Acked-by Tony & Mark

You forgot to fix the whitespace issues Mark mentioned, also
please use "git send-mail" or make sure, that your patchset
is threaded (patch 1-6 are replies to "patch" 0) and the
actual patch is inline and can be applied using "git am"
(which currently complains).

-- Sebastian

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

Re: [PATCH v3 0/6] BQ24190 charger fixes

[Liam Breck]

On Mon, Jan 16, 2017 at 7:26 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Mon, Jan 16, 2017 at 06:28:28PM -0800, Liam Breck wrote:
>> These are misc fixes, tested on a custom OMAP3 board.
>>
>> Changes in v3
>>
>> #1 fixes figure ID to 37
>> #5 fixes line over 80 chars
>> #* adds Acked-by Tony & Mark
>
> You forgot to fix the whitespace issues Mark mentioned, also
> please use "git send-mail" or make sure, that your patchset
> is threaded (patch 1-6 are replies to "patch" 0) and the
> actual patch is inline and can be applied using "git am"
> (which currently complains).

Hi, would you accept the v3 patches as-is were I the module
maintainer? Because I am the de facto maintainer :-)

I contracted Mark to write this driver, and he hasn't had hardware on
which to test it for a few years...

Re: [PATCH v3 0/6] BQ24190 charger fixes

[Sebastian Reichel]

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

Hi,

On Mon, Jan 16, 2017 at 10:01:34PM -0800, Liam Breck wrote:
> On Mon, Jan 16, 2017 at 7:26 PM, Sebastian Reichel <sre@kernel.org> wrote:
> > On Mon, Jan 16, 2017 at 06:28:28PM -0800, Liam Breck wrote:
> >> These are misc fixes, tested on a custom OMAP3 board.
> >>
> >> Changes in v3
> >>
> >> #1 fixes figure ID to 37
> >> #5 fixes line over 80 chars
> >> #* adds Acked-by Tony & Mark
> >
> > You forgot to fix the whitespace issues Mark mentioned, also
> > please use "git send-mail" or make sure, that your patchset
> > is threaded (patch 1-6 are replies to "patch" 0) and the
> > actual patch is inline and can be applied using "git am"
> > (which currently complains).
> 
> Hi, would you accept the v3 patches as-is were I the module
> maintainer? Because I am the de facto maintainer :-)
> 
> I contracted Mark to write this driver, and he hasn't had hardware on
> which to test it for a few years...

So I mentioned three issues:

- Mail Threading
  Please *always* do this on kernel mailinglists. Otherwise
  other unrelated patches might be in between your patch series.

- MIME Patches
  Your patches do not apply. See also Section 6 in
  in Documentation/process/submitting-patches.rst

- Whitespace Issue
  If you do style changes, do them in a separate patch. If you
  move this new patch to the end of the series, I can apply the
  other ones directly.
  
-- Sebastian

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

[PATCH v4 0/7] BQ24190 charger fixes

[Liam Breck]

Hi Sebastian, thanks for your patience :-)

These are misc bq24190_charger fixes, tested on a custom OMAP3 board.

Changes in v4

#4 factors out whitespace change in "Install irq_handler_thread() at end of probe()"

Note: The other whitespace issue Mark raised is related to the purpose of that patch "Don't read 
fault register outside irq_handle_thread()"

Changes in v3

#1 fixes figure ID to 37
#5 fixes line over 80 chars
#* adds Acked-by Tony & Mark

Changes in v2

#1 reorders:            Fix irq triggering to IRQF_TRIGGER_FALLING
#2 factors out part of: Call power_supply_changed() only for relevant component
#3 replaces:            Call enable_irq() only at the end of probe()
#4 reorders:            Call power_supply_changed() only for relevant component
#5 unscrambles:         Don't read fault register outside irq_handle_thread()
#6 factors out part of: Don't read fault register outside irq_handle_thread()

These are dropped; Tony will resubmit them in new patchset:
  Check the interrupt status on resume
  Use PM runtime autosuspend

Initial patchset v1

Liam Breck (4):
  power: bq24190_charger: Call enable_irq() only at the end of probe()
  power: bq24190_charger: Fix irq triggering to IRQF_TRIGGER_FALLING
  power: bq24190_charger: Call power_supply_changed() only for relevant component
  power: bq24190_charger: Don't read fault register outside irq_handle_thread()

Tony Lindgren (2):
  power: bq24190_charger: Check the interrupt status on resume
  power: bq24190_charger: Use PM runtime autosuspend

[PATCH v4 1/7] power: bq24190_charger: Fix irq trigger to IRQF_TRIGGER_FALLING

[Liam Breck]

The interrupt signal is TRIGGER_FALLING. This is is specified in the
data sheet PIN FUNCTIONS: "The INT pin sends active low, 256us
pulse to host to report charger device status and fault."

Also the direction can be seen in the data sheet Figure 37 "BQ24190
with D+/D- Detection and USB On-The-Go (OTG)" which shows a 10k
pull-up resistor installed for the sample configurations.

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 694c088..f5746b9 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1394,7 +1394,7 @@ static int bq24190_probe(struct i2c_client *client,
 
 	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
 			bq24190_irq_handler_thread,
-			IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");

[PATCH v4 2/7] power: bq24190_charger: Call set_mode_host() on pm_resume()

[Liam Breck]

pm_resume() does a register_reset() which clears charger host mode.

Fix by calling set_mode_host() after the reset.

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index f5746b9..b51eac1 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1494,6 +1494,7 @@ static int bq24190_pm_resume(struct device *dev)
 
 	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
+	bq24190_set_mode_host(bdi);
 	pm_runtime_put_sync(bdi->dev);
 
 	/* Things may have changed while suspended so alert upper layer */

[PATCH v4 3/7] power: bq24190_charger: Install irq_handler_thread() at end of probe()

[Liam Breck]

The device specific data is not fully initialized on
request_threaded_irq(). This may cause a crash when the IRQ handler
tries to reference them.

Fix the issue by installing IRQ handler at the end of the probe.

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 35 ++++++++++++++++---------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index b51eac1..54c8952 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1392,22 +1392,13 @@ static int bq24190_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
-			bq24190_irq_handler_thread,
-			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-			"bq24190-charger", bdi);
-	if (ret < 0) {
-		dev_err(dev, "Can't set up irq handler\n");
-		goto out1;
-	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_resume(dev);
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out2;
+		goto out1;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1418,7 +1409,7 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->charger)) {
 		dev_err(dev, "Can't register charger\n");
 		ret = PTR_ERR(bdi->charger);
-		goto out2;
+		goto out1;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1427,24 +1418,34 @@ static int bq24190_probe(struct i2c_client *client,
 	if (IS_ERR(bdi->battery)) {
 		dev_err(dev, "Can't register battery\n");
 		ret = PTR_ERR(bdi->battery);
-		goto out3;
+		goto out2;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
+		goto out3;
+	}
+
+	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
+			bq24190_irq_handler_thread,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"bq24190-charger", bdi);
+	if (ret < 0) {
+		dev_err(dev, "Can't set up irq handler\n");
 		goto out4;
 	}
 
 	return 0;
 
 out4:
-	power_supply_unregister(bdi->battery);
+	bq24190_sysfs_remove_group(bdi);
 out3:
-	power_supply_unregister(bdi->charger);
+	power_supply_unregister(bdi->battery);
 out2:
-	pm_runtime_disable(dev);
+	power_supply_unregister(bdi->charger);
 out1:
+	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
 

[PATCH v4 4/7] power: bq24190_charger: Adjust formatting

[Liam Breck]

Add breathing room in probe() out* section.

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark A. Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 54c8952..62194d8 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1440,15 +1440,17 @@ static int bq24190_probe(struct i2c_client *client,
 
 out4:
 	bq24190_sysfs_remove_group(bdi);
+
 out3:
 	power_supply_unregister(bdi->battery);
+
 out2:
 	power_supply_unregister(bdi->charger);
+
 out1:
 	pm_runtime_disable(dev);
 	if (bdi->gpio_int)
 		gpio_free(bdi->gpio_int);
-
 	return ret;
 }
 

[PATCH v4 5/7] power: bq24190_charger: Call power_supply_changed() for relevant component

[Liam Breck]

We wrongly get uevents for bq24190-charger and bq24190-battery on every
register change.

Fix by checking the association with charger and battery before
emitting uevent(s).

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 50 +++++++++++++++++++---------------
 1 file changed, 27 insertions(+), 23 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 62194d8..ba5a5b2 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -159,7 +159,6 @@ struct bq24190_dev_info {
 	unsigned int			gpio_int;
 	unsigned int			irq;
 	struct mutex			f_reg_lock;
-	bool				first_time;
 	bool				charger_health_valid;
 	bool				battery_health_valid;
 	bool				battery_status_valid;
@@ -1197,7 +1196,10 @@ static const struct power_supply_desc bq24190_battery_desc = {
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
-	bool alert_userspace = false;
+	const u8 battery_mask_ss = BQ24190_REG_SS_CHRG_STAT_MASK;
+	const u8 battery_mask_f = BQ24190_REG_F_BAT_FAULT_MASK
+				| BQ24190_REG_F_NTC_FAULT_MASK;
+	bool alert_charger = false, alert_battery = false;
 	u8 ss_reg = 0, f_reg = 0;
 	int ret;
 
@@ -1225,8 +1227,12 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 					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;
-		alert_userspace = true;
 	}
 
 	mutex_lock(&bdi->f_reg_lock);
@@ -1239,33 +1245,23 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	}
 
 	if (f_reg != bdi->f_reg) {
+		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;
-
-		alert_userspace = true;
 	}
 
 	mutex_unlock(&bdi->f_reg_lock);
 
-	/*
-	 * Sometimes bq24190 gives a steady trickle of interrupts even
-	 * though the watchdog timer is turned off and neither the STATUS
-	 * nor FAULT registers have changed.  Weed out these sprurious
-	 * interrupts so userspace isn't alerted for no reason.
-	 * In addition, the chip always generates an interrupt after
-	 * register reset so we should ignore that one (the very first
-	 * interrupt received).
-	 */
-	if (alert_userspace) {
-		if (!bdi->first_time) {
-			power_supply_changed(bdi->charger);
-			power_supply_changed(bdi->battery);
-		} else {
-			bdi->first_time = false;
-		}
-	}
+	if (alert_charger)
+		power_supply_changed(bdi->charger);
+	if (alert_battery)
+		power_supply_changed(bdi->battery);
 
 out:
 	pm_runtime_put_sync(bdi->dev);
@@ -1300,6 +1296,10 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 		goto out;
 
 	ret = bq24190_set_mode_host(bdi);
+	if (ret < 0)
+		goto out;
+
+	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 out:
 	pm_runtime_put_sync(bdi->dev);
 	return ret;
@@ -1375,7 +1375,8 @@ static int bq24190_probe(struct i2c_client *client,
 	bdi->model = id->driver_data;
 	strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
 	mutex_init(&bdi->f_reg_lock);
-	bdi->first_time = true;
+	bdi->f_reg = 0;
+	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 	bdi->charger_health_valid = false;
 	bdi->battery_health_valid = false;
 	bdi->battery_status_valid = false;
@@ -1491,13 +1492,16 @@ static int bq24190_pm_resume(struct device *dev)
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
 
+	bdi->f_reg = 0;
+	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
 	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);
 	bq24190_set_mode_host(bdi);
+	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 	pm_runtime_put_sync(bdi->dev);
 
 	/* Things may have changed while suspended so alert upper layer */

[PATCH v4 6/7] power: bq24190_charger: Don't read fault register outside irq_handle_thread()

[Liam Breck]

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 data sheet: "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")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 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,35 @@ 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 +1343,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; /* impossible state */
-	bdi->charger_health_valid = false;
-	bdi->battery_health_valid = false;
-	bdi->battery_status_valid = false;
 
 	i2c_set_clientdata(client, bdi);
 
@@ -1494,9 +1457,6 @@ static int bq24190_pm_resume(struct device *dev)
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK; /* impossible state */
-	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);

[PATCH v4 7/7] power: bq24190_charger: Handle fault before status on interrupt

[Liam Breck]

Reading both fault and status registers and logging any fault should
take priority over handling status register update.

Fix by moving the status handling to later in interrupt routine.

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190 Battery Charger")
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Mark Greer <mgreer@animalcreek.com>
Acked-by: Tony Lindgren <tony@atomide.com>
---
 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);
@@ -1224,6 +1201,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)

Re: [PATCH v4 0/7] BQ24190 charger fixes

[Sebastian Reichel]

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

Hi,

On Wed, Jan 18, 2017 at 09:26:47AM -0800, Liam Breck wrote:
> Hi Sebastian, thanks for your patience :-)
> 
> These are misc bq24190_charger fixes, tested on a custom OMAP3 board.
> 
> Changes in v4
> 
> #4 factors out whitespace change in "Install irq_handler_thread() at end of probe()"
> 
> Note: The other whitespace issue Mark raised is related to the purpose of that patch "Don't read 
> fault register outside irq_handle_thread()"

I queued the patches fixing the subject to contain "supply: ".
Apart from that I dropped Liam's Acked-By from the whitespace
patch, as he never Acked those changes and dropped the Fixes
line from the same patch.

-- Sebastian

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