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@xxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxx/
> >
> > 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.
