[PATCH v4 0/3] power: supply: bq24190_charger: Pending patches

[PATCH v4 0/3] power: supply: bq24190_charger: Pending patches

[Hans de Goede]

Hi Sebastian,

Here is a resend of my pending patches rebased on top of the fixes
from Liam you merged earlier today.

The first patch is new and is a bug-fix for the extcon support, this
relies on an extcon-core patch. The extcon maintainer has created
an inmutable branch for this which you can merge before merging
this patch:

git://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git ib-extcon-4.12

Regards,

Hans

[PATCH v4 1/3] power: supply: bq24190_charger: Use new extcon_register_notifier_all()

[Hans de Goede]

When I submitted the extcon handling I had a patch pending for the
extcon sub-system for extcon_register_notifier to take -1 as cable id
for listening for all type cable events on an extcon with a single
notifier.

In the end it was decided to instead add a new
extcon_register_notifier_all function for this, switch to using this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v4:
-This is a new patch in v4 of this patch-set
---
 drivers/power/supply/bq24190_charger.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index 7c893c0..bd9e5c3 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1502,8 +1502,8 @@ static int bq24190_probe(struct i2c_client *client,
 	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);
+		ret = devm_extcon_register_notifier_all(dev, bdi->extcon,
+							&bdi->extcon_nb);
 		if (ret) {
 			dev_err(dev, "Can't register extcon\n");
 			goto out_sysfs;
-- 
2.9.3

[PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

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

For all the above reasons this commit disables reset on probe and on
suspend / resume. If a specific setup really needs / wants to do a rest
in these cases, this can be requested by setting a reset-on-probe device
property on the 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
Changes in v3:
-Disable the reset code by default, but leave it around guarded by
 a check for a reset-on-probe device-property
Changes in v4:
-Rebase on top of current power-supply/next
---
 drivers/power/supply/bq24190_charger.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index bd9e5c3..ad429b7 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
 		return -ENODEV;
 	}
 
-	ret = bq24190_register_reset(bdi);
-	if (ret < 0)
-		return ret;
+	if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
+		ret = bq24190_register_reset(bdi);
+		if (ret < 0)
+			return ret;
+	}
 
 	ret = bq24190_set_mode_host(bdi);
 	if (ret < 0)
@@ -1547,7 +1549,8 @@ static int bq24190_remove(struct i2c_client *client)
 		pm_runtime_put_noidle(bdi->dev);
 	}
 
-	bq24190_register_reset(bdi);
+	if (device_property_read_bool(bdi->dev, "reset-on-probe"))
+		bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
 	power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
@@ -1600,7 +1603,8 @@ static __maybe_unused int bq24190_pm_suspend(struct device *dev)
 		pm_runtime_put_noidle(bdi->dev);
 	}
 
-	bq24190_register_reset(bdi);
+	if (device_property_read_bool(bdi->dev, "reset-on-probe"))
+		bq24190_register_reset(bdi);
 
 	if (error >= 0) {
 		pm_runtime_mark_last_busy(bdi->dev);
@@ -1625,8 +1629,10 @@ 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);
+	if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
+		bq24190_register_reset(bdi);
+		bq24190_set_mode_host(bdi);
+	}
 	bq24190_read(bdi, BQ24190_REG_SS, &bdi->ss_reg);
 
 	if (error >= 0) {
-- 
2.9.3

[PATCH v4 3/3] power: supply: bq24190_charger: Make battery iface registraton conditional

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

This commit adds a check for a "disable-battery-power-supply-iface"
device-property which can be set by platform-code if it knows that
there will be a fuel-gauge which will supply a battery interface.

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
Changes in v3:
-Check for "disable-battery-power-supply-iface" device-property instead of
 completely removing the battery interface
Changes in v4:
-Rebase on top of current power-supply/next
---
 drivers/power/supply/bq24190_charger.c | 41 ++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
index ad429b7..0c01f48 100644
--- a/drivers/power/supply/bq24190_charger.c
+++ b/drivers/power/supply/bq24190_charger.c
@@ -1268,8 +1268,12 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
 
 	if (alert_charger)
 		power_supply_changed(bdi->charger);
-	if (alert_battery)
-		power_supply_changed(bdi->battery);
+	if (alert_battery) {
+		if (bdi->battery)
+			power_supply_changed(bdi->battery);
+		else
+			power_supply_changed(bdi->charger);
+	}
 
 	dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
 }
@@ -1475,13 +1479,23 @@ static int bq24190_probe(struct i2c_client *client,
 		goto out_pmrt;
 	}
 
-	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 out_charger;
+	/*
+	 * The disable-battery-power-supply-iface is purely a temporary in
+	 * kernel interface for platform-code to use if it knows that there
+	 * will be a fuel-gauge which will supply a battery interface. The long
+	 * term plan is for all properties to move to the charger interface
+	 * and for the battery interface to be removed completely.
+	 */
+	if (!device_property_read_bool(bdi->dev,
+				       "disable-battery-power-supply-iface")) {
+		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 out_charger;
+		}
 	}
 
 	ret = bq24190_sysfs_create_group(bdi);
@@ -1526,7 +1540,8 @@ static int bq24190_probe(struct i2c_client *client,
 	bq24190_sysfs_remove_group(bdi);
 
 out_battery:
-	power_supply_unregister(bdi->battery);
+	if (bdi->battery)
+		power_supply_unregister(bdi->battery);
 
 out_charger:
 	power_supply_unregister(bdi->charger);
@@ -1552,7 +1567,8 @@ static int bq24190_remove(struct i2c_client *client)
 	if (device_property_read_bool(bdi->dev, "reset-on-probe"))
 		bq24190_register_reset(bdi);
 	bq24190_sysfs_remove_group(bdi);
-	power_supply_unregister(bdi->battery);
+	if (bdi->battery)
+		power_supply_unregister(bdi->battery);
 	power_supply_unregister(bdi->charger);
 	if (error >= 0)
 		pm_runtime_put_sync(bdi->dev);
@@ -1642,7 +1658,8 @@ 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);
+	if (bdi->battery)
+		power_supply_changed(bdi->battery);
 
 	return 0;
 }
-- 
2.9.3

Re: [PATCH v4 1/3] power: supply: bq24190_charger: Use new extcon_register_notifier_all()

[Liam Breck]

Hi Hans,

On Wed, Apr 12, 2017 at 11:18 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> When I submitted the extcon handling I had a patch pending for the
> extcon sub-system for extcon_register_notifier to take -1 as cable id
> for listening for all type cable events on an extcon with a single
> notifier.
>
> In the end it was decided to instead add a new
> extcon_register_notifier_all function for this, switch to using this.

Oop. Let's try to see pending dependencies land in -next before we
land clients :-)

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


> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v4:
> -This is a new patch in v4 of this patch-set
> ---
>  drivers/power/supply/bq24190_charger.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index 7c893c0..bd9e5c3 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1502,8 +1502,8 @@ static int bq24190_probe(struct i2c_client *client,
>         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);
> +               ret = devm_extcon_register_notifier_all(dev, bdi->extcon,
> +                                                       &bdi->extcon_nb);
>                 if (ret) {
>                         dev_err(dev, "Can't register extcon\n");
>                         goto out_sysfs;
> --
> 2.9.3
>

Re: [PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Liam Breck]

Hallo Hans,

On Wed, Apr 12, 2017 at 11:18 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.

Could you also mention which charger chip, board/host, and any known
hw config for the chip by the host?

Also mention whether or not there is a TI errata doc for the issue?

> For all the above reasons this commit disables reset on probe and on
> suspend / resume. If a specific setup really needs / wants to do a rest
> in these cases, this can be requested by setting a reset-on-probe device
> property on the 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
> Changes in v3:
> -Disable the reset code by default, but leave it around guarded by
>  a check for a reset-on-probe device-property
> Changes in v4:
> -Rebase on top of current power-supply/next
> ---
>  drivers/power/supply/bq24190_charger.c | 20 +++++++++++++-------
>  1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index bd9e5c3..ad429b7 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>                 return -ENODEV;
>         }
>
> -       ret = bq24190_register_reset(bdi);
> -       if (ret < 0)
> -               return ret;
> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
> +               ret = bq24190_register_reset(bdi);
> +               if (ret < 0)
> +                       return ret;
> +       }

Maybe check property and return if true in register_reset(), instead
of adjusting all the calls. There would be a set_mode_host() in
resume() which is unnecessary, but it's a no-op to set host mode when
we're already in it.

Perhaps device_property_present() since _read_bool() implies we expect
it to be set T or F?

Let's keep the original reset behavior as the default, since we only
get device props for an ACPI platform.

>         ret = bq24190_set_mode_host(bdi);
>         if (ret < 0)
> @@ -1547,7 +1549,8 @@ static int bq24190_remove(struct i2c_client *client)
>                 pm_runtime_put_noidle(bdi->dev);
>         }
>
> -       bq24190_register_reset(bdi);
> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
> +               bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
>         power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
> @@ -1600,7 +1603,8 @@ static __maybe_unused int bq24190_pm_suspend(struct device *dev)
>                 pm_runtime_put_noidle(bdi->dev);
>         }
>
> -       bq24190_register_reset(bdi);
> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
> +               bq24190_register_reset(bdi);
>
>         if (error >= 0) {
>                 pm_runtime_mark_last_busy(bdi->dev);
> @@ -1625,8 +1629,10 @@ 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);
> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
> +               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 v4 3/3] power: supply: bq24190_charger: Make battery iface registraton conditional

[Liam Breck]

Hi Hans,

On Wed, Apr 12, 2017 at 11:18 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.
>
> This commit adds a check for a "disable-battery-power-supply-iface"
> device-property which can be set by platform-code if it knows that
> there will be a fuel-gauge which will supply a battery interface.

I expect to have time to move the -battery properties to -charger this
weekend. Maybe wait for that?


> 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
> Changes in v3:
> -Check for "disable-battery-power-supply-iface" device-property instead of
>  completely removing the battery interface
> Changes in v4:
> -Rebase on top of current power-supply/next
> ---
>  drivers/power/supply/bq24190_charger.c | 41 ++++++++++++++++++++++++----------
>  1 file changed, 29 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
> index ad429b7..0c01f48 100644
> --- a/drivers/power/supply/bq24190_charger.c
> +++ b/drivers/power/supply/bq24190_charger.c
> @@ -1268,8 +1268,12 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>
>         if (alert_charger)
>                 power_supply_changed(bdi->charger);
> -       if (alert_battery)
> -               power_supply_changed(bdi->battery);
> +       if (alert_battery) {
> +               if (bdi->battery)
> +                       power_supply_changed(bdi->battery);
> +               else
> +                       power_supply_changed(bdi->charger);
> +       }
>
>         dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
>  }
> @@ -1475,13 +1479,23 @@ static int bq24190_probe(struct i2c_client *client,
>                 goto out_pmrt;
>         }
>
> -       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 out_charger;
> +       /*
> +        * The disable-battery-power-supply-iface is purely a temporary in
> +        * kernel interface for platform-code to use if it knows that there
> +        * will be a fuel-gauge which will supply a battery interface. The long
> +        * term plan is for all properties to move to the charger interface
> +        * and for the battery interface to be removed completely.
> +        */
> +       if (!device_property_read_bool(bdi->dev,
> +                                      "disable-battery-power-supply-iface")) {
> +               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 out_charger;
> +               }
>         }
>
>         ret = bq24190_sysfs_create_group(bdi);
> @@ -1526,7 +1540,8 @@ static int bq24190_probe(struct i2c_client *client,
>         bq24190_sysfs_remove_group(bdi);
>
>  out_battery:
> -       power_supply_unregister(bdi->battery);
> +       if (bdi->battery)
> +               power_supply_unregister(bdi->battery);
>
>  out_charger:
>         power_supply_unregister(bdi->charger);
> @@ -1552,7 +1567,8 @@ static int bq24190_remove(struct i2c_client *client)
>         if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>                 bq24190_register_reset(bdi);
>         bq24190_sysfs_remove_group(bdi);
> -       power_supply_unregister(bdi->battery);
> +       if (bdi->battery)
> +               power_supply_unregister(bdi->battery);
>         power_supply_unregister(bdi->charger);
>         if (error >= 0)
>                 pm_runtime_put_sync(bdi->dev);
> @@ -1642,7 +1658,8 @@ 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);
> +       if (bdi->battery)
> +               power_supply_changed(bdi->battery);
>
>         return 0;
>  }
> --
> 2.9.3
>

Re: [PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Hans de Goede]

Hi,

On 13-04-17 01:06, Liam Breck wrote:
> Hallo Hans,
>
> On Wed, Apr 12, 2017 at 11:18 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.
>
> Could you also mention which charger chip, board/host, and any known
> hw config for the chip by the host?
> Also mention whether or not there is a TI errata doc for the issue?

I will send a v5 with this both added to the commit msg.

>> For all the above reasons this commit disables reset on probe and on
>> suspend / resume. If a specific setup really needs / wants to do a rest
>> in these cases, this can be requested by setting a reset-on-probe device
>> property on the 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
>> Changes in v3:
>> -Disable the reset code by default, but leave it around guarded by
>>  a check for a reset-on-probe device-property
>> Changes in v4:
>> -Rebase on top of current power-supply/next
>> ---
>>  drivers/power/supply/bq24190_charger.c | 20 +++++++++++++-------
>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index bd9e5c3..ad429b7 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct bq24190_dev_info *bdi)
>>                 return -ENODEV;
>>         }
>>
>> -       ret = bq24190_register_reset(bdi);
>> -       if (ret < 0)
>> -               return ret;
>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>> +               ret = bq24190_register_reset(bdi);
>> +               if (ret < 0)
>> +                       return ret;
>> +       }
>
> Maybe check property and return if true in register_reset(), instead
> of adjusting all the calls. There would be a set_mode_host() in
> resume() which is unnecessary, but it's a no-op to set host mode when
> we're already in it.

i2c transfers are quite slow, so it is better to avoid doing unnecessary
i2c transfers.

> Perhaps device_property_present() since _read_bool() implies we expect
> it to be set T or F?

 From include/linux/property.h:

static inline bool device_property_read_bool(struct device *dev,
                                              const char *propname)
{
         return device_property_present(dev, propname);
}

So nope _read_bool() does not expect true or false, it works
just like devicetree properties

> Let's keep the original reset behavior as the default,

Tony Lindgren suggested to do disable reset by default:

On 04-04-17 02:56, Tony Lindgren wrote:

 > My guess is that the reset is left over from missing handling of
 > clearing of the EN_HIZ on errors. I recall that EN_HIZ error can
 > happen when plugging the cable a little bit sideways to a USB hub
 > with loose tolerance USB connectors.
 >
 > So it should be safe to limit the reset to only happen if something
 > like "reset-on-probe" is specified. Not sure we want to remove it
 > completely, maybe just add notes that reset may misbehave in
 > some conditions.

And I agree that that is the best way, as it simply does not seem
necessary to reset the charger.

> since we only get device props for an ACPI platform.

device_props is a firmware independent method of getting hardware
properties from the firmware, so you can just add "reset-on-probe;"
to the dt node describing the bq24190 in your dts if it turns out
you do need the reset for some reason and it will work as
expected.

Regards,

Hans


p.s.

I thought you were using device_properties for the new battery_properties
stuff too ? If not you really should, that way those properties can be
used on x86 too and it will work just as well for ARM + devicetree.

Hmm, wait I think you were doing a separate node for the battery and
putting them in the charger node, right ? Then device-props won't
work because there is no device to tie them too, so nevermind I guess :)



>
>>         ret = bq24190_set_mode_host(bdi);
>>         if (ret < 0)
>> @@ -1547,7 +1549,8 @@ static int bq24190_remove(struct i2c_client *client)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>> +               bq24190_register_reset(bdi);
>>         bq24190_sysfs_remove_group(bdi);
>>         power_supply_unregister(bdi->battery);
>>         power_supply_unregister(bdi->charger);
>> @@ -1600,7 +1603,8 @@ static __maybe_unused int bq24190_pm_suspend(struct device *dev)
>>                 pm_runtime_put_noidle(bdi->dev);
>>         }
>>
>> -       bq24190_register_reset(bdi);
>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>> +               bq24190_register_reset(bdi);
>>
>>         if (error >= 0) {
>>                 pm_runtime_mark_last_busy(bdi->dev);
>> @@ -1625,8 +1629,10 @@ 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);
>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>> +               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 v4 3/3] power: supply: bq24190_charger: Make battery iface registraton conditional

[Hans de Goede]

Hi,

On 13-04-17 01:21, Liam Breck wrote:
> Hi Hans,
>
> On Wed, Apr 12, 2017 at 11:18 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.
>>
>> This commit adds a check for a "disable-battery-power-supply-iface"
>> device-property which can be set by platform-code if it knows that
>> there will be a fuel-gauge which will supply a battery interface.
>
> I expect to have time to move the -battery properties to -charger this
> weekend. Maybe wait for that?

Ok, I will drop this patch from v5 of this patch-set.

Regards,

Hans


>
>
>> 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
>> Changes in v3:
>> -Check for "disable-battery-power-supply-iface" device-property instead of
>>  completely removing the battery interface
>> Changes in v4:
>> -Rebase on top of current power-supply/next
>> ---
>>  drivers/power/supply/bq24190_charger.c | 41 ++++++++++++++++++++++++----------
>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq24190_charger.c b/drivers/power/supply/bq24190_charger.c
>> index ad429b7..0c01f48 100644
>> --- a/drivers/power/supply/bq24190_charger.c
>> +++ b/drivers/power/supply/bq24190_charger.c
>> @@ -1268,8 +1268,12 @@ static void bq24190_check_status(struct bq24190_dev_info *bdi)
>>
>>         if (alert_charger)
>>                 power_supply_changed(bdi->charger);
>> -       if (alert_battery)
>> -               power_supply_changed(bdi->battery);
>> +       if (alert_battery) {
>> +               if (bdi->battery)
>> +                       power_supply_changed(bdi->battery);
>> +               else
>> +                       power_supply_changed(bdi->charger);
>> +       }
>>
>>         dev_dbg(bdi->dev, "ss_reg: 0x%02x, f_reg: 0x%02x\n", ss_reg, f_reg);
>>  }
>> @@ -1475,13 +1479,23 @@ static int bq24190_probe(struct i2c_client *client,
>>                 goto out_pmrt;
>>         }
>>
>> -       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 out_charger;
>> +       /*
>> +        * The disable-battery-power-supply-iface is purely a temporary in
>> +        * kernel interface for platform-code to use if it knows that there
>> +        * will be a fuel-gauge which will supply a battery interface. The long
>> +        * term plan is for all properties to move to the charger interface
>> +        * and for the battery interface to be removed completely.
>> +        */
>> +       if (!device_property_read_bool(bdi->dev,
>> +                                      "disable-battery-power-supply-iface")) {
>> +               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 out_charger;
>> +               }
>>         }
>>
>>         ret = bq24190_sysfs_create_group(bdi);
>> @@ -1526,7 +1540,8 @@ static int bq24190_probe(struct i2c_client *client,
>>         bq24190_sysfs_remove_group(bdi);
>>
>>  out_battery:
>> -       power_supply_unregister(bdi->battery);
>> +       if (bdi->battery)
>> +               power_supply_unregister(bdi->battery);
>>
>>  out_charger:
>>         power_supply_unregister(bdi->charger);
>> @@ -1552,7 +1567,8 @@ static int bq24190_remove(struct i2c_client *client)
>>         if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>>                 bq24190_register_reset(bdi);
>>         bq24190_sysfs_remove_group(bdi);
>> -       power_supply_unregister(bdi->battery);
>> +       if (bdi->battery)
>> +               power_supply_unregister(bdi->battery);
>>         power_supply_unregister(bdi->charger);
>>         if (error >= 0)
>>                 pm_runtime_put_sync(bdi->dev);
>> @@ -1642,7 +1658,8 @@ 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);
>> +       if (bdi->battery)
>> +               power_supply_changed(bdi->battery);
>>
>>         return 0;
>>  }
>> --
>> 2.9.3
>>

Re: [PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Liam Breck]

G'day Hans,

On Thu, Apr 13, 2017 at 4:54 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 13-04-17 01:06, Liam Breck wrote:
>>
>> Hallo Hans,
>>
>> On Wed, Apr 12, 2017 at 11:18 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.
>>
>>
>> Could you also mention which charger chip, board/host, and any known
>> hw config for the chip by the host?
>> Also mention whether or not there is a TI errata doc for the issue?
>
>
> I will send a v5 with this both added to the commit msg.
>
>
>>> For all the above reasons this commit disables reset on probe and on
>>> suspend / resume. If a specific setup really needs / wants to do a rest
>>> in these cases, this can be requested by setting a reset-on-probe device
>>> property on the 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
>>> Changes in v3:
>>> -Disable the reset code by default, but leave it around guarded by
>>>  a check for a reset-on-probe device-property
>>> Changes in v4:
>>> -Rebase on top of current power-supply/next
>>> ---
>>>  drivers/power/supply/bq24190_charger.c | 20 +++++++++++++-------
>>>  1 file changed, 13 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>> b/drivers/power/supply/bq24190_charger.c
>>> index bd9e5c3..ad429b7 100644
>>> --- a/drivers/power/supply/bq24190_charger.c
>>> +++ b/drivers/power/supply/bq24190_charger.c
>>> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct bq24190_dev_info
>>> *bdi)
>>>                 return -ENODEV;
>>>         }
>>>
>>> -       ret = bq24190_register_reset(bdi);
>>> -       if (ret < 0)
>>> -               return ret;
>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>>> +               ret = bq24190_register_reset(bdi);
>>> +               if (ret < 0)
>>> +                       return ret;
>>> +       }
>>
>>
>> Maybe check property and return if true in register_reset(), instead
>> of adjusting all the calls. There would be a set_mode_host() in
>> resume() which is unnecessary, but it's a no-op to set host mode when
>> we're already in it.

Please check the property in register_reset() instead of at the
callers. It's much cleaner.

> i2c transfers are quite slow, so it is better to avoid doing unnecessary
> i2c transfers.

It's only a few bytes, and only on resume, but an extra condition
there would be OK.

>> Perhaps device_property_present() since _read_bool() implies we expect
>> it to be set T or F?
>
>
> From include/linux/property.h:
>
> static inline bool device_property_read_bool(struct device *dev,
>                                              const char *propname)
> {
>         return device_property_present(dev, propname);
> }
>
> So nope _read_bool() does not expect true or false, it works
> just like devicetree properties

I did examine the source before suggesting _present(). It's a
semantics question: _read_bool() suggests that a property is expected,
whereas _present() doesn't.

>> Let's keep the original reset behavior as the default,
>
>
> Tony Lindgren suggested to do disable reset by default:
>
> On 04-04-17 02:56, Tony Lindgren wrote:
>
>> My guess is that the reset is left over from missing handling of
>> clearing of the EN_HIZ on errors. I recall that EN_HIZ error can
>> happen when plugging the cable a little bit sideways to a USB hub
>> with loose tolerance USB connectors.
>>
>> So it should be safe to limit the reset to only happen if something
>> like "reset-on-probe" is specified. Not sure we want to remove it
>> completely, maybe just add notes that reset may misbehave in
>> some conditions.
>
> And I agree that that is the best way, as it simply does not seem
> necessary to reset the charger.

However, I don't think it's kosher to change a default behavior that's
four years old without a deprecation warning which persists for
several kernel releases, including at least one LTS.

The absence of evidence for other users of this driver is not evidence
of their absence.

And we have yet to confirm that the confused-charger issue you see is
a flaw in the charger, vs that board. If it's not a charger bug, I
suspect that reset may be a valid default for probe/remove(), tho
perhaps not suspend/resume(). But that is a debate for another day.

So how about a property preset-registers = <reg id list>; which would
enable us to save/restore them if nec later. For the purpose of
skipping reset, just use _present(). A different term than "preset"
could work, e.g. programmed, configured...

I realize you feel that register_reset is the devil's own handiwork,
so I regret the disagreement, but I do appreciate your desire to
exorcise it :-)

Thanks!

>> since we only get device props for an ACPI platform.
>
>
> device_props is a firmware independent method of getting hardware
> properties from the firmware, so you can just add "reset-on-probe;"
> to the dt node describing the bq24190 in your dts if it turns out
> you do need the reset for some reason and it will work as
> expected.
>
> Regards,
>
> Hans
>
>
> p.s.
>
> I thought you were using device_properties for the new battery_properties
> stuff too ? If not you really should, that way those properties can be
> used on x86 too and it will work just as well for ARM + devicetree.
>
> Hmm, wait I think you were doing a separate node for the battery and
> putting them in the charger node, right ? Then device-props won't
> work because there is no device to tie them too, so nevermind I guess :)
>
>
>
>
>>
>>>         ret = bq24190_set_mode_host(bdi);
>>>         if (ret < 0)
>>> @@ -1547,7 +1549,8 @@ static int bq24190_remove(struct i2c_client
>>> *client)
>>>                 pm_runtime_put_noidle(bdi->dev);
>>>         }
>>>
>>> -       bq24190_register_reset(bdi);
>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>>> +               bq24190_register_reset(bdi);
>>>         bq24190_sysfs_remove_group(bdi);
>>>         power_supply_unregister(bdi->battery);
>>>         power_supply_unregister(bdi->charger);
>>> @@ -1600,7 +1603,8 @@ static __maybe_unused int bq24190_pm_suspend(struct
>>> device *dev)
>>>                 pm_runtime_put_noidle(bdi->dev);
>>>         }
>>>
>>> -       bq24190_register_reset(bdi);
>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe"))
>>> +               bq24190_register_reset(bdi);
>>>
>>>         if (error >= 0) {
>>>                 pm_runtime_mark_last_busy(bdi->dev);
>>> @@ -1625,8 +1629,10 @@ 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);
>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>>> +               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 v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Hans de Goede]

Hi,

On 14-04-17 12:34, Liam Breck wrote:
> G'day Hans,
>
> On Thu, Apr 13, 2017 at 4:54 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 13-04-17 01:06, Liam Breck wrote:

<snip>

>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>> b/drivers/power/supply/bq24190_charger.c
>>>> index bd9e5c3..ad429b7 100644
>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct bq24190_dev_info
>>>> *bdi)
>>>>                 return -ENODEV;
>>>>         }
>>>>
>>>> -       ret = bq24190_register_reset(bdi);
>>>> -       if (ret < 0)
>>>> -               return ret;
>>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>>>> +               ret = bq24190_register_reset(bdi);
>>>> +               if (ret < 0)
>>>> +                       return ret;
>>>> +       }
>>>
>>>
>>> Maybe check property and return if true in register_reset(), instead
>>> of adjusting all the calls. There would be a set_mode_host() in
>>> resume() which is unnecessary, but it's a no-op to set host mode when
>>> we're already in it.
>
> Please check the property in register_reset() instead of at the
> callers. It's much cleaner.
>
>> i2c transfers are quite slow, so it is better to avoid doing unnecessary
>> i2c transfers.
>
> It's only a few bytes, and only on resume,

Ok, I can send a v5 with the condition moved to bq24190_register_reset()

> but an extra condition
> there would be OK.
>
>>> Perhaps device_property_present() since _read_bool() implies we expect
>>> it to be set T or F?
>>
>>
>> From include/linux/property.h:
>>
>> static inline bool device_property_read_bool(struct device *dev,
>>                                              const char *propname)
>> {
>>         return device_property_present(dev, propname);
>> }
>>
>> So nope _read_bool() does not expect true or false, it works
>> just like devicetree properties
>
> I did examine the source before suggesting _present(). It's a
> semantics question: _read_bool() suggests that a property is expected,
> whereas _present() doesn't.

_read_bool suggests that we are checking a boolean value, which
is exactly what we're doing, _present() could be used to see
if any value integer / string / whatever is present which is
not what we want. That under the hood bool's are implement
by being present == true and not present == false is an
implementation detail which the _read_bool is abstracting
away.


>
>>> Let's keep the original reset behavior as the default,
>>
>>
>> Tony Lindgren suggested to do disable reset by default:
>>
>> On 04-04-17 02:56, Tony Lindgren wrote:
>>
>>> My guess is that the reset is left over from missing handling of
>>> clearing of the EN_HIZ on errors. I recall that EN_HIZ error can
>>> happen when plugging the cable a little bit sideways to a USB hub
>>> with loose tolerance USB connectors.
>>>
>>> So it should be safe to limit the reset to only happen if something
>>> like "reset-on-probe" is specified. Not sure we want to remove it
>>> completely, maybe just add notes that reset may misbehave in
>>> some conditions.
>>
>> And I agree that that is the best way, as it simply does not seem
>> necessary to reset the charger.
>
> However, I don't think it's kosher to change a default behavior that's
> four years old without a deprecation warning which persists for
> several kernel releases, including at least one LTS.

That is a fair argument.

> The absence of evidence for other users of this driver is not evidence
> of their absence.
>
> And we have yet to confirm that the confused-charger issue you see is
> a flaw in the charger, vs that board. If it's not a charger bug, I
> suspect that reset may be a valid default for probe/remove(), tho
> perhaps not suspend/resume(). But that is a debate for another day.
>
> So how about a property preset-registers = <reg id list>; which would
> enable us to save/restore them if nec later. For the purpose of
> skipping reset, just use _present(). A different term than "preset"
> could work, e.g. programmed, configured...

That sounds like something for a different patch. You've convinced
me that keeping the default behavior the same is the best, so I will
submit a v6 which adds a "disable-reset" property.

Regards,

Hans

Re: [PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Liam Breck]

On Fri, Apr 14, 2017 at 5:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
> On 14-04-17 12:34, Liam Breck wrote:
>>
>> G'day Hans,
>>
>> On Thu, Apr 13, 2017 at 4:54 AM, Hans de Goede <hdegoede@redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>>
>>> On 13-04-17 01:06, Liam Breck wrote:
>
>
> <snip>
>
>
>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>> index bd9e5c3..ad429b7 100644
>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct
>>>>> bq24190_dev_info
>>>>> *bdi)
>>>>>                 return -ENODEV;
>>>>>         }
>>>>>
>>>>> -       ret = bq24190_register_reset(bdi);
>>>>> -       if (ret < 0)
>>>>> -               return ret;
>>>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>>>>> +               ret = bq24190_register_reset(bdi);
>>>>> +               if (ret < 0)
>>>>> +                       return ret;
>>>>> +       }
>>>>
>>>>
>>>>
>>>> Maybe check property and return if true in register_reset(), instead
>>>> of adjusting all the calls. There would be a set_mode_host() in
>>>> resume() which is unnecessary, but it's a no-op to set host mode when
>>>> we're already in it.
>>
>>
>> Please check the property in register_reset() instead of at the
>> callers. It's much cleaner.
>>
>>> i2c transfers are quite slow, so it is better to avoid doing unnecessary
>>> i2c transfers.
>>
>>
>> It's only a few bytes, and only on resume,
>
>
> Ok, I can send a v5 with the condition moved to bq24190_register_reset()
>
>> but an extra condition
>> there would be OK.
>>
>>>> Perhaps device_property_present() since _read_bool() implies we expect
>>>> it to be set T or F?
>>>
>>>
>>>
>>> From include/linux/property.h:
>>>
>>> static inline bool device_property_read_bool(struct device *dev,
>>>                                              const char *propname)
>>> {
>>>         return device_property_present(dev, propname);
>>> }
>>>
>>> So nope _read_bool() does not expect true or false, it works
>>> just like devicetree properties
>>
>>
>> I did examine the source before suggesting _present(). It's a
>> semantics question: _read_bool() suggests that a property is expected,
>> whereas _present() doesn't.
>
>
> _read_bool suggests that we are checking a boolean value, which
> is exactly what we're doing, _present() could be used to see
> if any value integer / string / whatever is present which is
> not what we want. That under the hood bool's are implement
> by being present == true and not present == false is an
> implementation detail which the _read_bool is abstracting
> away.
>
>
>>
>>>> Let's keep the original reset behavior as the default,
>>>
>>>
>>>
>>> Tony Lindgren suggested to do disable reset by default:
>>>
>>> On 04-04-17 02:56, Tony Lindgren wrote:
>>>
>>>> My guess is that the reset is left over from missing handling of
>>>> clearing of the EN_HIZ on errors. I recall that EN_HIZ error can
>>>> happen when plugging the cable a little bit sideways to a USB hub
>>>> with loose tolerance USB connectors.
>>>>
>>>> So it should be safe to limit the reset to only happen if something
>>>> like "reset-on-probe" is specified. Not sure we want to remove it
>>>> completely, maybe just add notes that reset may misbehave in
>>>> some conditions.
>>>
>>>
>>> And I agree that that is the best way, as it simply does not seem
>>> necessary to reset the charger.
>>
>>
>> However, I don't think it's kosher to change a default behavior that's
>> four years old without a deprecation warning which persists for
>> several kernel releases, including at least one LTS.
>
>
> That is a fair argument.
>
>> The absence of evidence for other users of this driver is not evidence
>> of their absence.
>>
>> And we have yet to confirm that the confused-charger issue you see is
>> a flaw in the charger, vs that board. If it's not a charger bug, I
>> suspect that reset may be a valid default for probe/remove(), tho
>> perhaps not suspend/resume(). But that is a debate for another day.
>>
>> So how about a property preset-registers = <reg id list>; which would
>> enable us to save/restore them if nec later. For the purpose of
>> skipping reset, just use _present(). A different term than "preset"
>> could work, e.g. programmed, configured...
>
>
> That sounds like something for a different patch. You've convinced
> me that keeping the default behavior the same is the best, so I will
> submit a v6 which adds a "disable-reset" property.

But "preset-registers" is the actual circumstance here, and more
information/context, even if we don't use it all in this patch, is
preferable.

Re: [PATCH v4 2/3] power: supply: bq24190_charger: Do not reset the charger on probe by default

[Hans de Goede]

Hi,

On 14-04-17 15:25, Liam Breck wrote:
> On Fri, Apr 14, 2017 at 5:44 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 14-04-17 12:34, Liam Breck wrote:
>>>
>>> G'day Hans,
>>>
>>> On Thu, Apr 13, 2017 at 4:54 AM, Hans de Goede <hdegoede@redhat.com>
>>> wrote:
>>>>
>>>> Hi,
>>>>
>>>>
>>>> On 13-04-17 01:06, Liam Breck wrote:
>>
>>
>> <snip>
>>
>>
>>>>>> diff --git a/drivers/power/supply/bq24190_charger.c
>>>>>> b/drivers/power/supply/bq24190_charger.c
>>>>>> index bd9e5c3..ad429b7 100644
>>>>>> --- a/drivers/power/supply/bq24190_charger.c
>>>>>> +++ b/drivers/power/supply/bq24190_charger.c
>>>>>> @@ -1382,9 +1382,11 @@ static int bq24190_hw_init(struct
>>>>>> bq24190_dev_info
>>>>>> *bdi)
>>>>>>                 return -ENODEV;
>>>>>>         }
>>>>>>
>>>>>> -       ret = bq24190_register_reset(bdi);
>>>>>> -       if (ret < 0)
>>>>>> -               return ret;
>>>>>> +       if (device_property_read_bool(bdi->dev, "reset-on-probe")) {
>>>>>> +               ret = bq24190_register_reset(bdi);
>>>>>> +               if (ret < 0)
>>>>>> +                       return ret;
>>>>>> +       }
>>>>>
>>>>>
>>>>>
>>>>> Maybe check property and return if true in register_reset(), instead
>>>>> of adjusting all the calls. There would be a set_mode_host() in
>>>>> resume() which is unnecessary, but it's a no-op to set host mode when
>>>>> we're already in it.
>>>
>>>
>>> Please check the property in register_reset() instead of at the
>>> callers. It's much cleaner.
>>>
>>>> i2c transfers are quite slow, so it is better to avoid doing unnecessary
>>>> i2c transfers.
>>>
>>>
>>> It's only a few bytes, and only on resume,
>>
>>
>> Ok, I can send a v5 with the condition moved to bq24190_register_reset()
>>
>>> but an extra condition
>>> there would be OK.
>>>
>>>>> Perhaps device_property_present() since _read_bool() implies we expect
>>>>> it to be set T or F?
>>>>
>>>>
>>>>
>>>> From include/linux/property.h:
>>>>
>>>> static inline bool device_property_read_bool(struct device *dev,
>>>>                                              const char *propname)
>>>> {
>>>>         return device_property_present(dev, propname);
>>>> }
>>>>
>>>> So nope _read_bool() does not expect true or false, it works
>>>> just like devicetree properties
>>>
>>>
>>> I did examine the source before suggesting _present(). It's a
>>> semantics question: _read_bool() suggests that a property is expected,
>>> whereas _present() doesn't.
>>
>>
>> _read_bool suggests that we are checking a boolean value, which
>> is exactly what we're doing, _present() could be used to see
>> if any value integer / string / whatever is present which is
>> not what we want. That under the hood bool's are implement
>> by being present == true and not present == false is an
>> implementation detail which the _read_bool is abstracting
>> away.
>>
>>
>>>
>>>>> Let's keep the original reset behavior as the default,
>>>>
>>>>
>>>>
>>>> Tony Lindgren suggested to do disable reset by default:
>>>>
>>>> On 04-04-17 02:56, Tony Lindgren wrote:
>>>>
>>>>> My guess is that the reset is left over from missing handling of
>>>>> clearing of the EN_HIZ on errors. I recall that EN_HIZ error can
>>>>> happen when plugging the cable a little bit sideways to a USB hub
>>>>> with loose tolerance USB connectors.
>>>>>
>>>>> So it should be safe to limit the reset to only happen if something
>>>>> like "reset-on-probe" is specified. Not sure we want to remove it
>>>>> completely, maybe just add notes that reset may misbehave in
>>>>> some conditions.
>>>>
>>>>
>>>> And I agree that that is the best way, as it simply does not seem
>>>> necessary to reset the charger.
>>>
>>>
>>> However, I don't think it's kosher to change a default behavior that's
>>> four years old without a deprecation warning which persists for
>>> several kernel releases, including at least one LTS.
>>
>>
>> That is a fair argument.
>>
>>> The absence of evidence for other users of this driver is not evidence
>>> of their absence.
>>>
>>> And we have yet to confirm that the confused-charger issue you see is
>>> a flaw in the charger, vs that board. If it's not a charger bug, I
>>> suspect that reset may be a valid default for probe/remove(), tho
>>> perhaps not suspend/resume(). But that is a debate for another day.
>>>
>>> So how about a property preset-registers = <reg id list>; which would
>>> enable us to save/restore them if nec later. For the purpose of
>>> skipping reset, just use _present(). A different term than "preset"
>>> could work, e.g. programmed, configured...
>>
>>
>> That sounds like something for a different patch. You've convinced
>> me that keeping the default behavior the same is the best, so I will
>> submit a v6 which adds a "disable-reset" property.
>
> But "preset-registers" is the actual circumstance here, and more
> information/context, even if we don't use it all in this patch, is
> preferable.

Preset-registers is only part of the problem, even if we were to restore
those there still is the turning off of Q1 / not using Vbus even if present
on reset during the wrong part of the charging cycle, so we really want
to disable-reset period.

Regards,

Hans