linux-kernel - Re: [PATCH RESEND v2 3/3] power: supply: Introduce MM8013 fuel gauge driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 14 Sep 2023 20:46:21 +0200
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Sebastian Reichel <sebastian.reichel@...labora.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH RESEND v2 3/3] power: supply: Introduce MM8013 fuel gauge
 driver

On 13.09.2023 17:45, Sebastian Reichel wrote:
> Hi,
> 
> On Wed, Aug 23, 2023 at 04:36:15PM +0200, Konrad Dybcio wrote:
>> Add a driver for the Mitsumi MM8013 fuel gauge. The driver is a vastly
>> cleaned up and improved version of the one that shipped in some obscure
>> Lenovo downstream kernel [1], with some register definitions borrowed from
>> ChromeOS EC platform code [2].
>>
>> [1] https://github.com/adazem009/kernel_lenovo_bengal/commit/b6b346427a871715709bd22aae449b9383f3b66b
>> [2] https://chromium.googlesource.com/chromiumos/platform/ec/+/master/driver/battery/mm8013.h
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@...aro.org>
>> ---
[...]

> 
> Please use regmap.
Ack

[...]

>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_CAPACITY:
> 
> this is in %, while the next two are in uAh. So the fuel gauge does
> not provide the current capacity in uAh
> (POWER_SUPPLY_PROP_CHARGE_NOW)?
Yes. Doesn't seem like raw values are supported.

[...]

> 
> this is just 'val->intval = (s16) ret;'.
> 
>> +		val->intval *= -1000;
Ack

> 
> and this seems to be the only property, that properly scales its
> values, assuming the hardware reports data in mA.
> 

[...]

> 
> that's in **micro** volts
Oh, I didn't read enough docs..

> 
>> +		ret = mm8013_read_reg(client, REG_VOLTAGE);
> 
> this effectively does i2c_smbus_read_word_data() and thus the
> maximum is is 65536. 65536uV = 65mV. I have serious doubts, that
> this code does what you want. The same is true for a couple of
> the other properties.
Ack

[...]

> With the next submission please include a dump of the uevent
> in sysfs in the cover letter or below the fold, so that its
> easy to validty check if the reported values look sensible.
State of what-will-be-sent in v(n+1), with additional fixups:

POWER_SUPPLY_NAME=mm8013
POWER_SUPPLY_TYPE=Battery
POWER_SUPPLY_CAPACITY=100
POWER_SUPPLY_CHARGE_FULL=7124
POWER_SUPPLY_CHARGE_FULL_DESIGN=7500
POWER_SUPPLY_CURRENT_NOW=-122000
POWER_SUPPLY_CYCLE_COUNT=27
POWER_SUPPLY_HEALTH=Good
POWER_SUPPLY_PRESENT=1
POWER_SUPPLY_STATUS=Full
POWER_SUPPLY_TEMP=324
POWER_SUPPLY_VOLTAGE_NOW=4407000


> 
> Thanks and sorry for the slow processing,
No worries

Konrad