[PATCH v5 0/2] power: supply: bq24190_charger: Pending patches

[PATCH v5 0/2] power: supply: bq24190_charger: Pending patches

[Hans de Goede]

Hi Sebastian,

Here is a v5 of my pending patches rebased on top of the fixes from Liam
you merged earlier. Compared to v4 I've tweaked the commit-messages of
patch 1-2 somewhat and dropped patch 3 as Liam expects to have a better
solution for that soonish.

The first patch is still a bug-fix for the extcon support, which
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 v5 1/2] 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>
Acked-by: Liam Breck <kernel@networkimprov.net>
---
Changes in v4:
-This is a new patch in v4 of this patch-set
Changes in v5;
-Add Liam's Acked-by
---
 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 v5 2/2] 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.

This last problem is happening on a GPD-win mini PC with a bq24292i
charger chip combined with a max17047 fuel-gauge and a LiHV battery.
I've checked and TI does not list any errata for the bq24292i which
could explain this (there are no errata at all).

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
Changes in v5:
-Updated commit msg to describe the hw on which I'm seeing the charger getting
 stuck while reset during charging and Vbat is above the reset charge volt limit
---
 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

Re: [PATCH v5 1/2] power: supply: bq24190_charger: Use new extcon_register_notifier_all()

[Sebastian Reichel]

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

Hi,

On Thu, Apr 13, 2017 at 02:04:11PM +0200, Hans de Goede 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.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Acked-by: Liam Breck <kernel@networkimprov.net>

With extcon_register_notifier_all() being new and not available
in my tree I can't simply queue this patch. Also it can't simply
go through the extcon subsystem, since it depends on changes in
the power-supply subsystem and results in non-trivial merge conflict.
I see three merge solutions:

 1. Defer to 4.13 (since we are already quite near the merge window)
 2. I need a immutable branch with extcon_register_notifier_all()
    from the extcon subsystem
 3. It may count as Fix and could be queued during the 4.12-rc
    phase based von 4.12-rc1

-- Sebastian

> ---
> Changes in v4:
> -This is a new patch in v4 of this patch-set
> Changes in v5;
> -Add Liam's Acked-by
> ---
>  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
> 

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

Re: [PATCH v5 1/2] power: supply: bq24190_charger: Use new extcon_register_notifier_all()

[Sebastian Reichel]

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

Hi,

On Fri, Apr 14, 2017 at 01:35:26AM +0200, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Apr 13, 2017 at 02:04:11PM +0200, Hans de Goede 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.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > Acked-by: Liam Breck <kernel@networkimprov.net>
> 
> With extcon_register_notifier_all() being new and not available
> in my tree I can't simply queue this patch. Also it can't simply
> go through the extcon subsystem, since it depends on changes in
> the power-supply subsystem and results in non-trivial merge conflict.
> I see three merge solutions:
> 
>  1. Defer to 4.13 (since we are already quite near the merge window)
>  2. I need a immutable branch with extcon_register_notifier_all()
>     from the extcon subsystem
>  3. It may count as Fix and could be queued during the 4.12-rc
>     phase based von 4.12-rc1

Sorry for the noise, I just saw the immutable branch in the cover
mail. I will queue this now. Unfortunately it means rebasing the
power-supply tree to 4.11-rc4 (or ugly merge, which is annoying
in the pull-request). Chanwoo, please try to use -rc1 as base for
immutable branches in the future.

-- Sebastian

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

Re: [PATCH v5 1/2] power: supply: bq24190_charger: Use new extcon_register_notifier_all()

[Chanwoo Choi]

Hi Sebastian,

On 2017년 04월 14일 08:44, Sebastian Reichel wrote:
> Hi,
> 
> On Fri, Apr 14, 2017 at 01:35:26AM +0200, Sebastian Reichel wrote:
>> Hi,
>>
>> On Thu, Apr 13, 2017 at 02:04:11PM +0200, Hans de Goede 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.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> Acked-by: Liam Breck <kernel@networkimprov.net>
>>
>> With extcon_register_notifier_all() being new and not available
>> in my tree I can't simply queue this patch. Also it can't simply
>> go through the extcon subsystem, since it depends on changes in
>> the power-supply subsystem and results in non-trivial merge conflict.
>> I see three merge solutions:
>>
>>  1. Defer to 4.13 (since we are already quite near the merge window)
>>  2. I need a immutable branch with extcon_register_notifier_all()
>>     from the extcon subsystem
>>  3. It may count as Fix and could be queued during the 4.12-rc
>>     phase based von 4.12-rc1
> 
> Sorry for the noise, I just saw the immutable branch in the cover
> mail. I will queue this now. Unfortunately it means rebasing the
> power-supply tree to 4.11-rc4 (or ugly merge, which is annoying
> in the pull-request). Chanwoo, please try to use -rc1 as base for
> immutable branches in the future.

I'm really sorry about this. It is my fault. In the future,
I'll use the -rc1 for the immutable branch. Thanks.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics