[PATCH 2/5] power: supply: cw2015: Add support for dual-cell configurations

[Sebastian Reichel]

Hi,

On Fri, Aug 02, 2024 at 11:39:24PM GMT, Dragan Simic wrote:
> On 2024-07-31 19:02, Sebastian Reichel wrote:
> > On Wed, Jul 31, 2024 at 11:02:11AM GMT, Chris Morgan wrote:
> > > On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote:
> > > > On Fri, Jul 26, 2024 at 02:49:45PM GMT, Chris Morgan wrote:
> > > > > From: Chris Morgan <macromorgan at hotmail.com>
> > > > >
> > > > > The Cellwise cw2015 datasheet reports that it can handle two cells
> > > > > in a series configuration. Allow a device tree parameter to note
> > > > > this condition so that the driver reports the correct voltage values
> > > > > to userspace.
> > > >
> > > > I found this:
> > > >
> > > > http://www.cellwise-semi.com/Public/assests/menu/20230302/6400076806706.pdf
> > > >
> > > > Which says:
> > > >
> > > >   CW2015 can be used in 2 or more batteries connected in series, or
> > > >   several cells connected in parallel.
> > > >
> > > > So dual-cell seems like a bad property name. Instead the number of
> > > > serial cells should be provided. This property is then not really
> > > > specific to the Cellwise fuel gauge and instead a property of the
> > > > battery pack (i.e. simple-battery.yaml).
> > > 
> > > It's conflicting information (which further confuses me). I see in
> > > that
> > > datasheet it says 2 or more, whereas the datasheet found here says
> > > only
> > > 2 cells:
> > > 
> > > https://www.lestat.st/_media/informatique/projets/python-cw2015/cw2015-power-management-datasheet.pdf
> > > 
> > > But I agree in principle that we should be setting this as a property
> > > of a simple-battery rather than a manufacturer specific parameter.
> > > 
> > > >
> > > > > Signed-off-by: Chris Morgan <macromorgan at hotmail.com>
> > > > > ---
> > > > >  drivers/power/supply/cw2015_battery.c | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >
> > > > > diff --git a/drivers/power/supply/cw2015_battery.c b/drivers/power/supply/cw2015_battery.c
> > > > > index f63c3c410451..b23a6d4fa4fa 100644
> > > > > --- a/drivers/power/supply/cw2015_battery.c
> > > > > +++ b/drivers/power/supply/cw2015_battery.c
> > > > > @@ -77,6 +77,8 @@ struct cw_battery {
> > > > >  	u32 poll_interval_ms;
> > > > >  	u8 alert_level;
> > > > >
> > > > > +	bool dual_cell;
> > > > > +
> > > > >  	unsigned int read_errors;
> > > > >  	unsigned int charge_stuck_cnt;
> > > > >  };
> > > > > @@ -325,6 +327,9 @@ static int cw_get_voltage(struct cw_battery *cw_bat)
> > > > >  	 */
> > > > >  	voltage_mv = avg * 312 / 1024;
> > > > >
> > > > > +	if (cw_bat->dual_cell)
> > > > > +		voltage_mv *= 2;
> > > >
> > > > Unfortunately there are no details in the document, but this looks
> > > > very fishy. Does it only measure the first cell and hope that the
> > > > other cells have the same voltage?
> > > >
> > > > This (unmerged) series also applies to your problem to some degree:
> > > >
> > > > https://lore.kernel.org/all/20240416121818.543896-3-mike.looijmans@topic.nl/
> > > 
> > > I think based on the application diagram it is in fact measuring the
> > > cell voltage.
> > > 
> > > That said, this ultimately boils down to a cosmetic thing
> > > as not having this property simply means userspace sees the battery
> > > voltage as 3.8v instead of 7.6v as is written on the side.
> > 
> > With the cells being connected in serial, the voltage of both cells
> > can be different. There is not "the cell voltage". Instead there is
> > a voltage for cell 1 and a voltage for cell 2. In a perfect battery
> > they are the same, but in reality they are not. In the extreme case
> > one of the cells may be broken while the other is still fine. It
> > sounds as if this is just measuring the voltage from the first
> > cell and assumes the second cell has the same voltage.
> > 
> > Ideally the voltage on these platforms is not exposed via the normal
> > VOLTAGE property and instead uses a new property for telling
> > userspace the voltage for a single cell. The kernel simply does not
> > know the voltage of the whole battery pack.
> > 
> > FWIW this is the worst battery measurement system I've seen so far
> > if my understanding of the hardware design is correct.
> 
> Please note that two facts should be considered here:
> 
>  - The GenBook schematic [1] clearly shows that the individual battery
>    cells aren't exposed at its internal battery connector and, as a
>    result, aren't available for individual cell voltage monitoring
> 
>  - The GenBook uses a CW2013 as it fuel gauge, [1] instead of CW2015
>    as mentioned here a few times, but I haven't went through the CW2013
>    datasheet(s) yet to see what are the actual differences between it
>    and the CW2015
> 
> [1] https://wiki.cool-pi.com/notebook/coolpi-genbook-v20.pdf

Ah, thanks for pointing to the schematics. So the fuel gauge can
only measure the voltage of the whole battery, but VCELL register
provides the voltage for 1 cell? What is the origin of the dual-cell
property? Was this something you came up with yourself when noticing
the wrong voltage?

Based on the above information my guess would be that CW2013 uses a
different voltage resolution than CW2015 for the VCELL register. The
datasheet for CW2015 says 14bit ADC with 305uV resolution. Maybe
the CW2013 simply uses a different ADC. Do you have the datasheet
for the chip?

-- Sebastian
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-rockchip/attachments/20240804/a08fdece/attachment.sig>