Charging status for N900/RX-51 with bq27200

Commit Message

Sebastian Reichel Feb. 15, 2018, 8:41 p.m. UTC

Hi Merlijn,

On Thu, Feb 15, 2018 at 12:53:32PM +0100, Merlijn Wajer wrote:
> On my N900, with kernel 4.15-rc6 (I realise it's not the very latest),
> the (charging) status of the battery is confusing or plain wrong:
> 
> When no charger is connected, this is reported:
> 
> # cat
> /sys/devices/platform/68000000.ocp/48072000.i2c/i2c-2/2-0055/power_supply/bq27200-0/status
> Not charging
> 
> When a charged is connected, this is reported:
> 
> # cat
> /sys/devices/platform/68000000.ocp/48072000.i2c/i2c-2/2-0055/power_supply/bq27200-0/status
> Charging
> 
> The odd part here is "Not charging" -- on all my other devices, a device
> that is discharging is reported as "Discharging", not as "Not charging".

"Not charging" is reported, when the battery is neither charged, nor
dis-charged. It's supposed to be used when a charger is connected
without charging the battery.

> [...]
>
> The N900 reporting "Not charging" when it is in fact discharging
> confuses userspace, such as UPower.

It's a bug.

> I am not particularly versed in the N900/RX-51 drivers, but from a
> quick glance this line looks suspicious [1].
> 
> Cheers,
> Merlijn
> 
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/bq27xxx_battery.c#n1673

Basically the bq27xxx driver assumes, that charger is connected.
I guess the problem is, that the N900 devicetree does not specify,
that the bq27xxx is supplied by the charger, so
power_supply_am_i_supplied returns -ENODEV.

This can be fixed by adding the following to the N900 DT:

&bq27200 {
    power-supplies = <&bq24150a>;
};

But there is also a bug in the bq27xxx driver, that should cope
with no charger being specified. The following patch should fix
the issue. I currently don't have my N900 with me. Could you
give it a try?

-- Sebastian

--------------------------------------------------

From 47642af85cc9ffc829b5f478e71b1751a98e2659 Mon Sep 17 00:00:00 2001
From: Sebastian Reichel <[e-mail]>
Date: Thu, 15 Feb 2018 21:28:40 +0100
Subject: [PATCH] power: supply: bq27xxx: support missing supplier device

power_supply_am_i_supplied() can return negative error
codes. In this case we should assume, that no charger
is connected and the battery should be marked as
DISCHARGING instead of NOT_CHARGING.

Reported-by: Merlijn Wajer <[e-mail]>
Signed-off-by: Sebastian Reichel <[e-mail]>
---
 drivers/power/supply/bq27xxx_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Merlijn Wajer Feb. 16, 2018, 9:45 a.m. UTC | #1

Hi Sebastian,

On 15/02/18 21:41, Sebastian Reichel wrote:

> 
> "Not charging" is reported, when the battery is neither charged, nor
> dis-charged. It's supposed to be used when a charger is connected
> without charging the battery.

Makes sense.

> Basically the bq27xxx driver assumes, that charger is connected.
> I guess the problem is, that the N900 devicetree does not specify,
> that the bq27xxx is supplied by the charger, so
> power_supply_am_i_supplied returns -ENODEV.
> 
> This can be fixed by adding the following to the N900 DT:
> 
> &bq27200 {
>     power-supplies = <&bq24150a>;
> };
> 
> But there is also a bug in the bq27xxx driver, that should cope
> with no charger being specified. The following patch should fix
> the issue. I currently don't have my N900 with me. Could you
> give it a try?

I added the dts change and bq27xxx change on top of my kernel, but now
the bq27xxx-battery does not want to probe:

[   34.947082] power_supply bq27200-0: Not all required supplies found,
defer probe
[   34.958953] bq27xxx-battery 2-0055: failed to register battery

Full boot log here: https://ptpb.pw/UeEV

Thanks,
Cheers,
Merlijn

Patch

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index d99981542a46..7ce60519b1bc 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1670,7 +1670,7 @@  static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
 			status = POWER_SUPPLY_STATUS_FULL;
 		else if (di->cache.flags & BQ27000_FLAG_CHGS)
 			status = POWER_SUPPLY_STATUS_CHARGING;
-		else if (power_supply_am_i_supplied(di->bat))
+		else if (power_supply_am_i_supplied(di->bat) > 0)
 			status = POWER_SUPPLY_STATUS_NOT_CHARGING;
 		else
 			status = POWER_SUPPLY_STATUS_DISCHARGING;