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

[Dragan Simic <dsimic@manjaro.org>]

Hello all,

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

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