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@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? 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
Attachment:
signature.asc
Description: PGP signature
