[PATCH 0/7] power: supply: bq24190_charger: Various fixes + extcon support

[PATCH 0/7] power: supply: bq24190_charger: Various fixes + extcon support

[Hans de Goede]

Hi Sebastian, Liam, All,

As discussed here is a v2 of my bq24190 charges. I'm not 100% sure
the last patch should go upstream as is, we do want to remove the
battery power_supply but I'm not sure about the timing of doing so.

>From my pov the other patches are ready for merging, but I guess there
may be some remarks and a v3 of this set will be necessary :)

Regards,

Hans

[PATCH v2 1/7] power: supply: bq24190_charger: Use i2c-core irq-mapping code

[Hans de Goede]

The i2c-core already maps of irqs before calling the driver's probe
function and there are no in tree users of
bq24190_platform_data->gpio_int.

Remove the redundant custom irq-mapping code and just use client->irq.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Acked-by: Sebastian Reichel <sre@kernel.org>
---
Changes in v2:
-Completely drop platform_data and include/linux/power/bq24190_charger.h
---
 drivers/power/supply/bq24190_charger.c | 66 ++--------------------------------
 include/linux/power/bq24190_charger.h  | 16 ---------
 2 files changed, 3 insertions(+), 79 deletions(-)
 delete mode 100644 include/linux/power/bq24190_charger.h

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 451f2bc..fa2d2da 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -18,9 +18,6 @@
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 
-#include <linux/power/bq24190_charger.h>
-
-
 #define	BQ24190_MANUFACTURER	"Texas Instruments"
 
 #define BQ24190_REG_ISC		0x00 /* Input Source Control */
@@ -153,8 +150,6 @@ struct bq24190_dev_info {
 	struct power_supply		*battery;
 	char				model_name[I2C_NAME_SIZE];
 	kernel_ulong_t			model;
-	unsigned int			gpio_int;
-	unsigned int			irq;
 	bool				initialized;
 	bool				irq_event;
 	struct mutex			f_reg_lock;
@@ -1310,56 +1305,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 }
 
-#ifdef CONFIG_OF
-static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
-{
-	bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
-	if (bdi->irq <= 0)
-		return -1;
-
-	return 0;
-}
-#else
-static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
-{
-	return -1;
-}
-#endif
-
-static int bq24190_setup_pdata(struct bq24190_dev_info *bdi,
-		struct bq24190_platform_data *pdata)
-{
-	int ret;
-
-	if (!gpio_is_valid(pdata->gpio_int))
-		return -1;
-
-	ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev));
-	if (ret < 0)
-		return -1;
-
-	ret = gpio_direction_input(pdata->gpio_int);
-	if (ret < 0)
-		goto out;
-
-	bdi->irq = gpio_to_irq(pdata->gpio_int);
-	if (!bdi->irq)
-		goto out;
-
-	bdi->gpio_int = pdata->gpio_int;
-	return 0;
-
-out:
-	gpio_free(pdata->gpio_int);
-	return -1;
-}
-
 static int bq24190_probe(struct i2c_client *client,
 		const struct i2c_device_id *id)
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
-	struct bq24190_platform_data *pdata = client->dev.platform_data;
 	struct power_supply_config charger_cfg = {}, battery_cfg = {};
 	struct bq24190_dev_info *bdi;
 	int ret;
@@ -1385,12 +1335,7 @@ static int bq24190_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, bdi);
 
-	if (dev->of_node)
-		ret = bq24190_setup_dt(bdi);
-	else
-		ret = bq24190_setup_pdata(bdi, pdata);
-
-	if (ret) {
+	if (!client->irq) {
 		dev_err(dev, "Can't get irq info\n");
 		return -EINVAL;
 	}
@@ -1436,7 +1381,7 @@ static int bq24190_probe(struct i2c_client *client,
 
 	bdi->initialized = true;
 
-	ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
+	ret = devm_request_threaded_irq(dev, client->irq, NULL,
 			bq24190_irq_handler_thread,
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			"bq24190-charger", bdi);
@@ -1445,7 +1390,7 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out5;
 	}
 
-	enable_irq_wake(bdi->irq);
+	enable_irq_wake(client->irq);
 
 	pm_runtime_mark_last_busy(dev);
 	pm_runtime_put_autosuspend(dev);
@@ -1467,8 +1412,6 @@ static int bq24190_probe(struct i2c_client *client,
 out1:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
-	if (bdi->gpio_int)
-		gpio_free(bdi->gpio_int);
 	return ret;
 }
 
@@ -1492,9 +1435,6 @@ static int bq24190_remove(struct i2c_client *client)
 	pm_runtime_dont_use_autosuspend(bdi->dev);
 	pm_runtime_disable(bdi->dev);
 
-	if (bdi->gpio_int)
-		gpio_free(bdi->gpio_int);
-
 	return 0;
 }
 
diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
deleted file mode 100644
index 9f02837..0000000
--- a/include/linux/power/bq24190_charger.h
+++ /dev/null
@@ -1,16 +0,0 @@
-/*
- * Platform data for the TI bq24190 battery charger driver.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#ifndef _BQ24190_CHARGER_H_
-#define _BQ24190_CHARGER_H_
-
-struct bq24190_platform_data {
-	unsigned int	gpio_int;	/* GPIO pin that's connected to INT# */
-};
-
-#endif
-- 
2.9.3

Re: [PATCH v2 1/7] power: supply: bq24190_charger: Use i2c-core irq-mapping code

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> The i2c-core already maps of irqs before calling the driver's probe
> function and there are no in tree users of
> bq24190_platform_data->gpio_int.
> 
> Remove the redundant custom irq-mapping code and just use client->irq.

Looks right to me:

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

Re: [PATCH v2 1/7] power: supply: bq24190_charger: Use i2c-core irq-mapping code

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 03:55:30PM +0100, Hans de Goede wrote:
> The i2c-core already maps of irqs before calling the driver's probe
> function and there are no in tree users of
> bq24190_platform_data->gpio_int.
> 
> Remove the redundant custom irq-mapping code and just use client->irq.
> 
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Sebastian Reichel <sre@kernel.org>
> ---
> Changes in v2:
> -Completely drop platform_data and include/linux/power/bq24190_charger.h
> ---

Thanks, queued.

-- Sebastian

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

Re: [PATCH v2 1/7] power: supply: bq24190_charger: Use i2c-core irq-mapping code

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> The i2c-core already maps of irqs before calling the driver's probe
> function and there are no in tree users of
> bq24190_platform_data->gpio_int.
>
> Remove the redundant custom irq-mapping code and just use client->irq.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Acked-by: Sebastian Reichel <sre@kernel.org>

Acked-by: Liam Breck <kernel@networkimprov.net>

> ---
> Changes in v2:
> -Completely drop platform_data and include/linux/power/bq24190_charger.h
> ---
>  drivers/power/supply/bq24190_charger.c | 66 ++--------------------------------
>  include/linux/power/bq24190_charger.h  | 16 ---------
>  2 files changed, 3 insertions(+), 79 deletions(-)
>  delete mode 100644 include/linux/power/bq24190_charger.h
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 451f2bc..fa2d2da 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -18,9 +18,6 @@
>  #include <linux/gpio.h>
>  #include <linux/i2c.h>
>
> -#include <linux/power/bq24190_charger.h>
> -
> -
>  #define        BQ24190_MANUFACTURER    "Texas Instruments"
>
>  #define BQ24190_REG_ISC                0x00 /* Input Source Control */
> @@ -153,8 +150,6 @@ struct bq24190_dev_info {
>         struct power_supply             *battery;
>         char                            model_name[I2C_NAME_SIZE];
>         kernel_ulong_t                  model;
> -       unsigned int                    gpio_int;
> -       unsigned int                    irq;
>         bool                            initialized;
>         bool                            irq_event;
>         struct mutex                    f_reg_lock;
> @@ -1310,56 +1305,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>         return bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>  }
>
> -#ifdef CONFIG_OF
> -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> -{
> -       bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0);
> -       if (bdi->irq <= 0)
> -               return -1;
> -
> -       return 0;
> -}
> -#else
> -static int bq24190_setup_dt(struct bq24190_dev_info *bdi)
> -{
> -       return -1;
> -}
> -#endif
> -
> -static int bq24190_setup_pdata(struct bq24190_dev_info *bdi,
> -               struct bq24190_platform_data *pdata)
> -{
> -       int ret;
> -
> -       if (!gpio_is_valid(pdata->gpio_int))
> -               return -1;
> -
> -       ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev));
> -       if (ret < 0)
> -               return -1;
> -
> -       ret = gpio_direction_input(pdata->gpio_int);
> -       if (ret < 0)
> -               goto out;
> -
> -       bdi->irq = gpio_to_irq(pdata->gpio_int);
> -       if (!bdi->irq)
> -               goto out;
> -
> -       bdi->gpio_int = pdata->gpio_int;
> -       return 0;
> -
> -out:
> -       gpio_free(pdata->gpio_int);
> -       return -1;
> -}
> -
>  static int bq24190_probe(struct i2c_client *client,
>                 const struct i2c_device_id *id)
>  {
>         struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>         struct device *dev = &client->dev;
> -       struct bq24190_platform_data *pdata = client->dev.platform_data;
>         struct power_supply_config charger_cfg = {}, battery_cfg = {};
>         struct bq24190_dev_info *bdi;
>         int ret;
> @@ -1385,12 +1335,7 @@ static int bq24190_probe(struct i2c_client *client,
>
>         i2c_set_clientdata(client, bdi);
>
> -       if (dev->of_node)
> -               ret = bq24190_setup_dt(bdi);
> -       else
> -               ret = bq24190_setup_pdata(bdi, pdata);
> -
> -       if (ret) {
> +       if (!client->irq) {
>                 dev_err(dev, "Can't get irq info\n");
>                 return -EINVAL;
>         }
> @@ -1436,7 +1381,7 @@ static int bq24190_probe(struct i2c_client *client,
>
>         bdi->initialized = true;
>
> -       ret = devm_request_threaded_irq(dev, bdi->irq, NULL,
> +       ret = devm_request_threaded_irq(dev, client->irq, NULL,
>                         bq24190_irq_handler_thread,
>                         IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>                         "bq24190-charger", bdi);
> @@ -1445,7 +1390,7 @@ static int bq24190_probe(struct i2c_client *client,
>                 goto out5;
>         }
>
> -       enable_irq_wake(bdi->irq);
> +       enable_irq_wake(client->irq);
>
>         pm_runtime_mark_last_busy(dev);
>         pm_runtime_put_autosuspend(dev);
> @@ -1467,8 +1412,6 @@ static int bq24190_probe(struct i2c_client *client,
>  out1:
>         pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
> -       if (bdi->gpio_int)
> -               gpio_free(bdi->gpio_int);
>         return ret;
>  }
>
> @@ -1492,9 +1435,6 @@ static int bq24190_remove(struct i2c_client *client)
>         pm_runtime_dont_use_autosuspend(bdi->dev);
>         pm_runtime_disable(bdi->dev);
>
> -       if (bdi->gpio_int)
> -               gpio_free(bdi->gpio_int);
> -
>         return 0;
>  }
>
> diff --git a/include/linux/power/bq24190_charger.h b/include/linux/power/bq24190_charger.h
> deleted file mode 100644
> index 9f02837..0000000
> --- a/include/linux/power/bq24190_charger.h
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -/*
> - * Platform data for the TI bq24190 battery charger driver.
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> - */
> -
> -#ifndef _BQ24190_CHARGER_H_
> -#define _BQ24190_CHARGER_H_
> -
> -struct bq24190_platform_data {
> -       unsigned int    gpio_int;       /* GPIO pin that's connected to INT# */
> -};
> -
> -#endif
> --
> 2.9.3
>

[PATCH v2 2/7] power: supply: bq24190_charger: Add support for bq24192i

[Hans de Goede]

The bq24192 and bq24192i are mostly identical to the bq24190, TI even
published a single datasheet for all 3 of them. The difference
between the bq24190 and bq24192[i] is the way charger-type detection
is done, the bq24190 is to be directly connected to the USB a/b lines,
where as the the bq24192[i] has a gpio which should be driven high/low
externally depending on the type of charger connected, from a register
level access pov there is no difference.

The differences between the bq24192 and bq24192i are:
1) Lower default charge rate on the bq24192i
2) Pre-charge-current can be max 640 mA on the bq24192i

On x86/ACPI systems the code which instantiates the i2c client may not
know the exact variant being used, so instead of coding the model-id
in the i2c_id struct and bailing if it does not match, check the reported
model-id matches one of the supported variants.

This commit only adds support for the bq24192i as I don't
have a bq24192 to test with, adding support for the bq24192 should
be as simple as also accepting its model-id in the model-id test.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Improve commit msg
-Drop model-id from bq24190_i2c_ids table and bdi struct
-Only add support for the bq24192i and not for the bq24192
---
 drivers/power/supply/bq24190_charger.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index fa2d2da..d74c8cc 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -149,7 +149,6 @@ struct bq24190_dev_info {
 	struct power_supply		*charger;
 	struct power_supply		*battery;
 	char				model_name[I2C_NAME_SIZE];
-	kernel_ulong_t			model;
 	bool				initialized;
 	bool				irq_event;
 	struct mutex			f_reg_lock;
@@ -1291,8 +1290,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 	if (ret < 0)
 		return ret;
 
-	if (v != bdi->model)
+	if (v != BQ24190_REG_VPRS_PN_24190 &&
+	    v != BQ24190_REG_VPRS_PN_24192I) {
+		dev_err(bdi->dev, "Error unknown model: 0x%02x\n", v);
 		return -ENODEV;
+	}
 
 	ret = bq24190_register_reset(bdi);
 	if (ret < 0)
@@ -1327,7 +1329,6 @@ static int bq24190_probe(struct i2c_client *client,
 
 	bdi->client = client;
 	bdi->dev = dev;
-	bdi->model = id->driver_data;
 	strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
 	mutex_init(&bdi->f_reg_lock);
 	bdi->f_reg = 0;
@@ -1526,13 +1527,9 @@ static const struct dev_pm_ops bq24190_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(bq24190_pm_suspend, bq24190_pm_resume)
 };
 
-/*
- * Only support the bq24190 right now.  The bq24192, bq24192i, and bq24193
- * are similar but not identical so the driver needs to be extended to
- * support them.
- */
 static const struct i2c_device_id bq24190_i2c_ids[] = {
-	{ "bq24190", BQ24190_REG_VPRS_PN_24190 },
+	{ "bq24190" },
+	{ "bq24192i" },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
-- 
2.9.3

Re: [PATCH v2 2/7] power: supply: bq24190_charger: Add support for bq24192i

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 03:55:31PM +0100, Hans de Goede wrote:
> The bq24192 and bq24192i are mostly identical to the bq24190, TI even
> published a single datasheet for all 3 of them. The difference
> between the bq24190 and bq24192[i] is the way charger-type detection
> is done, the bq24190 is to be directly connected to the USB a/b lines,
> where as the the bq24192[i] has a gpio which should be driven high/low
> externally depending on the type of charger connected, from a register
> level access pov there is no difference.
> 
> The differences between the bq24192 and bq24192i are:
> 1) Lower default charge rate on the bq24192i
> 2) Pre-charge-current can be max 640 mA on the bq24192i
> 
> On x86/ACPI systems the code which instantiates the i2c client may not
> know the exact variant being used, so instead of coding the model-id
> in the i2c_id struct and bailing if it does not match, check the reported
> model-id matches one of the supported variants.
> 
> This commit only adds support for the bq24192i as I don't
> have a bq24192 to test with, adding support for the bq24192 should
> be as simple as also accepting its model-id in the model-id test.
> 
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Improve commit msg
> -Drop model-id from bq24190_i2c_ids table and bdi struct
> -Only add support for the bq24192i and not for the bq24192
> ---

Thanks, queued.

-- Sebastian

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

Re: [PATCH v2 2/7] power: supply: bq24190_charger: Add support for bq24192i

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> The bq24192 and bq24192i are mostly identical to the bq24190, TI even
> published a single datasheet for all 3 of them. The difference
> between the bq24190 and bq24192[i] is the way charger-type detection
> is done, the bq24190 is to be directly connected to the USB a/b lines,
> where as the the bq24192[i] has a gpio which should be driven high/low
> externally depending on the type of charger connected, from a register
> level access pov there is no difference.
>
> The differences between the bq24192 and bq24192i are:
> 1) Lower default charge rate on the bq24192i
> 2) Pre-charge-current can be max 640 mA on the bq24192i
>
> On x86/ACPI systems the code which instantiates the i2c client may not
> know the exact variant being used, so instead of coding the model-id
> in the i2c_id struct and bailing if it does not match, check the reported
> model-id matches one of the supported variants.
>
> This commit only adds support for the bq24192i as I don't
> have a bq24192 to test with, adding support for the bq24192 should
> be as simple as also accepting its model-id in the model-id test.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Liam Breck <kernel@networkimprov.net>


> ---
> Changes in v2:
> -Improve commit msg
> -Drop model-id from bq24190_i2c_ids table and bdi struct
> -Only add support for the bq24192i and not for the bq24192
> ---
>  drivers/power/supply/bq24190_charger.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index fa2d2da..d74c8cc 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -149,7 +149,6 @@ struct bq24190_dev_info {
>         struct power_supply             *charger;
>         struct power_supply             *battery;
>         char                            model_name[I2C_NAME_SIZE];
> -       kernel_ulong_t                  model;
>         bool                            initialized;
>         bool                            irq_event;
>         struct mutex                    f_reg_lock;
> @@ -1291,8 +1290,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>         if (ret < 0)
>                 return ret;
>
> -       if (v != bdi->model)
> +       if (v != BQ24190_REG_VPRS_PN_24190 &&
> +           v != BQ24190_REG_VPRS_PN_24192I) {
> +               dev_err(bdi->dev, "Error unknown model: 0x%02x\n", v);
>                 return -ENODEV;
> +       }
>
>         ret = bq24190_register_reset(bdi);
>         if (ret < 0)
> @@ -1327,7 +1329,6 @@ static int bq24190_probe(struct i2c_client *client,
>
>         bdi->client = client;
>         bdi->dev = dev;
> -       bdi->model = id->driver_data;
>         strncpy(bdi->model_name, id->name, I2C_NAME_SIZE);
>         mutex_init(&bdi->f_reg_lock);
>         bdi->f_reg = 0;
> @@ -1526,13 +1527,9 @@ static const struct dev_pm_ops bq24190_pm_ops = {
>         SET_SYSTEM_SLEEP_PM_OPS(bq24190_pm_suspend, bq24190_pm_resume)
>  };
>
> -/*
> - * Only support the bq24190 right now.  The bq24192, bq24192i, and bq24193
> - * are similar but not identical so the driver needs to be extended to
> - * support them.
> - */
>  static const struct i2c_device_id bq24190_i2c_ids[] = {
> -       { "bq24190", BQ24190_REG_VPRS_PN_24190 },
> +       { "bq24190" },
> +       { "bq24192i" },
>         { },
>  };
>  MODULE_DEVICE_TABLE(i2c, bq24190_i2c_ids);
> --
> 2.9.3
>

[PATCH v2 3/7] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Hans de Goede]

Add support for monitoring an extcon device with USB SDP/CDP/DCP and HOST
cables and adjust ilimit and enable/disable the 5V boost converter
accordingly. This is necessary on systems where the PSEL pin is hardwired
high and ILIM needs to be set by software based on the detected charger
type, as well as on systems where the 5V boost converter is used, as
that always needs to be enabled from software.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Sebastian Reichel <sre@kernel.org>
---
Changes in v2:
-Use a device-property for extcon-name instead of platform_data
-Delay 300ms before applying the extcon detected charger-type to iinlim
 (see the added comment for why this is done)
---
 drivers/power/supply/Kconfig           |   1 +
 drivers/power/supply/bq24190_charger.c | 107 +++++++++++++++++++++++++++++++++
 2 files changed, 108 insertions(+)

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f8b6e64..fd93110 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -442,6 +442,7 @@ config CHARGER_BQ2415X
 config CHARGER_BQ24190
 	tristate "TI BQ24190 battery charger driver"
 	depends on I2C
+	depends on EXTCON
 	depends on GPIOLIB || COMPILE_TEST
 	help
 	  Say Y to enable support for the TI BQ24190 battery charger.
diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index d74c8cc..7a2a496 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -11,10 +11,12 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/delay.h>
+#include <linux/extcon.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/power_supply.h>
+#include <linux/workqueue.h>
 #include <linux/gpio.h>
 #include <linux/i2c.h>
 
@@ -36,6 +38,8 @@
 #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_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)
@@ -148,6 +152,9 @@ struct bq24190_dev_info {
 	struct device			*dev;
 	struct power_supply		*charger;
 	struct power_supply		*battery;
+	struct extcon_dev		*extcon;
+	struct notifier_block		extcon_nb;
+	struct delayed_work		extcon_work;
 	char				model_name[I2C_NAME_SIZE];
 	bool				initialized;
 	bool				irq_event;
@@ -164,6 +171,11 @@ struct bq24190_dev_info {
  * number at that index in the array is the real-world value that it
  * represents.
  */
+
+/* REG00[2:0] (IINLIM) in uAh */
+static const int bq24190_iinlim_values[] = {
+	100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
+
 /* REG02[7:2] (ICHG) in uAh */
 static const int bq24190_ccc_ichg_values[] = {
 	 512000,  576000,  640000,  704000,  768000,  832000,  896000,  960000,
@@ -1277,6 +1289,78 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+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;
+
+	ret = pm_runtime_get_sync(bdi->dev);
+	if (ret < 0) {
+		dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
+		return;
+	}
+
+	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;
+	else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
+		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);
+	}
+
+	/*
+	 * 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);
+
+	pm_runtime_mark_last_busy(bdi->dev);
+	pm_runtime_put_autosuspend(bdi->dev);
+}
+
+static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
+				void *param)
+{
+	struct bq24190_dev_info *bdi =
+		container_of(nb, struct bq24190_dev_info, extcon_nb);
+
+	/*
+	 * The Power-Good detection may take up to 220ms, sometimes
+	 * the external charger detection is quicker, and the bq24190 will
+	 * reset to iinlim based on its own charger detection (which is not
+	 * hooked up when using external charger detection) resulting in
+	 * a too low default 500mA iinlim. Delay applying the extcon value
+	 * for 300ms to avoid this.
+	 */
+	queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
+
+	return NOTIFY_OK;
+}
+
 static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 {
 	u8 v;
@@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client,
 	struct device *dev = &client->dev;
 	struct power_supply_config charger_cfg = {}, battery_cfg = {};
 	struct bq24190_dev_info *bdi;
+	const char *name;
 	int ret;
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -1341,6 +1426,16 @@ static int bq24190_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
+	/*
+	 * The extcon-name property is purely an in kernel interface for
+	 * x86/ACPI use, DT platforms should get extcon via phandle.
+	 */
+	if (device_property_read_string(dev, "extcon-name", &name) == 0) {
+		bdi->extcon = extcon_get_extcon_dev(name);
+		if (!bdi->extcon)
+			return -EPROBE_DEFER;
+	}
+
 	pm_runtime_enable(dev);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_autosuspend_delay(dev, 600);
@@ -1391,6 +1486,18 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out5;
 	}
 
+	if (bdi->extcon) {
+		INIT_DELAYED_WORK(&bdi->extcon_work, bq24190_extcon_work);
+		bdi->extcon_nb.notifier_call = bq24190_extcon_event;
+		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
+						    &bdi->extcon_nb);
+		if (ret)
+			goto out5;
+
+		/* Sync initial cable state */
+		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
+	}
+
 	enable_irq_wake(client->irq);
 
 	pm_runtime_mark_last_busy(dev);
-- 
2.9.3

Re: [PATCH v2 3/7] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> Add support for monitoring an extcon device with USB SDP/CDP/DCP and HOST
> cables and adjust ilimit and enable/disable the 5V boost converter
> accordingly. This is necessary on systems where the PSEL pin is hardwired
> high and ILIM needs to be set by software based on the detected charger
> type, as well as on systems where the 5V boost converter is used, as
> that always needs to be enabled from software.

I wonder if these cross device and cross subsystem events really should
be handled at the framework level? Otherwise the permutations get out
of control fast..

Regards,

Tony

Re: [PATCH v2 3/7] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Hans de Goede]

Hi,

On 22-03-17 16:50, Tony Lindgren wrote:
> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>> Add support for monitoring an extcon device with USB SDP/CDP/DCP and HOST
>> cables and adjust ilimit and enable/disable the 5V boost converter
>> accordingly. This is necessary on systems where the PSEL pin is hardwired
>> high and ILIM needs to be set by software based on the detected charger
>> type, as well as on systems where the 5V boost converter is used, as
>> that always needs to be enabled from software.
>
> I wonder if these cross device and cross subsystem events really should
> be handled at the framework level? Otherwise the permutations get out
> of control fast..

There are a lot of hw specific things in here which are hard to generalize,
like not only setting iilim but also enabling the booster when there
is no charger and we are in host mode (the no charger condition is
necessary for USB ACAs).

Regards,

Hans

Re: [PATCH v2 3/7] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Add support for monitoring an extcon device with USB SDP/CDP/DCP and HOST
> cables and adjust ilimit and enable/disable the 5V boost converter
> accordingly. This is necessary on systems where the PSEL pin is hardwired
> high and ILIM needs to be set by software based on the detected charger
> type, as well as on systems where the 5V boost converter is used, as
> that always needs to be enabled from software.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Sebastian Reichel <sre@kernel.org>
> ---
> Changes in v2:
> -Use a device-property for extcon-name instead of platform_data
> -Delay 300ms before applying the extcon detected charger-type to iinlim
>  (see the added comment for why this is done)
> ---
>  drivers/power/supply/Kconfig           |   1 +
>  drivers/power/supply/bq24190_charger.c | 107 +++++++++++++++++++++++++++++++++
>  2 files changed, 108 insertions(+)
>
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f8b6e64..fd93110 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -442,6 +442,7 @@ config CHARGER_BQ2415X
>  config CHARGER_BQ24190
>         tristate "TI BQ24190 battery charger driver"
>         depends on I2C
> +       depends on EXTCON
>         depends on GPIOLIB || COMPILE_TEST
>         help
>           Say Y to enable support for the TI BQ24190 battery charger.
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index d74c8cc..7a2a496 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -11,10 +11,12 @@
>  #include <linux/module.h>
>  #include <linux/interrupt.h>
>  #include <linux/delay.h>
> +#include <linux/extcon.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/power_supply.h>
> +#include <linux/workqueue.h>
>  #include <linux/gpio.h>
>  #include <linux/i2c.h>
>
> @@ -36,6 +38,8 @@
>  #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_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)
> @@ -148,6 +152,9 @@ struct bq24190_dev_info {
>         struct device                   *dev;
>         struct power_supply             *charger;
>         struct power_supply             *battery;
> +       struct extcon_dev               *extcon;
> +       struct notifier_block           extcon_nb;
> +       struct delayed_work             extcon_work;
>         char                            model_name[I2C_NAME_SIZE];
>         bool                            initialized;
>         bool                            irq_event;
> @@ -164,6 +171,11 @@ struct bq24190_dev_info {
>   * number at that index in the array is the real-world value that it
>   * represents.
>   */
> +
> +/* REG00[2:0] (IINLIM) in uAh */
> +static const int bq24190_iinlim_values[] = {
> +       100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
> +
>  /* REG02[7:2] (ICHG) in uAh */
>  static const int bq24190_ccc_ichg_values[] = {
>          512000,  576000,  640000,  704000,  768000,  832000,  896000,  960000,
> @@ -1277,6 +1289,78 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>         return IRQ_HANDLED;
>  }
>
> +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;
> +
> +       ret = pm_runtime_get_sync(bdi->dev);
> +       if (ret < 0) {
> +               dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
> +               return;
> +       }
> +
> +       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;
> +       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
> +               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);
> +       }
> +
> +       /*
> +        * 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);
> +
> +       pm_runtime_mark_last_busy(bdi->dev);
> +       pm_runtime_put_autosuspend(bdi->dev);
> +}
> +
> +static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
> +                               void *param)
> +{
> +       struct bq24190_dev_info *bdi =
> +               container_of(nb, struct bq24190_dev_info, extcon_nb);
> +
> +       /*
> +        * The Power-Good detection may take up to 220ms, sometimes
> +        * the external charger detection is quicker, and the bq24190 will
> +        * reset to iinlim based on its own charger detection (which is not
> +        * hooked up when using external charger detection) resulting in
> +        * a too low default 500mA iinlim. Delay applying the extcon value
> +        * for 300ms to avoid this.
> +        */
> +       queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
> +
> +       return NOTIFY_OK;
> +}
> +
>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  {
>         u8 v;
> @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client,
>         struct device *dev = &client->dev;
>         struct power_supply_config charger_cfg = {}, battery_cfg = {};
>         struct bq24190_dev_info *bdi;
> +       const char *name;
>         int ret;
>
>         if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
> @@ -1341,6 +1426,16 @@ static int bq24190_probe(struct i2c_client *client,
>                 return -EINVAL;
>         }
>
> +       /*
> +        * The extcon-name property is purely an in kernel interface for
> +        * x86/ACPI use, DT platforms should get extcon via phandle.
> +        */
> +       if (device_property_read_string(dev, "extcon-name", &name) == 0) {
> +               bdi->extcon = extcon_get_extcon_dev(name);
> +               if (!bdi->extcon)
> +                       return -EPROBE_DEFER;

needs
                dev_info(bdi->dev, "using extcon device %s\n", name);

I didn't see the device property for preferred charge voltage limit
from v1 patchset?

> +       }
> +
>         pm_runtime_enable(dev);
>         pm_runtime_use_autosuspend(dev);
>         pm_runtime_set_autosuspend_delay(dev, 600);
> @@ -1391,6 +1486,18 @@ static int bq24190_probe(struct i2c_client *client,
>                 goto out5;
>         }
>
> +       if (bdi->extcon) {
> +               INIT_DELAYED_WORK(&bdi->extcon_work, bq24190_extcon_work);
> +               bdi->extcon_nb.notifier_call = bq24190_extcon_event;
> +               ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
> +                                                   &bdi->extcon_nb);
> +               if (ret)
> +                       goto out5;
> +
> +               /* Sync initial cable state */
> +               queue_delayed_work(system_wq, &bdi->extcon_work, 0);
> +       }
> +
>         enable_irq_wake(client->irq);
>
>         pm_runtime_mark_last_busy(dev);
> --
> 2.9.3
>

Re: [PATCH v2 3/7] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost

[Hans de Goede]

Hi,

On 22-03-17 19:50, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Add support for monitoring an extcon device with USB SDP/CDP/DCP and HOST
>> cables and adjust ilimit and enable/disable the 5V boost converter
>> accordingly. This is necessary on systems where the PSEL pin is hardwired
>> high and ILIM needs to be set by software based on the detected charger
>> type, as well as on systems where the 5V boost converter is used, as
>> that always needs to be enabled from software.
>>
>> Cc: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> Acked-by: Sebastian Reichel <sre@kernel.org>
>> ---
>> Changes in v2:
>> -Use a device-property for extcon-name instead of platform_data
>> -Delay 300ms before applying the extcon detected charger-type to iinlim
>>  (see the added comment for why this is done)
>> ---
>>  drivers/power/supply/Kconfig           |   1 +
>>  drivers/power/supply/bq24190_charger.c | 107 +++++++++++++++++++++++++++++++++
>>  2 files changed, 108 insertions(+)
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index f8b6e64..fd93110 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -442,6 +442,7 @@ config CHARGER_BQ2415X
>>  config CHARGER_BQ24190
>>         tristate "TI BQ24190 battery charger driver"
>>         depends on I2C
>> +       depends on EXTCON
>>         depends on GPIOLIB || COMPILE_TEST
>>         help
>>           Say Y to enable support for the TI BQ24190 battery charger.
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index d74c8cc..7a2a496 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -11,10 +11,12 @@
>>  #include <linux/module.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/delay.h>
>> +#include <linux/extcon.h>
>>  #include <linux/of_irq.h>
>>  #include <linux/of_device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/power_supply.h>
>> +#include <linux/workqueue.h>
>>  #include <linux/gpio.h>
>>  #include <linux/i2c.h>
>>
>> @@ -36,6 +38,8 @@
>>  #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_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)
>> @@ -148,6 +152,9 @@ struct bq24190_dev_info {
>>         struct device                   *dev;
>>         struct power_supply             *charger;
>>         struct power_supply             *battery;
>> +       struct extcon_dev               *extcon;
>> +       struct notifier_block           extcon_nb;
>> +       struct delayed_work             extcon_work;
>>         char                            model_name[I2C_NAME_SIZE];
>>         bool                            initialized;
>>         bool                            irq_event;
>> @@ -164,6 +171,11 @@ struct bq24190_dev_info {
>>   * number at that index in the array is the real-world value that it
>>   * represents.
>>   */
>> +
>> +/* REG00[2:0] (IINLIM) in uAh */
>> +static const int bq24190_iinlim_values[] = {
>> +       100000, 150000, 500000, 900000, 1200000, 1500000, 2000000, 3000000 };
>> +
>>  /* REG02[7:2] (ICHG) in uAh */
>>  static const int bq24190_ccc_ichg_values[] = {
>>          512000,  576000,  640000,  704000,  768000,  832000,  896000,  960000,
>> @@ -1277,6 +1289,78 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, void *data)
>>         return IRQ_HANDLED;
>>  }
>>
>> +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;
>> +
>> +       ret = pm_runtime_get_sync(bdi->dev);
>> +       if (ret < 0) {
>> +               dev_err(bdi->dev, "Error getting runtime-pm ref: %d\n", ret);
>> +               return;
>> +       }
>> +
>> +       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;
>> +       else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1)
>> +               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);
>> +       }
>> +
>> +       /*
>> +        * 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);
>> +
>> +       pm_runtime_mark_last_busy(bdi->dev);
>> +       pm_runtime_put_autosuspend(bdi->dev);
>> +}
>> +
>> +static int bq24190_extcon_event(struct notifier_block *nb, unsigned long event,
>> +                               void *param)
>> +{
>> +       struct bq24190_dev_info *bdi =
>> +               container_of(nb, struct bq24190_dev_info, extcon_nb);
>> +
>> +       /*
>> +        * The Power-Good detection may take up to 220ms, sometimes
>> +        * the external charger detection is quicker, and the bq24190 will
>> +        * reset to iinlim based on its own charger detection (which is not
>> +        * hooked up when using external charger detection) resulting in
>> +        * a too low default 500mA iinlim. Delay applying the extcon value
>> +        * for 300ms to avoid this.
>> +        */
>> +       queue_delayed_work(system_wq, &bdi->extcon_work, msecs_to_jiffies(300));
>> +
>> +       return NOTIFY_OK;
>> +}
>> +
>>  static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>>  {
>>         u8 v;
>> @@ -1314,6 +1398,7 @@ static int bq24190_probe(struct i2c_client *client,
>>         struct device *dev = &client->dev;
>>         struct power_supply_config charger_cfg = {}, battery_cfg = {};
>>         struct bq24190_dev_info *bdi;
>> +       const char *name;
>>         int ret;
>>
>>         if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA)) {
>> @@ -1341,6 +1426,16 @@ static int bq24190_probe(struct i2c_client *client,
>>                 return -EINVAL;
>>         }
>>
>> +       /*
>> +        * The extcon-name property is purely an in kernel interface for
>> +        * x86/ACPI use, DT platforms should get extcon via phandle.
>> +        */
>> +       if (device_property_read_string(dev, "extcon-name", &name) == 0) {
>> +               bdi->extcon = extcon_get_extcon_dev(name);
>> +               if (!bdi->extcon)
>> +                       return -EPROBE_DEFER;
>
> needs
>                 dev_info(bdi->dev, "using extcon device %s\n", name);

Right, I forgot about that v3 coming up.

> I didn't see the device property for preferred charge voltage limit
> from v1 patchset?

I decided to drop that patch, the limit set by the firmware is within
the limits of the LiHV battery it just is not an ideal limit from a
number of charge-cycles the battery will endure pov, but second
guessing the firmware here / doing our own tweaks is in hindsight
a bad idea.

Regards,

Hans




>
>> +       }
>> +
>>         pm_runtime_enable(dev);
>>         pm_runtime_use_autosuspend(dev);
>>         pm_runtime_set_autosuspend_delay(dev, 600);
>> @@ -1391,6 +1486,18 @@ static int bq24190_probe(struct i2c_client *client,
>>                 goto out5;
>>         }
>>
>> +       if (bdi->extcon) {
>> +               INIT_DELAYED_WORK(&bdi->extcon_work, bq24190_extcon_work);
>> +               bdi->extcon_nb.notifier_call = bq24190_extcon_event;
>> +               ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>> +                                                   &bdi->extcon_nb);
>> +               if (ret)
>> +                       goto out5;
>> +
>> +               /* Sync initial cable state */
>> +               queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>> +       }
>> +
>>         enable_irq_wake(client->irq);
>>
>>         pm_runtime_mark_last_busy(dev);
>> --
>> 2.9.3
>>

[PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Resetting the charger should never be necessary it should always have
sane values programmed. If it is running with invalid values while we
are not running (system turned off or suspended) there is a big problem
as that may lead to overcharging the battery.

The reset in suspend() is meant to put the charger back into default
mode, but this is not necessary and not a good idea. If the charger has
been programmed with a higher max charge_current / charge_voltage then
putting it back in default-mode will reset those to the safe power-on
defaults, leading to slower charging, or charging to a lower voltage
(and thus not using the full capacity) while suspended which is
undesirable. Reprogramming the max charge_current / charge_voltage
after the reset will not help here as that will put the charger back
in host mode and start the i2c watchdog if the host then does not do
anything for 40s (iow if we're suspended for more then 40s) the watchdog
expires resetting the device to default-mode, including resetting all
the registers to there safe power-on defaults. So the only way to keep
using custom charge settings while suspending is to keep the charger in
its normal running state with the i2c watchdog disabled. This is fine
as the charger will still automatically switch from constant current
to constant voltage and stop charging when the battery is full.

Besides never being necessary resetting the charger also causes problems
on systems where the charge voltage limit is set higher then the reset
value, if this is the case and the charger is reset while charging and
the battery voltage is between the 2 voltages, then about half the time
the charger gets confused and claims to be charging (REG08 contains 0x64)
but in reality the charger has decoupled itself from VBUS (Q1 off) and
is drawing 0A from VBUS, leaving the system running from the battery.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/power/supply/bq24190_charger.c | 58 ----------------------------------
 1 file changed, 58 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 7a2a496..b535f24 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -526,40 +526,6 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
 	return bq24190_write(bdi, BQ24190_REG_CTTC, v);
 }
 
-static int bq24190_register_reset(struct bq24190_dev_info *bdi)
-{
-	int ret, limit = 100;
-	u8 v;
-
-	/* Reset the registers */
-	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
-			BQ24190_REG_POC_RESET_MASK,
-			BQ24190_REG_POC_RESET_SHIFT,
-			0x1);
-	if (ret < 0)
-		return ret;
-
-	/* Reset bit will be cleared by hardware so poll until it is */
-	do {
-		ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
-				BQ24190_REG_POC_RESET_MASK,
-				BQ24190_REG_POC_RESET_SHIFT,
-				&v);
-		if (ret < 0)
-			return ret;
-
-		if (!v)
-			break;
-
-		udelay(10);
-	} while (--limit);
-
-	if (!limit)
-		return -EIO;
-
-	return 0;
-}
-
 /* Charger power supply property routines */
 
 static int bq24190_charger_get_charge_type(struct bq24190_dev_info *bdi,
@@ -1380,10 +1346,6 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 		return -ENODEV;
 	}
 
-	ret = bq24190_register_reset(bdi);
-	if (ret < 0)
-		return ret;
-
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
 		return ret;
@@ -1534,7 +1496,6 @@ static int bq24190_remove(struct i2c_client *client)
 		pm_runtime_put_noidle(bdi->dev);
 	}
 
-	bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
@@ -1577,23 +1538,6 @@ static __maybe_unused int bq24190_runtime_resume(struct device *dev)
 
 static __maybe_unused 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;
-
-	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);
-	}
-
-	bq24190_register_reset(bdi);
-
-	if (error >= 0) {
-		pm_runtime_mark_last_busy(bdi->dev);
-		pm_runtime_put_autosuspend(bdi->dev);
-	}
-
 	return 0;
 }
 
@@ -1612,8 +1556,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
 		pm_runtime_put_noidle(bdi->dev);
 	}
 
-	bq24190_register_reset(bdi);
-	bq24190_set_mode_host(bdi);
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 
 	if (error >= 0) {
-- 
2.9.3

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> Resetting the charger should never be necessary it should always have
> sane values programmed. If it is running with invalid values while we
> are not running (system turned off or suspended) there is a big problem
> as that may lead to overcharging the battery.

And some systems may only configure it in the bootloader with no
configuration passed to the kernel at all.

> The reset in suspend() is meant to put the charger back into default
> mode, but this is not necessary and not a good idea. If the charger has
> been programmed with a higher max charge_current / charge_voltage then
> putting it back in default-mode will reset those to the safe power-on
> defaults, leading to slower charging, or charging to a lower voltage
> (and thus not using the full capacity) while suspended which is
> undesirable. Reprogramming the max charge_current / charge_voltage
> after the reset will not help here as that will put the charger back
> in host mode and start the i2c watchdog if the host then does not do
> anything for 40s (iow if we're suspended for more then 40s) the watchdog
> expires resetting the device to default-mode, including resetting all
> the registers to there safe power-on defaults. So the only way to keep
> using custom charge settings while suspending is to keep the charger in
> its normal running state with the i2c watchdog disabled. This is fine
> as the charger will still automatically switch from constant current
> to constant voltage and stop charging when the battery is full.
> 
> Besides never being necessary resetting the charger also causes problems
> on systems where the charge voltage limit is set higher then the reset
> value, if this is the case and the charger is reset while charging and
> the battery voltage is between the 2 voltages, then about half the time
> the charger gets confused and claims to be charging (REG08 contains 0x64)
> but in reality the charger has decoupled itself from VBUS (Q1 off) and
> is drawing 0A from VBUS, leaving the system running from the battery.

We do have cases where Linux kernel is the bootloader though using
kexec. And in those cases we may need to reset, so I wonder if the
reset should just be optional based on a proper device tree
(or platform_data) configuration?

Regards,

Tony

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 22-03-17 16:43, Tony Lindgren wrote:
> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>> Resetting the charger should never be necessary it should always have
>> sane values programmed. If it is running with invalid values while we
>> are not running (system turned off or suspended) there is a big problem
>> as that may lead to overcharging the battery.
>
> And some systems may only configure it in the bootloader with no
> configuration passed to the kernel at all.

Right.

>> The reset in suspend() is meant to put the charger back into default
>> mode, but this is not necessary and not a good idea. If the charger has
>> been programmed with a higher max charge_current / charge_voltage then
>> putting it back in default-mode will reset those to the safe power-on
>> defaults, leading to slower charging, or charging to a lower voltage
>> (and thus not using the full capacity) while suspended which is
>> undesirable. Reprogramming the max charge_current / charge_voltage
>> after the reset will not help here as that will put the charger back
>> in host mode and start the i2c watchdog if the host then does not do
>> anything for 40s (iow if we're suspended for more then 40s) the watchdog
>> expires resetting the device to default-mode, including resetting all
>> the registers to there safe power-on defaults. So the only way to keep
>> using custom charge settings while suspending is to keep the charger in
>> its normal running state with the i2c watchdog disabled. This is fine
>> as the charger will still automatically switch from constant current
>> to constant voltage and stop charging when the battery is full.
>>
>> Besides never being necessary resetting the charger also causes problems
>> on systems where the charge voltage limit is set higher then the reset
>> value, if this is the case and the charger is reset while charging and
>> the battery voltage is between the 2 voltages, then about half the time
>> the charger gets confused and claims to be charging (REG08 contains 0x64)
>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>> is drawing 0A from VBUS, leaving the system running from the battery.
>
> We do have cases where Linux kernel is the bootloader though using
> kexec. And in those cases we may need to reset, so I wonder if the
> reset should just be optional based on a proper device tree
> (or platform_data) configuration?

As described above during my testing I've found that the chip does not
respond well to reset under certain conditions, so I think that if
we have settings to apply we should just overwrite the settings with
our settings, what does doing a reset before overwriting the registers
buy us? We still need to do the overwrite anyways.

Regards,

Hans

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 08:47]:
> Hi,
> 
> On 22-03-17 16:43, Tony Lindgren wrote:
> > * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> > > Resetting the charger should never be necessary it should always have
> > > sane values programmed. If it is running with invalid values while we
> > > are not running (system turned off or suspended) there is a big problem
> > > as that may lead to overcharging the battery.
> > 
> > And some systems may only configure it in the bootloader with no
> > configuration passed to the kernel at all.
> 
> Right.
> 
> > > The reset in suspend() is meant to put the charger back into default
> > > mode, but this is not necessary and not a good idea. If the charger has
> > > been programmed with a higher max charge_current / charge_voltage then
> > > putting it back in default-mode will reset those to the safe power-on
> > > defaults, leading to slower charging, or charging to a lower voltage
> > > (and thus not using the full capacity) while suspended which is
> > > undesirable. Reprogramming the max charge_current / charge_voltage
> > > after the reset will not help here as that will put the charger back
> > > in host mode and start the i2c watchdog if the host then does not do
> > > anything for 40s (iow if we're suspended for more then 40s) the watchdog
> > > expires resetting the device to default-mode, including resetting all
> > > the registers to there safe power-on defaults. So the only way to keep
> > > using custom charge settings while suspending is to keep the charger in
> > > its normal running state with the i2c watchdog disabled. This is fine
> > > as the charger will still automatically switch from constant current
> > > to constant voltage and stop charging when the battery is full.
> > > 
> > > Besides never being necessary resetting the charger also causes problems
> > > on systems where the charge voltage limit is set higher then the reset
> > > value, if this is the case and the charger is reset while charging and
> > > the battery voltage is between the 2 voltages, then about half the time
> > > the charger gets confused and claims to be charging (REG08 contains 0x64)
> > > but in reality the charger has decoupled itself from VBUS (Q1 off) and
> > > is drawing 0A from VBUS, leaving the system running from the battery.
> > 
> > We do have cases where Linux kernel is the bootloader though using
> > kexec. And in those cases we may need to reset, so I wonder if the
> > reset should just be optional based on a proper device tree
> > (or platform_data) configuration?
> 
> As described above during my testing I've found that the chip does not
> respond well to reset under certain conditions, so I think that if
> we have settings to apply we should just overwrite the settings with
> our settings, what does doing a reset before overwriting the registers
> buy us? We still need to do the overwrite anyways.

OK with me if that works and reset is buggy.

Regards,

Tony

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 22-03-17 16:43, Tony Lindgren wrote:
>>
>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>
>>> Resetting the charger should never be necessary it should always have
>>> sane values programmed. If it is running with invalid values while we
>>> are not running (system turned off or suspended) there is a big problem
>>> as that may lead to overcharging the battery.
>>
>>
>> And some systems may only configure it in the bootloader with no
>> configuration passed to the kernel at all.
>
>
> Right.
>
>
>>> The reset in suspend() is meant to put the charger back into default
>>> mode, but this is not necessary and not a good idea. If the charger has
>>> been programmed with a higher max charge_current / charge_voltage then
>>> putting it back in default-mode will reset those to the safe power-on
>>> defaults, leading to slower charging, or charging to a lower voltage
>>> (and thus not using the full capacity) while suspended which is
>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>> after the reset will not help here as that will put the charger back
>>> in host mode and start the i2c watchdog if the host then does not do
>>> anything for 40s (iow if we're suspended for more then 40s) the watchdog
>>> expires resetting the device to default-mode, including resetting all
>>> the registers to there safe power-on defaults. So the only way to keep
>>> using custom charge settings while suspending is to keep the charger in
>>> its normal running state with the i2c watchdog disabled. This is fine
>>> as the charger will still automatically switch from constant current
>>> to constant voltage and stop charging when the battery is full.
>>>
>>> Besides never being necessary resetting the charger also causes problems
>>> on systems where the charge voltage limit is set higher then the reset
>>> value, if this is the case and the charger is reset while charging and
>>> the battery voltage is between the 2 voltages, then about half the time
>>> the charger gets confused and claims to be charging (REG08 contains 0x64)
>>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>
>>
>> We do have cases where Linux kernel is the bootloader though using
>> kexec. And in those cases we may need to reset, so I wonder if the
>> reset should just be optional based on a proper device tree
>> (or platform_data) configuration?
>
>
> As described above during my testing I've found that the chip does not
> respond well to reset under certain conditions, so I think that if
> we have settings to apply we should just overwrite the settings with
> our settings, what does doing a reset before overwriting the registers
> buy us? We still need to do the overwrite anyways.

When driver loads on any conceivable hw, how do we know the chip is in
a state we predicted? If it isn't, how can we fix any undesirable
settings? Reset + fix a,b,c is simple way to do that.

I'm OK with abandoning reset on suspend. Tho I need to review the docs
about host mode...

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 23-03-17 08:16, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>
>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>
>>>> Resetting the charger should never be necessary it should always have
>>>> sane values programmed. If it is running with invalid values while we
>>>> are not running (system turned off or suspended) there is a big problem
>>>> as that may lead to overcharging the battery.
>>>
>>>
>>> And some systems may only configure it in the bootloader with no
>>> configuration passed to the kernel at all.
>>
>>
>> Right.
>>
>>
>>>> The reset in suspend() is meant to put the charger back into default
>>>> mode, but this is not necessary and not a good idea. If the charger has
>>>> been programmed with a higher max charge_current / charge_voltage then
>>>> putting it back in default-mode will reset those to the safe power-on
>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>> (and thus not using the full capacity) while suspended which is
>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>> after the reset will not help here as that will put the charger back
>>>> in host mode and start the i2c watchdog if the host then does not do
>>>> anything for 40s (iow if we're suspended for more then 40s) the watchdog
>>>> expires resetting the device to default-mode, including resetting all
>>>> the registers to there safe power-on defaults. So the only way to keep
>>>> using custom charge settings while suspending is to keep the charger in
>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>> as the charger will still automatically switch from constant current
>>>> to constant voltage and stop charging when the battery is full.
>>>>
>>>> Besides never being necessary resetting the charger also causes problems
>>>> on systems where the charge voltage limit is set higher then the reset
>>>> value, if this is the case and the charger is reset while charging and
>>>> the battery voltage is between the 2 voltages, then about half the time
>>>> the charger gets confused and claims to be charging (REG08 contains 0x64)
>>>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>
>>>
>>> We do have cases where Linux kernel is the bootloader though using
>>> kexec. And in those cases we may need to reset, so I wonder if the
>>> reset should just be optional based on a proper device tree
>>> (or platform_data) configuration?
>>
>>
>> As described above during my testing I've found that the chip does not
>> respond well to reset under certain conditions, so I think that if
>> we have settings to apply we should just overwrite the settings with
>> our settings, what does doing a reset before overwriting the registers
>> buy us? We still need to do the overwrite anyways.
>
> When driver loads on any conceivable hw, how do we know the chip is in
> a state we predicted?

Any non reset values have to come from somewhere, likely firmware,
and unless we've platform data telling us the exact settings we should
use why would we decide the reset values are better then the ones
the charger was *already using* before our driver loads ? As said in
the commit msg: "If it is running with invalid values while we
are not running (system turned off or suspended) there is a big problem
as that may lead to overcharging the battery."

> If it isn't, how can we fix any undesirable
> settings?

Why would the chip not be in a sane state? Where would the undesirable
settings come from ?

> Reset + fix a,b,c is simple way to do that.

As my testing using real-world hardware under real-world conditions
has shown resetting the charger IC while it is charging can cause it
to misbehave. So we have this theoretical problem of there somehow
being undesirable settings at boot vs a real-world problem...

Regards,

Hans

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 23-03-17 08:16, Liam Breck wrote:
>>
>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>
>>>>
>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>
>>>>>
>>>>> Resetting the charger should never be necessary it should always have
>>>>> sane values programmed. If it is running with invalid values while we
>>>>> are not running (system turned off or suspended) there is a big problem
>>>>> as that may lead to overcharging the battery.
>>>>
>>>>
>>>>
>>>> And some systems may only configure it in the bootloader with no
>>>> configuration passed to the kernel at all.
>>>
>>>
>>>
>>> Right.
>>>
>>>
>>>>> The reset in suspend() is meant to put the charger back into default
>>>>> mode, but this is not necessary and not a good idea. If the charger has
>>>>> been programmed with a higher max charge_current / charge_voltage then
>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>> (and thus not using the full capacity) while suspended which is
>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>> after the reset will not help here as that will put the charger back
>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>> watchdog
>>>>> expires resetting the device to default-mode, including resetting all
>>>>> the registers to there safe power-on defaults. So the only way to keep
>>>>> using custom charge settings while suspending is to keep the charger in
>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>> as the charger will still automatically switch from constant current
>>>>> to constant voltage and stop charging when the battery is full.
>>>>>
>>>>> Besides never being necessary resetting the charger also causes
>>>>> problems
>>>>> on systems where the charge voltage limit is set higher then the reset
>>>>> value, if this is the case and the charger is reset while charging and
>>>>> the battery voltage is between the 2 voltages, then about half the time
>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>> 0x64)
>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>
>>>>
>>>>
>>>> We do have cases where Linux kernel is the bootloader though using
>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>> reset should just be optional based on a proper device tree
>>>> (or platform_data) configuration?
>>>
>>>
>>>
>>> As described above during my testing I've found that the chip does not
>>> respond well to reset under certain conditions, so I think that if
>>> we have settings to apply we should just overwrite the settings with
>>> our settings, what does doing a reset before overwriting the registers
>>> buy us? We still need to do the overwrite anyways.
>>
>>
>> When driver loads on any conceivable hw, how do we know the chip is in
>> a state we predicted?
>
>
> Any non reset values have to come from somewhere, likely firmware,
> and unless we've platform data telling us the exact settings we should
> use why would we decide the reset values are better then the ones
> the charger was *already using* before our driver loads ? As said in
> the commit msg: "If it is running with invalid values while we
> are not running (system turned off or suspended) there is a big problem
> as that may lead to overcharging the battery."
>
>> If it isn't, how can we fix any undesirable
>> settings?
>
>
> Why would the chip not be in a sane state? Where would the undesirable
> settings come from ?
>
>> Reset + fix a,b,c is simple way to do that.
>
>
> As my testing using real-world hardware under real-world conditions
> has shown resetting the charger IC while it is charging can cause it
> to misbehave. So we have this theoretical problem of there somehow
> being undesirable settings at boot vs a real-world problem...

A bug in the charger? Or in the platform hw/fw?

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 23-03-17 12:36, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 23-03-17 08:16, Liam Breck wrote:
>>>
>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>
>>>>>
>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>
>>>>>>
>>>>>> Resetting the charger should never be necessary it should always have
>>>>>> sane values programmed. If it is running with invalid values while we
>>>>>> are not running (system turned off or suspended) there is a big problem
>>>>>> as that may lead to overcharging the battery.
>>>>>
>>>>>
>>>>>
>>>>> And some systems may only configure it in the bootloader with no
>>>>> configuration passed to the kernel at all.
>>>>
>>>>
>>>>
>>>> Right.
>>>>
>>>>
>>>>>> The reset in suspend() is meant to put the charger back into default
>>>>>> mode, but this is not necessary and not a good idea. If the charger has
>>>>>> been programmed with a higher max charge_current / charge_voltage then
>>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>> after the reset will not help here as that will put the charger back
>>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>> watchdog
>>>>>> expires resetting the device to default-mode, including resetting all
>>>>>> the registers to there safe power-on defaults. So the only way to keep
>>>>>> using custom charge settings while suspending is to keep the charger in
>>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>>> as the charger will still automatically switch from constant current
>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>
>>>>>> Besides never being necessary resetting the charger also causes
>>>>>> problems
>>>>>> on systems where the charge voltage limit is set higher then the reset
>>>>>> value, if this is the case and the charger is reset while charging and
>>>>>> the battery voltage is between the 2 voltages, then about half the time
>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>> 0x64)
>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>>
>>>>>
>>>>>
>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>> reset should just be optional based on a proper device tree
>>>>> (or platform_data) configuration?
>>>>
>>>>
>>>>
>>>> As described above during my testing I've found that the chip does not
>>>> respond well to reset under certain conditions, so I think that if
>>>> we have settings to apply we should just overwrite the settings with
>>>> our settings, what does doing a reset before overwriting the registers
>>>> buy us? We still need to do the overwrite anyways.
>>>
>>>
>>> When driver loads on any conceivable hw, how do we know the chip is in
>>> a state we predicted?
>>
>>
>> Any non reset values have to come from somewhere, likely firmware,
>> and unless we've platform data telling us the exact settings we should
>> use why would we decide the reset values are better then the ones
>> the charger was *already using* before our driver loads ? As said in
>> the commit msg: "If it is running with invalid values while we
>> are not running (system turned off or suspended) there is a big problem
>> as that may lead to overcharging the battery."
>>
>>> If it isn't, how can we fix any undesirable
>>> settings?
>>
>>
>> Why would the chip not be in a sane state? Where would the undesirable
>> settings come from ?
>>
>>> Reset + fix a,b,c is simple way to do that.
>>
>>
>> As my testing using real-world hardware under real-world conditions
>> has shown resetting the charger IC while it is charging can cause it
>> to misbehave. So we have this theoretical problem of there somehow
>> being undesirable settings at boot vs a real-world problem...
>
> A bug in the charger? Or in the platform hw/fw?

I'm pretty sure the fw is not touching the charger, still the problem
might be something specific to my hw. Either way there does not seem
to be a good reason to reset the chip and doing a reset despite it
not being necessary does cause real problems independently of the
root cause (I'm running this on production hardware which many
people have, so no way to modify the hardware).

If you want to test this on your systems you can easily add a
patch on top of your current branch which just comments out
all the reset calls (and the bq24190_set_mode_host call in
resume) to get the same result on top of your current branch.

Regards,

Hans

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 23-03-17 12:36, Liam Breck wrote:
>>
>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 23-03-17 08:16, Liam Breck wrote:
>>>>
>>>>
>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Resetting the charger should never be necessary it should always have
>>>>>>> sane values programmed. If it is running with invalid values while we
>>>>>>> are not running (system turned off or suspended) there is a big
>>>>>>> problem
>>>>>>> as that may lead to overcharging the battery.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> And some systems may only configure it in the bootloader with no
>>>>>> configuration passed to the kernel at all.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Right.
>>>>>
>>>>>
>>>>>>> The reset in suspend() is meant to put the charger back into default
>>>>>>> mode, but this is not necessary and not a good idea. If the charger
>>>>>>> has
>>>>>>> been programmed with a higher max charge_current / charge_voltage
>>>>>>> then
>>>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>>> after the reset will not help here as that will put the charger back
>>>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>>> watchdog
>>>>>>> expires resetting the device to default-mode, including resetting all
>>>>>>> the registers to there safe power-on defaults. So the only way to
>>>>>>> keep
>>>>>>> using custom charge settings while suspending is to keep the charger
>>>>>>> in
>>>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>>>> as the charger will still automatically switch from constant current
>>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>>
>>>>>>> Besides never being necessary resetting the charger also causes
>>>>>>> problems
>>>>>>> on systems where the charge voltage limit is set higher then the
>>>>>>> reset
>>>>>>> value, if this is the case and the charger is reset while charging
>>>>>>> and
>>>>>>> the battery voltage is between the 2 voltages, then about half the
>>>>>>> time
>>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>>> 0x64)
>>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off)
>>>>>>> and
>>>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>>> reset should just be optional based on a proper device tree
>>>>>> (or platform_data) configuration?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> As described above during my testing I've found that the chip does not
>>>>> respond well to reset under certain conditions, so I think that if
>>>>> we have settings to apply we should just overwrite the settings with
>>>>> our settings, what does doing a reset before overwriting the registers
>>>>> buy us? We still need to do the overwrite anyways.
>>>>
>>>>
>>>>
>>>> When driver loads on any conceivable hw, how do we know the chip is in
>>>> a state we predicted?
>>>
>>>
>>>
>>> Any non reset values have to come from somewhere, likely firmware,
>>> and unless we've platform data telling us the exact settings we should
>>> use why would we decide the reset values are better then the ones
>>> the charger was *already using* before our driver loads ? As said in
>>> the commit msg: "If it is running with invalid values while we
>>> are not running (system turned off or suspended) there is a big problem
>>> as that may lead to overcharging the battery."
>>>
>>>> If it isn't, how can we fix any undesirable
>>>> settings?
>>>
>>>
>>>
>>> Why would the chip not be in a sane state? Where would the undesirable
>>> settings come from ?
>>>
>>>> Reset + fix a,b,c is simple way to do that.
>>>
>>>
>>>
>>> As my testing using real-world hardware under real-world conditions
>>> has shown resetting the charger IC while it is charging can cause it
>>> to misbehave. So we have this theoretical problem of there somehow
>>> being undesirable settings at boot vs a real-world problem...
>>
>>
>> A bug in the charger? Or in the platform hw/fw?
>
>
> I'm pretty sure the fw is not touching the charger, still the problem
> might be something specific to my hw. Either way there does not seem
> to be a good reason to reset the chip and doing a reset despite it
> not being necessary does cause real problems independently of the
> root cause (I'm running this on production hardware which many
> people have, so no way to modify the hardware).
>
> If you want to test this on your systems you can easily add a
> patch on top of your current branch which just comments out
> all the reset calls (and the bq24190_set_mode_host call in
> resume) to get the same result on top of your current branch.

I'll try it.

Why would you not want set_mode_host()?

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 23-03-17 12:47, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 23-03-17 12:36, Liam Breck wrote:
>>>
>>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 23-03-17 08:16, Liam Breck wrote:
>>>>>
>>>>>
>>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Resetting the charger should never be necessary it should always have
>>>>>>>> sane values programmed. If it is running with invalid values while we
>>>>>>>> are not running (system turned off or suspended) there is a big
>>>>>>>> problem
>>>>>>>> as that may lead to overcharging the battery.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> And some systems may only configure it in the bootloader with no
>>>>>>> configuration passed to the kernel at all.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>>
>>>>>>>> The reset in suspend() is meant to put the charger back into default
>>>>>>>> mode, but this is not necessary and not a good idea. If the charger
>>>>>>>> has
>>>>>>>> been programmed with a higher max charge_current / charge_voltage
>>>>>>>> then
>>>>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>>>> after the reset will not help here as that will put the charger back
>>>>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>>>> watchdog
>>>>>>>> expires resetting the device to default-mode, including resetting all
>>>>>>>> the registers to there safe power-on defaults. So the only way to
>>>>>>>> keep
>>>>>>>> using custom charge settings while suspending is to keep the charger
>>>>>>>> in
>>>>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>>>>> as the charger will still automatically switch from constant current
>>>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>>>
>>>>>>>> Besides never being necessary resetting the charger also causes
>>>>>>>> problems
>>>>>>>> on systems where the charge voltage limit is set higher then the
>>>>>>>> reset
>>>>>>>> value, if this is the case and the charger is reset while charging
>>>>>>>> and
>>>>>>>> the battery voltage is between the 2 voltages, then about half the
>>>>>>>> time
>>>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>>>> 0x64)
>>>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off)
>>>>>>>> and
>>>>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>>>> reset should just be optional based on a proper device tree
>>>>>>> (or platform_data) configuration?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> As described above during my testing I've found that the chip does not
>>>>>> respond well to reset under certain conditions, so I think that if
>>>>>> we have settings to apply we should just overwrite the settings with
>>>>>> our settings, what does doing a reset before overwriting the registers
>>>>>> buy us? We still need to do the overwrite anyways.
>>>>>
>>>>>
>>>>>
>>>>> When driver loads on any conceivable hw, how do we know the chip is in
>>>>> a state we predicted?
>>>>
>>>>
>>>>
>>>> Any non reset values have to come from somewhere, likely firmware,
>>>> and unless we've platform data telling us the exact settings we should
>>>> use why would we decide the reset values are better then the ones
>>>> the charger was *already using* before our driver loads ? As said in
>>>> the commit msg: "If it is running with invalid values while we
>>>> are not running (system turned off or suspended) there is a big problem
>>>> as that may lead to overcharging the battery."
>>>>
>>>>> If it isn't, how can we fix any undesirable
>>>>> settings?
>>>>
>>>>
>>>>
>>>> Why would the chip not be in a sane state? Where would the undesirable
>>>> settings come from ?
>>>>
>>>>> Reset + fix a,b,c is simple way to do that.
>>>>
>>>>
>>>>
>>>> As my testing using real-world hardware under real-world conditions
>>>> has shown resetting the charger IC while it is charging can cause it
>>>> to misbehave. So we have this theoretical problem of there somehow
>>>> being undesirable settings at boot vs a real-world problem...
>>>
>>>
>>> A bug in the charger? Or in the platform hw/fw?
>>
>>
>> I'm pretty sure the fw is not touching the charger, still the problem
>> might be something specific to my hw. Either way there does not seem
>> to be a good reason to reset the chip and doing a reset despite it
>> not being necessary does cause real problems independently of the
>> root cause (I'm running this on production hardware which many
>> people have, so no way to modify the hardware).
>>
>> If you want to test this on your systems you can easily add a
>> patch on top of your current branch which just comments out
>> all the reset calls (and the bq24190_set_mode_host call in
>> resume) to get the same result on top of your current branch.
>
> I'll try it.
>
> Why would you not want set_mode_host()?

Doing set_mode_host() on resume is redundant if we do not
reset the chip on suspend/resume. Note we still want the
set_mode_host on boot / probe and my patch preservers that
set_mode_host call.

Regards,

Hans

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 23-03-17 12:36, Liam Breck wrote:
>>
>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 23-03-17 08:16, Liam Breck wrote:
>>>>
>>>>
>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Resetting the charger should never be necessary it should always have
>>>>>>> sane values programmed. If it is running with invalid values while we
>>>>>>> are not running (system turned off or suspended) there is a big
>>>>>>> problem
>>>>>>> as that may lead to overcharging the battery.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> And some systems may only configure it in the bootloader with no
>>>>>> configuration passed to the kernel at all.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Right.
>>>>>
>>>>>
>>>>>>> The reset in suspend() is meant to put the charger back into default
>>>>>>> mode, but this is not necessary and not a good idea. If the charger
>>>>>>> has
>>>>>>> been programmed with a higher max charge_current / charge_voltage
>>>>>>> then
>>>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>>> after the reset will not help here as that will put the charger back
>>>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>>> watchdog
>>>>>>> expires resetting the device to default-mode, including resetting all
>>>>>>> the registers to there safe power-on defaults. So the only way to
>>>>>>> keep
>>>>>>> using custom charge settings while suspending is to keep the charger
>>>>>>> in
>>>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>>>> as the charger will still automatically switch from constant current
>>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>>
>>>>>>> Besides never being necessary resetting the charger also causes
>>>>>>> problems
>>>>>>> on systems where the charge voltage limit is set higher then the
>>>>>>> reset
>>>>>>> value, if this is the case and the charger is reset while charging
>>>>>>> and
>>>>>>> the battery voltage is between the 2 voltages, then about half the
>>>>>>> time
>>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>>> 0x64)
>>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off)
>>>>>>> and
>>>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>>> reset should just be optional based on a proper device tree
>>>>>> (or platform_data) configuration?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> As described above during my testing I've found that the chip does not
>>>>> respond well to reset under certain conditions, so I think that if
>>>>> we have settings to apply we should just overwrite the settings with
>>>>> our settings, what does doing a reset before overwriting the registers
>>>>> buy us? We still need to do the overwrite anyways.
>>>>
>>>>
>>>>
>>>> When driver loads on any conceivable hw, how do we know the chip is in
>>>> a state we predicted?
>>>
>>>
>>>
>>> Any non reset values have to come from somewhere, likely firmware,
>>> and unless we've platform data telling us the exact settings we should
>>> use why would we decide the reset values are better then the ones
>>> the charger was *already using* before our driver loads ? As said in
>>> the commit msg: "If it is running with invalid values while we
>>> are not running (system turned off or suspended) there is a big problem
>>> as that may lead to overcharging the battery."
>>>
>>>> If it isn't, how can we fix any undesirable
>>>> settings?
>>>
>>>
>>>
>>> Why would the chip not be in a sane state? Where would the undesirable
>>> settings come from ?
>>>
>>>> Reset + fix a,b,c is simple way to do that.
>>>
>>>
>>>
>>> As my testing using real-world hardware under real-world conditions
>>> has shown resetting the charger IC while it is charging can cause it
>>> to misbehave. So we have this theoretical problem of there somehow
>>> being undesirable settings at boot vs a real-world problem...
>>
>>
>> A bug in the charger? Or in the platform hw/fw?
>
>
> I'm pretty sure the fw is not touching the charger, still the problem
> might be something specific to my hw. Either way there does not seem
> to be a good reason to reset the chip and doing a reset despite it
> not being necessary does cause real problems independently of the
> root cause (I'm running this on production hardware which many
> people have, so no way to modify the hardware).
>
> If you want to test this on your systems you can easily add a
> patch on top of your current branch which just comments out
> all the reset calls (and the bq24190_set_mode_host call in
> resume) to get the same result on top of your current branch.

My mistake... what I need to try is reproducing your failure due to
reset during charge. Can you explain what switches I need to flip and
dials to watch?

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 23-03-17 21:33, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 23-03-17 12:36, Liam Breck wrote:
>>>
>>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 23-03-17 08:16, Liam Breck wrote:
>>>>>
>>>>>
>>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> wrote:
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Resetting the charger should never be necessary it should always have
>>>>>>>> sane values programmed. If it is running with invalid values while we
>>>>>>>> are not running (system turned off or suspended) there is a big
>>>>>>>> problem
>>>>>>>> as that may lead to overcharging the battery.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> And some systems may only configure it in the bootloader with no
>>>>>>> configuration passed to the kernel at all.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> Right.
>>>>>>
>>>>>>
>>>>>>>> The reset in suspend() is meant to put the charger back into default
>>>>>>>> mode, but this is not necessary and not a good idea. If the charger
>>>>>>>> has
>>>>>>>> been programmed with a higher max charge_current / charge_voltage
>>>>>>>> then
>>>>>>>> putting it back in default-mode will reset those to the safe power-on
>>>>>>>> defaults, leading to slower charging, or charging to a lower voltage
>>>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>>>> after the reset will not help here as that will put the charger back
>>>>>>>> in host mode and start the i2c watchdog if the host then does not do
>>>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>>>> watchdog
>>>>>>>> expires resetting the device to default-mode, including resetting all
>>>>>>>> the registers to there safe power-on defaults. So the only way to
>>>>>>>> keep
>>>>>>>> using custom charge settings while suspending is to keep the charger
>>>>>>>> in
>>>>>>>> its normal running state with the i2c watchdog disabled. This is fine
>>>>>>>> as the charger will still automatically switch from constant current
>>>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>>>
>>>>>>>> Besides never being necessary resetting the charger also causes
>>>>>>>> problems
>>>>>>>> on systems where the charge voltage limit is set higher then the
>>>>>>>> reset
>>>>>>>> value, if this is the case and the charger is reset while charging
>>>>>>>> and
>>>>>>>> the battery voltage is between the 2 voltages, then about half the
>>>>>>>> time
>>>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>>>> 0x64)
>>>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off)
>>>>>>>> and
>>>>>>>> is drawing 0A from VBUS, leaving the system running from the battery.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>>>> reset should just be optional based on a proper device tree
>>>>>>> (or platform_data) configuration?
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> As described above during my testing I've found that the chip does not
>>>>>> respond well to reset under certain conditions, so I think that if
>>>>>> we have settings to apply we should just overwrite the settings with
>>>>>> our settings, what does doing a reset before overwriting the registers
>>>>>> buy us? We still need to do the overwrite anyways.
>>>>>
>>>>>
>>>>>
>>>>> When driver loads on any conceivable hw, how do we know the chip is in
>>>>> a state we predicted?
>>>>
>>>>
>>>>
>>>> Any non reset values have to come from somewhere, likely firmware,
>>>> and unless we've platform data telling us the exact settings we should
>>>> use why would we decide the reset values are better then the ones
>>>> the charger was *already using* before our driver loads ? As said in
>>>> the commit msg: "If it is running with invalid values while we
>>>> are not running (system turned off or suspended) there is a big problem
>>>> as that may lead to overcharging the battery."
>>>>
>>>>> If it isn't, how can we fix any undesirable
>>>>> settings?
>>>>
>>>>
>>>>
>>>> Why would the chip not be in a sane state? Where would the undesirable
>>>> settings come from ?
>>>>
>>>>> Reset + fix a,b,c is simple way to do that.
>>>>
>>>>
>>>>
>>>> As my testing using real-world hardware under real-world conditions
>>>> has shown resetting the charger IC while it is charging can cause it
>>>> to misbehave. So we have this theoretical problem of there somehow
>>>> being undesirable settings at boot vs a real-world problem...
>>>
>>>
>>> A bug in the charger? Or in the platform hw/fw?
>>
>>
>> I'm pretty sure the fw is not touching the charger, still the problem
>> might be something specific to my hw. Either way there does not seem
>> to be a good reason to reset the chip and doing a reset despite it
>> not being necessary does cause real problems independently of the
>> root cause (I'm running this on production hardware which many
>> people have, so no way to modify the hardware).
>>
>> If you want to test this on your systems you can easily add a
>> patch on top of your current branch which just comments out
>> all the reset calls (and the bq24190_set_mode_host call in
>> resume) to get the same result on top of your current branch.
>
> My mistake... what I need to try is reproducing your failure due to
> reset during charge. Can you explain what switches I need to flip and
> dials to watch?

The GPDwin mini laptop I'm developing this on /for has a bq24292i
(bq24192i compatible) with a LiHV battery and max charging
voltage set to 4.38V When resetting the charger while the
battery voltage is between 4.1V (reset max volt) and 4.38V
and the charger is charging. Sometimes instead of just
turning of charging as the bat voltage is now above the max
voltage, it turns of Q1 (disconnects itself from Vbus) while
it thinks it is still charging (observed from REG08 as well
as the charging LED.

Regards,

Hans

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Thu, Mar 23, 2017 at 3:01 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 23-03-17 21:33, Liam Breck wrote:
>>
>> On Thu, Mar 23, 2017 at 4:44 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 23-03-17 12:36, Liam Breck wrote:
>>>>
>>>>
>>>> On Thu, Mar 23, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 23-03-17 08:16, Liam Breck wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Wed, Mar 22, 2017 at 8:45 AM, Hans de Goede <hdegoede@redhat.com>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 22-03-17 16:43, Tony Lindgren wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> * Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Resetting the charger should never be necessary it should always
>>>>>>>>> have
>>>>>>>>> sane values programmed. If it is running with invalid values while
>>>>>>>>> we
>>>>>>>>> are not running (system turned off or suspended) there is a big
>>>>>>>>> problem
>>>>>>>>> as that may lead to overcharging the battery.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> And some systems may only configure it in the bootloader with no
>>>>>>>> configuration passed to the kernel at all.
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Right.
>>>>>>>
>>>>>>>
>>>>>>>>> The reset in suspend() is meant to put the charger back into
>>>>>>>>> default
>>>>>>>>> mode, but this is not necessary and not a good idea. If the charger
>>>>>>>>> has
>>>>>>>>> been programmed with a higher max charge_current / charge_voltage
>>>>>>>>> then
>>>>>>>>> putting it back in default-mode will reset those to the safe
>>>>>>>>> power-on
>>>>>>>>> defaults, leading to slower charging, or charging to a lower
>>>>>>>>> voltage
>>>>>>>>> (and thus not using the full capacity) while suspended which is
>>>>>>>>> undesirable. Reprogramming the max charge_current / charge_voltage
>>>>>>>>> after the reset will not help here as that will put the charger
>>>>>>>>> back
>>>>>>>>> in host mode and start the i2c watchdog if the host then does not
>>>>>>>>> do
>>>>>>>>> anything for 40s (iow if we're suspended for more then 40s) the
>>>>>>>>> watchdog
>>>>>>>>> expires resetting the device to default-mode, including resetting
>>>>>>>>> all
>>>>>>>>> the registers to there safe power-on defaults. So the only way to
>>>>>>>>> keep
>>>>>>>>> using custom charge settings while suspending is to keep the
>>>>>>>>> charger
>>>>>>>>> in
>>>>>>>>> its normal running state with the i2c watchdog disabled. This is
>>>>>>>>> fine
>>>>>>>>> as the charger will still automatically switch from constant
>>>>>>>>> current
>>>>>>>>> to constant voltage and stop charging when the battery is full.
>>>>>>>>>
>>>>>>>>> Besides never being necessary resetting the charger also causes
>>>>>>>>> problems
>>>>>>>>> on systems where the charge voltage limit is set higher then the
>>>>>>>>> reset
>>>>>>>>> value, if this is the case and the charger is reset while charging
>>>>>>>>> and
>>>>>>>>> the battery voltage is between the 2 voltages, then about half the
>>>>>>>>> time
>>>>>>>>> the charger gets confused and claims to be charging (REG08 contains
>>>>>>>>> 0x64)
>>>>>>>>> but in reality the charger has decoupled itself from VBUS (Q1 off)
>>>>>>>>> and
>>>>>>>>> is drawing 0A from VBUS, leaving the system running from the
>>>>>>>>> battery.
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> We do have cases where Linux kernel is the bootloader though using
>>>>>>>> kexec. And in those cases we may need to reset, so I wonder if the
>>>>>>>> reset should just be optional based on a proper device tree
>>>>>>>> (or platform_data) configuration?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> As described above during my testing I've found that the chip does
>>>>>>> not
>>>>>>> respond well to reset under certain conditions, so I think that if
>>>>>>> we have settings to apply we should just overwrite the settings with
>>>>>>> our settings, what does doing a reset before overwriting the
>>>>>>> registers
>>>>>>> buy us? We still need to do the overwrite anyways.
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> When driver loads on any conceivable hw, how do we know the chip is in
>>>>>> a state we predicted?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Any non reset values have to come from somewhere, likely firmware,
>>>>> and unless we've platform data telling us the exact settings we should
>>>>> use why would we decide the reset values are better then the ones
>>>>> the charger was *already using* before our driver loads ? As said in
>>>>> the commit msg: "If it is running with invalid values while we
>>>>> are not running (system turned off or suspended) there is a big problem
>>>>> as that may lead to overcharging the battery."
>>>>>
>>>>>> If it isn't, how can we fix any undesirable
>>>>>> settings?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> Why would the chip not be in a sane state? Where would the undesirable
>>>>> settings come from ?
>>>>>
>>>>>> Reset + fix a,b,c is simple way to do that.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> As my testing using real-world hardware under real-world conditions
>>>>> has shown resetting the charger IC while it is charging can cause it
>>>>> to misbehave. So we have this theoretical problem of there somehow
>>>>> being undesirable settings at boot vs a real-world problem...
>>>>
>>>>
>>>>
>>>> A bug in the charger? Or in the platform hw/fw?
>>>
>>>
>>>
>>> I'm pretty sure the fw is not touching the charger, still the problem
>>> might be something specific to my hw. Either way there does not seem
>>> to be a good reason to reset the chip and doing a reset despite it
>>> not being necessary does cause real problems independently of the
>>> root cause (I'm running this on production hardware which many
>>> people have, so no way to modify the hardware).
>>>
>>> If you want to test this on your systems you can easily add a
>>> patch on top of your current branch which just comments out
>>> all the reset calls (and the bq24190_set_mode_host call in
>>> resume) to get the same result on top of your current branch.
>>
>>
>> My mistake... what I need to try is reproducing your failure due to
>> reset during charge. Can you explain what switches I need to flip and
>> dials to watch?
>
>
> The GPDwin mini laptop I'm developing this on /for has a bq24292i
> (bq24192i compatible) with a LiHV battery and max charging
> voltage set to 4.38V When resetting the charger while the
> battery voltage is between 4.1V (reset max volt) and 4.38V
> and the charger is charging. Sometimes instead of just
> turning of charging as the bat voltage is now above the max
> voltage, it turns of Q1 (disconnects itself from Vbus) while
> it thinks it is still charging (observed from REG08 as well
> as the charging LED.

Thanks. It will take me a few days to get to this.

In the meantime, note that on a DT-configured system, we don't want
any charger settings made by the bootloader, as we're going to change
default settings (e.g. precharge & endcharge from my patchset) based
on DT params.

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Resetting the charger should never be necessary it should always have
> sane values programmed. If it is running with invalid values while we
> are not running (system turned off or suspended) there is a big problem
> as that may lead to overcharging the battery.
>
> The reset in suspend() is meant to put the charger back into default
> mode, but this is not necessary and not a good idea. If the charger has
> been programmed with a higher max charge_current / charge_voltage then
> putting it back in default-mode will reset those to the safe power-on
> defaults, leading to slower charging, or charging to a lower voltage
> (and thus not using the full capacity) while suspended which is
> undesirable. Reprogramming the max charge_current / charge_voltage
> after the reset will not help here as that will put the charger back
> in host mode and start the i2c watchdog if the host then does not do
> anything for 40s (iow if we're suspended for more then 40s) the watchdog
> expires resetting the device to default-mode, including resetting all
> the registers to there safe power-on defaults. So the only way to keep
> using custom charge settings while suspending is to keep the charger in
> its normal running state with the i2c watchdog disabled. This is fine
> as the charger will still automatically switch from constant current
> to constant voltage and stop charging when the battery is full.
>
> Besides never being necessary resetting the charger also causes problems
> on systems where the charge voltage limit is set higher then the reset
> value, if this is the case and the charger is reset while charging and
> the battery voltage is between the 2 voltages, then about half the time
> the charger gets confused and claims to be charging (REG08 contains 0x64)
> but in reality the charger has decoupled itself from VBUS (Q1 off) and
> is drawing 0A from VBUS, leaving the system running from the battery.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 58 ----------------------------------
>  1 file changed, 58 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 7a2a496..b535f24 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -526,40 +526,6 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>         return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>  }
>
> -static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> -{
> -       int ret, limit = 100;
> -       u8 v;
> -
> -       /* Reset the registers */
> -       ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
> -                       BQ24190_REG_POC_RESET_MASK,
> -                       BQ24190_REG_POC_RESET_SHIFT,
> -                       0x1);
> -       if (ret < 0)
> -               return ret;
> -
> -       /* Reset bit will be cleared by hardware so poll until it is */
> -       do {
> -               ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
> -                               BQ24190_REG_POC_RESET_MASK,
> -                               BQ24190_REG_POC_RESET_SHIFT,
> -                               &v);
> -               if (ret < 0)
> -                       return ret;
> -
> -               if (!v)
> -                       break;
> -
> -               udelay(10);
> -       } while (--limit);
> -
> -       if (!limit)
> -               return -EIO;
> -
> -       return 0;
> -}

Let's leave this in with __maybe_unused.

>  /* Charger power supply property routines */
>
>  static int bq24190_charger_get_charge_type(struct bq24190_dev_info *bdi,
> @@ -1380,10 +1346,6 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>                 return -ENODEV;
>         }
>
> -       ret = bq24190_register_reset(bdi);
> -       if (ret < 0)
> -               return ret;
> -
>         ret = bq24190_set_mode_host(bdi);
>         if (ret < 0)
>                 return ret;
> @@ -1534,7 +1496,6 @@ static int bq24190_remove(struct i2c_client *client)
>                 pm_runtime_put_noidle(bdi->dev);
>         }
>
> -       bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> @@ -1577,23 +1538,6 @@ static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>
>  static __maybe_unused 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;
> -
> -       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);
> -       }
> -
> -       bq24190_register_reset(bdi);
> -
> -       if (error >= 0) {
> -               pm_runtime_mark_last_busy(bdi->dev);
> -               pm_runtime_put_autosuspend(bdi->dev);
> -       }
> -
>         return 0;
>  }
>
> @@ -1612,8 +1556,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>                 pm_runtime_put_noidle(bdi->dev);
>         }
>
> -       bq24190_register_reset(bdi);
> -       bq24190_set_mode_host(bdi);
>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>
>         if (error >= 0) {
> --
> 2.9.3
>

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Hans de Goede]

Hi,

On 22-03-17 19:41, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Resetting the charger should never be necessary it should always have
>> sane values programmed. If it is running with invalid values while we
>> are not running (system turned off or suspended) there is a big problem
>> as that may lead to overcharging the battery.
>>
>> The reset in suspend() is meant to put the charger back into default
>> mode, but this is not necessary and not a good idea. If the charger has
>> been programmed with a higher max charge_current / charge_voltage then
>> putting it back in default-mode will reset those to the safe power-on
>> defaults, leading to slower charging, or charging to a lower voltage
>> (and thus not using the full capacity) while suspended which is
>> undesirable. Reprogramming the max charge_current / charge_voltage
>> after the reset will not help here as that will put the charger back
>> in host mode and start the i2c watchdog if the host then does not do
>> anything for 40s (iow if we're suspended for more then 40s) the watchdog
>> expires resetting the device to default-mode, including resetting all
>> the registers to there safe power-on defaults. So the only way to keep
>> using custom charge settings while suspending is to keep the charger in
>> its normal running state with the i2c watchdog disabled. This is fine
>> as the charger will still automatically switch from constant current
>> to constant voltage and stop charging when the battery is full.
>>
>> Besides never being necessary resetting the charger also causes problems
>> on systems where the charge voltage limit is set higher then the reset
>> value, if this is the case and the charger is reset while charging and
>> the battery voltage is between the 2 voltages, then about half the time
>> the charger gets confused and claims to be charging (REG08 contains 0x64)
>> but in reality the charger has decoupled itself from VBUS (Q1 off) and
>> is drawing 0A from VBUS, leaving the system running from the battery.
>>
>> Cc: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/power/supply/bq24190_charger.c | 58 ----------------------------------
>>  1 file changed, 58 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 7a2a496..b535f24 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -526,40 +526,6 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>>         return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>>  }
>>
>> -static int bq24190_register_reset(struct bq24190_dev_info *bdi)
>> -{
>> -       int ret, limit = 100;
>> -       u8 v;
>> -
>> -       /* Reset the registers */
>> -       ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
>> -                       BQ24190_REG_POC_RESET_MASK,
>> -                       BQ24190_REG_POC_RESET_SHIFT,
>> -                       0x1);
>> -       if (ret < 0)
>> -               return ret;
>> -
>> -       /* Reset bit will be cleared by hardware so poll until it is */
>> -       do {
>> -               ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
>> -                               BQ24190_REG_POC_RESET_MASK,
>> -                               BQ24190_REG_POC_RESET_SHIFT,
>> -                               &v);
>> -               if (ret < 0)
>> -                       return ret;
>> -
>> -               if (!v)
>> -                       break;
>> -
>> -               udelay(10);
>> -       } while (--limit);
>> -
>> -       if (!limit)
>> -               return -EIO;
>> -
>> -       return 0;
>> -}
>
> Let's leave this in with __maybe_unused.

I don't like leaving dead code around and bringing it back from git
history is quite easy.

Regards,

Hans


>
>>  /* Charger power supply property routines */
>>
>>  static int bq24190_charger_get_charge_type(struct bq24190_dev_info *bdi,
>> @@ -1380,10 +1346,6 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>>                 return -ENODEV;
>>         }
>>
>> -       ret = bq24190_register_reset(bdi);
>> -       if (ret < 0)
>> -               return ret;
>> -
>>         ret = bq24190_set_mode_host(bdi);
>>         if (ret < 0)
>>                 return ret;
>> @@ -1534,7 +1496,6 @@ static int bq24190_remove(struct i2c_client *client)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>>         bq24190_sysfs_remove_group(bdi);
>>         power_supply_unregister(bdi->battery);
>>         power_supply_unregister(bdi->charger);
>> @@ -1577,23 +1538,6 @@ static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>>
>>  static __maybe_unused 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;
>> -
>> -       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);
>> -       }
>> -
>> -       bq24190_register_reset(bdi);
>> -
>> -       if (error >= 0) {
>> -               pm_runtime_mark_last_busy(bdi->dev);
>> -               pm_runtime_put_autosuspend(bdi->dev);
>> -       }
>> -
>>         return 0;
>>  }
>>
>> @@ -1612,8 +1556,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>> -       bq24190_set_mode_host(bdi);
>>         bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>>
>>         if (error >= 0) {
>> --
>> 2.9.3
>>

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 11:41:14AM -0700, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > -static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> > -{
> > -       int ret, limit = 100;
> > -       u8 v;
> > -
> > -       /* Reset the registers */
> > -       ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
> > -                       BQ24190_REG_POC_RESET_MASK,
> > -                       BQ24190_REG_POC_RESET_SHIFT,
> > -                       0x1);
> > -       if (ret < 0)
> > -               return ret;
> > -
> > -       /* Reset bit will be cleared by hardware so poll until it is */
> > -       do {
> > -               ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
> > -                               BQ24190_REG_POC_RESET_MASK,
> > -                               BQ24190_REG_POC_RESET_SHIFT,
> > -                               &v);
> > -               if (ret < 0)
> > -                       return ret;
> > -
> > -               if (!v)
> > -                       break;
> > -
> > -               udelay(10);
> > -       } while (--limit);
> > -
> > -       if (!limit)
> > -               return -EIO;
> > -
> > -       return 0;
> > -}
> 
> Let's leave this in with __maybe_unused.

It's named __maybe_unused instead of __always_unused for a reason.

-- Sebastian

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

Re: [PATCH v2 4/7] power: supply: bq24190_charger: Never reset the charger chip

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 03:55:33PM +0100, Hans de Goede wrote:
> Resetting the charger should never be necessary it should always have
> sane values programmed. If it is running with invalid values while we
> are not running (system turned off or suspended) there is a big problem
> as that may lead to overcharging the battery.
> 
> The reset in suspend() is meant to put the charger back into default
> mode, but this is not necessary and not a good idea. If the charger has
> been programmed with a higher max charge_current / charge_voltage then
> putting it back in default-mode will reset those to the safe power-on
> defaults, leading to slower charging, or charging to a lower voltage
> (and thus not using the full capacity) while suspended which is
> undesirable. Reprogramming the max charge_current / charge_voltage
> after the reset will not help here as that will put the charger back
> in host mode and start the i2c watchdog if the host then does not do
> anything for 40s (iow if we're suspended for more then 40s) the watchdog
> expires resetting the device to default-mode, including resetting all
> the registers to there safe power-on defaults. So the only way to keep
> using custom charge settings while suspending is to keep the charger in
> its normal running state with the i2c watchdog disabled. This is fine
> as the charger will still automatically switch from constant current
> to constant voltage and stop charging when the battery is full.
> 
> Besides never being necessary resetting the charger also causes problems
> on systems where the charge voltage limit is set higher then the reset
> value, if this is the case and the charger is reset while charging and
> the battery voltage is between the 2 voltages, then about half the time
> the charger gets confused and claims to be charging (REG08 contains 0x64)
> but in reality the charger has decoupled itself from VBUS (Q1 off) and
> is drawing 0A from VBUS, leaving the system running from the battery.
> 
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---

I skipped this patch for now due to ongoing discussion and behaviour
change. I think your reasoning makes sense and will queue this, if I
don't hear it results in regression in the next few days.

-- Sebastian

>  drivers/power/supply/bq24190_charger.c | 58 ----------------------------------
>  1 file changed, 58 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 7a2a496..b535f24 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -526,40 +526,6 @@ static int bq24190_set_mode_host(struct bq24190_dev_info *bdi)
>  	return bq24190_write(bdi, BQ24190_REG_CTTC, v);
>  }
>  
> -static int bq24190_register_reset(struct bq24190_dev_info *bdi)
> -{
> -	int ret, limit = 100;
> -	u8 v;
> -
> -	/* Reset the registers */
> -	ret = bq24190_write_mask(bdi, BQ24190_REG_POC,
> -			BQ24190_REG_POC_RESET_MASK,
> -			BQ24190_REG_POC_RESET_SHIFT,
> -			0x1);
> -	if (ret < 0)
> -		return ret;
> -
> -	/* Reset bit will be cleared by hardware so poll until it is */
> -	do {
> -		ret = bq24190_read_mask(bdi, BQ24190_REG_POC,
> -				BQ24190_REG_POC_RESET_MASK,
> -				BQ24190_REG_POC_RESET_SHIFT,
> -				&v);
> -		if (ret < 0)
> -			return ret;
> -
> -		if (!v)
> -			break;
> -
> -		udelay(10);
> -	} while (--limit);
> -
> -	if (!limit)
> -		return -EIO;
> -
> -	return 0;
> -}
> -
>  /* Charger power supply property routines */
>  
>  static int bq24190_charger_get_charge_type(struct bq24190_dev_info *bdi,
> @@ -1380,10 +1346,6 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>  		return -ENODEV;
>  	}
>  
> -	ret = bq24190_register_reset(bdi);
> -	if (ret < 0)
> -		return ret;
> -
>  	ret = bq24190_set_mode_host(bdi);
>  	if (ret < 0)
>  		return ret;
> @@ -1534,7 +1496,6 @@ static int bq24190_remove(struct i2c_client *client)
>  		pm_runtime_put_noidle(bdi->dev);
>  	}
>  
> -	bq24190_register_reset(bdi);
>  	bq24190_sysfs_remove_group(bdi);
>  	power_supply_unregister(bdi->battery);
>  	power_supply_unregister(bdi->charger);
> @@ -1577,23 +1538,6 @@ static __maybe_unused int bq24190_runtime_resume(struct device *dev)
>  
>  static __maybe_unused 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;
> -
> -	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);
> -	}
> -
> -	bq24190_register_reset(bdi);
> -
> -	if (error >= 0) {
> -		pm_runtime_mark_last_busy(bdi->dev);
> -		pm_runtime_put_autosuspend(bdi->dev);
> -	}
> -
>  	return 0;
>  }
>  
> @@ -1612,8 +1556,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>  		pm_runtime_put_noidle(bdi->dev);
>  	}
>  
> -	bq24190_register_reset(bdi);
> -	bq24190_set_mode_host(bdi);
>  	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
>  
>  	if (error >= 0) {
> -- 
> 2.9.3
> 

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

[PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Hans de Goede]

If the charger gets unplugged before the battery is fully charged we will
get a one time Input fault. Ignore this rather then logging a message for
it. Likewise on the next interrupt after the one time Input fault all
fault flags will be 0, do not log a message when there are no faults.

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: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index b535f24..351e020 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 	} 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));
+		/*
+		 * Don't spam the logs if all faults are cleared, or when the
+		 * cable providing Vbus gets unplugged.
+		 */
+		if  (f_reg && f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT))
+			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))
-- 
2.9.3

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> If the charger gets unplugged before the battery is fully charged we will
> get a one time Input fault. Ignore this rather then logging a message for
> it. Likewise on the next interrupt after the one time Input fault all
> fault flags will be 0, do not log a message when there are no faults.
> 
> 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

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

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 03:55:34PM +0100, Hans de Goede wrote:
> If the charger gets unplugged before the battery is fully charged we will
> get a one time Input fault. Ignore this rather then logging a message for
> it. Likewise on the next interrupt after the one time Input fault all
> fault flags will be 0, do not log a message when there are no faults.
> 
> 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: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---

Thanks, queued.

-- Sebastian

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

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> If the charger gets unplugged before the battery is fully charged we will
> get a one time Input fault. Ignore this rather then logging a message for
> it. Likewise on the next interrupt after the one time Input fault all
> fault flags will be 0, do not log a message when there are no faults.
>
> 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: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index b535f24..351e020 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>         } 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));
> +               /*
> +                * Don't spam the logs if all faults are cleared, or when the
> +                * cable providing Vbus gets unplugged.
> +                */
> +               if  (f_reg && f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT))

I have seen this too, but I think you masked input over-voltage, as
it's not differentiated from under-voltage. Maybe check pgstat?

> +                       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))
> --
> 2.9.3
>

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> If the charger gets unplugged before the battery is fully charged we will
> get a one time Input fault. Ignore this rather then logging a message for
> it. Likewise on the next interrupt after the one time Input fault all
> fault flags will be 0, do not log a message when there are no faults.
>
> 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: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index b535f24..351e020 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>         } 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));
> +               /*
> +                * Don't spam the logs if all faults are cleared, or when the
> +                * cable providing Vbus gets unplugged.
> +                */
> +               if  (f_reg && f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT))

if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
    f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)) )

> +                       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))
> --
> 2.9.3
>

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Hans de Goede]

Hi,

On 23-03-17 22:31, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> If the charger gets unplugged before the battery is fully charged we will
>> get a one time Input fault. Ignore this rather then logging a message for
>> it. Likewise on the next interrupt after the one time Input fault all
>> fault flags will be 0, do not log a message when there are no faults.
>>
>> 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: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index b535f24..351e020 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>>         } 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));
>> +               /*
>> +                * Don't spam the logs if all faults are cleared, or when the
>> +                * cable providing Vbus gets unplugged.
>> +                */
>> +               if  (f_reg && f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT))
>
> if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
>     f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)) )

Sebastian has already merged the original patch into his for-next branch, please
provide a patch on top.

Regards,

Hans


>
>> +                       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))
>> --
>> 2.9.3
>>

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Liam Breck]

On Thu, Mar 23, 2017 at 3:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 23-03-17 22:31, Liam Breck wrote:
>>
>> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> If the charger gets unplugged before the battery is fully charged we will
>>> get a one time Input fault. Ignore this rather then logging a message for
>>> it. Likewise on the next interrupt after the one time Input fault all
>>> fault flags will be 0, do not log a message when there are no faults.
>>>
>>> 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: Liam Breck <kernel@networkimprov.net>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -This is a new patch in v2 of this patch-set
>>> ---
>>>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index b535f24..351e020 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct
>>> bq24190_dev_info *bdi)
>>>         } 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));
>>> +               /*
>>> +                * Don't spam the logs if all faults are cleared, or when
>>> the
>>> +                * cable providing Vbus gets unplugged.
>>> +                */
>>> +               if  (f_reg && f_reg != (1 <<
>>> BQ24190_REG_F_CHRG_FAULT_SHIFT))
>>
>>
>> if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
>>     f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)) )
>
>
> Sebastian has already merged the original patch into his for-next branch,
> please
> provide a patch on top.

I recall Sebastian merging and then dropping a commit from the other
series I'm working on.

I'll return to the charger once the dependency for my charger patchset
is merged. I just got a big change request for that.


>>> +                       dev_info(bdi->dev,

This should probably be dev_warn now.

>>> +                               "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))
>>> --
>>> 2.9.3
>>>
>

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Sebastian Reichel]

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

Hi,

On Thu, Mar 23, 2017 at 03:24:15PM -0700, Liam Breck wrote:
> On Thu, Mar 23, 2017 at 3:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> > On 23-03-17 22:31, Liam Breck wrote:
> >> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com>
> >> wrote:
> >>>
> >>> If the charger gets unplugged before the battery is fully charged we will
> >>> get a one time Input fault. Ignore this rather then logging a message for
> >>> it. Likewise on the next interrupt after the one time Input fault all
> >>> fault flags will be 0, do not log a message when there are no faults.
> >>>
> >>> 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: Liam Breck <kernel@networkimprov.net>
> >>> Cc: Tony Lindgren <tony@atomide.com>
> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>> ---
> >>> Changes in v2:
> >>> -This is a new patch in v2 of this patch-set
> >>> ---
> >>>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
> >>>  1 file changed, 11 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/power/supply/bq24190_charger.c
> >>> b/drivers/power/supply/bq24190_charger.c
> >>> index b535f24..351e020 100644
> >>> --- a/drivers/power/supply/bq24190_charger.c
> >>> +++ b/drivers/power/supply/bq24190_charger.c
> >>> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct
> >>> bq24190_dev_info *bdi)
> >>>         } 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));
> >>> +               /*
> >>> +                * Don't spam the logs if all faults are cleared, or when
> >>> the
> >>> +                * cable providing Vbus gets unplugged.
> >>> +                */
> >>> +               if  (f_reg && f_reg != (1 <<
> >>> BQ24190_REG_F_CHRG_FAULT_SHIFT))
> >>
> >>
> >> if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
> >>     f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)) )
> >
> >
> > Sebastian has already merged the original patch into his for-next branch,
> > please
> > provide a patch on top.
> 
> I recall Sebastian merging and then dropping a commit from the other
> series I'm working on.

Yeah, I thought it was a fix, but it was useless without the
remaining series.

> I'll return to the charger once the dependency for my charger patchset
> is merged. I just got a big change request for that.

Ok.

> 
> >>> +                       dev_info(bdi->dev,
> 
> This should probably be dev_warn now.

makes sense to me.

FWIW I think a follow-up commit for this is enough, but I can also
swap the existing commit.

-- Sebastian

> >>> +                               "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))
> >>> --
> >>> 2.9.3
> >>>
> >

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

Re: [PATCH v2 5/7] power: supply: bq24190_charger: Don't spam the logs on charger plug / unplug

[Liam Breck]

On Fri, Mar 24, 2017 at 2:25 AM, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Thu, Mar 23, 2017 at 03:24:15PM -0700, Liam Breck wrote:
>> On Thu, Mar 23, 2017 at 3:02 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> > On 23-03-17 22:31, Liam Breck wrote:
>> >> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com>
>> >> wrote:
>> >>>
>> >>> If the charger gets unplugged before the battery is fully charged we will
>> >>> get a one time Input fault. Ignore this rather then logging a message for
>> >>> it. Likewise on the next interrupt after the one time Input fault all
>> >>> fault flags will be 0, do not log a message when there are no faults.
>> >>>
>> >>> 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: Liam Breck <kernel@networkimprov.net>
>> >>> Cc: Tony Lindgren <tony@atomide.com>
>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> >>> ---
>> >>> Changes in v2:
>> >>> -This is a new patch in v2 of this patch-set
>> >>> ---
>> >>>  drivers/power/supply/bq24190_charger.c | 17 +++++++++++------
>> >>>  1 file changed, 11 insertions(+), 6 deletions(-)
>> >>>
>> >>> diff --git a/drivers/power/supply/bq24190_charger.c
>> >>> b/drivers/power/supply/bq24190_charger.c
>> >>> index b535f24..351e020 100644
>> >>> --- a/drivers/power/supply/bq24190_charger.c
>> >>> +++ b/drivers/power/supply/bq24190_charger.c
>> >>> @@ -1189,12 +1189,17 @@ static void bq24190_check_status(struct
>> >>> bq24190_dev_info *bdi)
>> >>>         } 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));
>> >>> +               /*
>> >>> +                * Don't spam the logs if all faults are cleared, or when
>> >>> the
>> >>> +                * cable providing Vbus gets unplugged.
>> >>> +                */
>> >>> +               if  (f_reg && f_reg != (1 <<
>> >>> BQ24190_REG_F_CHRG_FAULT_SHIFT))
>> >>
>> >>
>> >> if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
>> >>     f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)) )
>> >
>> >
>> > Sebastian has already merged the original patch into his for-next branch,
>> > please
>> > provide a patch on top.
>>
>> I recall Sebastian merging and then dropping a commit from the other
>> series I'm working on.
>
> Yeah, I thought it was a fix, but it was useless without the
> remaining series.
>
>> I'll return to the charger once the dependency for my charger patchset
>> is merged. I just got a big change request for that.
>
> Ok.
>
>>
>> >>> +                       dev_info(bdi->dev,
>>
>> This should probably be dev_warn now.
>
> makes sense to me.
>
> FWIW I think a follow-up commit for this is enough, but I can also
> swap the existing commit.

I posted a replacement for this commit.

In future could you wait for my ack on patches for this driver? I have
more time to study this stuff than Tony does.

>> >>> +                               "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))
>> >>> --
>> >>> 2.9.3
>> >>>
>> >

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

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

If the charger gets unplugged before the battery is fully charged we will
get an over/under voltage fault. Ignore this rather then logging a message for
it. Likewise on the next interrupt after the fault all fault flags will be 0,
do not log a message when there are no faults.

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>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Sebastian, here is a replacement for the original version.
I include Hans' sign-off since he wrote the original.

 drivers/power/supply/bq24190_charger.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 897491d..2e734ab 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1257,12 +1257,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 	} 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));
+		if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
+		    f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)))
+			/* omit over/under voltage fault after disconnect */
+			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),
+				!!(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))
-- 
2.9.3

Re: [PATCH] power: bq24190_charger: Limit over/under voltage fault logging

[Hans de Goede]

Hi,

On 24-03-17 11:29, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
>
> If the charger gets unplugged before the battery is fully charged we will
> get an over/under voltage fault. Ignore this rather then logging a message for
> it. Likewise on the next interrupt after the fault all fault flags will be 0,
> do not log a message when there are no faults.
>
> 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>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Sebastian, here is a replacement for the original version.
> I include Hans' sign-off since he wrote the original.

I can confirm that this still silences the log messages
on charger plug / unplug for me:

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

Regards,

Hans


>
>  drivers/power/supply/bq24190_charger.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 897491d..2e734ab 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1257,12 +1257,15 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  	} 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));
> +		if (f_reg && ((ss_reg & BQ24190_REG_SS_PG_STAT_MASK) ||
> +		    f_reg != (1 << BQ24190_REG_F_CHRG_FAULT_SHIFT)))
> +			/* omit over/under voltage fault after disconnect */
> +			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),
> +				!!(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))
>

[PATCH v2] 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>
---
v2: Don't hide clearance of other more urgent faults, or produce
    a uevent for this superficial fault.

 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 897491d..de1c54f 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1256,8 +1256,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

Re: [PATCH v2] power: bq24190_charger: Limit over/under voltage fault logging

[Hans de Goede]

Hi,

On 25-03-17 12:24, Liam Breck wrote:
> 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>
> ---
> v2: Don't hide clearance of other more urgent faults, or produce
>     a uevent for this superficial fault.
>
>  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 897491d..de1c54f 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1256,8 +1256,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),
>

You're now once again logging a warning when all faults clear,
I believe this should now be:

		if (freg)
			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),

Regards,

Hans

Re: [PATCH v2] power: bq24190_charger: Limit over/under voltage fault logging

[Liam Breck]

On Sun, Mar 26, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 25-03-17 12:24, Liam Breck wrote:
>>
>> 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>
>> ---
>> v2: Don't hide clearance of other more urgent faults, or produce
>>     a uevent for this superficial fault.
>>
>>  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 897491d..de1c54f 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1256,8 +1256,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),
>>
>
> You're now once again logging a warning when all faults clear,

By design; I want to know when any serious fault clears.

The under-voltage fault on disconnect will not trigger a cleared msg,
because the orig error is not recorded in bdi->f_reg.

> I believe this should now be:
>
>                 if (freg)
>                         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),
>
> Regards,
>
> Hans
>
>
>

Re: [PATCH v2] power: bq24190_charger: Limit over/under voltage fault logging

[Hans de Goede]

Hi,

On 26-03-17 12:56, Liam Breck wrote:
> On Sun, Mar 26, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 25-03-17 12:24, Liam Breck wrote:
>>>
>>> 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>
>>> ---
>>> v2: Don't hide clearance of other more urgent faults, or produce
>>>     a uevent for this superficial fault.
>>>
>>>  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 897491d..de1c54f 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -1256,8 +1256,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),
>>>
>>
>> You're now once again logging a warning when all faults clear,
>
> By design; I want to know when any serious fault clears.
>
> The under-voltage fault on disconnect will not trigger a cleared msg,
> because the orig error is not recorded in bdi->f_reg.

Ok fine by me.

Regards,

Hans


>
>> I believe this should now be:
>>
>>                 if (freg)
>>                         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),
>>
>> Regards,
>>
>> Hans
>>
>>
>>

Re: [PATCH v2] power: bq24190_charger: Limit over/under voltage fault logging

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170326 04:06]:
> Hi,
> 
> On 26-03-17 12:56, Liam Breck wrote:
> > On Sun, Mar 26, 2017 at 1:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> > > Hi,
> > > 
> > > 
> > > On 25-03-17 12:24, Liam Breck wrote:
> > > > 
> > > > 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>
> > > > ---
> > > > v2: Don't hide clearance of other more urgent faults, or produce
> > > >     a uevent for this superficial fault.
> > > > 
> > > >  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 897491d..de1c54f 100644
> > > > --- a/drivers/power/supply/bq24190_charger.c
> > > > +++ b/drivers/power/supply/bq24190_charger.c
> > > > @@ -1256,8 +1256,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),
> > > > 
> > > 
> > > You're now once again logging a warning when all faults clear,
> > 
> > By design; I want to know when any serious fault clears.
> > 
> > The under-voltage fault on disconnect will not trigger a cleared msg,
> > because the orig error is not recorded in bdi->f_reg.
> 
> Ok fine by me.

Fine with me too:

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

[PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Hans de Goede]

Names like out1, out2, etc. do not make it easier to follow what is
going on and make it harder (require renaming) if any steps are
later added / removed. Rename the labels to sane names.

This also folds out1 and out2 into one pm_runtime_disable step,
if pm_runtime_get_sync fails we still need to do the put, it
failing means that the device failed to resume, but the refcount
will have been incremented and we need to decrement it.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 351e020..5e3da66 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client *client,
 	pm_runtime_set_autosuspend_delay(dev, 600);
 	ret = pm_runtime_get_sync(dev);
 	if (ret < 0)
-		goto out1;
+		goto pm_runtime_disable;
 
 	ret = bq24190_hw_init(bdi);
 	if (ret < 0) {
 		dev_err(dev, "Hardware init failed\n");
-		goto out2;
+		goto pm_runtime_disable;
 	}
 
 	charger_cfg.drv_data = bdi;
@@ -1424,7 +1424,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 pm_runtime_disable;
 	}
 
 	battery_cfg.drv_data = bdi;
@@ -1433,13 +1433,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 unregister_charger;
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto out4;
+		goto unregister_battery;
 	}
 
 	bdi->initialized = true;
@@ -1450,7 +1450,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 remove_sysfs_group;
 	}
 
 	if (bdi->extcon) {
@@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
 		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
 						    &bdi->extcon_nb);
 		if (ret)
-			goto out5;
+			goto remove_sysfs_group;
 
 		/* Sync initial cable state */
 		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
@@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client *client,
 
 	return 0;
 
-out5:
+remove_sysfs_group:
 	bq24190_sysfs_remove_group(bdi);
 
-out4:
+unregister_battery:
 	power_supply_unregister(bdi->battery);
 
-out3:
+unregister_charger:
 	power_supply_unregister(bdi->charger);
 
-out2:
+pm_runtime_disable:
 	pm_runtime_put_sync(dev);
-
-out1:
 	pm_runtime_dont_use_autosuspend(dev);
 	pm_runtime_disable(dev);
 	return ret;
-- 
2.9.3

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Tony Lindgren]

* Hans de Goede <hdegoede@redhat.com> [170322 07:57]:
> Names like out1, out2, etc. do not make it easier to follow what is
> going on and make it harder (require renaming) if any steps are
> later added / removed. Rename the labels to sane names.
> 
> This also folds out1 and out2 into one pm_runtime_disable step,
> if pm_runtime_get_sync fails we still need to do the put, it
> failing means that the device failed to resume, but the refcount
> will have been incremented and we need to decrement it.

Seems OK to me:

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

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Names like out1, out2, etc. do not make it easier to follow what is
> going on and make it harder (require renaming) if any steps are
> later added / removed. Rename the labels to sane names.

Good idea. But the new labels look like function names, so maybe:
 out_charger, out_battery, out_sysfs, out_pmrt

> This also folds out1 and out2 into one pm_runtime_disable step,
> if pm_runtime_get_sync fails we still need to do the put, it
> failing means that the device failed to resume, but the refcount
> will have been incremented and we need to decrement it.
>
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 351e020..5e3da66 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client *client,
>         pm_runtime_set_autosuspend_delay(dev, 600);
>         ret = pm_runtime_get_sync(dev);
>         if (ret < 0)
> -               goto out1;
> +               goto pm_runtime_disable;
>
>         ret = bq24190_hw_init(bdi);
>         if (ret < 0) {
>                 dev_err(dev, "Hardware init failed\n");
> -               goto out2;
> +               goto pm_runtime_disable;
>         }
>
>         charger_cfg.drv_data = bdi;
> @@ -1424,7 +1424,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 pm_runtime_disable;
>         }
>
>         battery_cfg.drv_data = bdi;
> @@ -1433,13 +1433,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 unregister_charger;
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto out4;
> +               goto unregister_battery;
>         }
>
>         bdi->initialized = true;
> @@ -1450,7 +1450,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 remove_sysfs_group;
>         }
>
>         if (bdi->extcon) {
> @@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
>                 ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>                                                     &bdi->extcon_nb);
>                 if (ret)
> -                       goto out5;
> +                       goto remove_sysfs_group;
>
>                 /* Sync initial cable state */
>                 queue_delayed_work(system_wq, &bdi->extcon_work, 0);
> @@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client *client,
>
>         return 0;
>
> -out5:
> +remove_sysfs_group:
>         bq24190_sysfs_remove_group(bdi);
>
> -out4:
> +unregister_battery:
>         power_supply_unregister(bdi->battery);
>
> -out3:
> +unregister_charger:
>         power_supply_unregister(bdi->charger);
>
> -out2:
> +pm_runtime_disable:
>         pm_runtime_put_sync(dev);
> -
> -out1:
>         pm_runtime_dont_use_autosuspend(dev);
>         pm_runtime_disable(dev);
>         return ret;
> --
> 2.9.3
>

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Hans de Goede]

Hi,

On 22-03-17 20:09, Liam Breck wrote:
> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Names like out1, out2, etc. do not make it easier to follow what is
>> going on and make it harder (require renaming) if any steps are
>> later added / removed. Rename the labels to sane names.
>
> Good idea. But the new labels look like function names

labels named like this are used all throughout the kernel,
see for example drivers/power/supply/pm2301_charger.c,
drivers/power/supply/ab8500_*.c

 > so maybe: out_charger, out_battery, out_sysfs, out_pmrt

Maybe paint the bike-shed yellow ? (Sorry couldn't resist).

Regards,

Hans


>
>> This also folds out1 and out2 into one pm_runtime_disable step,
>> if pm_runtime_get_sync fails we still need to do the put, it
>> failing means that the device failed to resume, but the refcount
>> will have been incremented and we need to decrement it.
>>
>> Cc: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>>  drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 351e020..5e3da66 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client *client,
>>         pm_runtime_set_autosuspend_delay(dev, 600);
>>         ret = pm_runtime_get_sync(dev);
>>         if (ret < 0)
>> -               goto out1;
>> +               goto pm_runtime_disable;
>>
>>         ret = bq24190_hw_init(bdi);
>>         if (ret < 0) {
>>                 dev_err(dev, "Hardware init failed\n");
>> -               goto out2;
>> +               goto pm_runtime_disable;
>>         }
>>
>>         charger_cfg.drv_data = bdi;
>> @@ -1424,7 +1424,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 pm_runtime_disable;
>>         }
>>
>>         battery_cfg.drv_data = bdi;
>> @@ -1433,13 +1433,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 unregister_charger;
>>         }
>>
>>         ret = bq24190_sysfs_create_group(bdi);
>>         if (ret) {
>>                 dev_err(dev, "Can't create sysfs entries\n");
>> -               goto out4;
>> +               goto unregister_battery;
>>         }
>>
>>         bdi->initialized = true;
>> @@ -1450,7 +1450,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 remove_sysfs_group;
>>         }
>>
>>         if (bdi->extcon) {
>> @@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
>>                 ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>>                                                     &bdi->extcon_nb);
>>                 if (ret)
>> -                       goto out5;
>> +                       goto remove_sysfs_group;
>>
>>                 /* Sync initial cable state */
>>                 queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>> @@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client *client,
>>
>>         return 0;
>>
>> -out5:
>> +remove_sysfs_group:
>>         bq24190_sysfs_remove_group(bdi);
>>
>> -out4:
>> +unregister_battery:
>>         power_supply_unregister(bdi->battery);
>>
>> -out3:
>> +unregister_charger:
>>         power_supply_unregister(bdi->charger);
>>
>> -out2:
>> +pm_runtime_disable:
>>         pm_runtime_put_sync(dev);
>> -
>> -out1:
>>         pm_runtime_dont_use_autosuspend(dev);
>>         pm_runtime_disable(dev);
>>         return ret;
>> --
>> 2.9.3
>>

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Liam Breck]

On Thu, Mar 23, 2017 at 1:37 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 22-03-17 20:09, Liam Breck wrote:
>>
>> On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Names like out1, out2, etc. do not make it easier to follow what is
>>> going on and make it harder (require renaming) if any steps are
>>> later added / removed. Rename the labels to sane names.
>>
>>
>> Good idea. But the new labels look like function names
>
>
> labels named like this are used all throughout the kernel,
> see for example drivers/power/supply/pm2301_charger.c,
> drivers/power/supply/ab8500_*.c
>
>> so maybe: out_charger, out_battery, out_sysfs, out_pmrt
>
> Maybe paint the bike-shed yellow ? (Sorry couldn't resist).

Well the convention for one goto is "out", so it's sensible to use
out_whatever for multiple cases. Conventions and names matter, so
could you adjust these?


>
>
>
>>
>>> This also folds out1 and out2 into one pm_runtime_disable step,
>>> if pm_runtime_get_sync fails we still need to do the put, it
>>> failing means that the device failed to resume, but the refcount
>>> will have been incremented and we need to decrement it.
>>>
>>> Cc: Liam Breck <kernel@networkimprov.net>
>>> Cc: Tony Lindgren <tony@atomide.com>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>> Changes in v2:
>>> -This is a new patch in v2 of this patch-set
>>> ---
>>>  drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
>>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index 351e020..5e3da66 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client
>>> *client,
>>>         pm_runtime_set_autosuspend_delay(dev, 600);
>>>         ret = pm_runtime_get_sync(dev);
>>>         if (ret < 0)
>>> -               goto out1;
>>> +               goto pm_runtime_disable;
>>>
>>>         ret = bq24190_hw_init(bdi);
>>>         if (ret < 0) {
>>>                 dev_err(dev, "Hardware init failed\n");
>>> -               goto out2;
>>> +               goto pm_runtime_disable;
>>>         }
>>>
>>>         charger_cfg.drv_data = bdi;
>>> @@ -1424,7 +1424,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 pm_runtime_disable;
>>>         }
>>>
>>>         battery_cfg.drv_data = bdi;
>>> @@ -1433,13 +1433,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 unregister_charger;
>>>         }
>>>
>>>         ret = bq24190_sysfs_create_group(bdi);
>>>         if (ret) {
>>>                 dev_err(dev, "Can't create sysfs entries\n");
>>> -               goto out4;
>>> +               goto unregister_battery;
>>>         }
>>>
>>>         bdi->initialized = true;
>>> @@ -1450,7 +1450,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 remove_sysfs_group;
>>>         }
>>>
>>>         if (bdi->extcon) {
>>> @@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
>>>                 ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>>>                                                     &bdi->extcon_nb);
>>>                 if (ret)
>>> -                       goto out5;
>>> +                       goto remove_sysfs_group;
>>>
>>>                 /* Sync initial cable state */
>>>                 queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>>> @@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client
>>> *client,
>>>
>>>         return 0;
>>>
>>> -out5:
>>> +remove_sysfs_group:
>>>         bq24190_sysfs_remove_group(bdi);
>>>
>>> -out4:
>>> +unregister_battery:
>>>         power_supply_unregister(bdi->battery);
>>>
>>> -out3:
>>> +unregister_charger:
>>>         power_supply_unregister(bdi->charger);
>>>
>>> -out2:
>>> +pm_runtime_disable:
>>>         pm_runtime_put_sync(dev);
>>> -
>>> -out1:
>>>         pm_runtime_dont_use_autosuspend(dev);
>>>         pm_runtime_disable(dev);
>>>         return ret;
>>> --
>>> 2.9.3
>>>
>

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Sebastian Reichel]

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

Hi,

On Wed, Mar 22, 2017 at 03:55:35PM +0100, Hans de Goede wrote:
> Names like out1, out2, etc. do not make it easier to follow what is
> going on and make it harder (require renaming) if any steps are
> later added / removed. Rename the labels to sane names.
> 
> This also folds out1 and out2 into one pm_runtime_disable step,
> if pm_runtime_get_sync fails we still need to do the put, it
> failing means that the device failed to resume, but the refcount
> will have been incremented and we need to decrement it.
> 
> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---

I tried to queue this, but it does not apply without the reset
patch.

-- Sebastian

>  drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 351e020..5e3da66 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client *client,
>  	pm_runtime_set_autosuspend_delay(dev, 600);
>  	ret = pm_runtime_get_sync(dev);
>  	if (ret < 0)
> -		goto out1;
> +		goto pm_runtime_disable;
>  
>  	ret = bq24190_hw_init(bdi);
>  	if (ret < 0) {
>  		dev_err(dev, "Hardware init failed\n");
> -		goto out2;
> +		goto pm_runtime_disable;
>  	}
>  
>  	charger_cfg.drv_data = bdi;
> @@ -1424,7 +1424,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 pm_runtime_disable;
>  	}
>  
>  	battery_cfg.drv_data = bdi;
> @@ -1433,13 +1433,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 unregister_charger;
>  	}
>  
>  	ret = bq24190_sysfs_create_group(bdi);
>  	if (ret) {
>  		dev_err(dev, "Can't create sysfs entries\n");
> -		goto out4;
> +		goto unregister_battery;
>  	}
>  
>  	bdi->initialized = true;
> @@ -1450,7 +1450,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 remove_sysfs_group;
>  	}
>  
>  	if (bdi->extcon) {
> @@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
>  		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>  						    &bdi->extcon_nb);
>  		if (ret)
> -			goto out5;
> +			goto remove_sysfs_group;
>  
>  		/* Sync initial cable state */
>  		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
> @@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> -out5:
> +remove_sysfs_group:
>  	bq24190_sysfs_remove_group(bdi);
>  
> -out4:
> +unregister_battery:
>  	power_supply_unregister(bdi->battery);
>  
> -out3:
> +unregister_charger:
>  	power_supply_unregister(bdi->charger);
>  
> -out2:
> +pm_runtime_disable:
>  	pm_runtime_put_sync(dev);
> -
> -out1:
>  	pm_runtime_dont_use_autosuspend(dev);
>  	pm_runtime_disable(dev);
>  	return ret;
> -- 
> 2.9.3
> 

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

Re: [PATCH v2 6/7] power: supply: bq24190_charger: Cleanup error-exit labels in probe()

[Hans de Goede]

Hi,

On 23-03-17 12:21, Sebastian Reichel wrote:
> Hi,
>
> On Wed, Mar 22, 2017 at 03:55:35PM +0100, Hans de Goede wrote:
>> Names like out1, out2, etc. do not make it easier to follow what is
>> going on and make it harder (require renaming) if any steps are
>> later added / removed. Rename the labels to sane names.
>>
>> This also folds out1 and out2 into one pm_runtime_disable step,
>> if pm_runtime_get_sync fails we still need to do the put, it
>> failing means that the device failed to resume, but the refcount
>> will have been incremented and we need to decrement it.
>>
>> Cc: Liam Breck <kernel@networkimprov.net>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -This is a new patch in v2 of this patch-set
>> ---
>
> I tried to queue this, but it does not apply without the reset
> patch.

Thank you for queuing the others, this is mainly a preparation
patch to make the patch removing the battery interface cleaner,
so merging this can wait till some form of the reset patch
is merged.

Regards,

Hans



>
> -- Sebastian
>
>>  drivers/power/supply/bq24190_charger.c | 24 +++++++++++-------------
>>  1 file changed, 11 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index 351e020..5e3da66 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1408,12 +1408,12 @@ static int bq24190_probe(struct i2c_client *client,
>>  	pm_runtime_set_autosuspend_delay(dev, 600);
>>  	ret = pm_runtime_get_sync(dev);
>>  	if (ret < 0)
>> -		goto out1;
>> +		goto pm_runtime_disable;
>>
>>  	ret = bq24190_hw_init(bdi);
>>  	if (ret < 0) {
>>  		dev_err(dev, "Hardware init failed\n");
>> -		goto out2;
>> +		goto pm_runtime_disable;
>>  	}
>>
>>  	charger_cfg.drv_data = bdi;
>> @@ -1424,7 +1424,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 pm_runtime_disable;
>>  	}
>>
>>  	battery_cfg.drv_data = bdi;
>> @@ -1433,13 +1433,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 unregister_charger;
>>  	}
>>
>>  	ret = bq24190_sysfs_create_group(bdi);
>>  	if (ret) {
>>  		dev_err(dev, "Can't create sysfs entries\n");
>> -		goto out4;
>> +		goto unregister_battery;
>>  	}
>>
>>  	bdi->initialized = true;
>> @@ -1450,7 +1450,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 remove_sysfs_group;
>>  	}
>>
>>  	if (bdi->extcon) {
>> @@ -1459,7 +1459,7 @@ static int bq24190_probe(struct i2c_client *client,
>>  		ret = devm_extcon_register_notifier(dev, bdi->extcon, -1,
>>  						    &bdi->extcon_nb);
>>  		if (ret)
>> -			goto out5;
>> +			goto remove_sysfs_group;
>>
>>  		/* Sync initial cable state */
>>  		queue_delayed_work(system_wq, &bdi->extcon_work, 0);
>> @@ -1472,19 +1472,17 @@ static int bq24190_probe(struct i2c_client *client,
>>
>>  	return 0;
>>
>> -out5:
>> +remove_sysfs_group:
>>  	bq24190_sysfs_remove_group(bdi);
>>
>> -out4:
>> +unregister_battery:
>>  	power_supply_unregister(bdi->battery);
>>
>> -out3:
>> +unregister_charger:
>>  	power_supply_unregister(bdi->charger);
>>
>> -out2:
>> +pm_runtime_disable:
>>  	pm_runtime_put_sync(dev);
>> -
>> -out1:
>>  	pm_runtime_dont_use_autosuspend(dev);
>>  	pm_runtime_disable(dev);
>>  	return ret;
>> --
>> 2.9.3
>>

[PATCH v2 7/7] power: supply: bq24190_charger: Remove battery power_supply device

[Hans de Goede]

The TI bq24190 chip is purely a charger chip, as such it has very
little battery info to export to userspace and in practice it is almost
always used together with a separate fuel-gauge chip which does have
interesting info to export.

Since the userspace ABI requires there to be only 1 power_supply device
registered per physical battery, the limited battery power_supply device
the bq24190_charger driver is registering is actually getting in the way
of a fuel-gauge driver exporting more useful info, therefor this commit
removes the battery power_supply device.

Cc: Liam Breck <kernel@networkimprov.net>
Cc: Tony Lindgren <tony@atomide.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-This is a new patch in v2 of this patch-set
---
 drivers/power/supply/bq24190_charger.c | 290 +--------------------------------
 1 file changed, 5 insertions(+), 285 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 5e3da66..52d8756 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -151,7 +151,6 @@ struct bq24190_dev_info {
 	struct i2c_client		*client;
 	struct device			*dev;
 	struct power_supply		*charger;
-	struct power_supply		*battery;
 	struct extcon_dev		*extcon;
 	struct notifier_block		extcon_nb;
 	struct delayed_work		extcon_work;
@@ -910,266 +909,9 @@ static const struct power_supply_desc bq24190_charger_desc = {
 	.property_is_writeable	= bq24190_charger_property_is_writeable,
 };
 
-/* Battery power supply property routines */
-
-static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
-		union power_supply_propval *val)
-{
-	u8 ss_reg, chrg_fault;
-	int status, ret;
-
-	mutex_lock(&bdi->f_reg_lock);
-	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;
-
-	ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
-	if (ret < 0)
-		return ret;
-
-	/*
-	 * The battery must be discharging when any of these are true:
-	 * - there is no good power source;
-	 * - there is a charge fault.
-	 * Could also be discharging when in "supplement mode" but
-	 * there is no way to tell when its in that mode.
-	 */
-	if (!(ss_reg & BQ24190_REG_SS_PG_STAT_MASK) || chrg_fault) {
-		status = POWER_SUPPLY_STATUS_DISCHARGING;
-	} else {
-		ss_reg &= BQ24190_REG_SS_CHRG_STAT_MASK;
-		ss_reg >>= BQ24190_REG_SS_CHRG_STAT_SHIFT;
-
-		switch (ss_reg) {
-		case 0x0: /* Not Charging */
-			status = POWER_SUPPLY_STATUS_NOT_CHARGING;
-			break;
-		case 0x1: /* Pre-charge */
-		case 0x2: /* Fast Charging */
-			status = POWER_SUPPLY_STATUS_CHARGING;
-			break;
-		case 0x3: /* Charge Termination Done */
-			status = POWER_SUPPLY_STATUS_FULL;
-			break;
-		default:
-			ret = -EIO;
-		}
-	}
-
-	if (!ret)
-		val->intval = status;
-
-	return ret;
-}
-
-static int bq24190_battery_get_health(struct bq24190_dev_info *bdi,
-		union power_supply_propval *val)
-{
-	u8 v;
-	int health;
-
-	mutex_lock(&bdi->f_reg_lock);
-	v = bdi->f_reg;
-	mutex_unlock(&bdi->f_reg_lock);
-
-	if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
-		health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
-	} else {
-		v &= BQ24190_REG_F_NTC_FAULT_MASK;
-		v >>= BQ24190_REG_F_NTC_FAULT_SHIFT;
-
-		switch (v) {
-		case 0x0: /* Normal */
-			health = POWER_SUPPLY_HEALTH_GOOD;
-			break;
-		case 0x1: /* TS1 Cold */
-		case 0x3: /* TS2 Cold */
-		case 0x5: /* Both Cold */
-			health = POWER_SUPPLY_HEALTH_COLD;
-			break;
-		case 0x2: /* TS1 Hot */
-		case 0x4: /* TS2 Hot */
-		case 0x6: /* Both Hot */
-			health = POWER_SUPPLY_HEALTH_OVERHEAT;
-			break;
-		default:
-			health = POWER_SUPPLY_HEALTH_UNKNOWN;
-		}
-	}
-
-	val->intval = health;
-	return 0;
-}
-
-static int bq24190_battery_get_online(struct bq24190_dev_info *bdi,
-		union power_supply_propval *val)
-{
-	u8 batfet_disable;
-	int ret;
-
-	ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
-			BQ24190_REG_MOC_BATFET_DISABLE_MASK,
-			BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, &batfet_disable);
-	if (ret < 0)
-		return ret;
-
-	val->intval = !batfet_disable;
-	return 0;
-}
-
-static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
-		const union power_supply_propval *val)
-{
-	return bq24190_write_mask(bdi, BQ24190_REG_MOC,
-			BQ24190_REG_MOC_BATFET_DISABLE_MASK,
-			BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, !val->intval);
-}
-
-static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info *bdi,
-		union power_supply_propval *val)
-{
-	int temp, ret;
-
-	ret = bq24190_get_field_val(bdi, BQ24190_REG_ICTRC,
-			BQ24190_REG_ICTRC_TREG_MASK,
-			BQ24190_REG_ICTRC_TREG_SHIFT,
-			bq24190_ictrc_treg_values,
-			ARRAY_SIZE(bq24190_ictrc_treg_values), &temp);
-	if (ret < 0)
-		return ret;
-
-	val->intval = temp;
-	return 0;
-}
-
-static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info *bdi,
-		const union power_supply_propval *val)
-{
-	return bq24190_set_field_val(bdi, BQ24190_REG_ICTRC,
-			BQ24190_REG_ICTRC_TREG_MASK,
-			BQ24190_REG_ICTRC_TREG_SHIFT,
-			bq24190_ictrc_treg_values,
-			ARRAY_SIZE(bq24190_ictrc_treg_values), val->intval);
-}
-
-static int bq24190_battery_get_property(struct power_supply *psy,
-		enum power_supply_property psp, union power_supply_propval *val)
-{
-	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
-	int ret;
-
-	dev_dbg(bdi->dev, "prop: %d\n", psp);
-
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0)
-		return ret;
-
-	switch (psp) {
-	case POWER_SUPPLY_PROP_STATUS:
-		ret = bq24190_battery_get_status(bdi, val);
-		break;
-	case POWER_SUPPLY_PROP_HEALTH:
-		ret = bq24190_battery_get_health(bdi, val);
-		break;
-	case POWER_SUPPLY_PROP_ONLINE:
-		ret = bq24190_battery_get_online(bdi, val);
-		break;
-	case POWER_SUPPLY_PROP_TECHNOLOGY:
-		/* Could be Li-on or Li-polymer but no way to tell which */
-		val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
-		ret = 0;
-		break;
-	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
-		ret = bq24190_battery_get_temp_alert_max(bdi, val);
-		break;
-	case POWER_SUPPLY_PROP_SCOPE:
-		val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
-		ret = 0;
-		break;
-	default:
-		ret = -ENODATA;
-	}
-
-	pm_runtime_mark_last_busy(bdi->dev);
-	pm_runtime_put_autosuspend(bdi->dev);
-
-	return ret;
-}
-
-static int bq24190_battery_set_property(struct power_supply *psy,
-		enum power_supply_property psp,
-		const union power_supply_propval *val)
-{
-	struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
-	int ret;
-
-	dev_dbg(bdi->dev, "prop: %d\n", psp);
-
-	ret = pm_runtime_get_sync(bdi->dev);
-	if (ret < 0)
-		return ret;
-
-	switch (psp) {
-	case POWER_SUPPLY_PROP_ONLINE:
-		ret = bq24190_battery_set_online(bdi, val);
-		break;
-	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
-		ret = bq24190_battery_set_temp_alert_max(bdi, val);
-		break;
-	default:
-		ret = -EINVAL;
-	}
-
-	pm_runtime_mark_last_busy(bdi->dev);
-	pm_runtime_put_autosuspend(bdi->dev);
-
-	return ret;
-}
-
-static int bq24190_battery_property_is_writeable(struct power_supply *psy,
-		enum power_supply_property psp)
-{
-	int ret;
-
-	switch (psp) {
-	case POWER_SUPPLY_PROP_ONLINE:
-	case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
-		ret = 1;
-		break;
-	default:
-		ret = 0;
-	}
-
-	return ret;
-}
-
-static enum power_supply_property bq24190_battery_properties[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_ONLINE,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
-	POWER_SUPPLY_PROP_SCOPE,
-};
-
-static const struct power_supply_desc bq24190_battery_desc = {
-	.name			= "bq24190-battery",
-	.type			= POWER_SUPPLY_TYPE_BATTERY,
-	.properties		= bq24190_battery_properties,
-	.num_properties		= ARRAY_SIZE(bq24190_battery_properties),
-	.get_property		= bq24190_battery_get_property,
-	.set_property		= bq24190_battery_set_property,
-	.property_is_writeable	= bq24190_battery_property_is_writeable,
-};
-
 static void bq24190_check_status(struct bq24190_dev_info *bdi)
 {
-	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;
+	bool alert_charger = false;
 	u8 ss_reg = 0, f_reg = 0;
 	int i, ret;
 
@@ -1202,12 +944,9 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 				!!(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;
 		mutex_unlock(&bdi->f_reg_lock);
+		alert_charger = true;
 	}
 
 	if (ss_reg != bdi->ss_reg) {
@@ -1226,17 +965,12 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 					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_charger = true;
 	}
 
 	if (alert_charger)
 		power_supply_changed(bdi->charger);
-	if (alert_battery)
-		power_supply_changed(bdi->battery);
 
 	dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
 }
@@ -1363,7 +1097,7 @@ static int bq24190_probe(struct i2c_client *client,
 {
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct device *dev = &client->dev;
-	struct power_supply_config charger_cfg = {}, battery_cfg = {};
+	struct power_supply_config charger_cfg = {};
 	struct bq24190_dev_info *bdi;
 	const char *name;
 	int ret;
@@ -1427,19 +1161,10 @@ static int bq24190_probe(struct i2c_client *client,
 		goto pm_runtime_disable;
 	}
 
-	battery_cfg.drv_data = bdi;
-	bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
-						&battery_cfg);
-	if (IS_ERR(bdi->battery)) {
-		dev_err(dev, "Can't register battery\n");
-		ret = PTR_ERR(bdi->battery);
-		goto unregister_charger;
-	}
-
 	ret = bq24190_sysfs_create_group(bdi);
 	if (ret) {
 		dev_err(dev, "Can't create sysfs entries\n");
-		goto unregister_battery;
+		goto unregister_charger;
 	}
 
 	bdi->initialized = true;
@@ -1475,9 +1200,6 @@ static int bq24190_probe(struct i2c_client *client,
 remove_sysfs_group:
 	bq24190_sysfs_remove_group(bdi);
 
-unregister_battery:
-	power_supply_unregister(bdi->battery);
-
 unregister_charger:
 	power_supply_unregister(bdi->charger);
 
@@ -1500,7 +1222,6 @@ static int bq24190_remove(struct i2c_client *client)
 	}
 
 	bq24190_sysfs_remove_group(bdi);
-	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
 	if (error >= 0)
 		pm_runtime_put_sync(bdi->dev);
@@ -1568,7 +1289,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
 
 	/* Things may have changed while suspended so alert upper layer */
 	power_supply_changed(bdi->charger);
-	power_supply_changed(bdi->battery);
 
 	return 0;
 }
-- 
2.9.3

Re: [PATCH v2 7/7] power: supply: bq24190_charger: Remove battery power_supply device

[Liam Breck]

On Wed, Mar 22, 2017 at 7:55 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> The TI bq24190 chip is purely a charger chip, as such it has very
> little battery info to export to userspace and in practice it is almost
> always used together with a separate fuel-gauge chip which does have
> interesting info to export.
>
> Since the userspace ABI requires there to be only 1 power_supply device
> registered per physical battery, the limited battery power_supply device
> the bq24190_charger driver is registering is actually getting in the way
> of a fuel-gauge driver exporting more useful info, therefor this commit
> removes the battery power_supply device.

As discussed on the thread for my bq24190 patchset, this is not what's required.

I have a specific plan to move -battery properties to -charger, which
I will post when I get a chance.

Meanwhile...

> Cc: Liam Breck <kernel@networkimprov.net>
> Cc: Tony Lindgren <tony@atomide.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -This is a new patch in v2 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 290 +--------------------------------
>  1 file changed, 5 insertions(+), 285 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 5e3da66..52d8756 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -151,7 +151,6 @@ struct bq24190_dev_info {
>         struct i2c_client               *client;
>         struct device                   *dev;
>         struct power_supply             *charger;
> -       struct power_supply             *battery;
>         struct extcon_dev               *extcon;
>         struct notifier_block           extcon_nb;
>         struct delayed_work             extcon_work;
> @@ -910,266 +909,9 @@ static const struct power_supply_desc bq24190_charger_desc = {
>         .property_is_writeable  = bq24190_charger_property_is_writeable,
>  };
>
> -/* Battery power supply property routines */
> -
> -static int bq24190_battery_get_status(struct bq24190_dev_info *bdi,
> -               union power_supply_propval *val)
> -{
> -       u8 ss_reg, chrg_fault;
> -       int status, ret;
> -
> -       mutex_lock(&bdi->f_reg_lock);
> -       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;
> -
> -       ret = bq24190_read(bdi, BQ24190_REG_SS, &ss_reg);
> -       if (ret < 0)
> -               return ret;
> -
> -       /*
> -        * The battery must be discharging when any of these are true:
> -        * - there is no good power source;
> -        * - there is a charge fault.
> -        * Could also be discharging when in "supplement mode" but
> -        * there is no way to tell when its in that mode.
> -        */
> -       if (!(ss_reg & BQ24190_REG_SS_PG_STAT_MASK) || chrg_fault) {
> -               status = POWER_SUPPLY_STATUS_DISCHARGING;
> -       } else {
> -               ss_reg &= BQ24190_REG_SS_CHRG_STAT_MASK;
> -               ss_reg >>= BQ24190_REG_SS_CHRG_STAT_SHIFT;
> -
> -               switch (ss_reg) {
> -               case 0x0: /* Not Charging */
> -                       status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> -                       break;
> -               case 0x1: /* Pre-charge */
> -               case 0x2: /* Fast Charging */
> -                       status = POWER_SUPPLY_STATUS_CHARGING;
> -                       break;
> -               case 0x3: /* Charge Termination Done */
> -                       status = POWER_SUPPLY_STATUS_FULL;
> -                       break;
> -               default:
> -                       ret = -EIO;
> -               }
> -       }
> -
> -       if (!ret)
> -               val->intval = status;
> -
> -       return ret;
> -}
> -
> -static int bq24190_battery_get_health(struct bq24190_dev_info *bdi,
> -               union power_supply_propval *val)
> -{
> -       u8 v;
> -       int health;
> -
> -       mutex_lock(&bdi->f_reg_lock);
> -       v = bdi->f_reg;
> -       mutex_unlock(&bdi->f_reg_lock);
> -
> -       if (v & BQ24190_REG_F_BAT_FAULT_MASK) {
> -               health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> -       } else {
> -               v &= BQ24190_REG_F_NTC_FAULT_MASK;
> -               v >>= BQ24190_REG_F_NTC_FAULT_SHIFT;
> -
> -               switch (v) {
> -               case 0x0: /* Normal */
> -                       health = POWER_SUPPLY_HEALTH_GOOD;
> -                       break;
> -               case 0x1: /* TS1 Cold */
> -               case 0x3: /* TS2 Cold */
> -               case 0x5: /* Both Cold */
> -                       health = POWER_SUPPLY_HEALTH_COLD;
> -                       break;
> -               case 0x2: /* TS1 Hot */
> -               case 0x4: /* TS2 Hot */
> -               case 0x6: /* Both Hot */
> -                       health = POWER_SUPPLY_HEALTH_OVERHEAT;
> -                       break;
> -               default:
> -                       health = POWER_SUPPLY_HEALTH_UNKNOWN;
> -               }
> -       }
> -
> -       val->intval = health;
> -       return 0;
> -}
> -
> -static int bq24190_battery_get_online(struct bq24190_dev_info *bdi,
> -               union power_supply_propval *val)
> -{
> -       u8 batfet_disable;
> -       int ret;
> -
> -       ret = bq24190_read_mask(bdi, BQ24190_REG_MOC,
> -                       BQ24190_REG_MOC_BATFET_DISABLE_MASK,
> -                       BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, &batfet_disable);
> -       if (ret < 0)
> -               return ret;
> -
> -       val->intval = !batfet_disable;
> -       return 0;
> -}
> -
> -static int bq24190_battery_set_online(struct bq24190_dev_info *bdi,
> -               const union power_supply_propval *val)
> -{
> -       return bq24190_write_mask(bdi, BQ24190_REG_MOC,
> -                       BQ24190_REG_MOC_BATFET_DISABLE_MASK,
> -                       BQ24190_REG_MOC_BATFET_DISABLE_SHIFT, !val->intval);
> -}
> -
> -static int bq24190_battery_get_temp_alert_max(struct bq24190_dev_info *bdi,
> -               union power_supply_propval *val)
> -{
> -       int temp, ret;
> -
> -       ret = bq24190_get_field_val(bdi, BQ24190_REG_ICTRC,
> -                       BQ24190_REG_ICTRC_TREG_MASK,
> -                       BQ24190_REG_ICTRC_TREG_SHIFT,
> -                       bq24190_ictrc_treg_values,
> -                       ARRAY_SIZE(bq24190_ictrc_treg_values), &temp);
> -       if (ret < 0)
> -               return ret;
> -
> -       val->intval = temp;
> -       return 0;
> -}
> -
> -static int bq24190_battery_set_temp_alert_max(struct bq24190_dev_info *bdi,
> -               const union power_supply_propval *val)
> -{
> -       return bq24190_set_field_val(bdi, BQ24190_REG_ICTRC,
> -                       BQ24190_REG_ICTRC_TREG_MASK,
> -                       BQ24190_REG_ICTRC_TREG_SHIFT,
> -                       bq24190_ictrc_treg_values,
> -                       ARRAY_SIZE(bq24190_ictrc_treg_values), val->intval);
> -}
> -
> -static int bq24190_battery_get_property(struct power_supply *psy,
> -               enum power_supply_property psp, union power_supply_propval *val)
> -{
> -       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> -       int ret;
> -
> -       dev_dbg(bdi->dev, "prop: %d\n", psp);
> -
> -       ret = pm_runtime_get_sync(bdi->dev);
> -       if (ret < 0)
> -               return ret;
> -
> -       switch (psp) {
> -       case POWER_SUPPLY_PROP_STATUS:
> -               ret = bq24190_battery_get_status(bdi, val);
> -               break;
> -       case POWER_SUPPLY_PROP_HEALTH:
> -               ret = bq24190_battery_get_health(bdi, val);
> -               break;
> -       case POWER_SUPPLY_PROP_ONLINE:
> -               ret = bq24190_battery_get_online(bdi, val);
> -               break;
> -       case POWER_SUPPLY_PROP_TECHNOLOGY:
> -               /* Could be Li-on or Li-polymer but no way to tell which */
> -               val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN;
> -               ret = 0;
> -               break;
> -       case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> -               ret = bq24190_battery_get_temp_alert_max(bdi, val);
> -               break;
> -       case POWER_SUPPLY_PROP_SCOPE:
> -               val->intval = POWER_SUPPLY_SCOPE_SYSTEM;
> -               ret = 0;
> -               break;
> -       default:
> -               ret = -ENODATA;
> -       }
> -
> -       pm_runtime_mark_last_busy(bdi->dev);
> -       pm_runtime_put_autosuspend(bdi->dev);
> -
> -       return ret;
> -}
> -
> -static int bq24190_battery_set_property(struct power_supply *psy,
> -               enum power_supply_property psp,
> -               const union power_supply_propval *val)
> -{
> -       struct bq24190_dev_info *bdi = power_supply_get_drvdata(psy);
> -       int ret;
> -
> -       dev_dbg(bdi->dev, "prop: %d\n", psp);
> -
> -       ret = pm_runtime_get_sync(bdi->dev);
> -       if (ret < 0)
> -               return ret;
> -
> -       switch (psp) {
> -       case POWER_SUPPLY_PROP_ONLINE:
> -               ret = bq24190_battery_set_online(bdi, val);
> -               break;
> -       case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> -               ret = bq24190_battery_set_temp_alert_max(bdi, val);
> -               break;
> -       default:
> -               ret = -EINVAL;
> -       }
> -
> -       pm_runtime_mark_last_busy(bdi->dev);
> -       pm_runtime_put_autosuspend(bdi->dev);
> -
> -       return ret;
> -}
> -
> -static int bq24190_battery_property_is_writeable(struct power_supply *psy,
> -               enum power_supply_property psp)
> -{
> -       int ret;
> -
> -       switch (psp) {
> -       case POWER_SUPPLY_PROP_ONLINE:
> -       case POWER_SUPPLY_PROP_TEMP_ALERT_MAX:
> -               ret = 1;
> -               break;
> -       default:
> -               ret = 0;
> -       }
> -
> -       return ret;
> -}
> -
> -static enum power_supply_property bq24190_battery_properties[] = {
> -       POWER_SUPPLY_PROP_STATUS,
> -       POWER_SUPPLY_PROP_HEALTH,
> -       POWER_SUPPLY_PROP_ONLINE,
> -       POWER_SUPPLY_PROP_TECHNOLOGY,
> -       POWER_SUPPLY_PROP_TEMP_ALERT_MAX,
> -       POWER_SUPPLY_PROP_SCOPE,
> -};
> -
> -static const struct power_supply_desc bq24190_battery_desc = {
> -       .name                   = "bq24190-battery",
> -       .type                   = POWER_SUPPLY_TYPE_BATTERY,

...use _TYPE_UNKNOWN above in your fuel gauge branch so that this
sysfs device doesn't conflict with the gauge's _TYPE_BATTERY sysfs
device.


> -       .properties             = bq24190_battery_properties,
> -       .num_properties         = ARRAY_SIZE(bq24190_battery_properties),
> -       .get_property           = bq24190_battery_get_property,
> -       .set_property           = bq24190_battery_set_property,
> -       .property_is_writeable  = bq24190_battery_property_is_writeable,
> -};
> -
>  static void bq24190_check_status(struct bq24190_dev_info *bdi)
>  {
> -       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;
> +       bool alert_charger = false;
>         u8 ss_reg = 0, f_reg = 0;
>         int i, ret;
>
> @@ -1202,12 +944,9 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>                                 !!(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;
>                 mutex_unlock(&bdi->f_reg_lock);
> +               alert_charger = true;
>         }
>
>         if (ss_reg != bdi->ss_reg) {
> @@ -1226,17 +965,12 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>                                         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_charger = true;
>         }
>
>         if (alert_charger)
>                 power_supply_changed(bdi->charger);
> -       if (alert_battery)
> -               power_supply_changed(bdi->battery);
>
>         dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
>  }
> @@ -1363,7 +1097,7 @@ static int bq24190_probe(struct i2c_client *client,
>  {
>         struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>         struct device *dev = &client->dev;
> -       struct power_supply_config charger_cfg = {}, battery_cfg = {};
> +       struct power_supply_config charger_cfg = {};
>         struct bq24190_dev_info *bdi;
>         const char *name;
>         int ret;
> @@ -1427,19 +1161,10 @@ static int bq24190_probe(struct i2c_client *client,
>                 goto pm_runtime_disable;
>         }
>
> -       battery_cfg.drv_data = bdi;
> -       bdi->battery = power_supply_register(dev, &bq24190_battery_desc,
> -                                               &battery_cfg);
> -       if (IS_ERR(bdi->battery)) {
> -               dev_err(dev, "Can't register battery\n");
> -               ret = PTR_ERR(bdi->battery);
> -               goto unregister_charger;
> -       }
> -
>         ret = bq24190_sysfs_create_group(bdi);
>         if (ret) {
>                 dev_err(dev, "Can't create sysfs entries\n");
> -               goto unregister_battery;
> +               goto unregister_charger;
>         }
>
>         bdi->initialized = true;
> @@ -1475,9 +1200,6 @@ static int bq24190_probe(struct i2c_client *client,
>  remove_sysfs_group:
>         bq24190_sysfs_remove_group(bdi);
>
> -unregister_battery:
> -       power_supply_unregister(bdi->battery);
> -
>  unregister_charger:
>         power_supply_unregister(bdi->charger);
>
> @@ -1500,7 +1222,6 @@ static int bq24190_remove(struct i2c_client *client)
>         }
>
>         bq24190_sysfs_remove_group(bdi);
> -       power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
>         if (error >= 0)
>                 pm_runtime_put_sync(bdi->dev);
> @@ -1568,7 +1289,6 @@ static __maybe_unused int bq24190_pm_resume(struct device *dev)
>
>         /* Things may have changed while suspended so alert upper layer */
>         power_supply_changed(bdi->charger);
> -       power_supply_changed(bdi->battery);
>
>         return 0;
>  }
> --
> 2.9.3
>