Message-ID: <bc40326f-db40-4657-84a7-152def2ca9e3@uclouvain.be>
Date: Tue, 24 Jun 2025 17:46:53 +0200
From: Thomas Antoine <t.antoine@...ouvain.be>
To: Peter Griffin <peter.griffin@...aro.org>
Cc: 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>,
 André Draszik <andre.draszik@...aro.org>,
 Tudor Ambarus <tudor.ambarus@...aro.org>,
 Alim Akhtar <alim.akhtar@...sung.com>, 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 v4 2/5] power: supply: add support for max77759 fuel gauge



On 6/6/25 1:40 PM, Peter Griffin wrote:
> Hi Thomas,
> 
> Thanks for your patch and working to get fuel gauge functional on
> Pixel 6! I've tried to do quite an in-depth review comparing with the
> downstream driver.
Hi Peter,

Thank you very much for the in-depth review!

> On Fri, 23 May 2025 at 13:52, Thomas Antoine via B4 Relay
> <devnull+t.antoine.uclouvain.be@...nel.org> 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. A 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.
>>
>> Link: 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. The voltage must instead be read from V_CELL, the lowest voltage of
>> all cells. The mask to identify the chip is different. The computation of
>> the charge must also be changed to take into account TASKPERIOD, which
>> can add a factor 2 to the result.
>>
>> Add support for the MAX77759 by taking into account all of those
>> differences based on chip type.
>>
>> Do not advertise temp probes using the non-volatile memory as those are
>> not available.
>>
>> The regmap was proposed by André Draszik in
>>
>> Link: https://lore.kernel.org/all/d1bade77b5281c1de6b2ddcb4dbbd033e455a116.camel@linaro.org/
> 
> I think it would be worth noting in the commit message this is basic
> initial support for the M5 gauge in MAX77759, and things like loading
> & saving the m5 model aren't implemented yet.
> 
> That's important as some values such as the REPSOC register value used
> for POWER_SUPPLY_PROP_CAPACITY show the result after all processing
> including ModelGauge mixing etc, so these values won't be as accurate
> as downstream.

I will add that to the next version.

>>[...]
>> +static const enum power_supply_property max77759_battery_props[] = {
>> +       POWER_SUPPLY_PROP_PRESENT,
> 
> I checked the register values match downstream - this looks correct
> 
>> +       POWER_SUPPLY_PROP_CAPACITY,
> 
> I checked the register offset matchs downstream. The value reported
> varies a bit versus downstream. As mentioned above that is likely due
> to the REPSOC register reporting after mixing with the m5 model which
> is not loaded currently. Also the application specific values and cell
> characterization information used by the model isn't configured
> currently (see link below in _TEMP property below for the initial fuel
> gauge params used by downstream.
>

I have dumped the model written to my phone by a userdebug stock android.
If you think it is necessary, I can implement model loading where the
model is passed in the devicetree for next version.

>> +       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> 
> I checked the register offset matchs downstream. Values reported look sensible.
> 
>> +       POWER_SUPPLY_PROP_CHARGE_FULL,
> 
> Downstream has a slightly different implementation than upstream for
> this property. See here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x_battery.c#2244
>

Indeed, the main difference seems to be to use FULLCAPNOM instead of
FULLCAP. I will check out to see if both differ in value.

 
>> +       POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
> 
> I checked the register offset value is correct. However this is
> reporting 3000000 and downstream reports 4524000. I checked and it's
> just converting the register reset value of DESIGNCAP which is 0xbb8.
> 
> This is listed as a "application specific" value, so it maybe we just
> need to write the correct initial value to DESIGNCAP (see TEMP section
> below)
> 

The value  3000000 is the default value which will be set after a hardware
reset. I was able to extract the init sequence from a stock android.
It indeed writes the DESIGNCAP value. Here is a summary of the registers
written to upon a hardware reset. When (DTS) is written next to it,
it means that the value is exactly the same as given by the table in
maxim,cos-a1-2k at the link
https://android.googlesource.com/kernel/gs/+/refs/heads/android-gs-raviole-5.10-android15/arch/arm64/boot/dts/google/gs101-oriole-battery-data.dtsi

  "0x05 0x0000" #REPCAP
  "0x2a 0x0839" #RELAXCFG (DTS)
  "0x60 0x0080" #COMMAND UNLOCK_CFG
  "0x48 0x5722" #VFSOC0 (Value read from reg 0xff)
  "0x28 0x260e" #LEARNCFG (DTS)
  "0x1d 0x4217" #CONFIG (DTS)
  "0xbb 0x0090" #CONFIG2 (DTS)
  "0x13 0x5f00" #FULLSOCTHR (DTS)
  "0x35 0x08e8" #FULLCAPREP
  "0x18 0x08d6" #DESIGNCAP (DTS)
  "0x46 0x0c80" #DPACC
  "0x45 0x0094" #DQACC
  "0x00 0x0002" #STATUS
  "0x23 0x0905" #FULLCAPNOM
  "0x3a 0xa561" #VEMPTY (DTS)
  "0x12 0x2f80" #QRTABLE0 (DTS)
  "0x22 0x1400" #QRTABLE1 (DTS)
  "0x32 0x0680" #QRTABLE2 (DTS)
  "0x42 0x0580" #QRTABLE3 (DTS)
  "0x38 0x03bc" #RCOMP0
  "0x39 0x0c02" #TCOMPO
  "0x3c 0x2d00" #TASKPERIOD (DTS)
  "0x1e 0x05a0" #ICHGTERM (DTS)
  "0x2c 0xed51" #TGAIN (DTS)
  "0x2d 0x1eba" #TOFF (DTS)
  "0x2b 0x3870" #MISCCFG (DTS)
  "0x04 0x0000" #ATRATE (DTS)
  "0xb6 0x06c3" #CV_MIXCAP (value = 75% of FULLCAPNOM)
  "0xb7 0x0600" #CV_HALFTIME
  "0x49 0x2241" #CONVGCFG (DTS)
  "0x60 0x0000" #COMMAND LOCK_CFG
  "0xb9 0x0014" #CURVE (DTS)
  "0x29 0xc623" #FILTERCFG (DTS)
  "0x2e 0x0400" #CGAIN (hard coded)
  "0xbb 0x00b0" #CONFIG2 (DTS | 0x0020)
  "0x02 0x0780" #TALRTTH
  "0x00 0x0000" #STATUS
  "0x17 0x9320" #CYCLES

As can be seen, most values come directly from the devicetree but some
are not present in there or differ from the value given in the devicetree.

Without a similar init, charge and temperature will be non-functional
other values will most likely be wrong.
The fuel gauge stays powered through reboot so it doesn't reset even
when switching from android to linux, meaning that without any hardware
crash (e.g. empty batterry), the chip will look perfectly initialized.
A hardware reset of the fuel gauge can be forced by writing to
/proc/sysrq-trigger or by writing 0xf to 0x60.

I am unsure about what to do about this initalization, especially for values
which slightly differ from the devicetree. I think for next version, I
will have the same parameters be passed in the devicetree like android.
(except maybe IAvgEmpty which seems to be unused in the downsteam driver?)

>> +       POWER_SUPPLY_PROP_CHARGE_AVG,
> 
> This property isn't reported downstream. The value is changing and not
> just the reset value. I noticed REPSOC is an output of the ModelGauge
> algorithm so it is likely not to be completely accurate.
> 
>> +       POWER_SUPPLY_PROP_TEMP,
> 
> I checked the register offset value is correct. However the
> temperature is always being reported as the register reset value of
> 220. This is for obvious reasons quite an important one to report
> correctly.
> 
> I started debugging this a bit, and it is caused by an incorrectly
> configured CONFIG (0x1D) register. In particular the TEX[8] bit is 1
> on reset in this register which means temperature measurements are
> written from the host AP. When this bit is set to 0, measurements on
> the AIN pin are converted to a temperature value and stored in the
> Temperature register (I then saw values of 360 and the value
> changing).
> 
> See here for the bits in that CONFIG register
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5_reg.h#403
> 
> In downstream all these initial register settings are taken from DT
> here  https://android.googlesource.com/kernel/google-modules/raviole-device/+/refs/heads/android14-gs-pixel-6.1/arch/arm64/boot/dts/google/gs101-fake-battery-data.dtsi#27
> 
> For temperature when TEX=0, TGAIN, TOFF and TCURVE registers should
> also be configured to adjust the temperature measurement.
> 
> I think it would likely be worth initialising all the fuel gauge
> registers referenced in maxim,fg-params as that includes some of the
> application specific values for DESIGNCAP, also some of the cell
> characterization information, and hopefully we will get more accurate
> values from the fuel gauge generally.
>

See previous comment.

>> +       POWER_SUPPLY_PROP_CURRENT_NOW,
> 
> I checked the register offset matches downstream. Values reported look
> reasonable.
> 
>> +       POWER_SUPPLY_PROP_CURRENT_AVG,
> 
> I checked the register offset matches downstream. Values reported look
> reasonable.
> 
>> +       POWER_SUPPLY_PROP_MODEL_NAME,
> 
> This property isn't reported downstream.

Is this a problem?

>> [...]
>>  /*
>>   * Current and temperature is signed values, so unsigned regs
>>   * value must be converted to signed type
>> @@ -390,16 +507,36 @@ static int max1720x_battery_get_property(struct power_supply *psy,
>>                 val->intval = max172xx_percent_to_ps(reg_val);
>>                 break;
>>         case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> -               ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
>> -               val->intval = max172xx_voltage_to_ps(reg_val);
>> +               if (info->id == MAX1720X_ID) {
>> +                       ret = regmap_read(info->regmap, MAX172XX_BATT, &reg_val);
>> +                       val->intval = max172xx_voltage_to_ps(reg_val);
> 
> I think MAX1720X using MAX172XX_BATT register is likely a bug as the
> downstream driver uses MAX172XX_VCELL for that variant  see here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max1720x.h#304
>

Based on the comments from Sebastian Reichel, it seems that it is
downstream which is wrong:
https://lore.kernel.org/all/4cahu6dog7ly4ww6xyjmjigjfxs4m55mrnym2bjmzskscfvk34@guazy6wxbzfh/

> Having said that, if we do need to cope with differing register
> offsets for the different fuel gauge variants it would be nicer to
> abstract them in a way similar to the downstream driver. See here
> https://android.googlesource.com/kernel/google-modules/bms/+/1a68c36bef474573cc8629cc1d121eb6a81ab68c/max_m5.c#1235
> I think that would be more scalable in supporting multiple variants in
> one driver. Otherwise we will have an explosion of if(id==blah) else
> if (id==blah) in the driver.
>
> kind regards,
> 
> Peter


I completely agree about the need for abstraction if we want to keep both
chips in the same driver. I will try to implement that for next version.
Best regards,
Thomas

Powered by blists - more mailing lists