Message-ID: <033fe5e9d397db366b2fafaef152d20fc4a03107.camel@fi.rohmeurope.com>
Date:   Fri, 27 Nov 2020 09:07:55 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "mazziesaccount@...il.com" <mazziesaccount@...il.com>
CC:     "krzk@...nel.org" <krzk@...nel.org>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "digetx@...il.com" <digetx@...il.com>,
        "sebastian.reichel@...labora.com" <sebastian.reichel@...labora.com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>
Subject: Re: [RFC PATCH 2/2] power: supply: add sw-gauge for SOC estimation
 and CC correction

On Thu, 2020-11-26 at 12:13 +0200, Matti Vaittinen wrote:
> Add generic 'sw gauge' helper for performing iterative SOC estimation
> and coulomb counter correction for devices with a (drifting) coulomb
> counter. This should allow few charger/fuel-gauge drivers to use
> generic
> loop instead of implementing their own.
> 
> Charger/fuel-gauge drivers can register 'sw-gauge' which does
> periodically
> poll the driver and:
>  - get battery state
>  - adjust coulomb counter value (to fix drifting caused for example
> by ADC
>    offset) if:
>      - Battery is relaxed and OCV<=>SOC table is given
>      - Battery is full charged
>  - get battery age (cycles) from driver
>  - get battery temperature
>  - do battery capacity correction
>      - by battery temperature
>      - by battery age
>      - by computed Vbat/OCV difference at low-battery condition if
>        low-limit is set and OCV table given
>      - by IC specific low-battery correction if provided
>  - compute current State Of Charge (SOC)
>  - do periodical calibration if IC supports that. (Many ICs do
> calibration
>    of CC by shorting the ADC pins and getting the offset).
> 
> The SW gauge provides the last computed SOC as
> POWER_SUPPLY_PROP_CAPACITY
> to power_supply_class when requested.
> 
> Things that should/could be added but are missing from this commit:
>  - Support starting calibration in HW when entering to suspend. This
>    is useful for ICs supporting delayed calibration to mitigate CC
> error
>    during suspend - and to make periodical wake-up less critical.
>  - provide the user-space a consistent interface for getting/setting
> the
>    battery-cycle information for ICs which can't store the battery
> aging
>    information (like how many times battery has been charged to full)
>    over reset. Should POWER_SUPPLY_PROP_CYCLE_COUNT be used?
>  - periodical wake-up for performing SOC estimation computation (RTC
>    integration)
> 
> Signed-off-by: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>
> ---
> 
> Please do not spend hours with full formal review. The code here is
> provided to explain what I thought of working with and needs proper
> testing prior full review can be beneficial. I hope this still helps
> to explain what I have on mind. If this is seen worth the further
> work - then I will rework and test this + send a proper code for
> review. This one was only compile tested. I just hope to get opinion
> whether I should put further work on this or not :)
> 
>  drivers/power/supply/Kconfig                |   8 +
>  drivers/power/supply/Makefile               |   1 +
>  drivers/power/supply/power_supply_swgauge.c | 808
> ++++++++++++++++++++
>  include/linux/power/sw_gauge.h              | 203 +++++
>  include/linux/power_supply.h                |   6 +
>  5 files changed, 1026 insertions(+)
>  create mode 100644 drivers/power/supply/power_supply_swgauge.c
>  create mode 100644 include/linux/power/sw_gauge.h
> 

// Snip

> +
> +static int adjust_cc_full(struct sw_gauge *sw)
> +{
> +	int ret, full_uah;
> +
> +	if (sw->ops.get_uah_cc_full)
> 
> +	if (sw->ops.get_cycle) {+		ret = sw-
> >ops.get_uah_cc_full(sw, &full_uah);

I started converting the ROHM BD71827/BD71828/BD71815 drivers to this
format to get more concrete version... And I noticed I had completely
brainfarted here. Sorry! The idea is of course to set the new CC
value based on information that battery was fully charged. Eg, idea is
to allow driver to not implement get_uah_cc_full but
get_uah_cc_from_full() - to retrieve CC lost since charger informed
battery is full. 

> +	else
> +		ret = sw->ops.get_uah_cc(sw, &full_uah);
This one is nonsense too. If no get_uah_cc_from_full() is implemented
we assume battery is full right now. What we do next is substract the
uAh's lost since full from designed (and age/temp corrected? TBD)
battery capacity and set this value to CC.

I was after something like:
full_uah = sw->designed_cap - uah_from_full;

As I said - this RFC is not for final review but this particular error
probably obfuscated the logic completely :| Sorry!

> +	/*
> +	 * ROHM algorithm adjusts CC here based on designed capacity
> and not
> +	 * based on age/temperature corrected capacity.
> +	 *
> +	 * Let's try to use age-compensated capacity unlike original
> algorithm.
> +	 */
> +	ret = age_correct_cap(sw, &full_uah);
> +	if (ret) {
> +		pr_err("Age correction of battery failed\n");
> +		return ret;
> +	}
> +	return sw->ops.update_cc_uah(sw, full_uah);
> +}
> +/*
> + * Some charger ICs keep count of battery charge systems but can
> only store
> + * one or few cycles. They may need to clear the cycle counter and
> update
> + * counter in SW. This function fetches the counter from HW and
> allows HW to
> + * clear IC counter if needed.
> + *
> + * TODO: Add sysfs to export counter value to user-space and to
> allow
> + * user-space to set it after reset.

And while I was anyways writing this reply - this comment was leftover
from the hours before I noticed we already have the CYCLE information
is psy. We should (probably) use the existing set/get user-space IF for
cycles. (but more on that in next version :] )

// Snip



Br,
	Matti


--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)

Powered by blists - more mailing lists