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

[Sebastian Reichel]

Hi,

On Fri, May 23, 2025 at 02:51:45PM +0200, Thomas Antoine via B4 Relay wrote:
> From: Thomas Antoine <t.antoine@xxxxxxxxxxxx>
> 
> 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@xxxxxxxxxx/
> 
> Signed-off-by: Thomas Antoine <t.antoine@xxxxxxxxxxxx>
> ---
>  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

Attachment: signature.asc
Description: PGP signature