[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <9264729152cb70ec910b391ab4ced82dc62f953c.camel@linaro.org>
Date: Mon, 07 Apr 2025 12:29:09 +0100
From: André Draszik <andre.draszik@...aro.org>
To: Sebastian Reichel <sebastian.reichel@...labora.com>,
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>, 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 v2 1/4] power: supply: add support for max77759 fuel
gauge
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@...ouvain.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@...ouvain.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'
Powered by blists - more mailing lists