[PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Baolin Wang]

Introduce one property to present the battery internal resistance for battery
information.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - New patch in v2.
---
 .../devicetree/bindings/power/supply/battery.txt   |    2 ++
 drivers/power/supply/power_supply_core.c           |    3 +++
 include/linux/power_supply.h                       |    1 +
 3 files changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index f4d3b4a..25b9d2e 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -22,6 +22,7 @@ Optional Properties:
  - charge-term-current-microamp: current for charge termination phase
  - constant-charge-current-max-microamp: maximum constant input current
  - constant-charge-voltage-max-microvolt: maximum constant input voltage
+ - internal-resistance-micro-ohms: battery internal resistance
 
 Battery properties are named, where possible, for the corresponding
 elements in enum power_supply_property, defined in
@@ -42,6 +43,7 @@ Example:
 		charge-term-current-microamp = <128000>;
 		constant-charge-current-max-microamp = <900000>;
 		constant-charge-voltage-max-microvolt = <4200000>;
+		internal-resistance-micro-ohms = <250000>;
 	};
 
 	charger: charger@11 {
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index e853618..9f3452f 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -579,6 +579,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	info->charge_term_current_ua         = -EINVAL;
 	info->constant_charge_current_max_ua = -EINVAL;
 	info->constant_charge_voltage_max_uv = -EINVAL;
+	info->internal_resistance_uohm       = -EINVAL;
 
 	if (!psy->of_node) {
 		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
@@ -616,6 +617,8 @@ int power_supply_get_battery_info(struct power_supply *psy,
 			     &info->constant_charge_current_max_ua);
 	of_property_read_u32(battery_np, "constant_charge_voltage_max_microvolt",
 			     &info->constant_charge_voltage_max_uv);
+	of_property_read_u32(battery_np, "internal-resistance-micro-ohms",
+			     &info->internal_resistance_uohm);
 
 	return 0;
 }
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index f807691..019452d 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -326,6 +326,7 @@ struct power_supply_battery_info {
 	int charge_term_current_ua;	    /* microAmps */
 	int constant_charge_current_max_ua; /* microAmps */
 	int constant_charge_voltage_max_uv; /* microVolts */
+	int internal_resistance_uohm;       /* microOhms */
 };
 
 extern struct atomic_notifier_head power_supply_notifier;
-- 
1.7.9.5

[PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Baolin Wang]

Some battery driver will use the open circuit voltage (OCV) value to look
up the corresponding battery capacity percent in one certain degree Celsius.
Thus this patch provides some battery properties to present the OCV table
temperatures and OCV capacity table values.

Suggested-by: Sebastian Reichel <sre@kernel.org>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - New patch in v2.
---
 .../devicetree/bindings/power/supply/battery.txt   |   14 +++++
 drivers/power/supply/power_supply_core.c           |   63 +++++++++++++++++++-
 include/linux/power_supply.h                       |   11 ++++
 3 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index 25b9d2e..ac2b303 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -23,6 +23,16 @@ Optional Properties:
  - constant-charge-current-max-microamp: maximum constant input current
  - constant-charge-voltage-max-microvolt: maximum constant input voltage
  - internal-resistance-micro-ohms: battery internal resistance
+ - ocv-capacity-table-0: An array providing the battery capacity percent
+   with corresponding open circuit voltage (OCV) of the battery, which
+   is used to look up battery capacity according to current OCV value.
+ - ocv-capacity-table-1: Same as ocv-capacity-table-0
+ ......
+ - ocv-capacity-table-n: Same as ocv-capacity-table-0
+ - ocv-capacity-table-temperatures: An array containing the temperature
+   in degree Celsius, for each of the battery capacity lookup table.
+   The first temperature value specifies the OCV table 0, and the second
+   temperature value specifies the OCV table 1, and so on.
 
 Battery properties are named, where possible, for the corresponding
 elements in enum power_supply_property, defined in
@@ -44,6 +54,10 @@ Example:
 		constant-charge-current-max-microamp = <900000>;
 		constant-charge-voltage-max-microvolt = <4200000>;
 		internal-resistance-micro-ohms = <250000>;
+		ocv-capacity-table-temperatures = <(-10) 0 10>;
+		ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
+		ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
+		ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
 	};
 
 	charger: charger@11 {
diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 9f3452f..151ff03 100644
--- a/drivers/power/supply/power_supply_core.c
+++ b/drivers/power/supply/power_supply_core.c
@@ -570,7 +570,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
 {
 	struct device_node *battery_np;
 	const char *value;
-	int err;
+	int err, len, index;
 
 	info->energy_full_design_uwh         = -EINVAL;
 	info->charge_full_design_uah         = -EINVAL;
@@ -581,6 +581,12 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	info->constant_charge_voltage_max_uv = -EINVAL;
 	info->internal_resistance_uohm       = -EINVAL;
 
+	for (index = 0; index < POWER_SUPPLY_OCV_TEMP_MAX; index++) {
+		info->ocv_table[index]       = NULL;
+		info->ocv_temp[index]        = -EINVAL;
+		info->ocv_table_size[index]  = -EINVAL;
+	}
+
 	if (!psy->of_node) {
 		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
 			 __func__);
@@ -620,10 +626,65 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	of_property_read_u32(battery_np, "internal-resistance-micro-ohms",
 			     &info->internal_resistance_uohm);
 
+	len = of_property_count_u32_elems(battery_np,
+					  "ocv-capacity-table-temperatures");
+	if (len < 0 && len != -EINVAL) {
+		return len;
+	} else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
+		dev_err(&psy->dev, "Too many temperature values\n");
+		return -EINVAL;
+	} else if (len > 0) {
+		of_property_read_u32_array(battery_np,
+					   "ocv-capacity-table-temperatures",
+					   info->ocv_temp, len);
+	}
+
+	for (index = 0; index < len; index++) {
+		struct power_supply_battery_ocv_table *table;
+		char *propname;
+		const __be32 *list;
+		int i, tab_len, size;
+
+		propname = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d", index);
+		list = of_get_property(battery_np, propname, &size);
+		kfree(propname);
+		if (!list || !size) {
+			dev_err(&psy->dev, "failed to get ocv capacity table\n");
+			power_supply_put_battery_info(psy, info);
+			return -EINVAL;
+		}
+
+		tab_len = size / sizeof(*table);
+		info->ocv_table_size[index] = tab_len;
+
+		table = info->ocv_table[index] = devm_kzalloc(&psy->dev,
+						tab_len * sizeof(*table),
+						GFP_KERNEL);
+		if (!info->ocv_table[index]) {
+			power_supply_put_battery_info(psy, info);
+			return -ENOMEM;
+		}
+
+		for (i = 0; i < tab_len; i++) {
+			table[i].ocv = be32_to_cpu(*list++);
+			table[i].capacity = be32_to_cpu(*list++);
+		}
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
 
+void power_supply_put_battery_info(struct power_supply *psy,
+				   struct power_supply_battery_info *info)
+{
+	int i;
+
+	for (i = 0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++)
+		kfree(info->ocv_table[i]);
+}
+EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
+
 int power_supply_get_property(struct power_supply *psy,
 			    enum power_supply_property psp,
 			    union power_supply_propval *val)
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index 019452d..b0a2768 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -309,6 +309,12 @@ struct power_supply_info {
 	int use_for_apm;
 };
 
+struct power_supply_battery_ocv_table {
+	int ocv;	/* microVolts */
+	int capacity;	/* percent */
+};
+
+#define POWER_SUPPLY_OCV_TEMP_MAX 20
 /*
  * This is the recommended struct to manage static battery parameters,
  * populated by power_supply_get_battery_info(). Most platform drivers should
@@ -327,6 +333,9 @@ struct power_supply_battery_info {
 	int constant_charge_current_max_ua; /* microAmps */
 	int constant_charge_voltage_max_uv; /* microVolts */
 	int internal_resistance_uohm;       /* microOhms */
+	int ocv_temp[POWER_SUPPLY_OCV_TEMP_MAX];  /* celsius */
+	struct power_supply_battery_ocv_table *ocv_table[POWER_SUPPLY_OCV_TEMP_MAX];
+	int ocv_table_size[POWER_SUPPLY_OCV_TEMP_MAX];
 };
 
 extern struct atomic_notifier_head power_supply_notifier;
@@ -350,6 +359,8 @@ extern struct power_supply *devm_power_supply_get_by_phandle(
 
 extern int power_supply_get_battery_info(struct power_supply *psy,
 					 struct power_supply_battery_info *info);
+extern void power_supply_put_battery_info(struct power_supply *psy,
+					  struct power_supply_battery_info *info);
 extern void power_supply_changed(struct power_supply *psy);
 extern int power_supply_am_i_supplied(struct power_supply *psy);
 extern int power_supply_set_input_current_limit_from_supplier(
-- 
1.7.9.5

Re: [PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> Some battery driver will use the open circuit voltage (OCV) value to look
> up the corresponding battery capacity percent in one certain degree Celsius.
> Thus this patch provides some battery properties to present the OCV table
> temperatures and OCV capacity table values.
>
> Suggested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - New patch in v2.

To the best of my knowledge this is a very good binding and core
functionality:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> Some battery driver will use the open circuit voltage (OCV) value to look
> up the corresponding battery capacity percent in one certain degree Celsius.
> Thus this patch provides some battery properties to present the OCV table
> temperatures and OCV capacity table values.
>
> Suggested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - New patch in v2.

To the best of my knowledge this is a very good binding and core
functionality:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Sebastian Reichel]

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

Hi,

On Wed, Sep 26, 2018 at 10:59:12AM +0800, Baolin Wang wrote:
> Some battery driver will use the open circuit voltage (OCV) value to look
> up the corresponding battery capacity percent in one certain degree Celsius.
> Thus this patch provides some battery properties to present the OCV table
> temperatures and OCV capacity table values.
> 
> Suggested-by: Sebastian Reichel <sre@kernel.org>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - New patch in v2.
> ---
>  .../devicetree/bindings/power/supply/battery.txt   |   14 +++++
>  drivers/power/supply/power_supply_core.c           |   63 +++++++++++++++++++-
>  include/linux/power_supply.h                       |   11 ++++
>  3 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
> index 25b9d2e..ac2b303 100644
> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
> @@ -23,6 +23,16 @@ Optional Properties:
>   - constant-charge-current-max-microamp: maximum constant input current
>   - constant-charge-voltage-max-microvolt: maximum constant input voltage
>   - internal-resistance-micro-ohms: battery internal resistance
> + - ocv-capacity-table-0: An array providing the battery capacity percent
> +   with corresponding open circuit voltage (OCV) of the battery, which
> +   is used to look up battery capacity according to current OCV value.
> + - ocv-capacity-table-1: Same as ocv-capacity-table-0
> + ......
> + - ocv-capacity-table-n: Same as ocv-capacity-table-0
> + - ocv-capacity-table-temperatures: An array containing the temperature
> +   in degree Celsius, for each of the battery capacity lookup table.
> +   The first temperature value specifies the OCV table 0, and the second
> +   temperature value specifies the OCV table 1, and so on.
>  
>  Battery properties are named, where possible, for the corresponding
>  elements in enum power_supply_property, defined in
> @@ -44,6 +54,10 @@ Example:
>  		constant-charge-current-max-microamp = <900000>;
>  		constant-charge-voltage-max-microvolt = <4200000>;
>  		internal-resistance-micro-ohms = <250000>;
> +		ocv-capacity-table-temperatures = <(-10) 0 10>;
> +		ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
> +		ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
> +		ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
>  	};
>  
>  	charger: charger@11 {
> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
> index 9f3452f..151ff03 100644
> --- a/drivers/power/supply/power_supply_core.c
> +++ b/drivers/power/supply/power_supply_core.c
> @@ -570,7 +570,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  {
>  	struct device_node *battery_np;
>  	const char *value;
> -	int err;
> +	int err, len, index;
>  
>  	info->energy_full_design_uwh         = -EINVAL;
>  	info->charge_full_design_uah         = -EINVAL;
> @@ -581,6 +581,12 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	info->constant_charge_voltage_max_uv = -EINVAL;
>  	info->internal_resistance_uohm       = -EINVAL;
>  
> +	for (index = 0; index < POWER_SUPPLY_OCV_TEMP_MAX; index++) {
> +		info->ocv_table[index]       = NULL;
> +		info->ocv_temp[index]        = -EINVAL;
> +		info->ocv_table_size[index]  = -EINVAL;
> +	}
> +
>  	if (!psy->of_node) {
>  		dev_warn(&psy->dev, "%s currently only supports devicetree\n",
>  			 __func__);
> @@ -620,10 +626,65 @@ int power_supply_get_battery_info(struct power_supply *psy,
>  	of_property_read_u32(battery_np, "internal-resistance-micro-ohms",
>  			     &info->internal_resistance_uohm);
>  
> +	len = of_property_count_u32_elems(battery_np,
> +					  "ocv-capacity-table-temperatures");
> +	if (len < 0 && len != -EINVAL) {
> +		return len;
> +	} else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
> +		dev_err(&psy->dev, "Too many temperature values\n");
> +		return -EINVAL;
> +	} else if (len > 0) {
> +		of_property_read_u32_array(battery_np,
> +					   "ocv-capacity-table-temperatures",
> +					   info->ocv_temp, len);
> +	}
> +
> +	for (index = 0; index < len; index++) {
> +		struct power_supply_battery_ocv_table *table;
> +		char *propname;
> +		const __be32 *list;
> +		int i, tab_len, size;
> +
> +		propname = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d", index);
> +		list = of_get_property(battery_np, propname, &size);
> +		kfree(propname);
> +		if (!list || !size) {
> +			dev_err(&psy->dev, "failed to get ocv capacity table\n");

I think it's better to replace "ocv capacity table" with %s / propname.

> +			power_supply_put_battery_info(psy, info);
> +			return -EINVAL;
> +		}
> +
> +		tab_len = size / sizeof(*table);

I think this should be

tab_len = size / (2 * sizeof(__be32));

which decouples DT memory layout from kernel memory layout.

> +		info->ocv_table_size[index] = tab_len;
> +
> +		table = info->ocv_table[index] = devm_kzalloc(&psy->dev,
> +						tab_len * sizeof(*table),
> +						GFP_KERNEL);
> +		if (!info->ocv_table[index]) {
> +			power_supply_put_battery_info(psy, info);
> +			return -ENOMEM;
> +		}
> +
> +		for (i = 0; i < tab_len; i++) {
> +			table[i].ocv = be32_to_cpu(*list++);
> +			table[i].capacity = be32_to_cpu(*list++);
> +		}
> +	}
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
>  
> +void power_supply_put_battery_info(struct power_supply *psy,
> +				   struct power_supply_battery_info *info)
> +{
> +	int i;
> +
> +	for (i = 0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++)
> +		kfree(info->ocv_table[i]);
> +}
> +EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
> +
>  int power_supply_get_property(struct power_supply *psy,
>  			    enum power_supply_property psp,
>  			    union power_supply_propval *val)
> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> index 019452d..b0a2768 100644
> --- a/include/linux/power_supply.h
> +++ b/include/linux/power_supply.h
> @@ -309,6 +309,12 @@ struct power_supply_info {
>  	int use_for_apm;
>  };
>  
> +struct power_supply_battery_ocv_table {
> +	int ocv;	/* microVolts */
> +	int capacity;	/* percent */
> +};
> +
> +#define POWER_SUPPLY_OCV_TEMP_MAX 20
>  /*
>   * This is the recommended struct to manage static battery parameters,
>   * populated by power_supply_get_battery_info(). Most platform drivers should
> @@ -327,6 +333,9 @@ struct power_supply_battery_info {
>  	int constant_charge_current_max_ua; /* microAmps */
>  	int constant_charge_voltage_max_uv; /* microVolts */
>  	int internal_resistance_uohm;       /* microOhms */
> +	int ocv_temp[POWER_SUPPLY_OCV_TEMP_MAX];  /* celsius */
> +	struct power_supply_battery_ocv_table *ocv_table[POWER_SUPPLY_OCV_TEMP_MAX];
> +	int ocv_table_size[POWER_SUPPLY_OCV_TEMP_MAX];
>  };
>  
>  extern struct atomic_notifier_head power_supply_notifier;
> @@ -350,6 +359,8 @@ extern struct power_supply *devm_power_supply_get_by_phandle(
>  
>  extern int power_supply_get_battery_info(struct power_supply *psy,
>  					 struct power_supply_battery_info *info);
> +extern void power_supply_put_battery_info(struct power_supply *psy,
> +					  struct power_supply_battery_info *info);
>  extern void power_supply_changed(struct power_supply *psy);
>  extern int power_supply_am_i_supplied(struct power_supply *psy);
>  extern int power_supply_set_input_current_limit_from_supplier(

Looks good to me. Technically this can result in existing users of
power_supply_get_battery_info leaking memory, if they have an OCV
table in DT.

-- Sebastian

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

Re: [PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Baolin Wang]

On 26 September 2018 at 21:51, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Wed, Sep 26, 2018 at 10:59:12AM +0800, Baolin Wang wrote:
>> Some battery driver will use the open circuit voltage (OCV) value to look
>> up the corresponding battery capacity percent in one certain degree Celsius.
>> Thus this patch provides some battery properties to present the OCV table
>> temperatures and OCV capacity table values.
>>
>> Suggested-by: Sebastian Reichel <sre@kernel.org>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - New patch in v2.
>> ---
>>  .../devicetree/bindings/power/supply/battery.txt   |   14 +++++
>>  drivers/power/supply/power_supply_core.c           |   63 +++++++++++++++++++-
>>  include/linux/power_supply.h                       |   11 ++++
>>  3 files changed, 87 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
>> index 25b9d2e..ac2b303 100644
>> --- a/Documentation/devicetree/bindings/power/supply/battery.txt
>> +++ b/Documentation/devicetree/bindings/power/supply/battery.txt
>> @@ -23,6 +23,16 @@ Optional Properties:
>>   - constant-charge-current-max-microamp: maximum constant input current
>>   - constant-charge-voltage-max-microvolt: maximum constant input voltage
>>   - internal-resistance-micro-ohms: battery internal resistance
>> + - ocv-capacity-table-0: An array providing the battery capacity percent
>> +   with corresponding open circuit voltage (OCV) of the battery, which
>> +   is used to look up battery capacity according to current OCV value.
>> + - ocv-capacity-table-1: Same as ocv-capacity-table-0
>> + ......
>> + - ocv-capacity-table-n: Same as ocv-capacity-table-0
>> + - ocv-capacity-table-temperatures: An array containing the temperature
>> +   in degree Celsius, for each of the battery capacity lookup table.
>> +   The first temperature value specifies the OCV table 0, and the second
>> +   temperature value specifies the OCV table 1, and so on.
>>
>>  Battery properties are named, where possible, for the corresponding
>>  elements in enum power_supply_property, defined in
>> @@ -44,6 +54,10 @@ Example:
>>               constant-charge-current-max-microamp = <900000>;
>>               constant-charge-voltage-max-microvolt = <4200000>;
>>               internal-resistance-micro-ohms = <250000>;
>> +             ocv-capacity-table-temperatures = <(-10) 0 10>;
>> +             ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>, ...;
>> +             ocv-capacity-table-1 = <4200000 100>, <4185000 95>, <4113000 90>, ...;
>> +             ocv-capacity-table-2 = <4250000 100>, <4200000 95>, <4185000 90>, ...;
>>       };
>>
>>       charger: charger@11 {
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 9f3452f..151ff03 100644
>> --- a/drivers/power/supply/power_supply_core.c
>> +++ b/drivers/power/supply/power_supply_core.c
>> @@ -570,7 +570,7 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>  {
>>       struct device_node *battery_np;
>>       const char *value;
>> -     int err;
>> +     int err, len, index;
>>
>>       info->energy_full_design_uwh         = -EINVAL;
>>       info->charge_full_design_uah         = -EINVAL;
>> @@ -581,6 +581,12 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>       info->constant_charge_voltage_max_uv = -EINVAL;
>>       info->internal_resistance_uohm       = -EINVAL;
>>
>> +     for (index = 0; index < POWER_SUPPLY_OCV_TEMP_MAX; index++) {
>> +             info->ocv_table[index]       = NULL;
>> +             info->ocv_temp[index]        = -EINVAL;
>> +             info->ocv_table_size[index]  = -EINVAL;
>> +     }
>> +
>>       if (!psy->of_node) {
>>               dev_warn(&psy->dev, "%s currently only supports devicetree\n",
>>                        __func__);
>> @@ -620,10 +626,65 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>       of_property_read_u32(battery_np, "internal-resistance-micro-ohms",
>>                            &info->internal_resistance_uohm);
>>
>> +     len = of_property_count_u32_elems(battery_np,
>> +                                       "ocv-capacity-table-temperatures");
>> +     if (len < 0 && len != -EINVAL) {
>> +             return len;
>> +     } else if (len > POWER_SUPPLY_OCV_TEMP_MAX) {
>> +             dev_err(&psy->dev, "Too many temperature values\n");
>> +             return -EINVAL;
>> +     } else if (len > 0) {
>> +             of_property_read_u32_array(battery_np,
>> +                                        "ocv-capacity-table-temperatures",
>> +                                        info->ocv_temp, len);
>> +     }
>> +
>> +     for (index = 0; index < len; index++) {
>> +             struct power_supply_battery_ocv_table *table;
>> +             char *propname;
>> +             const __be32 *list;
>> +             int i, tab_len, size;
>> +
>> +             propname = kasprintf(GFP_KERNEL, "ocv-capacity-table-%d", index);
>> +             list = of_get_property(battery_np, propname, &size);
>> +             kfree(propname);
>> +             if (!list || !size) {
>> +                     dev_err(&psy->dev, "failed to get ocv capacity table\n");
>
> I think it's better to replace "ocv capacity table" with %s / propname.

Yes.

>
>> +                     power_supply_put_battery_info(psy, info);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             tab_len = size / sizeof(*table);
>
> I think this should be
>
> tab_len = size / (2 * sizeof(__be32));
>
> which decouples DT memory layout from kernel memory layout.

Sure.

>
>> +             info->ocv_table_size[index] = tab_len;
>> +
>> +             table = info->ocv_table[index] = devm_kzalloc(&psy->dev,
>> +                                             tab_len * sizeof(*table),
>> +                                             GFP_KERNEL);
>> +             if (!info->ocv_table[index]) {
>> +                     power_supply_put_battery_info(psy, info);
>> +                     return -ENOMEM;
>> +             }
>> +
>> +             for (i = 0; i < tab_len; i++) {
>> +                     table[i].ocv = be32_to_cpu(*list++);
>> +                     table[i].capacity = be32_to_cpu(*list++);
>> +             }
>> +     }
>> +
>>       return 0;
>>  }
>>  EXPORT_SYMBOL_GPL(power_supply_get_battery_info);
>>
>> +void power_supply_put_battery_info(struct power_supply *psy,
>> +                                struct power_supply_battery_info *info)
>> +{
>> +     int i;
>> +
>> +     for (i = 0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++)
>> +             kfree(info->ocv_table[i]);
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_put_battery_info);
>> +
>>  int power_supply_get_property(struct power_supply *psy,
>>                           enum power_supply_property psp,
>>                           union power_supply_propval *val)
>> diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
>> index 019452d..b0a2768 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -309,6 +309,12 @@ struct power_supply_info {
>>       int use_for_apm;
>>  };
>>
>> +struct power_supply_battery_ocv_table {
>> +     int ocv;        /* microVolts */
>> +     int capacity;   /* percent */
>> +};
>> +
>> +#define POWER_SUPPLY_OCV_TEMP_MAX 20
>>  /*
>>   * This is the recommended struct to manage static battery parameters,
>>   * populated by power_supply_get_battery_info(). Most platform drivers should
>> @@ -327,6 +333,9 @@ struct power_supply_battery_info {
>>       int constant_charge_current_max_ua; /* microAmps */
>>       int constant_charge_voltage_max_uv; /* microVolts */
>>       int internal_resistance_uohm;       /* microOhms */
>> +     int ocv_temp[POWER_SUPPLY_OCV_TEMP_MAX];  /* celsius */
>> +     struct power_supply_battery_ocv_table *ocv_table[POWER_SUPPLY_OCV_TEMP_MAX];
>> +     int ocv_table_size[POWER_SUPPLY_OCV_TEMP_MAX];
>>  };
>>
>>  extern struct atomic_notifier_head power_supply_notifier;
>> @@ -350,6 +359,8 @@ extern struct power_supply *devm_power_supply_get_by_phandle(
>>
>>  extern int power_supply_get_battery_info(struct power_supply *psy,
>>                                        struct power_supply_battery_info *info);
>> +extern void power_supply_put_battery_info(struct power_supply *psy,
>> +                                       struct power_supply_battery_info *info);
>>  extern void power_supply_changed(struct power_supply *psy);
>>  extern int power_supply_am_i_supplied(struct power_supply *psy);
>>  extern int power_supply_set_input_current_limit_from_supplier(
>
> Looks good to me. Technically this can result in existing users of
> power_supply_get_battery_info leaking memory, if they have an OCV
> table in DT.

Fortunately existing users did not have an OCV table. Moreover once
this patch merged, I will send patches to add
power_supply_put_battery_info() for existing users of battery info.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 2/4] power: supply: core: Introduce properties to present the battery OCV table

[Sebastian Reichel]

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

Hi,

On Thu, Sep 27, 2018 at 09:10:05AM +0800, Baolin Wang wrote:
> On 26 September 2018 at 21:51, Sebastian Reichel <sre@kernel.org> wrote:
> > Looks good to me. Technically this can result in existing users of
> > power_supply_get_battery_info leaking memory, if they have an OCV
> > table in DT.
> 
> Fortunately existing users did not have an OCV table.

Right. I was just talking about potential memory leak, if the DT
file of these boards has the OCV table defined. I don't think
that this will be a problem in real life.

> Moreover once this patch merged, I will send patches to add
> power_supply_put_battery_info() for existing users of battery
> info.

Thanks.

-- Sebastian

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

[PATCH v2 3/4] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

[Baolin Wang]

This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
fuel gauge unit device, which is used to calculate the battery capacity.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - Renamed GPIO property.
 - Use standand battery properties instead of 'sprd,inner-resist' and
   'sprd,ocv-cap-table'.
 - Remove battery node's description.
---
 .../devicetree/bindings/power/supply/sc27xx-fg.txt |   52 ++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt

diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
new file mode 100644
index 0000000..b161468
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
@@ -0,0 +1,52 @@
+Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
+
+Required properties:
+- compatible: Should be one of the following:
+  "sprd,sc2720-fgu",
+  "sprd,sc2721-fgu",
+  "sprd,sc2723-fgu",
+  "sprd,sc2730-fgu",
+  "sprd,sc2731-fgu".
+- reg: The address offset of fuel gauge unit.
+- battery-detect-gpios: GPIO for battery detection.
+- io-channels: Specify the IIO ADC channel to get temperature.
+- io-channel-names: Should be "bat-temp".
+- monitored-battery: Phandle of battery characteristics devicetree node.
+  See Documentation/devicetree/bindings/power/supply/battery.txt
+
+Example:
+
+	bat: battery {
+		compatible = "simple-battery";
+		charge-full-design-microamp-hours = <1900000>;
+		constant-charge-voltage-max-microvolt = <4350000>;
+		ocv-capacity-table-temperatures = <20>;
+		ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>,
+					<4022000 85>, <3983000 80>, <3949000 75>,
+					<3917000 70>, <3889000 65>, <3864000 60>,
+					<3835000 55>, <3805000 50>, <3787000 45>,
+					<3777000 40>, <3773000 35>, <3770000 30>,
+					<3765000 25>, <3752000 20>, <3724000 15>,
+					<3680000 10>, <3605000 5>, <3400000 0>;
+		......
+	};
+
+	sc2731_pmic: pmic@0 {
+		compatible = "sprd,sc2731";
+		reg = <0>;
+		spi-max-frequency = <26000000>;
+		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-controller;
+		#interrupt-cells = <2>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		fgu@a00 {
+			compatible = "sprd,sc2731-fgu";
+			reg = <0xa00>;
+			battery-detect-gpios = <&pmic_eic 9 GPIO_ACTIVE_HIGH>;
+			io-channels = <&pmic_adc 5>;
+			io-channel-names = "bat-temp";
+			monitored-battery = <&bat>;
+		};
+	};
-- 
1.7.9.5

Re: [PATCH v2 3/4] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
> fuel gauge unit device, which is used to calculate the battery capacity.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Renamed GPIO property.
>  - Use standand battery properties instead of 'sprd,inner-resist' and
>    'sprd,ocv-cap-table'.
>  - Remove battery node's description.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 3/4] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
> fuel gauge unit device, which is used to calculate the battery capacity.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Renamed GPIO property.
>  - Use standand battery properties instead of 'sprd,inner-resist' and
>    'sprd,ocv-cap-table'.
>  - Remove battery node's description.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v2 3/4] dt-bindings: power: Add Spreadtrum SC27XX fuel gauge unit documentation

[Sebastian Reichel]

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

Hi,

On Wed, Sep 26, 2018 at 10:59:13AM +0800, Baolin Wang wrote:
> This patch adds the binding documentation for Spreadtrum SC27XX series PMICs
> fuel gauge unit device, which is used to calculate the battery capacity.
> 
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---

Looks good to me.

-- Sebastian

> Changes from v1:
>  - Renamed GPIO property.
>  - Use standand battery properties instead of 'sprd,inner-resist' and
>    'sprd,ocv-cap-table'.
>  - Remove battery node's description.
> ---
>  .../devicetree/bindings/power/supply/sc27xx-fg.txt |   52 ++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> 
> diff --git a/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> new file mode 100644
> index 0000000..b161468
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/power/supply/sc27xx-fg.txt
> @@ -0,0 +1,52 @@
> +Spreadtrum SC27XX PMICs Fuel Gauge Unit Power Supply Bindings
> +
> +Required properties:
> +- compatible: Should be one of the following:
> +  "sprd,sc2720-fgu",
> +  "sprd,sc2721-fgu",
> +  "sprd,sc2723-fgu",
> +  "sprd,sc2730-fgu",
> +  "sprd,sc2731-fgu".
> +- reg: The address offset of fuel gauge unit.
> +- battery-detect-gpios: GPIO for battery detection.
> +- io-channels: Specify the IIO ADC channel to get temperature.
> +- io-channel-names: Should be "bat-temp".
> +- monitored-battery: Phandle of battery characteristics devicetree node.
> +  See Documentation/devicetree/bindings/power/supply/battery.txt
> +
> +Example:
> +
> +	bat: battery {
> +		compatible = "simple-battery";
> +		charge-full-design-microamp-hours = <1900000>;
> +		constant-charge-voltage-max-microvolt = <4350000>;
> +		ocv-capacity-table-temperatures = <20>;
> +		ocv-capacity-table-0 = <4185000 100>, <4113000 95>, <4066000 90>,
> +					<4022000 85>, <3983000 80>, <3949000 75>,
> +					<3917000 70>, <3889000 65>, <3864000 60>,
> +					<3835000 55>, <3805000 50>, <3787000 45>,
> +					<3777000 40>, <3773000 35>, <3770000 30>,
> +					<3765000 25>, <3752000 20>, <3724000 15>,
> +					<3680000 10>, <3605000 5>, <3400000 0>;
> +		......
> +	};
> +
> +	sc2731_pmic: pmic@0 {
> +		compatible = "sprd,sc2731";
> +		reg = <0>;
> +		spi-max-frequency = <26000000>;
> +		interrupts = <GIC_SPI 31 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-controller;
> +		#interrupt-cells = <2>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		fgu@a00 {
> +			compatible = "sprd,sc2731-fgu";
> +			reg = <0xa00>;
> +			battery-detect-gpios = <&pmic_eic 9 GPIO_ACTIVE_HIGH>;
> +			io-channels = <&pmic_adc 5>;
> +			io-channel-names = "bat-temp";
> +			monitored-battery = <&bat>;
> +		};
> +	};
> -- 
> 1.7.9.5
> 

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

[PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Baolin Wang]

This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
which is used to calculate the battery capacity.

Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
Changes from v1:
 - Use battery standard properties to get internal resistance and ocv table.
 - Change devm_gpiod_get_optional() to devm_gpiod_get().
 - Add power_supply_changed() when detecting battery present change.
 - Return micro volts for sc27xx_fgu_get_vbat_ocv().
---
 drivers/power/supply/Kconfig             |    7 +
 drivers/power/supply/Makefile            |    1 +
 drivers/power/supply/sc27xx_fuel_gauge.c |  691 ++++++++++++++++++++++++++++++
 3 files changed, 699 insertions(+)
 create mode 100644 drivers/power/supply/sc27xx_fuel_gauge.c

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index f27cf07..917f4b7 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -652,4 +652,11 @@ config CHARGER_SC2731
 	 Say Y here to enable support for battery charging with SC2731
 	 PMIC chips.
 
+config FUEL_GAUGE_SC27XX
+	tristate "Spreadtrum SC27XX fuel gauge driver"
+	depends on MFD_SC27XX_PMIC || COMPILE_TEST
+	help
+	 Say Y here to enable support for fuel gauge with SC27XX
+	 PMIC chips.
+
 endif # POWER_SUPPLY
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 767105b..b731c2a 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -86,3 +86,4 @@ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
 obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
 obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
 obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
+obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
new file mode 100644
index 0000000..d3422cf
--- /dev/null
+++ b/drivers/power/supply/sc27xx_fuel_gauge.c
@@ -0,0 +1,691 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 Spreadtrum Communications Inc.
+
+#include <linux/gpio/consumer.h>
+#include <linux/iio/consumer.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+
+/* PMIC global control registers definition */
+#define SC27XX_MODULE_EN0		0xc08
+#define SC27XX_CLK_EN0			0xc18
+#define SC27XX_FGU_EN			BIT(7)
+#define SC27XX_FGU_RTC_EN		BIT(6)
+
+/* FGU registers definition */
+#define SC27XX_FGU_START		0x0
+#define SC27XX_FGU_CONFIG		0x4
+#define SC27XX_FGU_ADC_CONFIG		0x8
+#define SC27XX_FGU_STATUS		0xc
+#define SC27XX_FGU_INT_EN		0x10
+#define SC27XX_FGU_INT_CLR		0x14
+#define SC27XX_FGU_INT_STS		0x1c
+#define SC27XX_FGU_VOLTAGE		0x20
+#define SC27XX_FGU_OCV			0x24
+#define SC27XX_FGU_POCV			0x28
+#define SC27XX_FGU_CURRENT		0x2c
+#define SC27XX_FGU_CLBCNT_SETH		0x50
+#define SC27XX_FGU_CLBCNT_SETL		0x54
+#define SC27XX_FGU_CLBCNT_VALH		0x68
+#define SC27XX_FGU_CLBCNT_VALL		0x6c
+#define SC27XX_FGU_CLBCNT_QMAXL		0x74
+
+#define SC27XX_WRITE_SELCLB_EN		BIT(0)
+#define SC27XX_FGU_CLBCNT_MASK		GENMASK(15, 0)
+#define SC27XX_FGU_CLBCNT_SHIFT		16
+
+#define SC27XX_FGU_1000MV_ADC		686
+#define SC27XX_FGU_1000MA_ADC		1372
+#define SC27XX_FGU_CUR_BASIC_ADC	8192
+#define SC27XX_FGU_SAMPLE_HZ		2
+
+/*
+ * struct sc27xx_fgu_data: describe the FGU device
+ * @regmap: regmap for register access
+ * @dev: platform device
+ * @battery: battery power supply
+ * @base: the base offset for the controller
+ * @lock: protect the structure
+ * @gpiod: GPIO for battery detection
+ * @channel: IIO channel to get battery temperature
+ * @internal_resist: the battery internal resistance in mOhm
+ * @total_cap: the total capacity of the battery in mAh
+ * @init_cap: the initial capacity of the battery in mAh
+ * @init_clbcnt: the initial coulomb counter
+ * @max_volt: the maximum constant input voltage in millivolt
+ * @table_len: the capacity table length
+ * @cap_table: capacity table with corresponding ocv
+ */
+struct sc27xx_fgu_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct power_supply *battery;
+	u32 base;
+	struct mutex lock;
+	struct gpio_desc *gpiod;
+	struct iio_channel *channel;
+	bool bat_present;
+	int internal_resist;
+	int total_cap;
+	int init_cap;
+	int init_clbcnt;
+	int max_volt;
+	int table_len;
+	struct power_supply_battery_ocv_table *cap_table;
+};
+
+static const char * const sc27xx_charger_supply_name[] = {
+	"sc2731_charger",
+	"sc2720_charger",
+	"sc2721_charger",
+	"sc2723_charger",
+};
+
+static int sc27xx_fgu_adc_to_current(int adc)
+{
+	return (adc * 1000) / SC27XX_FGU_1000MA_ADC;
+}
+
+static int sc27xx_fgu_adc_to_voltage(int adc)
+{
+	return (adc * 1000) / SC27XX_FGU_1000MV_ADC;
+}
+
+static int sc27xx_fgu_ocv_to_capacity(struct sc27xx_fgu_data *data, int ocv)
+{
+	struct power_supply_battery_ocv_table *tab = data->cap_table;
+	int n = data->table_len;
+	int i, cap, tmp;
+
+	/*
+	 * Find the position in the table for current battery OCV value,
+	 * then use these two points to calculate battery capacity
+	 * according to the linear method.
+	 */
+	for (i = 0; i < n; i++)
+		if (ocv > tab[i].ocv)
+			break;
+
+	if (i > 0 && i < n) {
+		tmp = (tab[i - 1].capacity - tab[i].capacity) *
+			(ocv - tab[i].ocv) * 2;
+		tmp /= tab[i - 1].ocv - tab[i].ocv;
+		tmp = (tmp + 1) / 2;
+		cap = tmp + tab[i].capacity;
+	} else if (i == 0) {
+		cap = tab[0].capacity;
+	} else {
+		cap = tab[n - 1].capacity;
+	}
+
+	return cap;
+}
+
+/*
+ * When system boots on, we can not read battery capacity from coulomb
+ * registers, since now the coulomb registers are invalid. So we should
+ * calculate the battery open circuit voltage, and get current battery
+ * capacity according to the capacity table.
+ */
+static int sc27xx_fgu_get_boot_capacity(struct sc27xx_fgu_data *data, int *cap)
+{
+	int volt, cur, oci, ocv, ret;
+
+	/*
+	 * After system booting on, the SC27XX_FGU_CLBCNT_QMAXL register saved
+	 * the first sampled open circuit current.
+	 */
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_QMAXL,
+			  &cur);
+	if (ret)
+		return ret;
+
+	cur <<= 1;
+	oci = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
+
+	/*
+	 * Should get the OCV from SC27XX_FGU_POCV register at the system
+	 * beginning. It is ADC values reading from registers which need to
+	 * convert the corresponding voltage.
+	 */
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_POCV, &volt);
+	if (ret)
+		return ret;
+
+	volt = sc27xx_fgu_adc_to_voltage(volt);
+	ocv = volt - (oci * data->internal_resist) / 1000;
+
+	/*
+	 * Parse the capacity table to look up the correct capacity percent
+	 * according to current battery's corresponding OCV values.
+	 */
+	*cap = sc27xx_fgu_ocv_to_capacity(data, ocv);
+
+	return 0;
+}
+
+static int sc27xx_fgu_set_clbcnt(struct sc27xx_fgu_data *data, int clbcnt)
+{
+	int ret;
+
+	clbcnt *= SC27XX_FGU_SAMPLE_HZ;
+
+	ret = regmap_update_bits(data->regmap,
+				 data->base + SC27XX_FGU_CLBCNT_SETL,
+				 SC27XX_FGU_CLBCNT_MASK, clbcnt);
+	if (ret)
+		return ret;
+
+	ret = regmap_update_bits(data->regmap,
+				 data->base + SC27XX_FGU_CLBCNT_SETH,
+				 SC27XX_FGU_CLBCNT_MASK,
+				 clbcnt >> SC27XX_FGU_CLBCNT_SHIFT);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, data->base + SC27XX_FGU_START,
+				 SC27XX_WRITE_SELCLB_EN,
+				 SC27XX_WRITE_SELCLB_EN);
+}
+
+static int sc27xx_fgu_get_clbcnt(struct sc27xx_fgu_data *data, int *clb_cnt)
+{
+	int ccl, cch, ret;
+
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALL,
+			  &ccl);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALH,
+			  &cch);
+	if (ret)
+		return ret;
+
+	*clb_cnt = ccl & SC27XX_FGU_CLBCNT_MASK;
+	*clb_cnt |= (cch & SC27XX_FGU_CLBCNT_MASK) << SC27XX_FGU_CLBCNT_SHIFT;
+	*clb_cnt /= SC27XX_FGU_SAMPLE_HZ;
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_capacity(struct sc27xx_fgu_data *data, int *cap)
+{
+	int ret, cur_clbcnt, delta_clbcnt, delta_cap, temp;
+
+	/* Get current coulomb counters firstly */
+	ret = sc27xx_fgu_get_clbcnt(data, &cur_clbcnt);
+	if (ret)
+		return ret;
+
+	delta_clbcnt = cur_clbcnt - data->init_clbcnt;
+
+	/*
+	 * Convert coulomb counter to delta capacity (mAh), and set multiplier
+	 * as 100 to improve the precision.
+	 */
+	temp = DIV_ROUND_CLOSEST(delta_clbcnt, 360);
+	temp = sc27xx_fgu_adc_to_current(temp);
+
+	/*
+	 * Convert to capacity percent of the battery total capacity,
+	 * and multiplier is 100 too.
+	 */
+	delta_cap = DIV_ROUND_CLOSEST(temp * 100, data->total_cap);
+	*cap = delta_cap + data->init_cap;
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
+{
+	int ret, vol;
+
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
+	if (ret)
+		return ret;
+
+	/*
+	 * It is ADC values reading from registers which need to convert to
+	 * corresponding voltage values.
+	 */
+	*val = sc27xx_fgu_adc_to_voltage(vol);
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
+{
+	int ret, cur;
+
+	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
+	if (ret)
+		return ret;
+
+	/*
+	 * It is ADC values reading from registers which need to convert to
+	 * corresponding current values.
+	 */
+	*val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
+{
+	int vol, cur, ret;
+
+	ret = sc27xx_fgu_get_vbat_vol(data, &vol);
+	if (ret)
+		return ret;
+
+	ret = sc27xx_fgu_get_current(data, &cur);
+	if (ret)
+		return ret;
+
+	/* Return the battery OCV in micro volts. */
+	*val = vol * 1000 - cur * data->internal_resist;
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
+{
+	return iio_read_channel_processed(data->channel, temp);
+}
+
+static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
+{
+	int ret, vol;
+
+	ret = sc27xx_fgu_get_vbat_vol(data, &vol);
+	if (ret)
+		return ret;
+
+	if (vol > data->max_volt)
+		*health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
+	else
+		*health = POWER_SUPPLY_HEALTH_GOOD;
+
+	return 0;
+}
+
+static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
+{
+	union power_supply_propval val;
+	struct power_supply *psy;
+	int i, ret = -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
+		psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
+		if (!psy)
+			continue;
+
+		ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
+						&val);
+		power_supply_put(psy);
+		if (ret)
+			return ret;
+
+		*status = val.intval;
+	}
+
+	return ret;
+}
+
+static int sc27xx_fgu_get_property(struct power_supply *psy,
+				   enum power_supply_property psp,
+				   union power_supply_propval *val)
+{
+	struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
+	int ret = 0;
+	int value;
+
+	mutex_lock(&data->lock);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		ret = sc27xx_fgu_get_status(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value;
+		break;
+
+	case POWER_SUPPLY_PROP_HEALTH:
+		ret = sc27xx_fgu_get_health(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value;
+		break;
+
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = data->bat_present;
+		break;
+
+	case POWER_SUPPLY_PROP_TEMP:
+		ret = sc27xx_fgu_get_temp(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value;
+		break;
+
+	case POWER_SUPPLY_PROP_TECHNOLOGY:
+		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
+		break;
+
+	case POWER_SUPPLY_PROP_CAPACITY:
+		ret = sc27xx_fgu_get_capacity(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		ret = sc27xx_fgu_get_vbat_vol(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value * 1000;
+		break;
+
+	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
+		ret = sc27xx_fgu_get_vbat_ocv(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value;
+		break;
+
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+	case POWER_SUPPLY_PROP_CURRENT_AVG:
+		ret = sc27xx_fgu_get_current(data, &value);
+		if (ret)
+			goto error;
+
+		val->intval = value * 1000;
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+error:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
+{
+	struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
+
+	power_supply_changed(data->battery);
+}
+
+static enum power_supply_property sc27xx_fgu_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_HEALTH,
+	POWER_SUPPLY_PROP_PRESENT,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_TECHNOLOGY,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_VOLTAGE_OCV,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CURRENT_AVG,
+};
+
+static const struct power_supply_desc sc27xx_fgu_desc = {
+	.name			= "sc27xx-fgu",
+	.type			= POWER_SUPPLY_TYPE_BATTERY,
+	.properties		= sc27xx_fgu_props,
+	.num_properties		= ARRAY_SIZE(sc27xx_fgu_props),
+	.get_property		= sc27xx_fgu_get_property,
+	.external_power_changed	= sc27xx_fgu_external_power_changed,
+};
+
+static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
+{
+	struct sc27xx_fgu_data *data = dev_id;
+	int state;
+
+	mutex_lock(&data->lock);
+
+	state = gpiod_get_value_cansleep(data->gpiod);
+	if (state < 0) {
+		dev_err(data->dev, "failed to get gpio state\n");
+		mutex_unlock(&data->lock);
+		return IRQ_RETVAL(state);
+	}
+
+	data->bat_present = !!state;
+
+	mutex_unlock(&data->lock);
+
+	power_supply_changed(data->battery);
+	return IRQ_HANDLED;
+}
+
+static void sc27xx_fgu_disable(void *_data)
+{
+	struct sc27xx_fgu_data *data = _data;
+
+	regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
+	regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
+}
+
+static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
+{
+	/*
+	 * Get current capacity (mAh) = battery total capacity (mAh) *
+	 * current capacity percent (capacity / 100).
+	 */
+	int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
+
+	/*
+	 * Convert current capacity (mAh) to coulomb counter according to the
+	 * formula: 1 mAh =3.6 coulomb.
+	 */
+	return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
+}
+
+static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
+{
+	struct power_supply_battery_info info = { };
+	struct power_supply_battery_ocv_table *table;
+	int ret, i;
+
+	ret = power_supply_get_battery_info(data->battery, &info);
+	if (ret) {
+		dev_err(data->dev, "failed to get battery information\n");
+		return ret;
+	}
+
+	data->total_cap = info.charge_full_design_uah / 1000;
+	data->max_volt = info.constant_charge_voltage_max_uv / 1000;
+	data->internal_resist = info.internal_resistance_uohm / 1000;
+	data->table_len = info.ocv_table_size[0];
+
+	/*
+	 * For SC27XX fuel gauge device, we only need one ocv-capacity
+	 * table in normal temperature.
+	 */
+	table = info.ocv_table[0];
+	if (!table)
+		return -EINVAL;
+
+	data->cap_table = devm_kzalloc(data->dev,
+				       data->table_len * sizeof(*table),
+				       GFP_KERNEL);
+	if (!data->cap_table) {
+		power_supply_put_battery_info(data->battery, &info);
+		return -ENOMEM;
+	}
+
+	for (i = 0; i < data->table_len; i++) {
+		data->cap_table[i].ocv = table[i].ocv / 1000;
+		data->cap_table[i].capacity = table[i].capacity;
+	}
+
+	power_supply_put_battery_info(data->battery, &info);
+
+	/* Enable the FGU module */
+	ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
+				 SC27XX_FGU_EN, SC27XX_FGU_EN);
+	if (ret) {
+		dev_err(data->dev, "failed to enable fgu\n");
+		return ret;
+	}
+
+	/* Enable the FGU RTC clock to make it work */
+	ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
+				 SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
+	if (ret) {
+		dev_err(data->dev, "failed to enable fgu RTC clock\n");
+		goto disable_fgu;
+	}
+
+	/*
+	 * Get the boot battery capacity when system powers on, which is used to
+	 * initialize the coulomb counter. After that, we can read the coulomb
+	 * counter to measure the battery capacity.
+	 */
+	ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
+	if (ret) {
+		dev_err(data->dev, "failed to get boot capacity\n");
+		goto disable_clk;
+	}
+
+	/*
+	 * Convert battery capacity to the corresponding initial coulomb counter
+	 * and set into coulomb counter registers.
+	 */
+	data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
+	ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
+	if (ret) {
+		dev_err(data->dev, "failed to initialize coulomb counter\n");
+		goto disable_clk;
+	}
+
+	return 0;
+
+disable_clk:
+	regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
+disable_fgu:
+	regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
+
+	return ret;
+}
+
+static int sc27xx_fgu_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct power_supply_config fgu_cfg = { };
+	struct sc27xx_fgu_data *data;
+	int ret, irq;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!data->regmap) {
+		dev_err(&pdev->dev, "failed to get regmap\n");
+		return -ENODEV;
+	}
+
+	ret = of_property_read_u32(np, "reg", &data->base);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to get fgu address\n");
+		return ret;
+	}
+
+	data->channel = devm_iio_channel_get(&pdev->dev, "bat-temp");
+	if (IS_ERR(data->channel)) {
+		dev_err(&pdev->dev, "failed to get IIO channel\n");
+		return PTR_ERR(data->channel);
+	}
+
+	data->gpiod = devm_gpiod_get(&pdev->dev, "bat-detect", GPIOD_IN);
+	if (IS_ERR(data->gpiod)) {
+		dev_err(&pdev->dev, "failed to get battery detection GPIO\n");
+		return PTR_ERR(data->gpiod);
+	}
+
+	ret = gpiod_get_value_cansleep(data->gpiod);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to get gpio state\n");
+		return ret;
+	}
+
+	data->bat_present = !!ret;
+	mutex_init(&data->lock);
+	data->dev = &pdev->dev;
+
+	fgu_cfg.drv_data = data;
+	fgu_cfg.of_node = np;
+	data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
+						   &fgu_cfg);
+	if (IS_ERR(data->battery)) {
+		dev_err(&pdev->dev, "failed to register power supply\n");
+		return PTR_ERR(data->battery);
+	}
+
+	ret = sc27xx_fgu_hw_init(data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
+		return ret;
+	}
+
+	ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
+	if (ret) {
+		sc27xx_fgu_disable(data);
+		dev_err(&pdev->dev, "failed to add fgu disable action\n");
+		return ret;
+	}
+
+	irq = gpiod_to_irq(data->gpiod);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
+		return irq;
+	}
+
+	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
+					sc27xx_fgu_bat_detection,
+					IRQF_ONESHOT | IRQF_TRIGGER_RISING |
+					IRQF_TRIGGER_FALLING,
+					pdev->name, data);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to request IRQ\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id sc27xx_fgu_of_match[] = {
+	{ .compatible = "sprd,sc2731-fgu", },
+	{ }
+};
+
+static struct platform_driver sc27xx_fgu_driver = {
+	.probe = sc27xx_fgu_probe,
+	.driver = {
+		.name = "sc27xx-fgu",
+		.of_match_table = sc27xx_fgu_of_match,
+	}
+};
+
+module_platform_driver(sc27xx_fgu_driver);
+
+MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
> which is used to calculate the battery capacity.
>
> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Use battery standard properties to get internal resistance and ocv table.
>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>  - Add power_supply_changed() when detecting battery present change.
>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I see that you need to have the internal resistance as parameter to
this equation so it needs to come in. (I guess as factor/typical
internal resistance as suggested.)

IIUC this reliance on an uncertain internal resistance is the reason
why fuel gauges / coloumb counters are seen as crude ways to
estimate battery capacity, and their accuracy and reliability worsen
over the lifetime of the battery, and this is why safety-critical devices
include ways to measure the internal resistance as well.

But the hardware is as it is, and probably for some good reasons,
like cost.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Linus Walleij]

On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
> which is used to calculate the battery capacity.
>
> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Use battery standard properties to get internal resistance and ocv table.
>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>  - Add power_supply_changed() when detecting battery present change.
>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I see that you need to have the internal resistance as parameter to
this equation so it needs to come in. (I guess as factor/typical
internal resistance as suggested.)

IIUC this reliance on an uncertain internal resistance is the reason
why fuel gauges / coloumb counters are seen as crude ways to
estimate battery capacity, and their accuracy and reliability worsen
over the lifetime of the battery, and this is why safety-critical devices
include ways to measure the internal resistance as well.

But the hardware is as it is, and probably for some good reasons,
like cost.

Yours,
Linus Walleij

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Baolin Wang]

Hi Linus,

On 26 September 2018 at 16:09, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
>> which is used to calculate the battery capacity.
>>
>> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - Use battery standard properties to get internal resistance and ocv table.
>>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>>  - Add power_supply_changed() when detecting battery present change.
>>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> I see that you need to have the internal resistance as parameter to
> this equation so it needs to come in. (I guess as factor/typical
> internal resistance as suggested.)
>
> IIUC this reliance on an uncertain internal resistance is the reason
> why fuel gauges / coloumb counters are seen as crude ways to
> estimate battery capacity, and their accuracy and reliability worsen
> over the lifetime of the battery, and this is why safety-critical devices
> include ways to measure the internal resistance as well.

Yes. Thanks for your reviewing.

>
> But the hardware is as it is, and probably for some good reasons,
> like cost.
>
> Yours,
> Linus Walleij



-- 
Baolin Wang
Best Regards

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Baolin Wang]

Hi Linus,

On 26 September 2018 at 16:09, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 26, 2018 at 5:00 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
>> which is used to calculate the battery capacity.
>>
>> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - Use battery standard properties to get internal resistance and ocv table.
>>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>>  - Add power_supply_changed() when detecting battery present change.
>>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().
>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>
> I see that you need to have the internal resistance as parameter to
> this equation so it needs to come in. (I guess as factor/typical
> internal resistance as suggested.)
>
> IIUC this reliance on an uncertain internal resistance is the reason
> why fuel gauges / coloumb counters are seen as crude ways to
> estimate battery capacity, and their accuracy and reliability worsen
> over the lifetime of the battery, and this is why safety-critical devices
> include ways to measure the internal resistance as well.

Yes. Thanks for your reviewing.

>
> But the hardware is as it is, and probably for some good reasons,
> like cost.
>
> Yours,
> Linus Walleij



-- 
Baolin Wang
Best Regards

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Sebastian Reichel]

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

Hi,

On Wed, Sep 26, 2018 at 10:59:14AM +0800, Baolin Wang wrote:
> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
> which is used to calculate the battery capacity.
> 
> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - Use battery standard properties to get internal resistance and ocv table.
>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>  - Add power_supply_changed() when detecting battery present change.
>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().
> ---
>  drivers/power/supply/Kconfig             |    7 +
>  drivers/power/supply/Makefile            |    1 +
>  drivers/power/supply/sc27xx_fuel_gauge.c |  691 ++++++++++++++++++++++++++++++
>  3 files changed, 699 insertions(+)
>  create mode 100644 drivers/power/supply/sc27xx_fuel_gauge.c
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index f27cf07..917f4b7 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -652,4 +652,11 @@ config CHARGER_SC2731
>  	 Say Y here to enable support for battery charging with SC2731
>  	 PMIC chips.
>  
> +config FUEL_GAUGE_SC27XX
> +	tristate "Spreadtrum SC27XX fuel gauge driver"
> +	depends on MFD_SC27XX_PMIC || COMPILE_TEST
> +	help
> +	 Say Y here to enable support for fuel gauge with SC27XX
> +	 PMIC chips.
> +
>  endif # POWER_SUPPLY
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 767105b..b731c2a 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -86,3 +86,4 @@ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>  obj-$(CONFIG_AXP288_CHARGER)	+= axp288_charger.o
>  obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
>  obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
> +obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
> diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
> new file mode 100644
> index 0000000..d3422cf
> --- /dev/null
> +++ b/drivers/power/supply/sc27xx_fuel_gauge.c
> @@ -0,0 +1,691 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2018 Spreadtrum Communications Inc.
> +
> +#include <linux/gpio/consumer.h>
> +#include <linux/iio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +
> +/* PMIC global control registers definition */
> +#define SC27XX_MODULE_EN0		0xc08
> +#define SC27XX_CLK_EN0			0xc18
> +#define SC27XX_FGU_EN			BIT(7)
> +#define SC27XX_FGU_RTC_EN		BIT(6)
> +
> +/* FGU registers definition */
> +#define SC27XX_FGU_START		0x0
> +#define SC27XX_FGU_CONFIG		0x4
> +#define SC27XX_FGU_ADC_CONFIG		0x8
> +#define SC27XX_FGU_STATUS		0xc
> +#define SC27XX_FGU_INT_EN		0x10
> +#define SC27XX_FGU_INT_CLR		0x14
> +#define SC27XX_FGU_INT_STS		0x1c
> +#define SC27XX_FGU_VOLTAGE		0x20
> +#define SC27XX_FGU_OCV			0x24
> +#define SC27XX_FGU_POCV			0x28
> +#define SC27XX_FGU_CURRENT		0x2c
> +#define SC27XX_FGU_CLBCNT_SETH		0x50
> +#define SC27XX_FGU_CLBCNT_SETL		0x54
> +#define SC27XX_FGU_CLBCNT_VALH		0x68
> +#define SC27XX_FGU_CLBCNT_VALL		0x6c
> +#define SC27XX_FGU_CLBCNT_QMAXL		0x74
> +
> +#define SC27XX_WRITE_SELCLB_EN		BIT(0)
> +#define SC27XX_FGU_CLBCNT_MASK		GENMASK(15, 0)
> +#define SC27XX_FGU_CLBCNT_SHIFT		16
> +
> +#define SC27XX_FGU_1000MV_ADC		686
> +#define SC27XX_FGU_1000MA_ADC		1372
> +#define SC27XX_FGU_CUR_BASIC_ADC	8192
> +#define SC27XX_FGU_SAMPLE_HZ		2
> +
> +/*
> + * struct sc27xx_fgu_data: describe the FGU device
> + * @regmap: regmap for register access
> + * @dev: platform device
> + * @battery: battery power supply
> + * @base: the base offset for the controller
> + * @lock: protect the structure
> + * @gpiod: GPIO for battery detection
> + * @channel: IIO channel to get battery temperature
> + * @internal_resist: the battery internal resistance in mOhm
> + * @total_cap: the total capacity of the battery in mAh
> + * @init_cap: the initial capacity of the battery in mAh
> + * @init_clbcnt: the initial coulomb counter
> + * @max_volt: the maximum constant input voltage in millivolt
> + * @table_len: the capacity table length
> + * @cap_table: capacity table with corresponding ocv
> + */
> +struct sc27xx_fgu_data {
> +	struct regmap *regmap;
> +	struct device *dev;
> +	struct power_supply *battery;
> +	u32 base;
> +	struct mutex lock;
> +	struct gpio_desc *gpiod;
> +	struct iio_channel *channel;
> +	bool bat_present;
> +	int internal_resist;
> +	int total_cap;
> +	int init_cap;
> +	int init_clbcnt;
> +	int max_volt;
> +	int table_len;
> +	struct power_supply_battery_ocv_table *cap_table;
> +};
> +
> +static const char * const sc27xx_charger_supply_name[] = {
> +	"sc2731_charger",
> +	"sc2720_charger",
> +	"sc2721_charger",
> +	"sc2723_charger",
> +};
> +
> +static int sc27xx_fgu_adc_to_current(int adc)
> +{
> +	return (adc * 1000) / SC27XX_FGU_1000MA_ADC;
> +}
> +
> +static int sc27xx_fgu_adc_to_voltage(int adc)
> +{
> +	return (adc * 1000) / SC27XX_FGU_1000MV_ADC;
> +}
> +
> +static int sc27xx_fgu_ocv_to_capacity(struct sc27xx_fgu_data *data, int ocv)
> +{
> +	struct power_supply_battery_ocv_table *tab = data->cap_table;
> +	int n = data->table_len;
> +	int i, cap, tmp;
> +
> +	/*
> +	 * Find the position in the table for current battery OCV value,
> +	 * then use these two points to calculate battery capacity
> +	 * according to the linear method.
> +	 */
> +	for (i = 0; i < n; i++)
> +		if (ocv > tab[i].ocv)
> +			break;
> +
> +	if (i > 0 && i < n) {
> +		tmp = (tab[i - 1].capacity - tab[i].capacity) *
> +			(ocv - tab[i].ocv) * 2;
> +		tmp /= tab[i - 1].ocv - tab[i].ocv;
> +		tmp = (tmp + 1) / 2;
> +		cap = tmp + tab[i].capacity;
> +	} else if (i == 0) {
> +		cap = tab[0].capacity;
> +	} else {
> +		cap = tab[n - 1].capacity;
> +	}
> +
> +	return cap;
> +}

We should have a function working on the table(s) from battery_info
instead, which can be shared by drivers. Maybe something like this:

int ocv2cap_simple(struct power_supply_battery_ocv_table *table, int ocv)
{
    // basically your code but working with the common table
}

struct power_supply_battery_ocv_table *find_ocv2cap_table(struct power_supply_battery_info *info, int temp)
{
    int best_temp_diff, temp_diff = INT_MAX;
    int best_index = 0;

    for (int i=0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++) {
        temp_diff = abs(info->ocv_temp[i] - temp);
        if (temp_diff < best_temp_diff) {
            best_temp_diff = temp_diff;
            best_index = i;
        }
    }

    return &info->ocv_table[i];
}

int batinfo_ocv2cap(struct power_supply_battery_info *info, int ocv, int temp)
{
    struct power_supply_battery_ocv_table *table = find_ocv2cap_table(battery_info, temp);
    return ocv2cap_simple(table, ocv);
}

> +
> +/*
> + * When system boots on, we can not read battery capacity from coulomb
> + * registers, since now the coulomb registers are invalid. So we should
> + * calculate the battery open circuit voltage, and get current battery
> + * capacity according to the capacity table.
> + */
> +static int sc27xx_fgu_get_boot_capacity(struct sc27xx_fgu_data *data, int *cap)
> +{
> +	int volt, cur, oci, ocv, ret;
> +
> +	/*
> +	 * After system booting on, the SC27XX_FGU_CLBCNT_QMAXL register saved
> +	 * the first sampled open circuit current.
> +	 */
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_QMAXL,
> +			  &cur);
> +	if (ret)
> +		return ret;
> +
> +	cur <<= 1;
> +	oci = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
> +
> +	/*
> +	 * Should get the OCV from SC27XX_FGU_POCV register at the system
> +	 * beginning. It is ADC values reading from registers which need to
> +	 * convert the corresponding voltage.
> +	 */
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_POCV, &volt);
> +	if (ret)
> +		return ret;
> +
> +	volt = sc27xx_fgu_adc_to_voltage(volt);
> +	ocv = volt - (oci * data->internal_resist) / 1000;
> +
> +	/*
> +	 * Parse the capacity table to look up the correct capacity percent
> +	 * according to current battery's corresponding OCV values.
> +	 */
> +	*cap = sc27xx_fgu_ocv_to_capacity(data, ocv);
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_set_clbcnt(struct sc27xx_fgu_data *data, int clbcnt)
> +{
> +	int ret;
> +
> +	clbcnt *= SC27XX_FGU_SAMPLE_HZ;
> +
> +	ret = regmap_update_bits(data->regmap,
> +				 data->base + SC27XX_FGU_CLBCNT_SETL,
> +				 SC27XX_FGU_CLBCNT_MASK, clbcnt);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(data->regmap,
> +				 data->base + SC27XX_FGU_CLBCNT_SETH,
> +				 SC27XX_FGU_CLBCNT_MASK,
> +				 clbcnt >> SC27XX_FGU_CLBCNT_SHIFT);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, data->base + SC27XX_FGU_START,
> +				 SC27XX_WRITE_SELCLB_EN,
> +				 SC27XX_WRITE_SELCLB_EN);
> +}
> +
> +static int sc27xx_fgu_get_clbcnt(struct sc27xx_fgu_data *data, int *clb_cnt)
> +{
> +	int ccl, cch, ret;
> +
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALL,
> +			  &ccl);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALH,
> +			  &cch);
> +	if (ret)
> +		return ret;
> +
> +	*clb_cnt = ccl & SC27XX_FGU_CLBCNT_MASK;
> +	*clb_cnt |= (cch & SC27XX_FGU_CLBCNT_MASK) << SC27XX_FGU_CLBCNT_SHIFT;
> +	*clb_cnt /= SC27XX_FGU_SAMPLE_HZ;
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_capacity(struct sc27xx_fgu_data *data, int *cap)
> +{
> +	int ret, cur_clbcnt, delta_clbcnt, delta_cap, temp;
> +
> +	/* Get current coulomb counters firstly */
> +	ret = sc27xx_fgu_get_clbcnt(data, &cur_clbcnt);
> +	if (ret)
> +		return ret;
> +
> +	delta_clbcnt = cur_clbcnt - data->init_clbcnt;
> +
> +	/*
> +	 * Convert coulomb counter to delta capacity (mAh), and set multiplier
> +	 * as 100 to improve the precision.
> +	 */
> +	temp = DIV_ROUND_CLOSEST(delta_clbcnt, 360);
> +	temp = sc27xx_fgu_adc_to_current(temp);
> +
> +	/*
> +	 * Convert to capacity percent of the battery total capacity,
> +	 * and multiplier is 100 too.
> +	 */
> +	delta_cap = DIV_ROUND_CLOSEST(temp * 100, data->total_cap);
> +	*cap = delta_cap + data->init_cap;
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
> +{
> +	int ret, vol;
> +
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * It is ADC values reading from registers which need to convert to
> +	 * corresponding voltage values.
> +	 */
> +	*val = sc27xx_fgu_adc_to_voltage(vol);
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
> +{
> +	int ret, cur;
> +
> +	ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * It is ADC values reading from registers which need to convert to
> +	 * corresponding current values.
> +	 */
> +	*val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
> +{
> +	int vol, cur, ret;
> +
> +	ret = sc27xx_fgu_get_vbat_vol(data, &vol);
> +	if (ret)
> +		return ret;
> +
> +	ret = sc27xx_fgu_get_current(data, &cur);
> +	if (ret)
> +		return ret;
> +
> +	/* Return the battery OCV in micro volts. */
> +	*val = vol * 1000 - cur * data->internal_resist;
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
> +{
> +	return iio_read_channel_processed(data->channel, temp);
> +}
> +
> +static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
> +{
> +	int ret, vol;
> +
> +	ret = sc27xx_fgu_get_vbat_vol(data, &vol);
> +	if (ret)
> +		return ret;
> +
> +	if (vol > data->max_volt)
> +		*health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
> +	else
> +		*health = POWER_SUPPLY_HEALTH_GOOD;
> +
> +	return 0;
> +}
> +
> +static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
> +{
> +	union power_supply_propval val;
> +	struct power_supply *psy;
> +	int i, ret = -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
> +		psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
> +		if (!psy)
> +			continue;
> +
> +		ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
> +						&val);
> +		power_supply_put(psy);
> +		if (ret)
> +			return ret;
> +
> +		*status = val.intval;
> +	}
> +
> +	return ret;
> +}
> +
> +static int sc27xx_fgu_get_property(struct power_supply *psy,
> +				   enum power_supply_property psp,
> +				   union power_supply_propval *val)
> +{
> +	struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
> +	int ret = 0;
> +	int value;
> +
> +	mutex_lock(&data->lock);
> +
> +	switch (psp) {
> +	case POWER_SUPPLY_PROP_STATUS:
> +		ret = sc27xx_fgu_get_status(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_HEALTH:
> +		ret = sc27xx_fgu_get_health(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_PRESENT:
> +		val->intval = data->bat_present;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_TEMP:
> +		ret = sc27xx_fgu_get_temp(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_TECHNOLOGY:
> +		val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_CAPACITY:
> +		ret = sc27xx_fgu_get_capacity(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> +		ret = sc27xx_fgu_get_vbat_vol(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value * 1000;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_VOLTAGE_OCV:
> +		ret = sc27xx_fgu_get_vbat_ocv(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value;
> +		break;
> +
> +	case POWER_SUPPLY_PROP_CURRENT_NOW:
> +	case POWER_SUPPLY_PROP_CURRENT_AVG:
> +		ret = sc27xx_fgu_get_current(data, &value);
> +		if (ret)
> +			goto error;
> +
> +		val->intval = value * 1000;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +error:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
> +{
> +	struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
> +
> +	power_supply_changed(data->battery);
> +}
> +
> +static enum power_supply_property sc27xx_fgu_props[] = {
> +	POWER_SUPPLY_PROP_STATUS,
> +	POWER_SUPPLY_PROP_HEALTH,
> +	POWER_SUPPLY_PROP_PRESENT,
> +	POWER_SUPPLY_PROP_TEMP,
> +	POWER_SUPPLY_PROP_TECHNOLOGY,
> +	POWER_SUPPLY_PROP_CAPACITY,
> +	POWER_SUPPLY_PROP_VOLTAGE_NOW,
> +	POWER_SUPPLY_PROP_VOLTAGE_OCV,
> +	POWER_SUPPLY_PROP_CURRENT_NOW,
> +	POWER_SUPPLY_PROP_CURRENT_AVG,
> +};
> +
> +static const struct power_supply_desc sc27xx_fgu_desc = {
> +	.name			= "sc27xx-fgu",
> +	.type			= POWER_SUPPLY_TYPE_BATTERY,
> +	.properties		= sc27xx_fgu_props,
> +	.num_properties		= ARRAY_SIZE(sc27xx_fgu_props),
> +	.get_property		= sc27xx_fgu_get_property,
> +	.external_power_changed	= sc27xx_fgu_external_power_changed,
> +};
> +
> +static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
> +{
> +	struct sc27xx_fgu_data *data = dev_id;
> +	int state;
> +
> +	mutex_lock(&data->lock);
> +
> +	state = gpiod_get_value_cansleep(data->gpiod);
> +	if (state < 0) {
> +		dev_err(data->dev, "failed to get gpio state\n");
> +		mutex_unlock(&data->lock);
> +		return IRQ_RETVAL(state);
> +	}
> +
> +	data->bat_present = !!state;
> +
> +	mutex_unlock(&data->lock);
> +
> +	power_supply_changed(data->battery);
> +	return IRQ_HANDLED;
> +}
> +
> +static void sc27xx_fgu_disable(void *_data)
> +{
> +	struct sc27xx_fgu_data *data = _data;
> +
> +	regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
> +	regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
> +}
> +
> +static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
> +{
> +	/*
> +	 * Get current capacity (mAh) = battery total capacity (mAh) *
> +	 * current capacity percent (capacity / 100).
> +	 */
> +	int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
> +
> +	/*
> +	 * Convert current capacity (mAh) to coulomb counter according to the
> +	 * formula: 1 mAh =3.6 coulomb.
> +	 */
> +	return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
> +}
> +
> +static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
> +{
> +	struct power_supply_battery_info info = { };
> +	struct power_supply_battery_ocv_table *table;
> +	int ret, i;
> +
> +	ret = power_supply_get_battery_info(data->battery, &info);
> +	if (ret) {
> +		dev_err(data->dev, "failed to get battery information\n");
> +		return ret;
> +	}
> +
> +	data->total_cap = info.charge_full_design_uah / 1000;
> +	data->max_volt = info.constant_charge_voltage_max_uv / 1000;
> +	data->internal_resist = info.internal_resistance_uohm / 1000;
> +	data->table_len = info.ocv_table_size[0];
> +
> +	/*
> +	 * For SC27XX fuel gauge device, we only need one ocv-capacity
> +	 * table in normal temperature.
> +	 */
> +	table = info.ocv_table[0];
> +	if (!table)
> +		return -EINVAL;
> +
> +	data->cap_table = devm_kzalloc(data->dev,
> +				       data->table_len * sizeof(*table),
> +				       GFP_KERNEL);
> +	if (!data->cap_table) {
> +		power_supply_put_battery_info(data->battery, &info);
> +		return -ENOMEM;
> +	}
> +
> +	for (i = 0; i < data->table_len; i++) {
> +		data->cap_table[i].ocv = table[i].ocv / 1000;
> +		data->cap_table[i].capacity = table[i].capacity;
> +	}
> +
> +	power_supply_put_battery_info(data->battery, &info);
> +
> +	/* Enable the FGU module */
> +	ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
> +				 SC27XX_FGU_EN, SC27XX_FGU_EN);
> +	if (ret) {
> +		dev_err(data->dev, "failed to enable fgu\n");
> +		return ret;
> +	}
> +
> +	/* Enable the FGU RTC clock to make it work */
> +	ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
> +				 SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
> +	if (ret) {
> +		dev_err(data->dev, "failed to enable fgu RTC clock\n");
> +		goto disable_fgu;
> +	}
> +
> +	/*
> +	 * Get the boot battery capacity when system powers on, which is used to
> +	 * initialize the coulomb counter. After that, we can read the coulomb
> +	 * counter to measure the battery capacity.
> +	 */
> +	ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
> +	if (ret) {
> +		dev_err(data->dev, "failed to get boot capacity\n");
> +		goto disable_clk;
> +	}
> +
> +	/*
> +	 * Convert battery capacity to the corresponding initial coulomb counter
> +	 * and set into coulomb counter registers.
> +	 */
> +	data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
> +	ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
> +	if (ret) {
> +		dev_err(data->dev, "failed to initialize coulomb counter\n");
> +		goto disable_clk;
> +	}
> +
> +	return 0;
> +
> +disable_clk:
> +	regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
> +disable_fgu:
> +	regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
> +
> +	return ret;
> +}
> +
> +static int sc27xx_fgu_probe(struct platform_device *pdev)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	struct power_supply_config fgu_cfg = { };
> +	struct sc27xx_fgu_data *data;
> +	int ret, irq;
> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!data->regmap) {
> +		dev_err(&pdev->dev, "failed to get regmap\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = of_property_read_u32(np, "reg", &data->base);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to get fgu address\n");
> +		return ret;
> +	}

Can you please use device_property_read_u32() instead.
If I didn't miss anything you can drop #include <linux/of.h> afterwards.

-- Sebastian

> +	data->channel = devm_iio_channel_get(&pdev->dev, "bat-temp");
> +	if (IS_ERR(data->channel)) {
> +		dev_err(&pdev->dev, "failed to get IIO channel\n");
> +		return PTR_ERR(data->channel);
> +	}
> +
> +	data->gpiod = devm_gpiod_get(&pdev->dev, "bat-detect", GPIOD_IN);
> +	if (IS_ERR(data->gpiod)) {
> +		dev_err(&pdev->dev, "failed to get battery detection GPIO\n");
> +		return PTR_ERR(data->gpiod);
> +	}
> +
> +	ret = gpiod_get_value_cansleep(data->gpiod);
> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to get gpio state\n");
> +		return ret;
> +	}
> +
> +	data->bat_present = !!ret;
> +	mutex_init(&data->lock);
> +	data->dev = &pdev->dev;
> +
> +	fgu_cfg.drv_data = data;
> +	fgu_cfg.of_node = np;
> +	data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
> +						   &fgu_cfg);
> +	if (IS_ERR(data->battery)) {
> +		dev_err(&pdev->dev, "failed to register power supply\n");
> +		return PTR_ERR(data->battery);
> +	}
> +
> +	ret = sc27xx_fgu_hw_init(data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
> +		return ret;
> +	}
> +
> +	ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
> +	if (ret) {
> +		sc27xx_fgu_disable(data);
> +		dev_err(&pdev->dev, "failed to add fgu disable action\n");
> +		return ret;
> +	}
> +
> +	irq = gpiod_to_irq(data->gpiod);
> +	if (irq < 0) {
> +		dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
> +		return irq;
> +	}
> +
> +	ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
> +					sc27xx_fgu_bat_detection,
> +					IRQF_ONESHOT | IRQF_TRIGGER_RISING |
> +					IRQF_TRIGGER_FALLING,
> +					pdev->name, data);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request IRQ\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sc27xx_fgu_of_match[] = {
> +	{ .compatible = "sprd,sc2731-fgu", },
> +	{ }
> +};
> +
> +static struct platform_driver sc27xx_fgu_driver = {
> +	.probe = sc27xx_fgu_probe,
> +	.driver = {
> +		.name = "sc27xx-fgu",
> +		.of_match_table = sc27xx_fgu_of_match,
> +	}
> +};
> +
> +module_platform_driver(sc27xx_fgu_driver);
> +
> +MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
> +MODULE_LICENSE("GPL v2");

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

Re: [PATCH v2 4/4] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Baolin Wang]

On 26 September 2018 at 23:30, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Wed, Sep 26, 2018 at 10:59:14AM +0800, Baolin Wang wrote:
>> This patch adds the Spreadtrum SC27XX serial PMICs fuel gauge support,
>> which is used to calculate the battery capacity.
>>
>> Original-by: Yuanjiang Yu <yuanjiang.yu@unisoc.com>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - Use battery standard properties to get internal resistance and ocv table.
>>  - Change devm_gpiod_get_optional() to devm_gpiod_get().
>>  - Add power_supply_changed() when detecting battery present change.
>>  - Return micro volts for sc27xx_fgu_get_vbat_ocv().
>> ---
>>  drivers/power/supply/Kconfig             |    7 +
>>  drivers/power/supply/Makefile            |    1 +
>>  drivers/power/supply/sc27xx_fuel_gauge.c |  691 ++++++++++++++++++++++++++++++
>>  3 files changed, 699 insertions(+)
>>  create mode 100644 drivers/power/supply/sc27xx_fuel_gauge.c
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index f27cf07..917f4b7 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -652,4 +652,11 @@ config CHARGER_SC2731
>>        Say Y here to enable support for battery charging with SC2731
>>        PMIC chips.
>>
>> +config FUEL_GAUGE_SC27XX
>> +     tristate "Spreadtrum SC27XX fuel gauge driver"
>> +     depends on MFD_SC27XX_PMIC || COMPILE_TEST
>> +     help
>> +      Say Y here to enable support for fuel gauge with SC27XX
>> +      PMIC chips.
>> +
>>  endif # POWER_SUPPLY
>> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
>> index 767105b..b731c2a 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -86,3 +86,4 @@ obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o
>>  obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o
>>  obj-$(CONFIG_CHARGER_CROS_USBPD)     += cros_usbpd-charger.o
>>  obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o
>> +obj-$(CONFIG_FUEL_GAUGE_SC27XX)      += sc27xx_fuel_gauge.o
>> diff --git a/drivers/power/supply/sc27xx_fuel_gauge.c b/drivers/power/supply/sc27xx_fuel_gauge.c
>> new file mode 100644
>> index 0000000..d3422cf
>> --- /dev/null
>> +++ b/drivers/power/supply/sc27xx_fuel_gauge.c
>> @@ -0,0 +1,691 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2018 Spreadtrum Communications Inc.
>> +
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/iio/consumer.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/power_supply.h>
>> +#include <linux/regmap.h>
>> +
>> +/* PMIC global control registers definition */
>> +#define SC27XX_MODULE_EN0            0xc08
>> +#define SC27XX_CLK_EN0                       0xc18
>> +#define SC27XX_FGU_EN                        BIT(7)
>> +#define SC27XX_FGU_RTC_EN            BIT(6)
>> +
>> +/* FGU registers definition */
>> +#define SC27XX_FGU_START             0x0
>> +#define SC27XX_FGU_CONFIG            0x4
>> +#define SC27XX_FGU_ADC_CONFIG                0x8
>> +#define SC27XX_FGU_STATUS            0xc
>> +#define SC27XX_FGU_INT_EN            0x10
>> +#define SC27XX_FGU_INT_CLR           0x14
>> +#define SC27XX_FGU_INT_STS           0x1c
>> +#define SC27XX_FGU_VOLTAGE           0x20
>> +#define SC27XX_FGU_OCV                       0x24
>> +#define SC27XX_FGU_POCV                      0x28
>> +#define SC27XX_FGU_CURRENT           0x2c
>> +#define SC27XX_FGU_CLBCNT_SETH               0x50
>> +#define SC27XX_FGU_CLBCNT_SETL               0x54
>> +#define SC27XX_FGU_CLBCNT_VALH               0x68
>> +#define SC27XX_FGU_CLBCNT_VALL               0x6c
>> +#define SC27XX_FGU_CLBCNT_QMAXL              0x74
>> +
>> +#define SC27XX_WRITE_SELCLB_EN               BIT(0)
>> +#define SC27XX_FGU_CLBCNT_MASK               GENMASK(15, 0)
>> +#define SC27XX_FGU_CLBCNT_SHIFT              16
>> +
>> +#define SC27XX_FGU_1000MV_ADC                686
>> +#define SC27XX_FGU_1000MA_ADC                1372
>> +#define SC27XX_FGU_CUR_BASIC_ADC     8192
>> +#define SC27XX_FGU_SAMPLE_HZ         2
>> +
>> +/*
>> + * struct sc27xx_fgu_data: describe the FGU device
>> + * @regmap: regmap for register access
>> + * @dev: platform device
>> + * @battery: battery power supply
>> + * @base: the base offset for the controller
>> + * @lock: protect the structure
>> + * @gpiod: GPIO for battery detection
>> + * @channel: IIO channel to get battery temperature
>> + * @internal_resist: the battery internal resistance in mOhm
>> + * @total_cap: the total capacity of the battery in mAh
>> + * @init_cap: the initial capacity of the battery in mAh
>> + * @init_clbcnt: the initial coulomb counter
>> + * @max_volt: the maximum constant input voltage in millivolt
>> + * @table_len: the capacity table length
>> + * @cap_table: capacity table with corresponding ocv
>> + */
>> +struct sc27xx_fgu_data {
>> +     struct regmap *regmap;
>> +     struct device *dev;
>> +     struct power_supply *battery;
>> +     u32 base;
>> +     struct mutex lock;
>> +     struct gpio_desc *gpiod;
>> +     struct iio_channel *channel;
>> +     bool bat_present;
>> +     int internal_resist;
>> +     int total_cap;
>> +     int init_cap;
>> +     int init_clbcnt;
>> +     int max_volt;
>> +     int table_len;
>> +     struct power_supply_battery_ocv_table *cap_table;
>> +};
>> +
>> +static const char * const sc27xx_charger_supply_name[] = {
>> +     "sc2731_charger",
>> +     "sc2720_charger",
>> +     "sc2721_charger",
>> +     "sc2723_charger",
>> +};
>> +
>> +static int sc27xx_fgu_adc_to_current(int adc)
>> +{
>> +     return (adc * 1000) / SC27XX_FGU_1000MA_ADC;
>> +}
>> +
>> +static int sc27xx_fgu_adc_to_voltage(int adc)
>> +{
>> +     return (adc * 1000) / SC27XX_FGU_1000MV_ADC;
>> +}
>> +
>> +static int sc27xx_fgu_ocv_to_capacity(struct sc27xx_fgu_data *data, int ocv)
>> +{
>> +     struct power_supply_battery_ocv_table *tab = data->cap_table;
>> +     int n = data->table_len;
>> +     int i, cap, tmp;
>> +
>> +     /*
>> +      * Find the position in the table for current battery OCV value,
>> +      * then use these two points to calculate battery capacity
>> +      * according to the linear method.
>> +      */
>> +     for (i = 0; i < n; i++)
>> +             if (ocv > tab[i].ocv)
>> +                     break;
>> +
>> +     if (i > 0 && i < n) {
>> +             tmp = (tab[i - 1].capacity - tab[i].capacity) *
>> +                     (ocv - tab[i].ocv) * 2;
>> +             tmp /= tab[i - 1].ocv - tab[i].ocv;
>> +             tmp = (tmp + 1) / 2;
>> +             cap = tmp + tab[i].capacity;
>> +     } else if (i == 0) {
>> +             cap = tab[0].capacity;
>> +     } else {
>> +             cap = tab[n - 1].capacity;
>> +     }
>> +
>> +     return cap;
>> +}
>
> We should have a function working on the table(s) from battery_info
> instead, which can be shared by drivers. Maybe something like this:
>
> int ocv2cap_simple(struct power_supply_battery_ocv_table *table, int ocv)
> {
>     // basically your code but working with the common table
> }
>
> struct power_supply_battery_ocv_table *find_ocv2cap_table(struct power_supply_battery_info *info, int temp)
> {
>     int best_temp_diff, temp_diff = INT_MAX;
>     int best_index = 0;
>
>     for (int i=0; i < POWER_SUPPLY_OCV_TEMP_MAX; i++) {
>         temp_diff = abs(info->ocv_temp[i] - temp);
>         if (temp_diff < best_temp_diff) {
>             best_temp_diff = temp_diff;
>             best_index = i;
>         }
>     }
>
>     return &info->ocv_table[i];
> }
>
> int batinfo_ocv2cap(struct power_supply_battery_info *info, int ocv, int temp)
> {
>     struct power_supply_battery_ocv_table *table = find_ocv2cap_table(battery_info, temp);
>     return ocv2cap_simple(table, ocv);
> }

OK. Will add these helpers in next version.

>> +
>> +/*
>> + * When system boots on, we can not read battery capacity from coulomb
>> + * registers, since now the coulomb registers are invalid. So we should
>> + * calculate the battery open circuit voltage, and get current battery
>> + * capacity according to the capacity table.
>> + */
>> +static int sc27xx_fgu_get_boot_capacity(struct sc27xx_fgu_data *data, int *cap)
>> +{
>> +     int volt, cur, oci, ocv, ret;
>> +
>> +     /*
>> +      * After system booting on, the SC27XX_FGU_CLBCNT_QMAXL register saved
>> +      * the first sampled open circuit current.
>> +      */
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_QMAXL,
>> +                       &cur);
>> +     if (ret)
>> +             return ret;
>> +
>> +     cur <<= 1;
>> +     oci = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
>> +
>> +     /*
>> +      * Should get the OCV from SC27XX_FGU_POCV register at the system
>> +      * beginning. It is ADC values reading from registers which need to
>> +      * convert the corresponding voltage.
>> +      */
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_POCV, &volt);
>> +     if (ret)
>> +             return ret;
>> +
>> +     volt = sc27xx_fgu_adc_to_voltage(volt);
>> +     ocv = volt - (oci * data->internal_resist) / 1000;
>> +
>> +     /*
>> +      * Parse the capacity table to look up the correct capacity percent
>> +      * according to current battery's corresponding OCV values.
>> +      */
>> +     *cap = sc27xx_fgu_ocv_to_capacity(data, ocv);
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_set_clbcnt(struct sc27xx_fgu_data *data, int clbcnt)
>> +{
>> +     int ret;
>> +
>> +     clbcnt *= SC27XX_FGU_SAMPLE_HZ;
>> +
>> +     ret = regmap_update_bits(data->regmap,
>> +                              data->base + SC27XX_FGU_CLBCNT_SETL,
>> +                              SC27XX_FGU_CLBCNT_MASK, clbcnt);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_update_bits(data->regmap,
>> +                              data->base + SC27XX_FGU_CLBCNT_SETH,
>> +                              SC27XX_FGU_CLBCNT_MASK,
>> +                              clbcnt >> SC27XX_FGU_CLBCNT_SHIFT);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return regmap_update_bits(data->regmap, data->base + SC27XX_FGU_START,
>> +                              SC27XX_WRITE_SELCLB_EN,
>> +                              SC27XX_WRITE_SELCLB_EN);
>> +}
>> +
>> +static int sc27xx_fgu_get_clbcnt(struct sc27xx_fgu_data *data, int *clb_cnt)
>> +{
>> +     int ccl, cch, ret;
>> +
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALL,
>> +                       &ccl);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CLBCNT_VALH,
>> +                       &cch);
>> +     if (ret)
>> +             return ret;
>> +
>> +     *clb_cnt = ccl & SC27XX_FGU_CLBCNT_MASK;
>> +     *clb_cnt |= (cch & SC27XX_FGU_CLBCNT_MASK) << SC27XX_FGU_CLBCNT_SHIFT;
>> +     *clb_cnt /= SC27XX_FGU_SAMPLE_HZ;
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_capacity(struct sc27xx_fgu_data *data, int *cap)
>> +{
>> +     int ret, cur_clbcnt, delta_clbcnt, delta_cap, temp;
>> +
>> +     /* Get current coulomb counters firstly */
>> +     ret = sc27xx_fgu_get_clbcnt(data, &cur_clbcnt);
>> +     if (ret)
>> +             return ret;
>> +
>> +     delta_clbcnt = cur_clbcnt - data->init_clbcnt;
>> +
>> +     /*
>> +      * Convert coulomb counter to delta capacity (mAh), and set multiplier
>> +      * as 100 to improve the precision.
>> +      */
>> +     temp = DIV_ROUND_CLOSEST(delta_clbcnt, 360);
>> +     temp = sc27xx_fgu_adc_to_current(temp);
>> +
>> +     /*
>> +      * Convert to capacity percent of the battery total capacity,
>> +      * and multiplier is 100 too.
>> +      */
>> +     delta_cap = DIV_ROUND_CLOSEST(temp * 100, data->total_cap);
>> +     *cap = delta_cap + data->init_cap;
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_vbat_vol(struct sc27xx_fgu_data *data, int *val)
>> +{
>> +     int ret, vol;
>> +
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_VOLTAGE, &vol);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /*
>> +      * It is ADC values reading from registers which need to convert to
>> +      * corresponding voltage values.
>> +      */
>> +     *val = sc27xx_fgu_adc_to_voltage(vol);
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_current(struct sc27xx_fgu_data *data, int *val)
>> +{
>> +     int ret, cur;
>> +
>> +     ret = regmap_read(data->regmap, data->base + SC27XX_FGU_CURRENT, &cur);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /*
>> +      * It is ADC values reading from registers which need to convert to
>> +      * corresponding current values.
>> +      */
>> +     *val = sc27xx_fgu_adc_to_current(cur - SC27XX_FGU_CUR_BASIC_ADC);
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_vbat_ocv(struct sc27xx_fgu_data *data, int *val)
>> +{
>> +     int vol, cur, ret;
>> +
>> +     ret = sc27xx_fgu_get_vbat_vol(data, &vol);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = sc27xx_fgu_get_current(data, &cur);
>> +     if (ret)
>> +             return ret;
>> +
>> +     /* Return the battery OCV in micro volts. */
>> +     *val = vol * 1000 - cur * data->internal_resist;
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_temp(struct sc27xx_fgu_data *data, int *temp)
>> +{
>> +     return iio_read_channel_processed(data->channel, temp);
>> +}
>> +
>> +static int sc27xx_fgu_get_health(struct sc27xx_fgu_data *data, int *health)
>> +{
>> +     int ret, vol;
>> +
>> +     ret = sc27xx_fgu_get_vbat_vol(data, &vol);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (vol > data->max_volt)
>> +             *health = POWER_SUPPLY_HEALTH_OVERVOLTAGE;
>> +     else
>> +             *health = POWER_SUPPLY_HEALTH_GOOD;
>> +
>> +     return 0;
>> +}
>> +
>> +static int sc27xx_fgu_get_status(struct sc27xx_fgu_data *data, int *status)
>> +{
>> +     union power_supply_propval val;
>> +     struct power_supply *psy;
>> +     int i, ret = -EINVAL;
>> +
>> +     for (i = 0; i < ARRAY_SIZE(sc27xx_charger_supply_name); i++) {
>> +             psy = power_supply_get_by_name(sc27xx_charger_supply_name[i]);
>> +             if (!psy)
>> +                     continue;
>> +
>> +             ret = power_supply_get_property(psy, POWER_SUPPLY_PROP_STATUS,
>> +                                             &val);
>> +             power_supply_put(psy);
>> +             if (ret)
>> +                     return ret;
>> +
>> +             *status = val.intval;
>> +     }
>> +
>> +     return ret;
>> +}
>> +
>> +static int sc27xx_fgu_get_property(struct power_supply *psy,
>> +                                enum power_supply_property psp,
>> +                                union power_supply_propval *val)
>> +{
>> +     struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
>> +     int ret = 0;
>> +     int value;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     switch (psp) {
>> +     case POWER_SUPPLY_PROP_STATUS:
>> +             ret = sc27xx_fgu_get_status(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_HEALTH:
>> +             ret = sc27xx_fgu_get_health(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_PRESENT:
>> +             val->intval = data->bat_present;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_TEMP:
>> +             ret = sc27xx_fgu_get_temp(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_TECHNOLOGY:
>> +             val->intval = POWER_SUPPLY_TECHNOLOGY_LION;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_CAPACITY:
>> +             ret = sc27xx_fgu_get_capacity(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_VOLTAGE_NOW:
>> +             ret = sc27xx_fgu_get_vbat_vol(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value * 1000;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_VOLTAGE_OCV:
>> +             ret = sc27xx_fgu_get_vbat_ocv(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value;
>> +             break;
>> +
>> +     case POWER_SUPPLY_PROP_CURRENT_NOW:
>> +     case POWER_SUPPLY_PROP_CURRENT_AVG:
>> +             ret = sc27xx_fgu_get_current(data, &value);
>> +             if (ret)
>> +                     goto error;
>> +
>> +             val->intval = value * 1000;
>> +             break;
>> +
>> +     default:
>> +             ret = -EINVAL;
>> +             break;
>> +     }
>> +
>> +error:
>> +     mutex_unlock(&data->lock);
>> +     return ret;
>> +}
>> +
>> +static void sc27xx_fgu_external_power_changed(struct power_supply *psy)
>> +{
>> +     struct sc27xx_fgu_data *data = power_supply_get_drvdata(psy);
>> +
>> +     power_supply_changed(data->battery);
>> +}
>> +
>> +static enum power_supply_property sc27xx_fgu_props[] = {
>> +     POWER_SUPPLY_PROP_STATUS,
>> +     POWER_SUPPLY_PROP_HEALTH,
>> +     POWER_SUPPLY_PROP_PRESENT,
>> +     POWER_SUPPLY_PROP_TEMP,
>> +     POWER_SUPPLY_PROP_TECHNOLOGY,
>> +     POWER_SUPPLY_PROP_CAPACITY,
>> +     POWER_SUPPLY_PROP_VOLTAGE_NOW,
>> +     POWER_SUPPLY_PROP_VOLTAGE_OCV,
>> +     POWER_SUPPLY_PROP_CURRENT_NOW,
>> +     POWER_SUPPLY_PROP_CURRENT_AVG,
>> +};
>> +
>> +static const struct power_supply_desc sc27xx_fgu_desc = {
>> +     .name                   = "sc27xx-fgu",
>> +     .type                   = POWER_SUPPLY_TYPE_BATTERY,
>> +     .properties             = sc27xx_fgu_props,
>> +     .num_properties         = ARRAY_SIZE(sc27xx_fgu_props),
>> +     .get_property           = sc27xx_fgu_get_property,
>> +     .external_power_changed = sc27xx_fgu_external_power_changed,
>> +};
>> +
>> +static irqreturn_t sc27xx_fgu_bat_detection(int irq, void *dev_id)
>> +{
>> +     struct sc27xx_fgu_data *data = dev_id;
>> +     int state;
>> +
>> +     mutex_lock(&data->lock);
>> +
>> +     state = gpiod_get_value_cansleep(data->gpiod);
>> +     if (state < 0) {
>> +             dev_err(data->dev, "failed to get gpio state\n");
>> +             mutex_unlock(&data->lock);
>> +             return IRQ_RETVAL(state);
>> +     }
>> +
>> +     data->bat_present = !!state;
>> +
>> +     mutex_unlock(&data->lock);
>> +
>> +     power_supply_changed(data->battery);
>> +     return IRQ_HANDLED;
>> +}
>> +
>> +static void sc27xx_fgu_disable(void *_data)
>> +{
>> +     struct sc27xx_fgu_data *data = _data;
>> +
>> +     regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
>> +     regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
>> +}
>> +
>> +static int sc27xx_fgu_cap_to_clbcnt(struct sc27xx_fgu_data *data, int capacity)
>> +{
>> +     /*
>> +      * Get current capacity (mAh) = battery total capacity (mAh) *
>> +      * current capacity percent (capacity / 100).
>> +      */
>> +     int cur_cap = DIV_ROUND_CLOSEST(data->total_cap * capacity, 100);
>> +
>> +     /*
>> +      * Convert current capacity (mAh) to coulomb counter according to the
>> +      * formula: 1 mAh =3.6 coulomb.
>> +      */
>> +     return DIV_ROUND_CLOSEST(cur_cap * 36, 10);
>> +}
>> +
>> +static int sc27xx_fgu_hw_init(struct sc27xx_fgu_data *data)
>> +{
>> +     struct power_supply_battery_info info = { };
>> +     struct power_supply_battery_ocv_table *table;
>> +     int ret, i;
>> +
>> +     ret = power_supply_get_battery_info(data->battery, &info);
>> +     if (ret) {
>> +             dev_err(data->dev, "failed to get battery information\n");
>> +             return ret;
>> +     }
>> +
>> +     data->total_cap = info.charge_full_design_uah / 1000;
>> +     data->max_volt = info.constant_charge_voltage_max_uv / 1000;
>> +     data->internal_resist = info.internal_resistance_uohm / 1000;
>> +     data->table_len = info.ocv_table_size[0];
>> +
>> +     /*
>> +      * For SC27XX fuel gauge device, we only need one ocv-capacity
>> +      * table in normal temperature.
>> +      */
>> +     table = info.ocv_table[0];
>> +     if (!table)
>> +             return -EINVAL;
>> +
>> +     data->cap_table = devm_kzalloc(data->dev,
>> +                                    data->table_len * sizeof(*table),
>> +                                    GFP_KERNEL);
>> +     if (!data->cap_table) {
>> +             power_supply_put_battery_info(data->battery, &info);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     for (i = 0; i < data->table_len; i++) {
>> +             data->cap_table[i].ocv = table[i].ocv / 1000;
>> +             data->cap_table[i].capacity = table[i].capacity;
>> +     }
>> +
>> +     power_supply_put_battery_info(data->battery, &info);
>> +
>> +     /* Enable the FGU module */
>> +     ret = regmap_update_bits(data->regmap, SC27XX_MODULE_EN0,
>> +                              SC27XX_FGU_EN, SC27XX_FGU_EN);
>> +     if (ret) {
>> +             dev_err(data->dev, "failed to enable fgu\n");
>> +             return ret;
>> +     }
>> +
>> +     /* Enable the FGU RTC clock to make it work */
>> +     ret = regmap_update_bits(data->regmap, SC27XX_CLK_EN0,
>> +                              SC27XX_FGU_RTC_EN, SC27XX_FGU_RTC_EN);
>> +     if (ret) {
>> +             dev_err(data->dev, "failed to enable fgu RTC clock\n");
>> +             goto disable_fgu;
>> +     }
>> +
>> +     /*
>> +      * Get the boot battery capacity when system powers on, which is used to
>> +      * initialize the coulomb counter. After that, we can read the coulomb
>> +      * counter to measure the battery capacity.
>> +      */
>> +     ret = sc27xx_fgu_get_boot_capacity(data, &data->init_cap);
>> +     if (ret) {
>> +             dev_err(data->dev, "failed to get boot capacity\n");
>> +             goto disable_clk;
>> +     }
>> +
>> +     /*
>> +      * Convert battery capacity to the corresponding initial coulomb counter
>> +      * and set into coulomb counter registers.
>> +      */
>> +     data->init_clbcnt = sc27xx_fgu_cap_to_clbcnt(data, data->init_cap);
>> +     ret = sc27xx_fgu_set_clbcnt(data, data->init_clbcnt);
>> +     if (ret) {
>> +             dev_err(data->dev, "failed to initialize coulomb counter\n");
>> +             goto disable_clk;
>> +     }
>> +
>> +     return 0;
>> +
>> +disable_clk:
>> +     regmap_update_bits(data->regmap, SC27XX_CLK_EN0, SC27XX_FGU_RTC_EN, 0);
>> +disable_fgu:
>> +     regmap_update_bits(data->regmap, SC27XX_MODULE_EN0, SC27XX_FGU_EN, 0);
>> +
>> +     return ret;
>> +}
>> +
>> +static int sc27xx_fgu_probe(struct platform_device *pdev)
>> +{
>> +     struct device_node *np = pdev->dev.of_node;
>> +     struct power_supply_config fgu_cfg = { };
>> +     struct sc27xx_fgu_data *data;
>> +     int ret, irq;
>> +
>> +     data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +     if (!data)
>> +             return -ENOMEM;
>> +
>> +     data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +     if (!data->regmap) {
>> +             dev_err(&pdev->dev, "failed to get regmap\n");
>> +             return -ENODEV;
>> +     }
>> +
>> +     ret = of_property_read_u32(np, "reg", &data->base);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to get fgu address\n");
>> +             return ret;
>> +     }
>
> Can you please use device_property_read_u32() instead.
> If I didn't miss anything you can drop #include <linux/of.h> afterwards.

Sure. Thanks.

>
> -- Sebastian
>
>> +     data->channel = devm_iio_channel_get(&pdev->dev, "bat-temp");
>> +     if (IS_ERR(data->channel)) {
>> +             dev_err(&pdev->dev, "failed to get IIO channel\n");
>> +             return PTR_ERR(data->channel);
>> +     }
>> +
>> +     data->gpiod = devm_gpiod_get(&pdev->dev, "bat-detect", GPIOD_IN);
>> +     if (IS_ERR(data->gpiod)) {
>> +             dev_err(&pdev->dev, "failed to get battery detection GPIO\n");
>> +             return PTR_ERR(data->gpiod);
>> +     }
>> +
>> +     ret = gpiod_get_value_cansleep(data->gpiod);
>> +     if (ret < 0) {
>> +             dev_err(&pdev->dev, "failed to get gpio state\n");
>> +             return ret;
>> +     }
>> +
>> +     data->bat_present = !!ret;
>> +     mutex_init(&data->lock);
>> +     data->dev = &pdev->dev;
>> +
>> +     fgu_cfg.drv_data = data;
>> +     fgu_cfg.of_node = np;
>> +     data->battery = devm_power_supply_register(&pdev->dev, &sc27xx_fgu_desc,
>> +                                                &fgu_cfg);
>> +     if (IS_ERR(data->battery)) {
>> +             dev_err(&pdev->dev, "failed to register power supply\n");
>> +             return PTR_ERR(data->battery);
>> +     }
>> +
>> +     ret = sc27xx_fgu_hw_init(data);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to initialize fgu hardware\n");
>> +             return ret;
>> +     }
>> +
>> +     ret = devm_add_action(&pdev->dev, sc27xx_fgu_disable, data);
>> +     if (ret) {
>> +             sc27xx_fgu_disable(data);
>> +             dev_err(&pdev->dev, "failed to add fgu disable action\n");
>> +             return ret;
>> +     }
>> +
>> +     irq = gpiod_to_irq(data->gpiod);
>> +     if (irq < 0) {
>> +             dev_err(&pdev->dev, "failed to translate GPIO to IRQ\n");
>> +             return irq;
>> +     }
>> +
>> +     ret = devm_request_threaded_irq(&pdev->dev, irq, NULL,
>> +                                     sc27xx_fgu_bat_detection,
>> +                                     IRQF_ONESHOT | IRQF_TRIGGER_RISING |
>> +                                     IRQF_TRIGGER_FALLING,
>> +                                     pdev->name, data);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "failed to request IRQ\n");
>> +             return ret;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct of_device_id sc27xx_fgu_of_match[] = {
>> +     { .compatible = "sprd,sc2731-fgu", },
>> +     { }
>> +};
>> +
>> +static struct platform_driver sc27xx_fgu_driver = {
>> +     .probe = sc27xx_fgu_probe,
>> +     .driver = {
>> +             .name = "sc27xx-fgu",
>> +             .of_match_table = sc27xx_fgu_of_match,
>> +     }
>> +};
>> +
>> +module_platform_driver(sc27xx_fgu_driver);
>> +
>> +MODULE_DESCRIPTION("Spreadtrum SC27XX PMICs Fual Gauge Unit Driver");
>> +MODULE_LICENSE("GPL v2");



-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Linus Walleij]

On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> Introduce one property to present the battery internal resistance for battery
> information.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - New patch in v2.

I'm a bit confused by the physics in this patch.

The internal resistance of a battery is not a constant in its life cycle,
this varies over the age of the battery, and the reason I thing is
chemical residuals accumulating on the anode and cathode inside
the battery and the energy storage medium aging. (Plus/minus my
ignorance about how batteries actually work.)

AFAIK the fact that the internal resistance varies is of high
importance for people developing algorithms of battery capacity
and longevity. Such that some (hardware) capacity monitors go
to great lengths to measure with high precision the current
internal resistance of the battery for their algorithms.

Sorry for making things more complex, but should it be named
"factory-internal-resistance-micro-ohms" or
"typical-internal-resistance-micro-ohms"?

Yours,
Linus Walleij

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Linus Walleij]

On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:

> Introduce one property to present the battery internal resistance for battery
> information.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> ---
> Changes from v1:
>  - New patch in v2.

I'm a bit confused by the physics in this patch.

The internal resistance of a battery is not a constant in its life cycle,
this varies over the age of the battery, and the reason I thing is
chemical residuals accumulating on the anode and cathode inside
the battery and the energy storage medium aging. (Plus/minus my
ignorance about how batteries actually work.)

AFAIK the fact that the internal resistance varies is of high
importance for people developing algorithms of battery capacity
and longevity. Such that some (hardware) capacity monitors go
to great lengths to measure with high precision the current
internal resistance of the battery for their algorithms.

Sorry for making things more complex, but should it be named
"factory-internal-resistance-micro-ohms" or
"typical-internal-resistance-micro-ohms"?

Yours,
Linus Walleij

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Baolin Wang]

Hi Linus,

On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> Introduce one property to present the battery internal resistance for battery
>> information.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - New patch in v2.
>
> I'm a bit confused by the physics in this patch.
>
> The internal resistance of a battery is not a constant in its life cycle,
> this varies over the age of the battery, and the reason I thing is
> chemical residuals accumulating on the anode and cathode inside
> the battery and the energy storage medium aging. (Plus/minus my
> ignorance about how batteries actually work.)

Yes, you are right. The internal resistance can be affected by
temperature or battery age or other factors. But our solution just
uses one constant internal resistance to calculate OCV value to look
up the capacity table when system boots on, in this case we do not
need one more accuracy OCV, since we will calculate the battery
capacity in future. So we just introduce one estimation constant
internal resistance.

>
> AFAIK the fact that the internal resistance varies is of high
> importance for people developing algorithms of battery capacity
> and longevity. Such that some (hardware) capacity monitors go
> to great lengths to measure with high precision the current
> internal resistance of the battery for their algorithms.
>
> Sorry for making things more complex, but should it be named
> "factory-internal-resistance-micro-ohms" or
> "typical-internal-resistance-micro-ohms"?

I am fine with this change. If Sebastian also agree with this change,
I will fix.
Thanks for your reviewing and comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Baolin Wang]

Hi Linus,

On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>
>> Introduce one property to present the battery internal resistance for battery
>> information.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> ---
>> Changes from v1:
>>  - New patch in v2.
>
> I'm a bit confused by the physics in this patch.
>
> The internal resistance of a battery is not a constant in its life cycle,
> this varies over the age of the battery, and the reason I thing is
> chemical residuals accumulating on the anode and cathode inside
> the battery and the energy storage medium aging. (Plus/minus my
> ignorance about how batteries actually work.)

Yes, you are right. The internal resistance can be affected by
temperature or battery age or other factors. But our solution just
uses one constant internal resistance to calculate OCV value to look
up the capacity table when system boots on, in this case we do not
need one more accuracy OCV, since we will calculate the battery
capacity in future. So we just introduce one estimation constant
internal resistance.

>
> AFAIK the fact that the internal resistance varies is of high
> importance for people developing algorithms of battery capacity
> and longevity. Such that some (hardware) capacity monitors go
> to great lengths to measure with high precision the current
> internal resistance of the battery for their algorithms.
>
> Sorry for making things more complex, but should it be named
> "factory-internal-resistance-micro-ohms" or
> "typical-internal-resistance-micro-ohms"?

I am fine with this change. If Sebastian also agree with this change,
I will fix.
Thanks for your reviewing and comments.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Sebastian Reichel]

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

Hi,

On Wed, Sep 26, 2018 at 04:30:39PM +0800, Baolin Wang wrote:
> Hi Linus,
> 
> On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> >> Introduce one property to present the battery internal resistance for battery
> >> information.
> >>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >> Changes from v1:
> >>  - New patch in v2.
> >
> > I'm a bit confused by the physics in this patch.
> >
> > The internal resistance of a battery is not a constant in its life cycle,
> > this varies over the age of the battery, and the reason I thing is
> > chemical residuals accumulating on the anode and cathode inside
> > the battery and the energy storage medium aging. (Plus/minus my
> > ignorance about how batteries actually work.)
> 
> Yes, you are right. The internal resistance can be affected by
> temperature or battery age or other factors. But our solution just
> uses one constant internal resistance to calculate OCV value to look
> up the capacity table when system boots on, in this case we do not
> need one more accuracy OCV, since we will calculate the battery
> capacity in future. So we just introduce one estimation constant
> internal resistance.
> 
> >
> > AFAIK the fact that the internal resistance varies is of high
> > importance for people developing algorithms of battery capacity
> > and longevity. Such that some (hardware) capacity monitors go
> > to great lengths to measure with high precision the current
> > internal resistance of the battery for their algorithms.
> >
> > Sorry for making things more complex, but should it be named
> > "factory-internal-resistance-micro-ohms" or
> > "typical-internal-resistance-micro-ohms"?
> 
> I am fine with this change. If Sebastian also agree with this change,
> I will fix. Thanks for your reviewing and comments.

Ack.

FWIW for proper battery status you need to collect battery specific
statistics, that is the reason fuel gauge chip providers recommend to
combine the chip with the battery cells into a "smart battery".

-- Sebastian

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

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Sebastian Reichel]

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

Hi,

On Wed, Sep 26, 2018 at 04:30:39PM +0800, Baolin Wang wrote:
> Hi Linus,
> 
> On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
> >
> >> Introduce one property to present the battery internal resistance for battery
> >> information.
> >>
> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> >> ---
> >> Changes from v1:
> >>  - New patch in v2.
> >
> > I'm a bit confused by the physics in this patch.
> >
> > The internal resistance of a battery is not a constant in its life cycle,
> > this varies over the age of the battery, and the reason I thing is
> > chemical residuals accumulating on the anode and cathode inside
> > the battery and the energy storage medium aging. (Plus/minus my
> > ignorance about how batteries actually work.)
> 
> Yes, you are right. The internal resistance can be affected by
> temperature or battery age or other factors. But our solution just
> uses one constant internal resistance to calculate OCV value to look
> up the capacity table when system boots on, in this case we do not
> need one more accuracy OCV, since we will calculate the battery
> capacity in future. So we just introduce one estimation constant
> internal resistance.
> 
> >
> > AFAIK the fact that the internal resistance varies is of high
> > importance for people developing algorithms of battery capacity
> > and longevity. Such that some (hardware) capacity monitors go
> > to great lengths to measure with high precision the current
> > internal resistance of the battery for their algorithms.
> >
> > Sorry for making things more complex, but should it be named
> > "factory-internal-resistance-micro-ohms" or
> > "typical-internal-resistance-micro-ohms"?
> 
> I am fine with this change. If Sebastian also agree with this change,
> I will fix. Thanks for your reviewing and comments.

Ack.

FWIW for proper battery status you need to collect battery specific
statistics, that is the reason fuel gauge chip providers recommend to
combine the chip with the battery cells into a "smart battery".

-- Sebastian

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

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Baolin Wang]

On 26 September 2018 at 20:45, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Wed, Sep 26, 2018 at 04:30:39PM +0800, Baolin Wang wrote:
>> Hi Linus,
>>
>> On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
>> > On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>> >
>> >> Introduce one property to present the battery internal resistance for battery
>> >> information.
>> >>
>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> >> ---
>> >> Changes from v1:
>> >>  - New patch in v2.
>> >
>> > I'm a bit confused by the physics in this patch.
>> >
>> > The internal resistance of a battery is not a constant in its life cycle,
>> > this varies over the age of the battery, and the reason I thing is
>> > chemical residuals accumulating on the anode and cathode inside
>> > the battery and the energy storage medium aging. (Plus/minus my
>> > ignorance about how batteries actually work.)
>>
>> Yes, you are right. The internal resistance can be affected by
>> temperature or battery age or other factors. But our solution just
>> uses one constant internal resistance to calculate OCV value to look
>> up the capacity table when system boots on, in this case we do not
>> need one more accuracy OCV, since we will calculate the battery
>> capacity in future. So we just introduce one estimation constant
>> internal resistance.
>>
>> >
>> > AFAIK the fact that the internal resistance varies is of high
>> > importance for people developing algorithms of battery capacity
>> > and longevity. Such that some (hardware) capacity monitors go
>> > to great lengths to measure with high precision the current
>> > internal resistance of the battery for their algorithms.
>> >
>> > Sorry for making things more complex, but should it be named
>> > "factory-internal-resistance-micro-ohms" or
>> > "typical-internal-resistance-micro-ohms"?
>>
>> I am fine with this change. If Sebastian also agree with this change,
>> I will fix. Thanks for your reviewing and comments.
>
> Ack.
>
> FWIW for proper battery status you need to collect battery specific
> statistics, that is the reason fuel gauge chip providers recommend to
> combine the chip with the battery cells into a "smart battery".

OK. I will rename it as "factory-internal-resistance-micro-ohms" in
next version. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v2 1/4] power: supply: core: Introduce one property to present the battery internal resistance

[Baolin Wang]

On 26 September 2018 at 20:45, Sebastian Reichel <sre@kernel.org> wrote:
> Hi,
>
> On Wed, Sep 26, 2018 at 04:30:39PM +0800, Baolin Wang wrote:
>> Hi Linus,
>>
>> On 26 September 2018 at 16:00, Linus Walleij <linus.walleij@linaro.org> wrote:
>> > On Wed, Sep 26, 2018 at 4:59 AM Baolin Wang <baolin.wang@linaro.org> wrote:
>> >
>> >> Introduce one property to present the battery internal resistance for battery
>> >> information.
>> >>
>> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> >> ---
>> >> Changes from v1:
>> >>  - New patch in v2.
>> >
>> > I'm a bit confused by the physics in this patch.
>> >
>> > The internal resistance of a battery is not a constant in its life cycle,
>> > this varies over the age of the battery, and the reason I thing is
>> > chemical residuals accumulating on the anode and cathode inside
>> > the battery and the energy storage medium aging. (Plus/minus my
>> > ignorance about how batteries actually work.)
>>
>> Yes, you are right. The internal resistance can be affected by
>> temperature or battery age or other factors. But our solution just
>> uses one constant internal resistance to calculate OCV value to look
>> up the capacity table when system boots on, in this case we do not
>> need one more accuracy OCV, since we will calculate the battery
>> capacity in future. So we just introduce one estimation constant
>> internal resistance.
>>
>> >
>> > AFAIK the fact that the internal resistance varies is of high
>> > importance for people developing algorithms of battery capacity
>> > and longevity. Such that some (hardware) capacity monitors go
>> > to great lengths to measure with high precision the current
>> > internal resistance of the battery for their algorithms.
>> >
>> > Sorry for making things more complex, but should it be named
>> > "factory-internal-resistance-micro-ohms" or
>> > "typical-internal-resistance-micro-ohms"?
>>
>> I am fine with this change. If Sebastian also agree with this change,
>> I will fix. Thanks for your reviewing and comments.
>
> Ack.
>
> FWIW for proper battery status you need to collect battery specific
> statistics, that is the reason fuel gauge chip providers recommend to
> combine the chip with the battery cells into a "smart battery".

OK. I will rename it as "factory-internal-resistance-micro-ohms" in
next version. Thanks.

-- 
Baolin Wang
Best Regards