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

Chris Morgan
2024-07-31T16:02:31Z

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@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@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.

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