linux-kernel - Re: [PATCH v2 1/4] power: supply: add support for max77759 fuel gauge

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7d8d2406-fd49-446d-82e9-e088eaa7a9d1@uclouvain.be>
Date: Fri, 10 Jan 2025 17:56:24 +0100
From: Thomas Antoine <t.antoine@...ouvain.be>
To: André Draszik <andre.draszik@...aro.org>,
 Sebastian Reichel <sre@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Dimitri Fedrau <dima.fedrau@...il.com>,
 Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
 Peter Griffin <peter.griffin@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>
Cc: linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH v2 1/4] power: supply: add support for max77759 fuel gauge

Hi,

Thanks for taking the time to test the system.

On 1/7/25 12:00, André Draszik wrote:
> Hi Thomas,
> 
> Thanks for your patch!
> 
> On Thu, 2025-01-02 at 12:15 +0100, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@...ouvain.be>
>>
>> The interface of the Maxim max77759 fuel gauge has a lot of common with the
>> Maxim max1720x. The major difference is the lack of non-volatile memory
>> slave address. No slave is available at address 0xb of the i2c bus, which
>> is coherent with the following driver from google: line 5836 disables
>> non-volatile memory for m5 gauge.
>>
>> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c
>>
>> Other differences include the lack of V_BATT register to read the battery
>> level and a difference in the way to identify the chip (the same register
>> is used but not the same mask).
> 
> It also seems the reported POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN is
> quite a bit off - on my Pixel 6, it reports ca. 1131mAh, but the downstream
> stack reports a more reasonable 4524mAh. Interestingly, this is an exact
> multiple of four.
> 
> POWER_SUPPLY_PROP_CHARGE_FULL is off in a similar way, and I suspect that
> related properties like charge_avg, time_to_empty, time_to_full are
> reported incorrectly as well.

Indeed, now that I check the code, the current computation is wrong.
In the downstream kernel, reg_to_capacity_uah is used to translate the register
value. In the end, it computes the value as follows:

div_s64((s64) val * 500000, rsense) * lsb;
Link: https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-5.10-android15/max1720x_battery.h#36

whereas the mainline driver does val * 500.
Based on what I saw, lsb should be 1 to 2 based on the value of the register
MAX_M5_TASKPERIOD.

Basically, if lsb is 1 and given the default rsense of the mainline driver,
the two functions will return the same. 

>From the datasheet of the max17201, capacity LSB is "5.0μVh/RSENSE".
So it seems that the current mainline driver is only right if rsense is equal
to 10mOhms.

The factor 4 that you see should thus come from
1. a factor 2 because we do 5.0μVh/10mOhm instead of 5.0μVh/5mOhm.
2. a factor 2 because we do not take into account lsb.

MAX_M5_TASKPERIOD is reg 0x3c which is not mentionned at all in the datasheet
of the max17201. I guess this might be another difference between the two
devices.

I think the best course of action is to correct the computation to take into
account rsense and to then multiply by lsb only for max77759.
This would make the behaviour of the max17201 follow the datasheet.

> [...]
> 
>> @@ -483,14 +608,27 @@ static int max1720x_probe(struct i2c_client *client)
>>  	psy_cfg.drv_data = info;
>>  	psy_cfg.fwnode = dev_fwnode(dev);
>>  	i2c_set_clientdata(client, info);
>> -	info->regmap = devm_regmap_init_i2c(client, &max1720x_regmap_cfg);
>> +
>> +	data = device_get_match_data(dev);
>> +	if (!data)
>> +		return dev_err_probe(dev, ret, "Failed to get chip data\n");
>                                           ^^^
> This should be -EINVAL.

Indeed, will fix.

Best regards,
Thomas