Re: [PATCH v6 2/2] dt-bindings: power: supply: add support for MAX77759 fuel gauge

[Thomas Antoine <t.antoine@uclouvain.be>]

Hi,

On 9/19/25 12:32 AM, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Sep 18, 2025 at 02:02:55PM +0100, André Draszik wrote:
>>> On 9/15/25 7:31 PM, Conor Dooley wrote:
>>>> On Mon, Sep 15, 2025 at 12:14:11PM +0200, Thomas Antoine via B4 Relay wrote:
>>>>> From: Thomas Antoine <t.antoine@uclouvain.be>
>>>>>
>>>>> The Maxim MAX77759 is a companion PMIC for USB Type-C. It contains
>>>>> Battery Charger, Fuel Gauge, temperature sensors, USB Type-C Port
>>>>> Controller (TCPC), NVMEM, and additional GPIO interfaces
>>>>>
>>>>> Use max77759-fg compatible to avoid conflict with drivers for other
>>>>> functions.
>>>>>
>>>>> The battery node is used to pass the REPCAP and ICHGTERM values
>>>>> needed for the initialization of the fuel gauge.
>>>>>
>>>>> The nvmem cells are used to get initialization values and to backup
>>>>> the learning and the number of cycles. It should work out of the box
>>>>> with gs101-oriole and gs101-raven which were previously running
>>>>> Android.
>>>>>
>>>>> Signed-off-by: Thomas Antoine <t.antoine@uclouvain.be>
>>>>> ---
>>>>>  .../bindings/power/supply/maxim,max77759.yaml      | 78 ++++++++++++++++++++++
>>>>>  1 file changed, 78 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/power/supply/maxim,max77759.yaml
>>>>> b/Documentation/devicetree/bindings/power/supply/maxim,max77759.yaml
>>>>> new file mode 100644
>>>>> index 0000000000000000000000000000000000000000..4d45739fcaf26273ec57b60049d6d0421df38efb
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/power/supply/maxim,max77759.yaml
>>>>> @@ -0,0 +1,78 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/power/supply/maxim,max77759.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Maxim Integrated MAX77759 fuel gauge
>>>>> +
>>>>> +maintainers:
>>>>> +  - Thomas Antoine <t.antoine@uclouvain.be>
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: power-supply.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    const: maxim,max77759-fg
>>>>
>>>> Compatible doesn't match the filename, why?
>>>> I assume the "fg" is fuel-gauge, but can this device be anything else?
>>>
>>> The max77759 is a multifunction chip.
>>> The following compatibles are already used for some of those functions:
>>> - maxim,max77759 (for the pmic)
>>> - maxim,max77759-gpio
>>> - maxim,max77759-nvmem
>>> - maxim,max77759-tcpci
>>>
>>> The fuel gauge functionality that is added with this patch is very similar
>>> to the functionality of the max1720x which is why the filename was chosen
>>> to fit other maxim fuel gauge chips pattern.
>>>
>>> Maybe it would be better to use the maxim,max77759-battery compatible to
>>> match the filename? It would also fit with the already existing
>>> maxim,max77705-battery and maxim,max77849-battery compatibles.
>>
>> It also has a (battery) charger, a -battery compatible could be misleading.
>> The datasheet refers to these subblocks as FG (for fuelgauge) and CHARGER.
>> I'd suggest keeping those terms.
>>
>> Additionally, the FG block can also measure temperature and battery ID. For
>> those, a combination of (top-level) PMIC and FG registers are needed
>> unfortunately. Which means that the FG should probably be an MFD child
>> device, even though the FG itself doesn't depend on the top-level. Otherwise
>> it'd be hard to access the top-level PMIC register.
> 
> My understanding is, that the FG has a dedicated I2C device address
> and thus cannot be a simple MFD child of the PMIC. Did I misunderstood
> that part?
> 
> Assuming I understood things correctly, I think I suggest to model
> things like this for the battery temperature/ID:
> 
> i2c {
>     pmic: pmic@42 {
>         compatible = "maxim,max77759";
>         ...
> 
>         pmic_adc: adc {
>             compatible = "maxim,max77759-adc";
>             ...
>         };
>     };
> 
>     fuel-gauge@43 {
>         compatible = "maxim,max77759-fg";
>         ...
>         io-channels = <&pmic_adc MAX77759_ADC_BAT_TEMP>, <&pmic_adc MAX77759_ADC_BAT_ID>;
>         io-channel-names = "temperature", "ID";
>     };
> };

I have been looking into this and I don't think I understand how this
is needed.

When looking at the downstream driver for the battery level, battery
temperature is just returned based on the current level returned by
the chip. There is an optional filtering function:
LINK: https://android.googlesource.com/kernel/google-modules/bms/+/refs/heads/android-gs-raviole-mainline/google_battery.c#732
And from what I see in the downstream devicetree, it is not enabled.
So the current temperature should behave like the one on downstream if
the chip is correctly initiated.

For the battery id, it is stored in the EEPROM at address 50 of hsi2c_8
which I plan to use throught the nvmem framework like in v5 of this patch.

Am I missing something which would actually require this io-channels setup?

Greetings,
Thomas