linux-kernel - Re: [PATCH v2 1/2] dt-bindings: power: supply: Document max17040 extensions

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]
Message-ID: <c7747c37-58c0-79ee-0416-71bb460d168d@gmail.com>
Date:   Fri, 19 Jun 2020 23:01:32 +0300
From:   Iskren Chernev <iskren.chernev@...il.com>
To:     Sebastian Reichel <sre@...nel.org>
Cc:     Rob Herring <robh+dt@...nel.org>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        ~postmarketos/upstreaming@...ts.sr.ht,
        Jonathan Bakker <xc-racer2@...e.ca>,
        Vladimir Barinov <vladimir.barinov@...entembedded.com>
Subject: Re: [PATCH v2 1/2] dt-bindings: power: supply: Document max17040
 extensions


On 6/19/20 6:59 PM, Sebastian Reichel wrote:
> Hi,
>
> On Thu, Jun 18, 2020 at 01:13:39PM +0300, Iskren Chernev wrote:
>> Maxim max17040 is a fuel gauge from a larger family utilising the Model
>> Gauge technology. Document all different compatible strings that the
>> max17040 driver recognizes.
>>
>> Some devices in the wild report double the capacity. The
>> maxim,double-soc (from State-Of-Charge) property fixes that.
>>
>> Complete device reset might lead to very inaccurate readings. Specify
>> maxim,skip-reset to avoid that.
>>
>> To compensate for the battery chemistry and operating conditions the
>> chips support a compensation value. Specify one or two byte compensation
>> via the maxim,rcomp byte array.
>>
>> Signed-off-by: Iskren Chernev <iskren.chernev@...il.com>
>> ---
>> v1: https://lkml.org/lkml/2020/6/8/682
>>
>> Changes in v2:
>> - add maxim,skip-reset
>> - remove 2 byte rcomp from example, the specified compat string supports 1 byte
>>   rcomp
>>
>>  .../power/supply/max17040_battery.txt         | 24 ++++++++++++++++++-
>>  1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> index 4e0186b8380fa..3ee91c295027f 100644
>> --- a/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/max17040_battery.txt
>> @@ -2,7 +2,9 @@ max17040_battery
>>  ~~~~~~~~~~~~~~~~
>>
>>  Required properties :
>> - - compatible : "maxim,max17040" or "maxim,max77836-battery"
>> + - compatible : "maxim,max17040", "maxim,max17041", "maxim,max17043",
>> +         "maxim,max17044", "maxim,max17048", "maxim,max17049",
>> +        "maxim,max17058", "maxim,max17059" or "maxim,max77836-battery"
>>   - reg: i2c slave address
>>
>>  Optional properties :
>> @@ -11,6 +13,18 @@ Optional properties :
>>                  generated. Can be configured from 1 up to 32
>>                  (%). If skipped the power up default value of
>>                  4 (%) will be used.
>> +- maxim,double-soc :         Certain devices return double the capacity.
>> +                Specify this boolean property to divide the
>> +                reported value in 2 and thus normalize it.
>> +                SOC == State of Charge == Capacity.
>
> Can this be derived from the compatible?
>

So far, I'm aware of 2 devices using max17048 that need this setting. That
would be 100% of the 17048 devices I know of [1]. At the same time, according to
the max17048 documentation this is not the case.

[1] These are the Samsung Galaxy S5 (klte) and the LG Nexus 5 (hammerhead).

>> +- maxim,skip-reset :        Do not reset device on driver initialization.
>> +                Some devices report extremely inaccurately after
>> +                a hard reset.
>
> Same question.
>
> -- Sebastian
>

This is even weirder. There is no mention in the documentation about whether
the device should be reset on boot (because the fuelgauge is on even during
device off times). Testing on 17040 and 17043 reveals no issues with resetting
on boot, but on the 17048 I have; the readings are up to 2x wrong if you do
reset on boot. Not doing a reset *might* leave the device in a state that is
not 100% known by the driver, but I don't know of any real world problems
stemming from that.

So in a sense, I can apply both of these quirks for 17048, and it will work for
all devices I have tested on, but it won't follow the spec. That is why
I decided to mark them as special behavior needing configuration per use case.
On the other hand, I can incorporate them in 17048, and if someone complains
the bindings can be introduced later (because they are stable API and
introducing them now is a bit over engineered).

>> +- maxim,rcomp :            A value to compensate readings for various
>> +                battery chemistries and operating temperatures.
>> +                max17040,41 have 2 byte rcomp, default to
>> +                0x97 0x00. All other devices have one byte
>> +                rcomp, default to 0x97.
>>  - interrupts :             Interrupt line see Documentation/devicetree/
>>                  bindings/interrupt-controller/interrupts.txt
>>  - wakeup-source :        This device has wakeup capabilities. Use this
>> @@ -31,3 +45,11 @@ Example:
>>          interrupts = <2 IRQ_TYPE_EDGE_FALLING>;
>>          wakeup-source;
>>      };
>> +
>> +    battery-fuel-gauge@36 {
>> +        compatible = "maxim,max17048";
>> +        reg = <0x36>;
>> +        maxim,rcomp = /bits/ 8 <0x97>;
>> +        maxim,alert-low-soc-level = <10>;
>> +        maxim,double-soc;
>> +    };
>>
>> base-commit: 1713116fa907cc7290020f0d8632ec646d2936f8
>> --
>> 2.27.0
>>

P.S sorry for the double email, forgot reply-all