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

[Dragan Simic]

Hello Sebastian,

(I'm adding a couple of Cool Pi contacts to the Cc list.)

On 2024-08-04 18:29, Sebastian Reichel wrote:
> 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?

I'm not sure where does the idea for the dual-cell property come from,
but please note that it wasn't me who invented it, so to speak. :)
I don't even have the required hardware to test and develop these 
patches
on, i.e. I don't have a CM5-based GenBook.

> 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?

I've managed to find a few (seemingly a bit different) CW2013 
datasheets,
but as usual with CellWise, some of them may contain a bit confusing or
incomplete information.  I'd suggest that you wait for a couple of days
until I sift through the obtained datasheets, to save you from doing the
same. :)  Of course, I'll then send over the datasheet that seems 
correct
and the most complete to me.

I'll see to add support for CW2013 to the cw2015 driver, for which I
already have a couple of pending cleanup patches.