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

[Iskren Chernev]

On 6/20/20 12:31 AM, Sebastian Reichel wrote:
> Hi,
>
> On Fri, Jun 19, 2020 at 10:57:19PM +0300, Iskren Chernev wrote:
>> On 6/19/20 6:59 PM, Sebastian Reichel wrote:
>>> 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@xxxxxxxxx>
>>>> ---
>>>> 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).
>
> Wouldn't be the first time, that documentation is wrong. Have you
> checked how its handled in the downstream Android kernel?

Yes, I have checked both klte[1] and hammerhead[2] downstream. About the state
of charge -- the klte has the spec formula and the double formula, and the
hammerhead has provisions in it's DTS to configure max/min raw values,
effectively enabling arbitrary scaling of the value.

About the reset on start, klte does a quick-start (0x4000 to MODE reg).
Hammerhead resets the RI status bit (which in itself does not reset anything).

I now also looked at downstream implementation of the other fuelgauges.
Here are examples of 17040[3], 17043[4], 17058[5].  It looks like most of the
family (except 17040) is prone to this doubling. [4] and [5] have both formulas
toggleable with a macro. So I think leaving double-soc as an option in DTS is
the right choice.

About the reset -- looks like only 17040 is reset on start. The newer ones are
either quick-started or nothing. So hard-resetting only the 17040 might be
a good choice.

[1] https://github.com/LineageOS/android_kernel_samsung_msm8974/blob/5dddbe776631650086c4491ec4037fbe73fa5795/drivers/battery/max17048_fuelgauge.c#L135
[2] https://github.com/LineageOS/android_kernel_lge_hammerhead/blob/62154f7111663d42655400f83c3c9d10a965571c/drivers/power/max17048_battery.c#L834
[3] https://github.com/LineageOS/android_kernel_lenovo_msm8916/blob/9a4a2ca2832653c90af7fae7503fc64687968700/drivers/power/max17040_battery.c
[4] https://github.com/LineageOS/android_kernel_lge_v500/blob/b4fe00e1f8f09c173a6c28a42ca69ff9529cc13b/drivers/power/max17043_fuelgauge.c#L343
[5] https://github.com/LineageOS/android_kernel_asus_moorefield/blob/c3eae894ce8092c2a9a51f9a4924c8df714d6b3c/drivers/power/ASUS_BATTERY/max17058_battery.c#L551

>>>> +- 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).
>
> Yes, just apply the quirks for 17048.

As I mentioned in the previous paragraph, double-soc could benefit most of the
family (excluding 17040), so I suggest we keep it, whereas doing a reset could
remain for the 17040 only (removing the need for a flag in bindings).

> -- Sebastian
>
>>>> +- 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
>>>>
>>