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

[Sebastian Reichel]

Hi,

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

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.

-- Sebastian

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