[PATCH 0/2] power: supply: max17042_battery: Improve properties

[PATCH 0/2] power: supply: max17042_battery: Improve properties

[Wolfgang Wiedmeyer]

These two patches fix the capacity property and add the technology
property.

Wolfgang Wiedmeyer (2):
  power: supply: max17042_battery: use VF SOC register for capacity
    property
  power: supply: max17042_battery: add technology property support

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

-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[PATCH 1/2] power: supply: max17042_battery: use VF SOC register for capacity property

[Wolfgang Wiedmeyer]

The capacity property uses the RepSOC register to report the current state
of charge. This register did not provide a reliable SOC value during my
testing with the max17047 variant on a Galaxy S3 (Trats2/GT-I9300). The
reported value did not change or even stayed zero in some cases.
However, the VF SOC register provided an accurate SOC value at all times.
It uses the voltage fuel gauge to determine the SOC.

Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
---
 drivers/power/max17042_battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index da7a75f..20cb1fd 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -246,7 +246,7 @@ 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);
+		ret = regmap_read(map, MAX17042_VFSOC, &data);
 		if (ret < 0)
 			return ret;
 
-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

Re: [PATCH 1/2] power: supply: max17042_battery: use VF SOC register for capacity property

[Krzysztof Kozlowski]

On Sun, Sep 25, 2016 at 11:10:10PM +0200, Wolfgang Wiedmeyer wrote:
> The capacity property uses the RepSOC register to report the current state
> of charge. This register did not provide a reliable SOC value during my
> testing with the max17047 variant on a Galaxy S3 (Trats2/GT-I9300). The
> reported value did not change or even stayed zero in some cases.
> However, the VF SOC register provided an accurate SOC value at all times.
> It uses the voltage fuel gauge to determine the SOC.
> 
> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> ---
>  drivers/power/max17042_battery.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> index da7a75f..20cb1fd 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -246,7 +246,7 @@ 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);
> +		ret = regmap_read(map, MAX17042_VFSOC, &data);
>  		if (ret < 0)
>  			return ret;

The RepSOC is for ModelGauge m3 which requires current sense resistor. I
don't remember whether the resistor is present on Trats2. If not, then
m1 is used. However in both cases (m1 and m3) the battery
characteristics (cell information) should be loaded which in case of DT
driver is not supported.

Overall, I am not sure whether your change is correct. It might fix this
particular scenario because:
1. We are not providing the cell information,
2. We mre not providing the SNS resistor value so we are in m1 mode (if
there is no SNS resistor).  but it might break other applications where
SNS is present and cell configuration is provided. Unless you tested it
in such?

Probably this should be based on Device Tree property describing what is
configured (e.g. missing model data). Maybe existing maxim,rsns-microohm
could be used - in case of lack of it, fall back to reading VFSOC?

Best regards,
Krzysztof

Re: [PATCH 1/2] power: supply: max17042_battery: use VF SOC register for capacity property

[Wolfgang Wiedmeyer]

[-- Attachment #1: Type: text/plain, Size: 2549 bytes --]


Krzysztof Kozlowski writes:

> On Sun, Sep 25, 2016 at 11:10:10PM +0200, Wolfgang Wiedmeyer wrote:
>> The capacity property uses the RepSOC register to report the current state
>> of charge. This register did not provide a reliable SOC value during my
>> testing with the max17047 variant on a Galaxy S3 (Trats2/GT-I9300). The
>> reported value did not change or even stayed zero in some cases.
>> However, the VF SOC register provided an accurate SOC value at all times.
>> It uses the voltage fuel gauge to determine the SOC.
>> 
>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>> ---
>>  drivers/power/max17042_battery.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
>> index da7a75f..20cb1fd 100644
>> --- a/drivers/power/max17042_battery.c
>> +++ b/drivers/power/max17042_battery.c
>> @@ -246,7 +246,7 @@ 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);
>> +		ret = regmap_read(map, MAX17042_VFSOC, &data);
>>  		if (ret < 0)
>>  			return ret;
>
> The RepSOC is for ModelGauge m3 which requires current sense resistor. I
> don't remember whether the resistor is present on Trats2. If not, then
> m1 is used. However in both cases (m1 and m3) the battery
> characteristics (cell information) should be loaded which in case of DT
> driver is not supported.
>
> Overall, I am not sure whether your change is correct. It might fix this
> particular scenario because:
> 1. We are not providing the cell information,
> 2. We mre not providing the SNS resistor value so we are in m1 mode (if
> there is no SNS resistor).  but it might break other applications where
> SNS is present and cell configuration is provided. Unless you tested it
> in such?

I'm not able to test other applications than the Galaxy S3.

> Probably this should be based on Device Tree property describing what is
> configured (e.g. missing model data). Maybe existing maxim,rsns-microohm
> could be used - in case of lack of it, fall back to reading VFSOC?

Ok, I'll include a check if maxim,rsns-microohm exists and do the
fallback to VFSOC if it's not there.

Thanks,
Wolfgang

-- 
Website: https://fossencdi.org
Jabber: wolfgang@wiedmeyer.de
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[PATCH 2/2] power: supply: max17042_battery: add technology property support

[Wolfgang Wiedmeyer]

This patch reports the battery technology as Li-ion.

Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
---
 drivers/power/max17042_battery.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
index 20cb1fd..43cb5df 100644
--- a/drivers/power/max17042_battery.c
+++ b/drivers/power/max17042_battery.c
@@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
 	POWER_SUPPLY_PROP_TEMP_MIN,
 	POWER_SUPPLY_PROP_TEMP_MAX,
 	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
 	POWER_SUPPLY_PROP_CURRENT_NOW,
 	POWER_SUPPLY_PROP_CURRENT_AVG,
 };
@@ -296,6 +297,9 @@ static int max17042_get_property(struct power_supply *psy,
 		if (ret < 0)
 			return ret;
 		break;
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
 	case POWER_SUPPLY_PROP_CURRENT_NOW:
 		if (chip->pdata->enable_current_sense) {
 			ret = regmap_read(map, MAX17042_Current, &data);
-- 
Website: https://fossencdi.org
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

Re: [PATCH 2/2] power: supply: max17042_battery: add technology property support

[Krzysztof Kozlowski]

On Sun, Sep 25, 2016 at 11:10:11PM +0200, Wolfgang Wiedmeyer wrote:
> This patch reports the battery technology as Li-ion.
> 
> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> ---
>  drivers/power/max17042_battery.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> index 20cb1fd..43cb5df 100644
> --- a/drivers/power/max17042_battery.c
> +++ b/drivers/power/max17042_battery.c
> @@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
>  	POWER_SUPPLY_PROP_TEMP_MIN,
>  	POWER_SUPPLY_PROP_TEMP_MAX,
>  	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
>  	POWER_SUPPLY_PROP_CURRENT_NOW,
>  	POWER_SUPPLY_PROP_CURRENT_AVG,
>  };
> @@ -296,6 +297,9 @@ static int max17042_get_property(struct power_supply *psy,
>  		if (ret < 0)
>  			return ret;
>  		break;
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;

How can you be sure it is always Li-Ion? For wearables and mobiles, rather yes, but
the driver is also used in other devices. Technically, specs are saying
it might be used also with Li-Poly applications.

Best regards,
Krzysztof

Re: [PATCH 2/2] power: supply: max17042_battery: add technology property support

[Wolfgang Wiedmeyer]

[-- Attachment #1: Type: text/plain, Size: 1698 bytes --]


Krzysztof Kozlowski writes:

> On Sun, Sep 25, 2016 at 11:10:11PM +0200, Wolfgang Wiedmeyer wrote:
>> This patch reports the battery technology as Li-ion.
>> 
>> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
>> ---
>>  drivers/power/max17042_battery.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
>> index 20cb1fd..43cb5df 100644
>> --- a/drivers/power/max17042_battery.c
>> +++ b/drivers/power/max17042_battery.c
>> @@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
>>  	POWER_SUPPLY_PROP_TEMP_MIN,
>>  	POWER_SUPPLY_PROP_TEMP_MAX,
>>  	POWER_SUPPLY_PROP_HEALTH,
>> +	POWER_SUPPLY_PROP_TECHNOLOGY,
>>  	POWER_SUPPLY_PROP_CURRENT_NOW,
>>  	POWER_SUPPLY_PROP_CURRENT_AVG,
>>  };
>> @@ -296,6 +297,9 @@ static int max17042_get_property(struct power_supply *psy,
>>  		if (ret < 0)
>>  			return ret;
>>  		break;
>> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
>> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>
> How can you be sure it is always Li-Ion? For wearables and mobiles, rather yes, but
> the driver is also used in other devices. Technically, specs are saying
> it might be used also with Li-Poly applications.

I suppose that there is no way to detect this. Would it be ok if I add
an optional Device Tree property that allows to specify if it's Li-Ion
or Li-Poly? If the property is not supplied, then "unknown" is returned.

Thanks,
Wolfgang

-- 
Website: https://fossencdi.org
Jabber: wolfgang@wiedmeyer.de
OpenPGP: 0F30 D1A0 2F73 F70A 6FEE  048E 5816 A24C 1075 7FC4
Key download: https://wiedmeyer.de/keys/ww.asc

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH 2/2] power: supply: max17042_battery: add technology property support

[Krzysztof Kozlowski]

On Mon, Sep 26, 2016 at 02:56:44PM +0200, Wolfgang Wiedmeyer wrote:
> 
> Krzysztof Kozlowski writes:
> 
> > On Sun, Sep 25, 2016 at 11:10:11PM +0200, Wolfgang Wiedmeyer wrote:
> >> This patch reports the battery technology as Li-ion.
> >> 
> >> Signed-off-by: Wolfgang Wiedmeyer <wolfgit@wiedmeyer.de>
> >> ---
> >>  drivers/power/max17042_battery.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >> 
> >> diff --git a/drivers/power/max17042_battery.c b/drivers/power/max17042_battery.c
> >> index 20cb1fd..43cb5df 100644
> >> --- a/drivers/power/max17042_battery.c
> >> +++ b/drivers/power/max17042_battery.c
> >> @@ -92,6 +92,7 @@ static enum power_supply_property max17042_battery_props[] = {
> >>  	POWER_SUPPLY_PROP_TEMP_MIN,
> >>  	POWER_SUPPLY_PROP_TEMP_MAX,
> >>  	POWER_SUPPLY_PROP_HEALTH,
> >> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> >>  	POWER_SUPPLY_PROP_CURRENT_NOW,
> >>  	POWER_SUPPLY_PROP_CURRENT_AVG,
> >>  };
> >> @@ -296,6 +297,9 @@ static int max17042_get_property(struct power_supply *psy,
> >>  		if (ret < 0)
> >>  			return ret;
> >>  		break;
> >> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> >> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> >
> > How can you be sure it is always Li-Ion? For wearables and mobiles, rather yes, but
> > the driver is also used in other devices. Technically, specs are saying
> > it might be used also with Li-Poly applications.
> 
> I suppose that there is no way to detect this. Would it be ok if I add
> an optional Device Tree property that allows to specify if it's Li-Ion
> or Li-Poly? If the property is not supplied, then "unknown" is returned.

I am not sure in such case what will be the benefit of exposing this to
user-space... but it won't harm neither and sounds like a valid usage of
DT properties. Fine with me.

Best regards,
Krzysztof