[PATCH 0/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[PATCH 0/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Henrik Grimler]

Hi,

This fixes so that the capacity is correctly reported for Galaxy S3.
It is a v2 of a patch sent by Wolfgang Wiedmeyer some years ago [1].
I have not been able to get in contact with him, but have put him as
Suggested-by acknowledge that it is his fix.  Please let me know if
Co-developed-by, or some other tag combination, is more appropriate
for giving credit in this case.

[1] https://lkml.org/lkml/2016/9/25/195

Best regards,
Henrik Grimler

Henrik Grimler (1):
  power: supply: max17042_battery: use VFSOC for capacity when no rsns

 drivers/power/supply/max17042_battery.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


base-commit: 6a7ababc0268063d0798c46d5859a90ee996612f
-- 
2.33.0

[PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Henrik Grimler]

On Galaxy S3 (i9300/i9305), which has the max17047 fuel gauge and no
current sense resistor (rsns), the RepSOC register does not provide an
accurate state of charge value. The reported value is wrong, and does
not change over time. VFSOC however, which uses the voltage fuel gauge
to determine the state of charge, always shows an accurate value.

At least one max170xx driver, found in Asus' Z00D kernel [1], chooses
how to get the capacity based on if current sense is available or not.
Lets change the mainline driver to match the Asus Z00D driver's
behaviour and thereby fix so that correct state of charge values are
obtained on Galaxy S3.

[1] https://github.com/LineageOS/android_kernel_asus_Z00D/blob/c7ab0e3ec5b5/drivers/power/max17042_battery.c#L1103-L1105

Suggested-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
Signed-off-by: Henrik Grimler <henrik@grimler.se>
---
 drivers/power/supply/max17042_battery.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index 622bdae6182c..7233670978d0 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -317,7 +317,10 @@ static int max17042_get_property(struct power_supply *psy,
 		val->intval = data * 625 / 8;
 		break;
 	case POWER_SUPPLY_PROP_CAPACITY:
-		ret = regmap_read(map, MAX17042_RepSOC, &data);
+		if (chip->pdata->enable_current_sense)
+			ret = regmap_read(map, MAX17042_RepSOC, &data);
+		else
+			ret = regmap_read(map, MAX17042_VFSOC, &data);
 		if (ret < 0)
 			return ret;
 
-- 
2.33.0

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Krzysztof Kozlowski]

On 19/09/2021 22:07, Henrik Grimler wrote:
> On Galaxy S3 (i9300/i9305), which has the max17047 fuel gauge and no
> current sense resistor (rsns), the RepSOC register does not provide an
> accurate state of charge value. The reported value is wrong, and does
> not change over time. VFSOC however, which uses the voltage fuel gauge
> to determine the state of charge, always shows an accurate value.
> 
> At least one max170xx driver, found in Asus' Z00D kernel [1], chooses
> how to get the capacity based on if current sense is available or not.
> Lets change the mainline driver to match the Asus Z00D driver's
> behaviour and thereby fix so that correct state of charge values are
> obtained on Galaxy S3.
> 
> [1] https://github.com/LineageOS/android_kernel_asus_Z00D/blob/c7ab0e3ec5b5/drivers/power/max17042_battery.c#L1103-L1105
> 
> Suggested-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> Signed-off-by: Henrik Grimler <henrik@grimler.se>
> ---
>  drivers/power/supply/max17042_battery.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index 622bdae6182c..7233670978d0 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -317,7 +317,10 @@ static int max17042_get_property(struct power_supply *psy,
>  		val->intval = data * 625 / 8;
>  		break;
>  	case POWER_SUPPLY_PROP_CAPACITY:
> -		ret = regmap_read(map, MAX17042_RepSOC, &data);
> +		if (chip->pdata->enable_current_sense)
> +			ret = regmap_read(map, MAX17042_RepSOC, &data);
> +		else
> +			ret = regmap_read(map, MAX17042_VFSOC, &data);

Thanks for the patch. I found also my comments to Wolfgang's patch in
2016, which you resolve here but I have more. :)

I think my previous message about current sense are not correct. What is
important is whether ModelGauge is being used/configured. For example
none of DT platforms support it but ACPI might.

There is incoming patch around it:
https://lore.kernel.org/lkml/5702731.UytLkSCjyO@pliszka/

If you switch to VSSoc, I think you need to modify the SOC Alert Config
in MiscCFG register (bits 0:1 to 0x1). Otherwise the alerts will be
generated on different value.

Different topic:
When touching Exynos-based boards (like Galaxy S3), please Cc me as
well, even if I don't pop up in the maintainers.

For max17042 we need to Cc broader group of users, for example using it
in ACPI platforms. The best is to pick the contributors.

Best regards,
Krzysztof

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Henrik Grimler]

On Fri, Sep 24, 2021 at 01:45:29PM +0200, Krzysztof Kozlowski wrote:
> Thanks for the patch. I found also my comments to Wolfgang's patch in
> 2016, which you resolve here but I have more. :)
> 
> I think my previous message about current sense are not correct. What is
> important is whether ModelGauge is being used/configured. For example
> none of DT platforms support it but ACPI might.
> There is incoming patch around it:
> https://lore.kernel.org/lkml/5702731.UytLkSCjyO@pliszka/
> 
> If you switch to VSSoc, I think you need to modify the SOC Alert Config
> in MiscCFG register (bits 0:1 to 0x1). Otherwise the alerts will be
> generated on different value.

So, 0x1 should correspond to AvSOC (i.e. non-filtered RepSOC), while
right now we write 0x3 (VFSOC) to MiscCFG for devices without current
sense [1]. Could you elaborate on why AvSOC should be used for alert
if we use VFSOC to get PROP_CAPACITY?

On this particular device it does not seem to make a difference what I
use for the SOC alert, the alert triggers all the time in any case
since RepSOC does not give an accurate value. Supposedly this happens
because ModelGauge configuration is incomplete, as you said. Looking
at the registers used by the ModelGauge it seems that only the
"characterization table" at 0x80 - 0xAF is missing. The rest (FullCap,
DesignCap, ICHGTerm, ..) are set to the same values as with vendor
kernel.

> Different topic:
> When touching Exynos-based boards (like Galaxy S3), please Cc me as
> well, even if I don't pop up in the maintainers.
> 
> For max17042 we need to Cc broader group of users, for example using it
> in ACPI platforms. The best is to pick the contributors.

Thanks for taking the time to explain this.

> Best regards,
> Krzysztof

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/power/supply/max17042_battery.c#n1092

Best regards,
Henrik Grimler

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Krzysztof Kozlowski]

On 27/09/2021 21:05, Henrik Grimler wrote:
> On Fri, Sep 24, 2021 at 01:45:29PM +0200, Krzysztof Kozlowski wrote:
>> Thanks for the patch. I found also my comments to Wolfgang's patch in
>> 2016, which you resolve here but I have more. :)
>>
>> I think my previous message about current sense are not correct. What is
>> important is whether ModelGauge is being used/configured. For example
>> none of DT platforms support it but ACPI might.
>> There is incoming patch around it:
>> https://lore.kernel.org/lkml/5702731.UytLkSCjyO@pliszka/
>>
>> If you switch to VSSoc, I think you need to modify the SOC Alert Config
>> in MiscCFG register (bits 0:1 to 0x1). Otherwise the alerts will be
>> generated on different value.
> 
> So, 0x1 should correspond to AvSOC (i.e. non-filtered RepSOC), while
> right now we write 0x3 (VFSOC) to MiscCFG for devices without current
> sense [1]. Could you elaborate on why AvSOC should be used for alert
> if we use VFSOC to get PROP_CAPACITY?

I meant that same measurement should be used for both: for PROP_CAPACITY
and for alerts.

I double checked the driver and your change is actually aligned with it.
If !enable_current_sense, the driver will set MiscCFG to 0x3 to use
VFSOC for alerts. I think you can ignore that part of my comment before.

However still remaining issue is that switching to VFSoc should happen
not only if !enable_current_sense but also if ModelGauge m3 is not
configured.

> On this particular device it does not seem to make a difference what I
> use for the SOC alert, the alert triggers all the time in any case
> since RepSOC does not give an accurate value. Supposedly this happens
> because ModelGauge configuration is incomplete, as you said. Looking
> at the registers used by the ModelGauge it seems that only the
> "characterization table" at 0x80 - 0xAF is missing. The rest (FullCap,
> DesignCap, ICHGTerm, ..) are set to the same values as with vendor
> kernel.

Are you sure? I could not find setting of these (e.g.
MAX17042_FullCAP/config->fullcap) for a DT platform.

Best regards,
Krzysztof

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Henrik Grimler]

On Tue, Sep 28, 2021 at 11:18:27AM +0200, Krzysztof Kozlowski wrote:
> On 27/09/2021 21:05, Henrik Grimler wrote:
> > On Fri, Sep 24, 2021 at 01:45:29PM +0200, Krzysztof Kozlowski wrote:
> >> If you switch to VSSoc, I think you need to modify the SOC Alert Config
> >> in MiscCFG register (bits 0:1 to 0x1). Otherwise the alerts will be
> >> generated on different value.
> > 
> > So, 0x1 should correspond to AvSOC (i.e. non-filtered RepSOC), while
> > right now we write 0x3 (VFSOC) to MiscCFG for devices without current
> > sense [1]. Could you elaborate on why AvSOC should be used for alert
> > if we use VFSOC to get PROP_CAPACITY?
> 
> I meant that same measurement should be used for both: for PROP_CAPACITY
> and for alerts.
> 
> I double checked the driver and your change is actually aligned with it.
> If !enable_current_sense, the driver will set MiscCFG to 0x3 to use
> VFSOC for alerts. I think you can ignore that part of my comment before.

Makes sense, thanks!

> However still remaining issue is that switching to VFSoc should happen
> not only if !enable_current_sense but also if ModelGauge m3 is not
> configured.

If I manage to get ModelGauge working on this device in the future I
can address this.

> > On this particular device it does not seem to make a difference what I
> > use for the SOC alert, the alert triggers all the time in any case
> > since RepSOC does not give an accurate value. Supposedly this happens
> > because ModelGauge configuration is incomplete, as you said. Looking
> > at the registers used by the ModelGauge it seems that only the
> > "characterization table" at 0x80 - 0xAF is missing. The rest (FullCap,
> > DesignCap, ICHGTerm, ..) are set to the same values as with vendor
> > kernel.
> 
> Are you sure? I could not find setting of these (e.g.
> MAX17042_FullCAP/config->fullcap) for a DT platform.

Actually, it seems that the registers are set to the default Power-On
Reset (POR) values in both mainline and vendor kernel.  Printing all
the Cell Characterization Information Registers (given in Table 1 in
the MAX17047 datasheet) with something like:

    u32 tmp;
    regmap_read(chip->regmap, MAX17042_FullCAP, &tmp);
    dev_err(&chip->client->dev, "Fullcap %04x\n", tmp);

in both vendor kernel and mainline gives the same values:

    Fullcap    07d0
    DesignCap  07d0
    ICHGTerm   03c0
    FullCapNom 0667
    RCOMP0     0065
    Iavg_empty 0780
    TempCo     0930
    QRes 00    1e2f
    QRes 10    1e00
    QRes 20    1306
    QRes 30    0c00

and these are the POR values (seen in Table 5 of the datasheet).  Only
difference between vendor and mainline is that MAX17042_MODELChrTbl is
all zeros on mainline, while with vendor kernel I get something like:

    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
    00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 
    00 50 05 c0 a0 85 a9 f1 e0 5d 84 f1 01 00 00 00

and the values here change over time as well, as the algorithm learns
about the battery(?).

Maybe this means that the ModelGauge algorithm is not configured with
vendor kernel either, and that a full battery characterization
(described in [1]) is needed if we are to use ModelGauge.  ModelGauge
is also not mentioned in the vendor kernel driver for max17047 [2],
but it is mentioned in the very similar max17042 [3] and max17050
drivers in the same kernel.

> Best regards,
> Krzysztof

[1] https://pdfserv.maximintegrated.com/en/an/AN4799.pdf
[2] https://github.com/LineageOS/android_kernel_samsung_smdk4412/blob/2489007e7d74/drivers/battery/max17047_fuelgauge.c
[3] https://github.com/LineageOS/android_kernel_samsung_smdk4412/blob/2489007e7d74/drivers/battery/max17042_fuelgauge.c

Best regards,
Henrik Grimler

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Krzysztof Kozlowski]

On 19/09/2021 22:07, Henrik Grimler wrote:
> On Galaxy S3 (i9300/i9305), which has the max17047 fuel gauge and no
> current sense resistor (rsns), the RepSOC register does not provide an
> accurate state of charge value. The reported value is wrong, and does
> not change over time. VFSOC however, which uses the voltage fuel gauge
> to determine the state of charge, always shows an accurate value.
> 
> At least one max170xx driver, found in Asus' Z00D kernel [1], chooses
> how to get the capacity based on if current sense is available or not.
> Lets change the mainline driver to match the Asus Z00D driver's
> behaviour and thereby fix so that correct state of charge values are
> obtained on Galaxy S3.
> 
> [1] https://github.com/LineageOS/android_kernel_asus_Z00D/blob/c7ab0e3ec5b5/drivers/power/max17042_battery.c#L1103-L1105
> 
> Suggested-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> Signed-off-by: Henrik Grimler <henrik@grimler.se>
> ---
>  drivers/power/supply/max17042_battery.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 

After explanation and double-checking of driver this makes sense. The
driver already uses VFSoc for alerts (MiscCFG register) if
!enable_current_sense.

Fixes: 359ab9f5b154 ("power_supply: Add MAX17042 Fuel Gauge Driver")
Cc: <stable@vger.kernel.org>

These could be added when applying but maybe Sebastian wants a v2 with it?

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

Best regards,
Krzysztof

Re: [PATCH 1/1] power: supply: max17042_battery: use VFSOC for capacity when no rsns

[Henrik Grimler]

On Tue, Sep 28, 2021 at 11:22:15AM +0200, Krzysztof Kozlowski wrote:
> On 19/09/2021 22:07, Henrik Grimler wrote:
> > On Galaxy S3 (i9300/i9305), which has the max17047 fuel gauge and no
> > current sense resistor (rsns), the RepSOC register does not provide an
> > accurate state of charge value. The reported value is wrong, and does
> > not change over time. VFSOC however, which uses the voltage fuel gauge
> > to determine the state of charge, always shows an accurate value.
> > 
> > At least one max170xx driver, found in Asus' Z00D kernel [1], chooses
> > how to get the capacity based on if current sense is available or not.
> > Lets change the mainline driver to match the Asus Z00D driver's
> > behaviour and thereby fix so that correct state of charge values are
> > obtained on Galaxy S3.
> > 
> > [1] https://github.com/LineageOS/android_kernel_asus_Z00D/blob/c7ab0e3ec5b5/drivers/power/max17042_battery.c#L1103-L1105
> > 
> > Suggested-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> > Signed-off-by: Henrik Grimler <henrik@grimler.se>
> > ---
> >  drivers/power/supply/max17042_battery.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> 
> After explanation and double-checking of driver this makes sense. The
> driver already uses VFSoc for alerts (MiscCFG register) if
> !enable_current_sense.
> 
> Fixes: 359ab9f5b154 ("power_supply: Add MAX17042 Fuel Gauge Driver")
> Cc: <stable@vger.kernel.org>
> 
> These could be added when applying but maybe Sebastian wants a v2 with it?
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>

I can send a v2 with updated commit message to mention that we already
use VFSOC for the alert on this device, and sneak in a patch to fix a
typo in Iavg_empty parameter.

> Best regards,
> Krzysztof

Best regards,
Henrik Grimler