On Fri, Jul 26, 2024 at 11:06:21PM +0200, Sebastian Reichel wrote: > Hi, > > 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. I think for simplification sake I will remove this from the series, add a note to the device tree, and wait for the other battery series to get pulled. When it gets pulled I'll request a device tree property so we can add POWER_SUPPLY_PROP_NUMBER_OF_SERIAL_CELLS to the simple-battery and then parse this value. Or if that series ends up getting abandoned I can just add a quick and dirty new property like this. Thank you, Chris > > -- Sebastian > > > dev_dbg(cw_bat->dev, "Read voltage: %d mV, raw=0x%04x\n", > > voltage_mv, reg_val); > > return voltage_mv; > > @@ -587,6 +592,8 @@ static int cw2015_parse_properties(struct cw_battery *cw_bat) > > return ret; > > } > > > > + cw_bat->dual_cell = device_property_read_bool(dev, "cellwise,dual-cell"); > > + > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", > > &cw_bat->poll_interval_ms); > > if (ret) { > > -- > > 2.34.1 > > > >
