[PATCH 0/6] Few bq24190-charger fixes and improvments

[PATCH 0/6] Few bq24190-charger fixes and improvments

[Tony Lindgren]

Hi Sebastian,

Here are some bq24190 fixes. The first four are fixes from Liam Breck
that all can wait for the merge window.

The last two patches improve PM runtime for the driver.

Regards,

Tony

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

 drivers/power/supply/bq24190_charger.c | 309 ++++++++++++++++++++-------------
 1 file changed, 190 insertions(+), 119 deletions(-)

-- 
2.11.0

[PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Tony Lindgren]

From: Liam Breck <kernel@networkimprov.net>

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@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
[tony@atomide.com: cleaned up patch description a bit]
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 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:
-- 
2.11.0

[PATCH 2/6] power: bq24190_charger: Fix irq triggering to IRQF_TRIGGER_FALLING

[Tony Lindgren]

From: Liam Breck <kernel@networkimprov.net>

The interrupt signal is TRIGGER_FALLING. This is is specified
in the data sheet "PIN FUNCTIONS":

"The INT pin sends active low, 256 us pulse to hos to report charger
 device status and fault."

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

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190
Battery Charger")
Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
[tony@atomide.com: updated patch description]
Signed-off-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
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1395,7 +1395,7 @@ static int bq24190_probe(struct i2c_client *client,
 	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,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-- 
2.11.0

[PATCH 3/6] power: bq24190_charger: Call power_supply_changed() only for relevant component

[Tony Lindgren]

From: Liam Breck <kernel@networkimprov.net>

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

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

Fixes: d7bf353fd0aa3 ("bq24190_charger: Add support for TI BQ24190
Battery Charger")
Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
[tony@atomide.com: cleaned up a bit]
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 54 +++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 23 deletions(-)

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
@@ -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,15 @@ 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 +1248,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 +1299,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 +1378,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;
 	bdi->charger_health_valid = false;
 	bdi->battery_health_valid = false;
 	bdi->battery_status_valid = false;
@@ -1491,12 +1495,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;
 	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 */
-- 
2.11.0

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

[Tony Lindgren]

From: Liam Breck <kernel@networkimprov.net>

Caching the fault register after a single read may not keep an accurate
value.

Fix the issue by doing two reads of the fault register as specified in the
data sheet. And let's do this only irq_handle_thread() to avoid accidentally
clearing the fault status as specified in 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")
Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
[tony@atomide.com: cleaned up a bit]
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 99 ++++++++++------------------------
 1 file changed, 29 insertions(+), 70 deletions(-)

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
@@ -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,15 +1165,19 @@ 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);
 
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
-	if (ret < 0) {
-		dev_err(bdi->dev, "Can't read SS reg: %d\n", ret);
-		goto out;
-	}
+	/* We need to read f_reg twice if fault is set to get correct value */
+	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 (ss_reg != bdi->ss_reg) {
 		/*
@@ -1235,32 +1203,29 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 		    (ss_reg & ~battery_mask_ss))
 			alert_charger = true;
 
+		mutex_lock(&bdi->f_reg_lock);
 		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;
 	}
 
 	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));
+
 		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;
 
+		mutex_lock(&bdi->f_reg_lock);
 		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)
@@ -1380,9 +1345,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);
 
@@ -1497,9 +1459,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);
-- 
2.11.0

[PATCH 5/6] power: bq24190_charger: Check the interrupt status on resume

[Tony Lindgren]

Some SoCs like omap3 can configure GPIO irqs to use Linux generic
dedicated wakeirq support. If the dedicated wakeirq is configured,
the SoC will use a always-on interrupt controller to produce wake-up
events.

If bq24190 is configured for dedicated wakeirq, we need to check the
interrupt status on PM runtime resume. This is because the Linux
generic wakeirq will call pm_runtime_resume() on the device on a
wakeirq. And as the bq24190 interrupt is falling edge sensitive
and only active for 250 us, there will be no device interrupt seen
by the runtime SoC IRQ controller.

Note that this can cause spurious interrupts on omap3 devices with
bq24190 connected to gpio banks 2 - 5 as there's a glitch on those
pins waking from off mode as listed in "Advisory 1.45". Devices
with this issue should not configure the optional wakeirq interrupt
in the dts file.

Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Cc: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 58 ++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 9 deletions(-)

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
@@ -156,6 +156,8 @@ struct bq24190_dev_info {
 	unsigned int			gpio_int;
 	unsigned int			irq;
 	struct mutex			f_reg_lock;
+	bool				initialized;
+	bool				irq_event;
 	u8				f_reg;
 	u8				ss_reg;
 	u8				watchdog;
@@ -1157,9 +1159,8 @@ static const struct power_supply_desc bq24190_battery_desc = {
 	.property_is_writeable	= bq24190_battery_property_is_writeable,
 };
 
-static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
+static void bq24190_check_status(struct bq24190_dev_info *bdi)
 {
-	struct bq24190_dev_info *bdi = data;
 	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;
@@ -1167,15 +1168,13 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	u8 ss_reg = 0, f_reg = 0;
 	int i, ret;
 
-	pm_runtime_get_sync(bdi->dev);
-
 	/* We need to read f_reg twice if fault is set to get correct value */
 	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;
+			return;
 		}
 	} while (f_reg && ++i < 2);
 
@@ -1231,11 +1230,18 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	if (alert_battery)
 		power_supply_changed(bdi->battery);
 
-out:
-	pm_runtime_put_sync(bdi->dev);
-
 	dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
+}
 
+static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
+{
+	struct bq24190_dev_info *bdi = data;
+
+	bdi->irq_event = true;
+	pm_runtime_get_sync(bdi->dev);
+	bq24190_check_status(bdi);
+	pm_runtime_put_sync(bdi->dev);
+	bdi->irq_event = false;
 	return IRQ_HANDLED;
 }
 
@@ -1404,6 +1410,7 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	enable_irq(bdi->irq);
+	bdi->initialized = 1;
 
 	return 0;
 
@@ -1439,6 +1446,35 @@ static int bq24190_remove(struct i2c_client *client)
 	return 0;
 }
 
+static int bq24190_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+
+	if (!bdi->initialized)
+		return 0;
+
+	dev_dbg(bdi->dev, "%s\n", __func__);
+
+	return 0;
+}
+
+static int bq24190_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+
+	if (!bdi->initialized)
+		return 0;
+
+	if (!bdi->irq_event) {
+		dev_dbg(bdi->dev, "checking events on possible wakeirq\n");
+		bq24190_check_status(bdi);
+	}
+
+	return 0;
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int bq24190_pm_suspend(struct device *dev)
 {
@@ -1474,7 +1510,11 @@ static int bq24190_pm_resume(struct device *dev)
 }
 #endif
 
-static SIMPLE_DEV_PM_OPS(bq24190_pm_ops, bq24190_pm_suspend, bq24190_pm_resume);
+static const struct dev_pm_ops bq24190_pm_ops = {
+	SET_RUNTIME_PM_OPS(bq24190_runtime_suspend, bq24190_runtime_resume,
+			   NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(bq24190_pm_suspend, bq24190_pm_resume)
+};
 
 /*
  * Only support the bq24190 right now.  The bq24192, bq24192i, and bq24193
-- 
2.11.0

[PATCH 6/6] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

We can get quite a few interrupts when the battery is trickle charging.
Let's enable PM runtime autosuspend to avoid constantly toggling device
state.

Let's use a 600 ms timeout as that's how long the USB chager detection
might take.

Cc: Mark A. Greer <mgreer@animalcreek.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Cc: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
 drivers/power/supply/bq24190_charger.c | 101 ++++++++++++++++++++++++++-------
 1 file changed, 81 insertions(+), 20 deletions(-)

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
@@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	struct power_supply *psy = dev_get_drvdata(dev);
 	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
 	struct bq24190_sysfs_field_info *info;
+	ssize_t count;
 	int ret;
 	u8 v;
 
@@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
 	if (!info)
 		return -EINVAL;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
 	if (ret)
-		return ret;
+		count = ret;
+	else
+		count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
-	return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
+	return count;
 }
 
 static ssize_t bq24190_sysfs_store(struct device *dev,
@@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
 	if (ret < 0)
 		return ret;
 
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
+
 	ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
 	if (ret)
-		return ret;
+		count = ret;
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return count;
 }
@@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_CHARGE_TYPE:
@@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 
 	dev_dbg(bdi->dev, "prop: %d\n", psp);
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	switch (psp) {
 	case POWER_SUPPLY_PROP_STATUS:
@@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
 		ret = -ENODATA;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1119,7 +1148,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
 		ret = -EINVAL;
 	}
 
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1236,11 +1267,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 {
 	struct bq24190_dev_info *bdi = data;
+	int ret;
 
 	bdi->irq_event = true;
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 	bq24190_check_status(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 	bdi->irq_event = false;
 	return IRQ_HANDLED;
 }
@@ -1250,7 +1285,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	u8 v;
 	int ret;
 
-	pm_runtime_get_sync(bdi->dev);
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		return ret;
 
 	/* First check that the device really is what its supposed to be */
 	ret = bq24190_read_mask(bdi, BQ24190_REG_VPRS,
@@ -1275,7 +1312,9 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 
 	ret = bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 out:
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+
 	return ret;
 }
 
@@ -1373,9 +1412,14 @@ static int bq24190_probe(struct i2c_client *client,
 		dev_err(dev, "Can't set up irq handler\n");
 		goto out1;
 	}
+	enable_irq_wake(bdi->irq);
 
 	pm_runtime_enable(dev);
-	pm_runtime_resume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 600);
+	pm_runtime_use_autosuspend(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0)
+		goto out2;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
@@ -1412,6 +1456,9 @@ static int bq24190_probe(struct i2c_client *client,
 	enable_irq(bdi->irq);
 	bdi->initialized = 1;
 
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
 	return 0;
 
 out4:
@@ -1419,6 +1466,7 @@ static int bq24190_probe(struct i2c_client *client,
 out3:
 	power_supply_unregister(bdi->charger);
 out2:
+	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 out1:
 	if (bdi->gpio_int)
@@ -1430,9 +1478,14 @@ static int bq24190_probe(struct i2c_client *client,
 static int bq24190_remove(struct i2c_client *client)
 {
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int ret;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
 
-	pm_runtime_get_sync(bdi->dev);
 	bq24190_register_reset(bdi);
+	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_put_sync(bdi->dev);
 
 	bq24190_sysfs_remove_group(bdi);
@@ -1480,10 +1533,14 @@ static int bq24190_pm_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
 	struct bq24190_dev_info *bdi = i2c_get_clientdata(client);
+	int error;
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
 	bq24190_register_reset(bdi);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	return 0;
 }
@@ -1492,15 +1549,19 @@ 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);
+	int error;
 
 	bdi->f_reg = 0;
 	bdi->ss_reg = BQ24190_REG_SS_VBUS_STAT_MASK;
 
-	pm_runtime_get_sync(bdi->dev);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0)
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
 	bq24190_register_reset(bdi);
 	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
-	pm_runtime_put_sync(bdi->dev);
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-- 
2.11.0

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

[Liam Breck]

On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Caching the fault register after a single read may not keep an accurate
> value.
>
> Fix the issue by doing two reads of the fault register as specified in the
> data sheet. And let's do this only irq_handle_thread() to avoid accidentally
> clearing the fault status as specified in 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")
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> [tony@atomide.com: cleaned up a bit]
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 99 ++++++++++------------------------
>  1 file changed, 29 insertions(+), 70 deletions(-)

This got a bit mangled in transition, and the fix will prevent the
following patches from applying.

I'll post a fix shortly.

Re: [PATCH 6/6] power: bq24190_charger: Use PM runtime autosuspend

[Liam Breck]

On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> We can get quite a few interrupts when the battery is trickle charging.
> Let's enable PM runtime autosuspend to avoid constantly toggling device
> state.
>
> Let's use a 600 ms timeout as that's how long the USB chager detection
> might take.
>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Cc: Liam Breck <kernel@networkimprov.net>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 101 ++++++++++++++++++++++++++-------
>  1 file changed, 81 insertions(+), 20 deletions(-)
>
> 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
> @@ -420,6 +420,7 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         struct power_supply *psy = dev_get_drvdata(dev);
>         struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
>         struct bq24190_sysfs_field_info *info;
> +       ssize_t count;
>         int ret;
>         u8 v;
>
> @@ -427,11 +428,20 @@ static ssize_t bq24190_sysfs_show(struct device *dev,
>         if (!info)
>                 return -EINVAL;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_read_mask(bdi, info->reg, info->mask, info->shift, &v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +       else
> +               count = scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
> -       return scnprintf(buf, PAGE_SIZE, "%hhx\n", v);
> +       return count;
>  }
>
>  static ssize_t bq24190_sysfs_store(struct device *dev,
> @@ -451,9 +461,16 @@ static ssize_t bq24190_sysfs_store(struct device *dev,
>         if (ret < 0)
>                 return ret;
>
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ret = bq24190_write_mask(bdi, info->reg, info->mask, info->shift, v);
>         if (ret)
> -               return ret;
> +               count = ret;
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
>
>         return count;
>  }
> @@ -795,7 +812,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -835,7 +854,9 @@ static int bq24190_charger_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -848,7 +869,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_CHARGE_TYPE:
> @@ -864,7 +887,9 @@ static int bq24190_charger_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1065,7 +1090,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>
>         dev_dbg(bdi->dev, "prop: %d\n", psp);
>
> -       pm_runtime_get_sync(bdi->dev);
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0)
> +               return ret;
>
>         switch (psp) {
>         case POWER_SUPPLY_PROP_STATUS:
> @@ -1093,7 +1120,9 @@ static int bq24190_battery_get_property(struct power_supply *psy,
>                 ret = -ENODATA;
>         }
>
> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }
>
> @@ -1119,7 +1148,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
>                 ret = -EINVAL;
>         }

pm_runtime_get_sync() in this function needs its return value checked.


> -       pm_runtime_put_sync(bdi->dev);
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +
>         return ret;
>  }

Please submit this and #5 as a separate patchset after I resubmit 1-4.

Re: [PATCH 5/6] power: bq24190_charger: Check the interrupt status on resume

[Liam Breck]

On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> Some SoCs like omap3 can configure GPIO irqs to use Linux generic
> dedicated wakeirq support. If the dedicated wakeirq is configured,
> the SoC will use a always-on interrupt controller to produce wake-up
> events.
>
> If bq24190 is configured for dedicated wakeirq, we need to check the
> interrupt status on PM runtime resume. This is because the Linux
> generic wakeirq will call pm_runtime_resume() on the device on a
> wakeirq. And as the bq24190 interrupt is falling edge sensitive
> and only active for 250 us, there will be no device interrupt seen
> by the runtime SoC IRQ controller.
>
> Note that this can cause spurious interrupts on omap3 devices with
> bq24190 connected to gpio banks 2 - 5 as there's a glitch on those
> pins waking from off mode as listed in "Advisory 1.45". Devices
> with this issue should not configure the optional wakeirq interrupt
> in the dts file.
>
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Cc: Liam Breck <kernel@networkimprov.net>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 58 ++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
>
> 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
> @@ -156,6 +156,8 @@ struct bq24190_dev_info {
>         unsigned int                    gpio_int;
>         unsigned int                    irq;
>         struct mutex                    f_reg_lock;
> +       bool                            initialized;
> +       bool                            irq_event;
>         u8                              f_reg;
>         u8                              ss_reg;
>         u8                              watchdog;
> @@ -1157,9 +1159,8 @@ static const struct power_supply_desc bq24190_battery_desc = {
>         .property_is_writeable  = bq24190_battery_property_is_writeable,
>  };
>
> -static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> +static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  {
> -       struct bq24190_dev_info *bdi = data;
>         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;
> @@ -1167,15 +1168,13 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         u8 ss_reg = 0, f_reg = 0;
>         int i, ret;
>
> -       pm_runtime_get_sync(bdi->dev);
> -
>         /* We need to read f_reg twice if fault is set to get correct value */
>         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;
> +                       return;
>                 }
>         } while (f_reg && ++i < 2);
>
> @@ -1231,11 +1230,18 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         if (alert_battery)
>                 power_supply_changed(bdi->battery);
>
> -out:
> -       pm_runtime_put_sync(bdi->dev);
> -
>         dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
> +}
>
> +static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
> +{
> +       struct bq24190_dev_info *bdi = data;
> +
> +       bdi->irq_event = true;
> +       pm_runtime_get_sync(bdi->dev);
> +       bq24190_check_status(bdi);
> +       pm_runtime_put_sync(bdi->dev);
> +       bdi->irq_event = false;
>         return IRQ_HANDLED;
>  }
>
> @@ -1404,6 +1410,7 @@ static int bq24190_probe(struct i2c_client *client,
>         }
>
>         enable_irq(bdi->irq);
> +       bdi->initialized = 1;

Should initialized be set before enable_irq()?

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

[Liam Breck]

On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> Caching the fault register after a single read may not keep an accurate
> value.
>
> Fix the issue by doing two reads of the fault register as specified in the
> data sheet. And let's do this only irq_handle_thread() to avoid accidentally
> clearing the fault status as specified in 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")
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> [tony@atomide.com: cleaned up a bit]
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  drivers/power/supply/bq24190_charger.c | 99 ++++++++++------------------------
>  1 file changed, 29 insertions(+), 70 deletions(-)
>
> 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
> @@ -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);

bdi->f_reg is the only thing protected by the mutex, and is only ever
set or read. Is it OK to use an atomic type here?

Re: [PATCH 6/6] power: bq24190_charger: Use PM runtime autosuspend

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170111 18:03]:
> On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> > @@ -1119,7 +1148,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
> >                 ret = -EINVAL;
> >         }
> 
> pm_runtime_get_sync() in this function needs its return value checked.

OK will check.

> > -       pm_runtime_put_sync(bdi->dev);
> > +       pm_runtime_mark_last_busy(bdi->dev);
> > +       pm_runtime_put_autosuspend(bdi->dev);
> > +
> >         return ret;
> >  }
> 
> Please submit this and #5 as a separate patchset after I resubmit 1-4.

Sure that works for me.

Tony

Re: [PATCH 5/6] power: bq24190_charger: Check the interrupt status on resume

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170111 18:06]:
> > @@ -1404,6 +1410,7 @@ static int bq24190_probe(struct i2c_client *client,
> >         }
> >
> >         enable_irq(bdi->irq);
> > +       bdi->initialized = 1;
> 
> Should initialized be set before enable_irq()?

Yes good catch will check.

Thanks,

Tony

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

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170111 18:12]:
> On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> > From: Liam Breck <kernel@networkimprov.net>
> >
> > Caching the fault register after a single read may not keep an accurate
> > value.
> >
> > Fix the issue by doing two reads of the fault register as specified in the
> > data sheet. And let's do this only irq_handle_thread() to avoid accidentally
> > clearing the fault status as specified in 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")
> > Cc: Mark A. Greer <mgreer@animalcreek.com>
> > Cc: Matt Ranostay <matt@ranostay.consulting>
> > Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > [tony@atomide.com: cleaned up a bit]
> > Signed-off-by: Tony Lindgren <tony@atomide.com>
> > ---
> >  drivers/power/supply/bq24190_charger.c | 99 ++++++++++------------------------
> >  1 file changed, 29 insertions(+), 70 deletions(-)
> >
> > 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
> > @@ -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);
> 
> bdi->f_reg is the only thing protected by the mutex, and is only ever
> set or read. Is it OK to use an atomic type here?

Yeah seems that should work if we don't need to protect any longer
sections of the code with the mutex. Such as a read-write over I2c
to the bq24190 registers.

Regards,

Tony

Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Sebastian Reichel]

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

Hi Liam & Tony,

On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> 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@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> [tony@atomide.com: cleaned up patch description a bit]
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
>  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.

-- Sebastian

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

Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Liam Breck]

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

On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi Liam & Tony,
>
> On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> 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@animalcreek.com>
>> Cc: Matt Ranostay <matt@ranostay.consulting>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> [tony@atomide.com: cleaned up patch description a bit]
>> Signed-off-by: Tony Lindgren <tony@atomide.com>
>> ---
>>  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?

It looks like this:

[-- Attachment #2: temp-paste.txt --]
[-- Type: text/plain, Size: 1979 bytes --]

diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
index b51eac1..54c8952 100644
--- a/drivers/power/bq24190_charger.c
+++ b/drivers/power/bq24190_charger.c
@@ -1366,22 +1366,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_RISING | 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;
@@ -1392,7 +1383,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;
@@ -1401,24 +1392,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_RISING | 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);

Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170112 12:25]:
> On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > Hi Liam & Tony,
> >
> > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >>
> >> 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@animalcreek.com>
> >> Cc: Matt Ranostay <matt@ranostay.consulting>
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> [tony@atomide.com: cleaned up patch description a bit]
> >> Signed-off-by: Tony Lindgren <tony@atomide.com>
> >> ---
> >>  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.

Regards,

Tony

> It looks like this:

> diff --git a/drivers/power/bq24190_charger.c b/drivers/power/bq24190_charger.c
> index b51eac1..54c8952 100644
> --- a/drivers/power/bq24190_charger.c
> +++ b/drivers/power/bq24190_charger.c
> @@ -1366,22 +1366,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_RISING | 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;
> @@ -1392,7 +1383,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;
> @@ -1401,24 +1392,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_RISING | 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);

Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Sebastian Reichel]

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

Hi,

On Thu, Jan 12, 2017 at 12:58:11PM -0800, Tony Lindgren wrote:
> * Liam Breck <liam@networkimprov.net> [170112 12:25]:
> > On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@kernel.org> wrote:
> > > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
> > >> From: Liam Breck <kernel@networkimprov.net>
> > >>
> > >> 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@animalcreek.com>
> > >> Cc: Matt Ranostay <matt@ranostay.consulting>
> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > >> [tony@atomide.com: cleaned up patch description a bit]
> > >> Signed-off-by: Tony Lindgren <tony@atomide.com>
> > >> ---
> > >>  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.

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

-- Sebastian

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

Re: [PATCH 1/6] power: bq24190_charger: Call enable_irq() only at the end of probe()

[Liam Breck]

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

On Thu, Jan 12, 2017 at 1:40 PM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Jan 12, 2017 at 12:58:11PM -0800, Tony Lindgren wrote:
>> * Liam Breck <liam@networkimprov.net> [170112 12:25]:
>> > On Thu, Jan 12, 2017 at 9:44 AM, Sebastian Reichel <sre@kernel.org> wrote:
>> > > On Wed, Jan 11, 2017 at 04:41:49PM -0800, Tony Lindgren wrote:
>> > >> From: Liam Breck <kernel@networkimprov.net>
>> > >>
>> > >> 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@animalcreek.com>
>> > >> Cc: Matt Ranostay <matt@ranostay.consulting>
>> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> > >> [tony@atomide.com: cleaned up patch description a bit]
>> > >> Signed-off-by: Tony Lindgren <tony@atomide.com>
>> > >> ---
>> > >>  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:

[-- Attachment #2: temp-paste.txt --]
[-- Type: text/plain, Size: 9862 bytes --]

From ea15efae6ecfa5045a95b4158a45a05544dcc259 Mon Sep 17 00:00:00 2001
From: Liam <github@networkimprov.net>
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@animalcreek.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 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@networkimprov.net>
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@animalcreek.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 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)

Re: [PATCH 5/6] power: bq24190_charger: Check the interrupt status on resume

[Mark Greer]

On Wed, Jan 11, 2017 at 04:41:53PM -0800, Tony Lindgren wrote:
> Some SoCs like omap3 can configure GPIO irqs to use Linux generic
> dedicated wakeirq support. If the dedicated wakeirq is configured,
> the SoC will use a always-on interrupt controller to produce wake-up
> events.
> 
> If bq24190 is configured for dedicated wakeirq, we need to check the
> interrupt status on PM runtime resume. This is because the Linux
> generic wakeirq will call pm_runtime_resume() on the device on a
> wakeirq. And as the bq24190 interrupt is falling edge sensitive
> and only active for 250 us, there will be no device interrupt seen
> by the runtime SoC IRQ controller.
> 
> Note that this can cause spurious interrupts on omap3 devices with
> bq24190 connected to gpio banks 2 - 5 as there's a glitch on those
> pins waking from off mode as listed in "Advisory 1.45". Devices
> with this issue should not configure the optional wakeirq interrupt
> in the dts file.
> 
> Cc: Mark A. Greer <mgreer@animalcreek.com>
> Cc: Matt Ranostay <matt@ranostay.consulting>
> Cc: Liam Breck <kernel@networkimprov.net>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---

Looks good except for what Liam pointed out so assuming that gets fixed:

Acked-by: Mark Greer <mgreer@animalcreek.com>

Re: [PATCH 6/6] power: bq24190_charger: Use PM runtime autosuspend

[Mark Greer]

On Thu, Jan 12, 2017 at 07:46:26AM -0800, Tony Lindgren wrote:
> * Liam Breck <liam@networkimprov.net> [170111 18:03]:
> > On Wed, Jan 11, 2017 at 4:41 PM, Tony Lindgren <tony@atomide.com> wrote:
> > > @@ -1119,7 +1148,9 @@ static int bq24190_battery_set_property(struct power_supply *psy,
> > >                 ret = -EINVAL;
> > >         }
> > 
> > pm_runtime_get_sync() in this function needs its return value checked.
> 
> OK will check.

Tony, if you add the check, feel free to add my Acked-by:

Acked-by: Mark Greer <mgreer@animalcreek.com>