linux-kernel - Re: [PATCH v4 2/5] power: supply: add support for max77759 fuel gauge

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: <5n5nkgbviv2dyuqiji3n5ehp5bo5d4azqrlbcz45m2wpmnl525@4vgwghcaytc6>
Date: Mon, 7 Jul 2025 01:58:57 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Thomas Antoine <t.antoine@...ouvain.be>
Cc: 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>, Peter Griffin <peter.griffin@...aro.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

Hi,

On Wed, Jun 25, 2025 at 02:12:21PM +0200, Thomas Antoine wrote:
> > On Fri, May 23, 2025 at 02:51:45PM +0200, Thomas Antoine via B4 Relay 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/
> >>
> >> Signed-off-by: Thomas Antoine <t.antoine@...ouvain.be>
> >> ---
> >>  drivers/power/supply/max1720x_battery.c | 265 ++++++++++++++++++++++++++++----
> >>  1 file changed, 238 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/power/supply/max1720x_battery.c b/drivers/power/supply/max1720x_battery.c
> >> index 68b5314ecf3a234f906ec8fe400e586855b69cd9..c9ad452ada9d0a2a51f37d04fd8c3260be522405 100644
> >> --- a/drivers/power/supply/max1720x_battery.c
> >> +++ b/drivers/power/supply/max1720x_battery.c
> >> @@ -37,6 +37,7 @@
> >>  #define MAX172XX_REPCAP			0x05	/* Average capacity */
> >>  #define MAX172XX_REPSOC			0x06	/* Percentage of charge */
> >>  #define MAX172XX_TEMP			0x08	/* Temperature */
> >> +#define MAX172XX_VCELL			0x09	/* Lowest cell voltage */
> > [...]
> >>  	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > [...]
> >> +			ret = regmap_read(info->regmap, MAX172XX_VCELL, &reg_val);
> >> +			val->intval = max172xx_cell_voltage_to_ps(reg_val);
> > 
> > I haven't reviewed this fully due to all the feedback you already
> > got from Peter Griffin and the DT binding being broken, but something
> > that catched my eye:
> > 
> > POWER_SUPPLY_PROP_VOLTAGE_NOW provides the voltage of the whole
> > battery and not of a single cell. E.g. a typical Li-Ion battery
> > with two serial cells has a nominal voltage of roughly 7.4V while
> > each cell has just 3.7V.
> > 
> > Greetings,
> > 
> > -- Sebastian
> 
> Downstream just reports this value which is usually a bit over 4V when I
> record it.
> 
> Also from what I saw online, it seems that the battery does output
> voltages around 3.86V as it is written on the battery:
> https://cdn.shopify.com/s/files/1/0092/8133/9443/files/QeqxTLOL6eAp6OpZ.jpg?v=1728588266&width=2048
> 
> So I guess that there could only be a single cell in the battery? Which
> would explain why they only report the lowest one.
> 
> It thus should be ok if we consider only the Google Pixel 6 (Pro). If we
> need to take into account the possibility that the chip would be
> used with other batteries, we could take from the devicetree the number
> of cells and only provide the voltage if the number of cells is 1. Would
> this be a good solution?

That's fine with me. But getting the DT binding prepared will take
some time (the DT property should definetly go via
Documentation/devicetree/bindings/power/supply/battery.yaml), we
had some prior discussions around this IIRC. It might be sensible
to move adding VOLTAGE_NOW support to an extra series.

> There is also a VSYS register (0xb1) but I couldn't find anything
> about it. Taking a LSB of 156uV (twice the one of VCell), I see a
> clear correlation with VCell, except for it being very slightly
> lower. As it is mostly guesswork, I don't think it would be a good
> idea to use this without any confirmation it is correct.

I see the datasheet does not have the register listed for < 0x10.
Have you tried contacting the original Google driver author to shed
some light to this?

Greetings,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)