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

[Chris Morgan]

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

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