linux-kernel - Re: [PATCH v3 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] [day] [month] [year] [list]

Message-ID: <ieh6g5m4oectmje2gowa6rl2blzjuovjpyd3cmbvoql4qn2c7m@2osiwclwxi43>
Date: Wed, 30 Apr 2025 02:36:16 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: t.antoine@...ouvain.be
Cc: Rob Herring <robh@...nel.org>, 
	Peter Griffin <peter.griffin@...aro.org>, André Draszik <andre.draszik@...aro.org>, 
	Tudor Ambarus <tudor.ambarus@...aro.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>, 
	Dimitri Fedrau <dima.fedrau@...il.com>, linux-arm-kernel@...ts.infradead.org, 
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 2/5] power: supply: add support for max77759 fuel gauge

Hi,

On Mon, Apr 21, 2025 at 08:13:33PM +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>
> ---

[...] (I actually skipped reviewing big parts here for now)

>
> +		ret = of_property_read_u32(dev->of_node,
> +					   "shunt-resistor-micro-ohms", &val);

device_property_read_u32(dev, ...)

> [...]
> +	ret = of_property_read_u32(dev->of_node,
> +				   "charge-full-design-microamp-hours", &info->charge_full_design);
> +	if (ret)
> +		info->charge_full_design = 0;

This is not in the DT binding and thus not allowed. Also I will NAK
adding it to the DT binding, since the following should be used:

Documentation/devicetree/bindings/power/supply/battery.yaml

followed by using power_supply_get_battery_info() in this driver.

Adding this support is probably a good idea, since it allows
providing all kind of static information from DT and you are
missing the non-volatile memory part from the existing chip.

Note that the power-supply core will pick up and expose any
of these properties automatically.

> +	ret = max1720x_get_rsense(dev, info, data);
>  	if (ret)
> -		return dev_err_probe(dev, ret, "Failed to probe nvmem\n");
> +		return dev_err_probe(dev, ret, "Failed to get RSense\n");

You can either drop this error print, or the ones in
max1720x_get_rsense(). No need to print two errors.

Greetings,

-- Sebastian

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