[PATCH v5 0/4] BQ24190 charger fixes

[PATCH v5 0/4] BQ24190 charger fixes

[Liam Breck]

Hi Sebastian,

Here in one patchset are two patches I posted recently, and two more:

Patch 1 is a replacement (as agreed) for
  "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
  original patch, let me know.
Patch 3 (repost) fixes some issues with PM runtime error handling.
Patch 4 fixes a nit in register_reset().

Changed in v5:
* move condition in extcon OTG boost code
* document extcon client invocation of charger driver
* udelay -> usleep_range in register_reset()

Changes in v4:
* fix definition of bq24190_reg_poc_chg_config_*
* revert to if (device_property_read... == 0)

[PATCH v5 1/4] power: bq24190_charger: Limit over/under voltage fault logging

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

If the charger is unplugged before the battery is full we may
see an over/under voltage fault. Ignore this rather then emitting
a message or uevent.

This fixes messages like these getting logged on charger unplug + replug:
bq24190-charger 15-006b: Fault: boost 0, charge 1, battery 0, ntc 0
bq24190-charger 15-006b: Fault: boost 0, charge 0, battery 0, ntc 0

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a699ad3..64a48b9 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1222,8 +1222,13 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 		}
 	} while (f_reg && ++i < 2);
 
+	/* ignore over/under voltage fault after disconnect */
+	if (f_reg == (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT) &&
+	    !(ss_reg & BQ24190_REG_SS_PG_STAT_MASK))
+		f_reg = 0;
+
 	if (f_reg != bdi->f_reg) {
-		dev_info(bdi->dev,
+		dev_warn(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),
-- 
2.9.3

[PATCH v5 2/4] power: bq24190_charger: Clean up extcon code

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Polishing and fixes for initial extcon patch.

Fixes: 4db249b6f3b4 ("power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost")
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 84 +++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 38 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 64a48b9..69cd344 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -38,8 +38,9 @@
 #define BQ24190_REG_POC_WDT_RESET_SHIFT		6
 #define BQ24190_REG_POC_CHG_CONFIG_MASK		(BIT(5) | BIT(4))
 #define BQ24190_REG_POC_CHG_CONFIG_SHIFT	4
-#define BQ24190_REG_POC_CHG_CONFIG_CHARGE	1
-#define BQ24190_REG_POC_CHG_CONFIG_OTG		2
+#define BQ24190_REG_POC_CHG_CONFIG_DISABLE		0x0
+#define BQ24190_REG_POC_CHG_CONFIG_CHARGE		0x1
+#define BQ24190_REG_POC_CHG_CONFIG_OTG			0x2
 #define BQ24190_REG_POC_SYS_MIN_MASK		(BIT(3) | BIT(2) | BIT(1))
 #define BQ24190_REG_POC_SYS_MIN_SHIFT		1
 #define BQ24190_REG_POC_BOOST_LIM_MASK		BIT(0)
@@ -173,8 +174,9 @@ struct bq24190_dev_info {
  */
 
 /* REG00[2:0] (IINLIM) in uAh */
-static const int bq24190_iinlim_values[] = {
-	100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
+static const int bq24190_isc_iinlim_values[] = {
+	 100000,  150000,  500000,  900000, 1200000, 1500000, 2000000, 3000000
+};
 
 /* REG02[7:2] (ICHG) in uAh */
 static const int bq24190_ccc_ichg_values[] = {
@@ -1298,16 +1300,18 @@ static void bq24190_extcon_work(struct work_struct *work)
 {
 	struct bq24190_dev_info *bdi =
 		container_of(work, struct bq24190_dev_info, extcon_work.work);
-	int ret, iinlim = 0;
+	int error, iinlim = 0;
+	u8 v;
 
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0) {
-		dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
 		return;
 	}
 
-	if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
-		iinlim = 500000;
+	if      (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1)
+		iinlim =  500000;
 	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 ||
 		 extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1)
 		iinlim = 1500000;
@@ -1315,33 +1319,28 @@ static void bq24190_extcon_work(struct work_struct *work)
 		iinlim = 2000000;
 
 	if (iinlim) {
-		ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
-				BQ24190_REG_ISC_IINLIM_MASK,
-				BQ24190_REG_ISC_IINLIM_SHIFT,
-				bq24190_iinlim_values,
-				ARRAY_SIZE(bq24190_iinlim_values),
-				iinlim);
-		if (ret)
-			dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret);
+		error = bq24190_set_field_val(bdi, BQ24190_REG_ISC,
+					      BQ24190_REG_ISC_IINLIM_MASK,
+					      BQ24190_REG_ISC_IINLIM_SHIFT,
+					      bq24190_isc_iinlim_values,
+					      ARRAY_SIZE(bq24190_isc_iinlim_values),
+					      iinlim);
+		if (error < 0)
+			dev_err(bdi->dev, "Can't set IINLIM: %d\n", error);
 	}
 
-	/*
-	 * If no charger has been detected and host mode is requested, activate
-	 * the 5V boost converter, otherwise deactivate it.
-	 */
-	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) {
-		ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
-					 BQ24190_REG_POC_CHG_CONFIG_MASK,
-					 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
-					 BQ24190_REG_POC_CHG_CONFIG_OTG);
-	} else {
-		ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
-					 BQ24190_REG_POC_CHG_CONFIG_MASK,
-					 BQ24190_REG_POC_CHG_CONFIG_SHIFT,
-					 BQ24190_REG_POC_CHG_CONFIG_CHARGE);
-	}
-	if (ret)
-		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", ret);
+	/* if no charger found and in USB host mode, set OTG 5V boost, else normal */
+	if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1)
+		v = BQ24190_REG_POC_CHG_CONFIG_OTG;
+	else
+		v = BQ24190_REG_POC_CHG_CONFIG_CHARGE;
+
+	error = bq24190_write_mask(bdi, BQ24190_REG_POC,
+				   BQ24190_REG_POC_CHG_CONFIG_MASK,
+				   BQ24190_REG_POC_CHG_CONFIG_SHIFT,
+				   v);
+	if (error < 0)
+		dev_err(bdi->dev, "Can't set CHG_CONFIG: %d\n", error);
 
 	pm_runtime_mark_last_busy(bdi->dev);
 	pm_runtime_put_autosuspend(bdi->dev);
@@ -1432,8 +1431,15 @@ static int bq24190_probe(struct i2c_client *client,
 	}
 
 	/*
-	 * The extcon-name property is purely an in kernel interface for
-	 * x86/ACPI use, DT platforms should get extcon via phandle.
+	 * Devicetree platforms should get extcon via phandle (not yet supported).
+	 * On ACPI platforms, extcon clients may invoke us with:
+	 * struct property_entry pe[] =
+	 *   { PROPERTY_ENTRY_STRING("extcon-name", client_name), ... };
+	 * struct i2c_board_info bi =
+	 *   { .type = "bq24190", .addr = 0x6b, .properties = pe, .irq = irq };
+	 * struct i2c_adapter ad = { ... };
+	 * i2c_add_adapter(&ad);
+	 * i2c_new_device(&ad, &bi);
 	 */
 	if (device_property_read_string(dev, "extcon-name", &name) == 0) {
 		bdi->extcon = extcon_get_extcon_dev(name);
@@ -1498,8 +1504,10 @@ static int bq24190_probe(struct i2c_client *client,
 		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
 		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
 						    &bdi->extcon_nb);
-		if (ret)
+		if (ret) {
+			dev_err(dev, "Can't register extcon\n");
 			goto out5;
+		}
 
 		/* Sync initial cable state */
 		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
-- 
2.9.3

[PATCH v5 3/4] power: bq24190_charger: Uniform pm_runtime_get() failure handling

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

On pm_runtime_get() failure, always emit an error message.
Prevent unbalanced pm_runtime_get by calling:
  pm_runtime_put_noidle() in irq handler
  pm_runtime_put_sync() on any probe() failure
Rename probe() out labels instead of renumbering them.

Fixes: 13d6fa8447fa ("power: bq24190_charger: Use PM runtime autosuspend")
Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Acked-by: Tony Lindgren <tony@atomide.com>
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/power/supply/bq24190_charger.c | 37 +++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 69cd344..a904abb 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1280,12 +1280,13 @@ 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;
+	int error;
 
 	bdi->irq_event = true;
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0) {
-		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", ret);
+	error = pm_runtime_get_sync(bdi->dev);
+	if (error < 0) {
+		dev_warn(bdi->dev, "pm_runtime_get failed: %i\n", error);
+		pm_runtime_put_noidle(bdi->dev);
 		return IRQ_NONE;
 	}
 	bq24190_check_status(bdi);
@@ -1453,13 +1454,15 @@ static int bq24190_probe(struct i2c_client *client,
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
 	ret = pm_runtime_get_sync(dev);
-	if (ret < 0)
-		goto out1;
+	if (ret < 0) {
+		dev_err(dev, "pm_runtime_get failed: %i\n", ret);
+		goto out_pmrt;
+	}
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out2;
+		goto out_pmrt;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1470,7 +1473,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 out_pmrt;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1479,13 +1482,13 @@ 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 out_charger;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out4;
+		goto out_battery;
 	}
 
 	bdi->initialized = true;
@@ -1496,7 +1499,7 @@ static int bq24190_probe(struct i2c_client *client,
 			"bq24190-charger", bdi);
 	if (ret < 0) {
 		dev_err(dev, "Can't set up irq handler\n");
-		goto out5;
+		goto out_sysfs;
 	}
 
 	if (bdi->extcon) {
@@ -1506,7 +1509,7 @@ static int bq24190_probe(struct i2c_client *client,
 						    &bdi->extcon_nb);
 		if (ret) {
 			dev_err(dev, "Can't register extcon\n");
-			goto out5;
+			goto out_sysfs;
 		}
 
 		/* Sync initial cable state */
@@ -1520,19 +1523,17 @@ static int bq24190_probe(struct i2c_client *client,
 
 	return 0;
 
-out5:
+out_sysfs:
 	bq24190_sysfs_remove_group(bdi);
 
-out4:
+out_battery:
 	power_supply_unregister(bdi->battery);
 
-out3:
+out_charger:
 	power_supply_unregister(bdi->charger);
 
-out2:
+out_pmrt:
 	pm_runtime_put_sync(dev);
-
-out1:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	return ret;
-- 
2.9.3

[PATCH v5 4/4] power: bq24190_charger: Longer delay while polling reset flag

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

On chip reset, polling loop used udelay(10) which is too short
to be useful. Instead, use usleep_range(100, 200).

Cc: Tony Lindgren <tony@atomide.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq24190_charger.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index a904abb..f581042 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -550,16 +550,13 @@ static int bq24190_register_reset(struct bq24190_dev_info *bdi)
 		if (ret < 0)
 			return ret;
 
+		if (v == 0)
+			return 0;
-		if (!v)
-			break;
 
-		udelay(10);
+		usleep_range(100, 200);
 	} while (--limit);
 
-	if (!limit)
-		return -EIO;
-
-	return 0;
+	return -EIO;
 }
 
 /* Charger power supply property routines */
-- 
2.9.3

Re: [PATCH v5 4/4] power: bq24190_charger: Longer delay while polling reset flag

[Tony Lindgren]

* Liam Breck <liam@networkimprov.net> [170411 05:02]:
> From: Liam Breck <kernel@networkimprov.net>
> 
> On chip reset, polling loop used udelay(10) which is too short
> to be useful. Instead, use usleep_range(100, 200).

Good to see this get rid of udelay in favor of usleep:

Acked-by: Tony Lindgren <tony@atomide.com>

> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq24190_charger.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index a904abb..f581042 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -550,16 +550,13 @@ static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>  		if (ret < 0)
>  			return ret;
>  
> +		if (v == 0)
> +			return 0;
> -		if (!v)
> -			break;
>  
> -		udelay(10);
> +		usleep_range(100, 200);
>  	} while (--limit);
>  
> -	if (!limit)
> -		return -EIO;
> -
> -	return 0;
> +	return -EIO;
>  }
>  
>  /* Charger power supply property routines */
> -- 
> 2.9.3
> 

Re: [PATCH v5 0/4] BQ24190 charger fixes

[Hans de Goede]

Hi,

On 11-04-17 13:59, Liam Breck wrote:
> Hi Sebastian,
>
> Here in one patchset are two patches I posted recently, and two more:
>
> Patch 1 is a replacement (as agreed) for
>   "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
> Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
>   original patch, let me know.
> Patch 3 (repost) fixes some issues with PM runtime error handling.
> Patch 4 fixes a nit in register_reset().
>
> Changed in v5:
> * move condition in extcon OTG boost code
> * document extcon client invocation of charger driver
> * udelay -> usleep_range in register_reset()
>
> Changes in v4:
> * fix definition of bq24190_reg_poc_chg_config_*
> * revert to if (device_property_read... == 0)

The entire series looks good to me and is:

Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH v5 0/4] BQ24190 charger fixes

[Sebastian Reichel]

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

Hi,

On Tue, Apr 11, 2017 at 04:59:53AM -0700, Liam Breck wrote:
> Here in one patchset are two patches I posted recently, and two more:
> 
> Patch 1 is a replacement (as agreed) for
>   "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
> Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
>   original patch, let me know.
> Patch 3 (repost) fixes some issues with PM runtime error handling.
> Patch 4 fixes a nit in register_reset().
> 
> Changed in v5:
> * move condition in extcon OTG boost code
> * document extcon client invocation of charger driver
> * udelay -> usleep_range in register_reset()
> 
> Changes in v4:
> * fix definition of bq24190_reg_poc_chg_config_*
> * revert to if (device_property_read... == 0)

Thanks, queued.

-- Sebastian

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

Re: [PATCH v5 0/4] BQ24190 charger fixes

[Liam Breck]

Hi Sebastian,

On Wed, Apr 12, 2017 at 7:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Apr 11, 2017 at 04:59:53AM -0700, Liam Breck wrote:
>> Here in one patchset are two patches I posted recently, and two more:
>>
>> Patch 1 is a replacement (as agreed) for
>>   "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
>> Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
>>   original patch, let me know.
>> Patch 3 (repost) fixes some issues with PM runtime error handling.
>> Patch 4 fixes a nit in register_reset().
>>
>> Changed in v5:
>> * move condition in extcon OTG boost code
>> * document extcon client invocation of charger driver
>> * udelay -> usleep_range in register_reset()
>>
>> Changes in v4:
>> * fix definition of bq24190_reg_poc_chg_config_*
>> * revert to if (device_property_read... == 0)
>
> Thanks, queued.

Two of these (below) referenced another patch with
  Fixes: SHA ("...")
but the SHAs are wrong because the ref'd patches aren't in mainline.
Should they be fixed?

power: supply: bq24190_charger: Clean up extcon code
power: supply: bq24190_charger: Uniform pm_runtime_get() failure handling

Re: [PATCH v5 0/4] BQ24190 charger fixes

[Liam Breck]

Hi Sebastian,

On Wed, Apr 12, 2017 at 7:52 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Tue, Apr 11, 2017 at 04:59:53AM -0700, Liam Breck wrote:
>> Here in one patchset are two patches I posted recently, and two more:
>>
>> Patch 1 is a replacement (as agreed) for
>>   "power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug"
>> Patch 2 fixes up Hans' extcon patch. If you would like me to squash it with his
>>   original patch, let me know.
>> Patch 3 (repost) fixes some issues with PM runtime error handling.
>> Patch 4 fixes a nit in register_reset().
>>
>> Changed in v5:
>> * move condition in extcon OTG boost code
>> * document extcon client invocation of charger driver
>> * udelay -> usleep_range in register_reset()
>>
>> Changes in v4:
>> * fix definition of bq24190_reg_poc_chg_config_*
>> * revert to if (device_property_read... == 0)
>
> Thanks, queued.

Two of these (below) referenced another patch with
  Fixes: SHA ("...")
but the SHAs are wrong because the ref'd patches weren't in mainline
when I pasted them. Should they be fixed?

power: supply: bq24190_charger: Clean up extcon code
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=03add17fe338af82a28b41bda6afeb3b25c0a323
  Fixes should ref: da26580f4eb8

power: supply: bq24190_charger: Uniform pm_runtime_get() failure handling
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e3ebc381a966f7fb41b13c2823ff96f9d6d7a377
  Fixes should ref: f385e6e2a153