Re: [PATCH v2 1/4] power: supply: add support for max77759 fuel gauge

["André Draszik" <andre.draszik@linaro.org>]

Hi Sebastian,

On Wed, 2025-01-15 at 22:30 +0100, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jan 02, 2025 at 12:15:03PM +0100, 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. The 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 and a difference in the way to identify the chip (the same register
> > is used but not the same mask).
> > 
> > Add support for the max77759 by allowing to use the non-volatile
> > memory or not based on the chip. Also add the V_CELL regsister as a
> > fallback to read voltage value in the case where read of V_BATT fails.
> > 
> > The cast is necessary to avoid an overflow when the value of the register
> > is above 54975 (equivalent to a voltage around 4.29 V).
> > 
> > The regmap of the max77759 will lead the read to fail for V_BATT and to
> > correctly use V_CELL instead. This 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>
> > ---
> 
> Please add output from to the cover letter to allow easily verifying
> that all values are correctly scaled.
> 
> ./tools/testing/selftests/power_supply/test_power_supply_properties.sh
> 
> > +static const struct regmap_access_table max77759_write_table = {
> > +	.yes_ranges = max77759_registers,
> > +	.n_yes_ranges = ARRAY_SIZE(max77759_registers),
> > +	.no_ranges = max77759_ro_registers,
> > +	.n_no_ranges = ARRAY_SIZE(max77759_ro_registers),
> > +};
> 
> Drop the yes_range from the write table. It is wrong and confusing.

Can you please clarify why having yes_ranges is wrong? Without yes_ranges,
all registers not in no_ranges are allowed to be written to.

Here, max77759_registers already specifies all the registers that exist
(and is also used in the max77759_read_table), and for write-access this is
further limited by the read-only registers in no_ranges.

As an example, register 0x50 doesn't exist, and without yes_ranges this
would allow write access to it.

If yes_ranges was dropped, all the information about non-existing registers
would have to be duplicated into no_ranges by inversing max77759_registers.
We already know the non-existing registers, and inversing that list just to
add to no_ranges seems non-ideal, error-prone, and just duplicated information.


Cheers,
Andre'