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

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

Hi,

On 4/30/25 02:36, Sebastian Reichel wrote:
> Hi,
> 
> On Mon, Apr 21, 2025 at 08:13:33PM +0200, Thomas Antoine via B4 Relay wrote:
>> From: Thomas Antoine <t.antoine@uclouvain.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@uclouvain.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.
>

As I said to Dimitri Fedrau, this is actually a mistake on my part coming from
an attempt at getting things working which failed. Charge full design should
be correctly computed now so I don't think it would be useful to get it from
DT.
 
>> +	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

I will do that in v4.

Best regards,
Thomas