[PATCH v6 1/6] dt-bindings: power: Introduce one property to present the battery internal resistance

[PATCH v6 1/6] dt-bindings: power: Introduce one property to present the battery internal resistance

[Baolin Wang]

The internal resistance of a battery is not a constant in its life cycle,
this varies over the age of the battery or temperature and so on. But we
just want use one constant battery internal resistance to estimate the
battery capacity. Thus this patch introduces one property to present
the battery factory internal resistance for battery information.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
 - Split binding into one separate patch.
 - Add LinusW reviewed tag.

Changes from v2:
 - Rename the property.
 - Improve the commit message.

Changes from v1:
 - New patch in v2.
---
 .../devicetree/bindings/power/supply/battery.txt   |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index f4d3b4a..938d027 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
+ - factory-internal-resistance-micro-ohms: battery factory 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>;
+		factory-internal-resistance-micro-ohms = <250000>;
 	};
 
 	charger: charger@11 {
-- 
1.7.9.5

[PATCH v6 2/6] power: supply: core: Add one field to present the battery internal resistance

[Baolin Wang]

Add one field for 'struct power_supply_battery_info' to present the battery
factory internal resistance.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
 - New patch in v3, which splits code into one separate patch.
 - Add Linusw reviewed tag.
---
 drivers/power/supply/power_supply_core.c |    3 +++
 include/linux/power_supply.h             |    1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index e853618..307e0995 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->factory_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, "factory-internal-resistance-micro-ohms",
+			     &info->factory_internal_resistance_uohm);
 
 	return 0;
 }
diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
index f807691..d089566 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 factory_internal_resistance_uohm;   /* microOhms */
 };
 
 extern struct atomic_notifier_head power_supply_notifier;
-- 
1.7.9.5

[PATCH v6 3/6] dt-bindings: power: Introduce properties to present the battery OCV capacity 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes from v5:
 - None.

Changes from v4:
 - Improve the description of ocv-capacity-table-n to make the order clear.

Changes from v3:
 - Split binding into one separate patch.
 - Rename ocv-capacity-table-temperatures to ocv-capacity-celsius.
 - Add some words to specify the OCV's unit.

Changes from v2:
 - Use type __be32 to calculate the table length.
 - Update error messages.
 - Add some helper functions.

Changes from v1:
 - New patch in v2.
---
 .../devicetree/bindings/power/supply/battery.txt   |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/power/supply/battery.txt b/Documentation/devicetree/bindings/power/supply/battery.txt
index 938d027..89871ab 100644
--- a/Documentation/devicetree/bindings/power/supply/battery.txt
+++ b/Documentation/devicetree/bindings/power/supply/battery.txt
@@ -23,6 +23,17 @@ Optional Properties:
  - constant-charge-current-max-microamp: maximum constant input current
  - constant-charge-voltage-max-microvolt: maximum constant input voltage
  - factory-internal-resistance-micro-ohms: battery factory internal resistance
+ - ocv-capacity-table-0: An array providing the open circuit voltage (OCV)
+   of the battery and corresponding battery capacity percent, which is used
+   to look up battery capacity according to current OCV value. And the open
+   circuit voltage unit is microvolt.
+ - ocv-capacity-table-1: Same as ocv-capacity-table-0
+ ......
+ - ocv-capacity-table-n: Same as ocv-capacity-table-0
+ - ocv-capacity-celsius: 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 +55,10 @@ Example:
 		constant-charge-current-max-microamp = <900000>;
 		constant-charge-voltage-max-microvolt = <4200000>;
 		factory-internal-resistance-micro-ohms = <250000>;
+		ocv-capacity-celsius = <(-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 {
-- 
1.7.9.5

[PATCH v6 4/6] power: supply: core: Add some helpers to use the battery OCV capacity table

[Baolin Wang]

We have introduced some battery properties to present the OCV table
temperatures and OCV capacity table values. Thus this patch add OCV
temperature and OCV table for battery information, as well as providing
some helper functions to use the OCV capacity table for users.

Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
 - Split core modification into one separate patch.
 - Rename ocv-capacity-table-temperatures to ocv-capacity-celsius.

Changes from v2:
 - Use type __be32 to calculate the table length.
 - Update error messages.
 - Add some helper functions.

Changes from v1:
 - New patch in v2.
---
 drivers/power/supply/power_supply_core.c |  123 +++++++++++++++++++++++++++++-
 include/linux/power_supply.h             |   19 +++++
 2 files changed, 141 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
index 307e0995..58c4309 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->factory_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,125 @@ int power_supply_get_battery_info(struct power_supply *psy,
 	of_property_read_u32(battery_np, "factory-internal-resistance-micro-ohms",
 			     &info->factory_internal_resistance_uohm);
 
+	len = of_property_count_u32_elems(battery_np, "ocv-capacity-celsius");
+	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-celsius",
+					   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);
+		if (!list || !size) {
+			dev_err(&psy->dev, "failed to get %s\n", propname);
+			kfree(propname);
+			power_supply_put_battery_info(psy, info);
+			return -EINVAL;
+		}
+
+		kfree(propname);
+		tab_len = size / (2 * sizeof(__be32));
+		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_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
+				int table_len, int ocv)
+{
+	int i, cap, tmp;
+
+	for (i = 0; i < table_len; i++)
+		if (ocv > table[i].ocv)
+			break;
+
+	if (i > 0 && i < table_len) {
+		tmp = (table[i - 1].capacity - table[i].capacity) *
+			(ocv - table[i].ocv);
+		tmp /= table[i - 1].ocv - table[i].ocv;
+		cap = tmp + table[i].capacity;
+	} else if (i == 0) {
+		cap = table[0].capacity;
+	} else {
+		cap = table[table_len - 1].capacity;
+	}
+
+	return cap;
+}
+EXPORT_SYMBOL_GPL(power_supply_ocv2cap_simple);
+
+struct power_supply_battery_ocv_table *
+power_supply_find_ocv2cap_table(struct power_supply_battery_info *info,
+				int temp, int *table_len)
+{
+	int best_temp_diff = INT_MAX, best_index = 0, temp_diff, i;
+
+	if (!info->ocv_table[0])
+		return NULL;
+
+	for (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;
+		}
+	}
+
+	*table_len = info->ocv_table_size[best_index];
+	return info->ocv_table[best_index];
+}
+EXPORT_SYMBOL_GPL(power_supply_find_ocv2cap_table);
+
+int power_supply_batinfo_ocv2cap(struct power_supply_battery_info *info,
+				 int ocv, int temp)
+{
+	struct power_supply_battery_ocv_table *table;
+	int table_len;
+
+	table = power_supply_find_ocv2cap_table(info, temp, &table_len);
+	if (!table)
+		return -EINVAL;
+
+	return power_supply_ocv2cap_simple(table, table_len, ocv);
+}
+EXPORT_SYMBOL_GPL(power_supply_batinfo_ocv2cap);
+
 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 d089566..84fe93f 100644
--- a/include/linux/power_supply.h
+++ b/include/linux/power_supply.h
@@ -309,6 +309,13 @@ 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 +334,9 @@ struct power_supply_battery_info {
 	int constant_charge_current_max_ua; /* microAmps */
 	int constant_charge_voltage_max_uv; /* microVolts */
 	int factory_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 +360,15 @@ 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 int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
+				       int table_len, int ocv);
+extern struct power_supply_battery_ocv_table *
+power_supply_find_ocv2cap_table(struct power_supply_battery_info *info,
+				int temp, int *table_len);
+extern int power_supply_batinfo_ocv2cap(struct power_supply_battery_info *info,
+					int ocv, int temp);
 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

[PATCH v6 5/6] 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>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes from v5:
 - None.

Changes from v4:
 - None.

Changes from v3:
 - Add reviewed tag from Rob.

Changes from v2:
 - Add reviewed tag from Linus.

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..98a2400
--- /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-celsius = <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

[PATCH v6 6/6] 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>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
---
Changes from v5:
 - Save the OCV values in micro volts for OCV capacity table.
 - Use devm_kmemdup() instead of devm_kzalloc() in sc27xx_fgu_hw_init()

Changes from v4:
 - None.

Changes from v3:
 - None.

Changes from v2:
 - Use core helper functions to look up OCV capacity table.
 - Use device_property_read_u32() instead of of_property_read_u32().
 - Add acked tag from Linus.

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 |  656 ++++++++++++++++++++++++++++++
 3 files changed, 664 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..4af677a
--- /dev/null
+++ b/drivers/power/supply/sc27xx_fuel_gauge.c
@@ -0,0 +1,656 @@
+// 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;
+}
+
+/*
+ * 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 * 1000 - oci * data->internal_resist;
+
+	/*
+	 * Parse the capacity table to look up the correct capacity percent
+	 * according to current battery's corresponding OCV values.
+	 */
+	*cap = power_supply_ocv2cap_simple(data->cap_table, data->table_len,
+					   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;
+
+	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.factory_internal_resistance_uohm / 1000;
+
+	/*
+	 * For SC27XX fuel gauge device, we only use one ocv-capacity
+	 * table in normal temperature 20 Celsius.
+	 */
+	table = power_supply_find_ocv2cap_table(&info, 20, &data->table_len);
+	if (!table)
+		return -EINVAL;
+
+	data->cap_table = devm_kmemdup(data->dev, table,
+				       data->table_len * sizeof(*table),
+				       GFP_KERNEL);
+	if (!data->cap_table) {
+		power_supply_put_battery_info(data->battery, &info);
+		return -ENOMEM;
+	}
+
+	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 = device_property_read_u32(&pdev->dev, "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 v6 3/6] dt-bindings: power: Introduce properties to present the battery OCV capacity table

[Rob Herring]

On Mon, 22 Oct 2018 15:43:59 +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>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes from v5:
>  - None.
> 
> Changes from v4:
>  - Improve the description of ocv-capacity-table-n to make the order clear.
> 
> Changes from v3:
>  - Split binding into one separate patch.
>  - Rename ocv-capacity-table-temperatures to ocv-capacity-celsius.
>  - Add some words to specify the OCV's unit.
> 
> Changes from v2:
>  - Use type __be32 to calculate the table length.
>  - Update error messages.
>  - Add some helper functions.
> 
> Changes from v1:
>  - New patch in v2.
> ---
>  .../devicetree/bindings/power/supply/battery.txt   |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v6 1/6] dt-bindings: power: Introduce one property to present the battery internal resistance

[Baolin Wang]

Hi Sebastian,

On 22 October 2018 at 15:43, Baolin Wang <baolin.wang@linaro.org> wrote:
> The internal resistance of a battery is not a constant in its life cycle,
> this varies over the age of the battery or temperature and so on. But we
> just want use one constant battery internal resistance to estimate the
> battery capacity. Thus this patch introduces one property to present
> the battery factory internal resistance for battery information.
>
> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes from v5:
>  - None.
>
> Changes from v4:
>  - None.
>
> Changes from v3:
>  - Split binding into one separate patch.
>  - Add LinusW reviewed tag.
>
> Changes from v2:
>  - Rename the property.
>  - Improve the commit message.
>
> Changes from v1:
>  - New patch in v2.
> ---

I think this v6 patch set have addressed your comments on v5, so could
you apply this patch set if there are no other comments? I hope this
driver can be merged into 4.20 finally. Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v6 1/6] dt-bindings: power: Introduce one property to present the battery internal resistance

[Sebastian Reichel]

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

Hi Baolin,

On Thu, Oct 25, 2018 at 10:01:55AM +0800, Baolin Wang wrote:
> Hi Sebastian,
> 
> On 22 October 2018 at 15:43, Baolin Wang <baolin.wang@linaro.org> wrote:
> > The internal resistance of a battery is not a constant in its life cycle,
> > this varies over the age of the battery or temperature and so on. But we
> > just want use one constant battery internal resistance to estimate the
> > battery capacity. Thus this patch introduces one property to present
> > the battery factory internal resistance for battery information.
> >
> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > ---
> > Changes from v5:
> >  - None.
> >
> > Changes from v4:
> >  - None.
> >
> > Changes from v3:
> >  - Split binding into one separate patch.
> >  - Add LinusW reviewed tag.
> >
> > Changes from v2:
> >  - Rename the property.
> >  - Improve the commit message.
> >
> > Changes from v1:
> >  - New patch in v2.
> > ---
> 
> I think this v6 patch set have addressed your comments on v5, so could
> you apply this patch set if there are no other comments? I hope this
> driver can be merged into 4.20 finally. Thanks.

Everything looks fine to me. I will merge this directly after the
4.20 merge window has been closed and linux-next is open again.
I'm not merging non-trivial, non-bugfix patches this lates during the
release process.

-- Sebastian

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

Re: [PATCH v6 1/6] dt-bindings: power: Introduce one property to present the battery internal resistance

[Baolin Wang]

On 26 October 2018 at 04:13, Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
> Hi Baolin,
>
> On Thu, Oct 25, 2018 at 10:01:55AM +0800, Baolin Wang wrote:
>> Hi Sebastian,
>>
>> On 22 October 2018 at 15:43, Baolin Wang <baolin.wang@linaro.org> wrote:
>> > The internal resistance of a battery is not a constant in its life cycle,
>> > this varies over the age of the battery or temperature and so on. But we
>> > just want use one constant battery internal resistance to estimate the
>> > battery capacity. Thus this patch introduces one property to present
>> > the battery factory internal resistance for battery information.
>> >
>> > Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> > ---
>> > Changes from v5:
>> >  - None.
>> >
>> > Changes from v4:
>> >  - None.
>> >
>> > Changes from v3:
>> >  - Split binding into one separate patch.
>> >  - Add LinusW reviewed tag.
>> >
>> > Changes from v2:
>> >  - Rename the property.
>> >  - Improve the commit message.
>> >
>> > Changes from v1:
>> >  - New patch in v2.
>> > ---
>>
>> I think this v6 patch set have addressed your comments on v5, so could
>> you apply this patch set if there are no other comments? I hope this
>> driver can be merged into 4.20 finally. Thanks.
>
> Everything looks fine to me. I will merge this directly after the
> 4.20 merge window has been closed and linux-next is open again.
> I'm not merging non-trivial, non-bugfix patches this lates during the
> release process.

Understood. Thanks Sebastian.

-- 
Baolin Wang
Best Regards

Re: [PATCH v6 6/6] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Linus Walleij]

On Mon, Oct 22, 2018 at 9:44 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>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Changes from v5:
>  - Save the OCV values in micro volts for OCV capacity table.
>  - Use devm_kmemdup() instead of devm_kzalloc() in sc27xx_fgu_hw_init()

Hi Baolin, you can keep my ACK, just adding some nitpicking:

> +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;

Can  the above really be negative or should these int:s really
be unsigned int?

> +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;
> +}

Would you maybe use
DIV_ROUND_CLOSEST(adc*1000, SC27XX_FGU_1000MV_ADC)
on these?

Overall this is a very fine driver and really pretty compared to some
other stuff we have in drivers/power.

Yours,
Linus Walleij

Re: [PATCH v6 6/6] power: supply: Add Spreadtrum SC27XX fuel gauge unit driver

[Baolin Wang]

On 31 October 2018 at 16:56, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Mon, Oct 22, 2018 at 9:44 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>
>> Acked-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> Changes from v5:
>>  - Save the OCV values in micro volts for OCV capacity table.
>>  - Use devm_kmemdup() instead of devm_kzalloc() in sc27xx_fgu_hw_init()
>
> Hi Baolin, you can keep my ACK, just adding some nitpicking:

Thanks.

>
>> +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;
>
> Can  the above really be negative or should these int:s really
> be unsigned int?

 I think the table_len can be changed to u32, but for others, I'd like
to keep consistency with the power supply battery information。

>
>> +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;
>> +}
>
> Would you maybe use
> DIV_ROUND_CLOSEST(adc*1000, SC27XX_FGU_1000MV_ADC)
> on these?

Good point. Will change in next version. Thanks for your comments.

>
> Overall this is a very fine driver and really pretty compared to some
> other stuff we have in drivers/power.

Thanks.

-- 
Baolin Wang
Best Regards

Re: [PATCH v6 4/6] power: supply: core: Add some helpers to use the battery OCV capacity table

[Baolin Wang]

Hi Quentin,

On 29 October 2018 at 22:48, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> Hi all,
>
> Just chiming in, hopefully I got the message header fine as I don't have
> the original of the mail.
>
>>
>> We have introduced some battery properties to present the OCV table
>> temperatures and OCV capacity table values. Thus this patch add OCV
>> temperature and OCV table for battery information, as well as providing
>> some helper functions to use the OCV capacity table for users.
>>
>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> ---
>> Changes from v5:
>>  - None.
>>
>> Changes from v4:
>>  - None.
>>
>> Changes from v3:
>>  - Split core modification into one separate patch.
>>  - Rename ocv-capacity-table-temperatures to ocv-capacity-celsius.
>>
>> Changes from v2:
>>  - Use type __be32 to calculate the table length.
>>  - Update error messages.
>>  - Add some helper functions.
>>
>> Changes from v1:
>>  - New patch in v2.
>> ---
>>  drivers/power/supply/power_supply_core.c |  123 +++++++++++++++++++++++++++++-
>>  include/linux/power_supply.h             |   19 +++++
>>  2 files changed, 141 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/power_supply_core.c b/drivers/power/supply/power_supply_core.c
>> index 307e0995..58c4309 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;
>
> index can be an unsigned int or even a u8 given the size of
> POWER_SUPPLY_OCV_TEMP_MAX which is the maximum allowed.

Sure.

>
>>
>>       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->factory_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,125 @@ int power_supply_get_battery_info(struct power_supply *psy,
>>       of_property_read_u32(battery_np, "factory-internal-resistance-micro-ohms",
>>                            &info->factory_internal_resistance_uohm);
>>
>> +     len = of_property_count_u32_elems(battery_np, "ocv-capacity-celsius");
>> +     if (len < 0 && len != -EINVAL) {
>
> You may want to separate the case for -EINVAL to display something at
> boot at least.

-EINVAL means we did not set the "ocv-capacity-celsius" property, then
we should not return errors, since the "ocv-capacity-celsius" property
is optional.

>
>> +             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-celsius",
>> +                                        info->ocv_temp, len);
>> +     }
>> +
>> +     for (index = 0; index < len; index++) {
>
> This won't work as intended as we can reach this part of the code with
> len = -EINVAL;

No, the condition (index < len) is false if len = -EINVAL, maybe one
reason keep index as 'int' type.

> If you had a separate condition for len == -EINVAL, you could reset len
> to 0 and bypass this loop for example.
>
>> +             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);
>> +             if (!list || !size) {
>> +                     dev_err(&psy->dev, "failed to get %s\n", propname);
>> +                     kfree(propname);
>> +                     power_supply_put_battery_info(psy, info);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             kfree(propname);
>> +             tab_len = size / (2 * sizeof(__be32));
>> +             info->ocv_table_size[index] = tab_len;
>> +
>> +             table = info->ocv_table[index] =
>> +                     devm_kzalloc(&psy->dev, tab_len * sizeof(*table),
>> +                                  GFP_KERNEL);
>
> If you name `index` `j` or anything short enough, you can remove the
> temp variable table and not worry about the 80char limit.
>
> If I'm not mistaken, you should use devm_kzalloc_array here.

Sure.

>
>> +             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++);
>
> Please check these are valid values.

Um, It is hard to validate the values for OCV and capacity, because
now we do not know each battery's parameters. Any good suggestion?

>
>> +             }
>> +     }
>> +
>>       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);
>> +
>
> Do we really need this? Won't this mess with the reference used by the
> devm_ function?

Yes, I should change to use devm_kfree().

>
> What is the use case for letting a user call this function? I can
> understand you want to delete the arrays if they're invalid in the
> get_function, which could be done thanks to a goto statement or with
> this function if you really want (if it does not mess with refs) but I
> don't see why you want to export this function.

Cause some drivers will copy the OCV tables into their own structures,
one helper function to help to free memory of battery information. In
future, this can be expanded to clean up more resources of battery
information.

>> +int power_supply_ocv2cap_simple(struct power_supply_battery_ocv_table *table,
>> +                             int table_len, int ocv)
>> +{
>> +     int i, cap, tmp;
>> +
>> +     for (i = 0; i < table_len; i++)
>> +             if (ocv > table[i].ocv)
>> +                     break;
>> +
>
> It is NOT stated in the DT binding that you want the array to be ordered
> AND ordered descending. You should fix that either in the DT binding or
> here.

Sure. Will add some function description for this simple helper.

>
>> +     if (i > 0 && i < table_len) {
>> +             tmp = (table[i - 1].capacity - table[i].capacity) *
>> +                     (ocv - table[i].ocv);
>> +             tmp /= table[i - 1].ocv - table[i].ocv;
>> +             cap = tmp + table[i].capacity;
>> +     } else if (i == 0) {
>> +             cap = table[0].capacity;
>> +     } else {
>> +             cap = table[table_len - 1].capacity;
>> +     }
>> +
>> +     return cap;
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_ocv2cap_simple);
>> +
>> +struct power_supply_battery_ocv_table *
>> +power_supply_find_ocv2cap_table(struct power_supply_battery_info *info,
>> +                             int temp, int *table_len)
>> +{
>> +     int best_temp_diff = INT_MAX, best_index = 0, temp_diff, i;
>> +
>
> i and best index can be unsigned (and even a u8).

Sure.

>
>> +     if (!info->ocv_table[0])
>> +             return NULL;
>> +
>> +     for (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;
>> +             }
>> +     }
>> +
>> +     *table_len = info->ocv_table_size[best_index];
>> +     return info->ocv_table[best_index];
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_find_ocv2cap_table);
>> +
>> +int power_supply_batinfo_ocv2cap(struct power_supply_battery_info *info,
>> +                              int ocv, int temp)
>> +{
>> +     struct power_supply_battery_ocv_table *table;
>> +     int table_len;
>> +
>> +     table = power_supply_find_ocv2cap_table(info, temp, &table_len);
>> +     if (!table)
>> +             return -EINVAL;
>> +
>> +     return power_supply_ocv2cap_simple(table, table_len, ocv);
>> +}
>> +EXPORT_SYMBOL_GPL(power_supply_batinfo_ocv2cap);
>> +
>>  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 d089566..84fe93f 100644
>> --- a/include/linux/power_supply.h
>> +++ b/include/linux/power_supply.h
>> @@ -309,6 +309,13 @@ 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
>> +
>
> Why limiting to 20? What if I want something more precise?

We have not so much temperature values until now, so I think 20 can be
covered most cases. If you need more precise temperature values, then
I think we can expand the number.
Hint, the number 20 is for temperature values, each temperature can
have one corresponding OCV table, please see the dt-bindings.
https://lore.kernel.org/patchwork/patch/1002350/

>
> That's it for my review of this patch.
>
> Thanks for the patch, I'm sure many people are interested by this
> feature!

Thanks for your comments.

>
> [...]
>
> I would like to give my specific use case of the OCV on X-Powers PMICs
> so that hopefully we can get things sorted out before it's too late to
> make modifications that might be needed.
>
> I'm adding a few people on Cc that work on the X-Power PMICs so that
> hopefully we can have all the correct pieces of information and go
> towards the right solution.
>
> In the AXP818/AXP288 datasheet[1] (p.57), we have access to OCV values
> and battery RDC register.
>
> The hardware learns from the battery or from the given OCV and RDC
> values and then updates the returned battery capacity accordingly (in
> another register).
> I've 32 values (see the issue with a max of 20 values?) to be written in

I think you misunderstood the 20, it is means we can have max 20 OCV tables now.

> different registers that represent the battery capacity at a given
> voltage. I do not have to do any computation on the kernel side, I just
> have to set the registers with the correct values and the battery
> capacity will be auto-updated by the PMIC. With this patch series,
> should I just call power_supply_get_battery_info, get the OCV tables and

I think so.

> do my thing in the driver? Could we have a more generic way to do it
> (does it make sense to have a generic one?)?

Sorry, maybe I did not follow you here. You said your hardware will
help to get the capacity automatically if you set the register, so
what generic way you want to introduce? Could you elaborate on?

>
> We also definitely need a sysfs entry so that the user can enter the new
> values of the RDC/OCV since it changes during the life cycle of the
> battery IIRC.

Why it changed? Due to different temperatures or other factors? If the
factor is temperature, I think we have supplied one generic way:
https://lore.kernel.org/patchwork/patch/1002350/

>
> I already know of the POWER_SUPPLY_PROP_VOLTAGE_OCV property but it
> doesn't seem to be very appropriate as of today.
>
> Other PMICs from X-Powers have more or less the same mechanism according
> to the BSP even though it's not documented anywhere.
>
> Is everything clear enough? What are your thoughts?
>
> Thanks,
> Quentin
>
> [1] http://linux-sunxi.org/images/7/73/AXP818_datasheet_Revision1.0.pdf



-- 
Baolin Wang
Best Regards

Re: [PATCH v6 4/6] power: supply: core: Add some helpers to use the battery OCV capacity table

[Quentin Schulz]

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

Hi Baolin,

On Thu, Nov 01, 2018 at 03:22:18PM +0800, Baolin Wang wrote:
> Hi Quentin,
> 
> On 29 October 2018 at 22:48, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
[...]
> >
> >> +             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-celsius",
> >> +                                        info->ocv_temp, len);
> >> +     }
> >> +
> >> +     for (index = 0; index < len; index++) {
> >
> > This won't work as intended as we can reach this part of the code with
> > len = -EINVAL;
> 
> No, the condition (index < len) is false if len = -EINVAL, maybe one
> reason keep index as 'int' type.
> 

Ugh. Next time I'll make sure my brain is wired before reviewing a
patch, sorry :)

[...]
> >> +             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++);
> >
> > Please check these are valid values.
> 
> Um, It is hard to validate the values for OCV and capacity, because
> now we do not know each battery's parameters. Any good suggestion?
> 

You know the capacity is between 0 and 100 (since it's an unsigned
value, checking against 100 is enough), that's a good start.

For the OCV, I'd say it's basically between the minimum and maximum
voltage a battery can hold. I don't know enough about batteries to
give the correct bounds unfortunately :(

[...]
> > What is the use case for letting a user call this function? I can
> > understand you want to delete the arrays if they're invalid in the
> > get_function, which could be done thanks to a goto statement or with
> > this function if you really want (if it does not mess with refs) but I
> > don't see why you want to export this function.
> 
> Cause some drivers will copy the OCV tables into their own structures,
> one helper function to help to free memory of battery information. In
> future, this can be expanded to clean up more resources of battery
> information.
> 

Couldn't they only use a pointer to the appropriate table? Or do you
plan on the drivers directly modifying the table but wanting to keep a
clean copy on the core side?

I find it weird to free the tables. I'd suppose that a driver wants to
keep all tables available to be able to chose the appropriate one
depending on the current temperature of the battery (which is what
power_supply_batinfo_ocv2cap is for if I understood correctly) and not
only at definite time (probe function for the driver you attached to the
patch series IIRC). If you need to keep all tables, why would you want
to copy them to the driver and not keep them in the core and use a
pointer to the table in current use?

I'm sure I'm missing something, let me know what it is. Thanks.

[...]
> > I would like to give my specific use case of the OCV on X-Powers PMICs
> > so that hopefully we can get things sorted out before it's too late to
> > make modifications that might be needed.
> >
> > I'm adding a few people on Cc that work on the X-Power PMICs so that
> > hopefully we can have all the correct pieces of information and go
> > towards the right solution.
> >
> > In the AXP818/AXP288 datasheet[1] (p.57), we have access to OCV values
> > and battery RDC register.
> >
> > The hardware learns from the battery or from the given OCV and RDC
> > values and then updates the returned battery capacity accordingly (in
> > another register).
> > I've 32 values (see the issue with a max of 20 values?) to be written in
> 
> I think you misunderstood the 20, it is means we can have max 20 OCV tables now.
> 

Indeed, misread the patch, thanks for clarifying.

> > different registers that represent the battery capacity at a given
> > voltage. I do not have to do any computation on the kernel side, I just
> > have to set the registers with the correct values and the battery
> > capacity will be auto-updated by the PMIC. With this patch series,
> > should I just call power_supply_get_battery_info, get the OCV tables and
> 
> I think so.
> 
> > do my thing in the driver? Could we have a more generic way to do it
> > (does it make sense to have a generic one?)?
> 
> Sorry, maybe I did not follow you here. You said your hardware will
> help to get the capacity automatically if you set the register, so
> what generic way you want to introduce? Could you elaborate on?
> 

I think I got my thoughts tangled-up, I can't honestly find a generic
way right now.

> >
> > We also definitely need a sysfs entry so that the user can enter the new
> > values of the RDC/OCV since it changes during the life cycle of the
> > battery IIRC.
> 
> Why it changed? Due to different temperatures or other factors? If the
> factor is temperature, I think we have supplied one generic way:
> https://lore.kernel.org/patchwork/patch/1002350/
> 

Apparently age is a factor in the OCV curve. I don't know if it's
substantial enough to care about though.

Anyway, I have a very strong bias towards not having to modify the
Device Tree or recompile the kernel when a piece of hardware can be
replaced easily by something different. I see the battery as such a
piece of hardware. I understand the need to define the battery that
comes with a product but I also like to let the users switch the battery
(for a bigger one for example) without getting their hands dirty with
getting the kernel sources, recompiling, etc.

What if the provided OCV curves are not the best ones and the user has
found better ones?

For that, I like to let the user configure parameters that impact the
battery after we've optionaly set the default one.

I'd be mad to have to recompile the Device Tree or kernel when switching
devices on a USB bus for example.

Does that make sense?

Thanks,
Quentin

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

Re: [PATCH v6 4/6] power: supply: core: Add some helpers to use the battery OCV capacity table

[Baolin Wang]

Hi Quentin,

On 1 November 2018 at 21:50, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> Hi Baolin,
>
> On Thu, Nov 01, 2018 at 03:22:18PM +0800, Baolin Wang wrote:
>> Hi Quentin,
>>
>> On 29 October 2018 at 22:48, Quentin Schulz <quentin.schulz@bootlin.com> wrote:
> [...]
>> >
>> >> +             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-celsius",
>> >> +                                        info->ocv_temp, len);
>> >> +     }
>> >> +
>> >> +     for (index = 0; index < len; index++) {
>> >
>> > This won't work as intended as we can reach this part of the code with
>> > len = -EINVAL;
>>
>> No, the condition (index < len) is false if len = -EINVAL, maybe one
>> reason keep index as 'int' type.
>>
>
> Ugh. Next time I'll make sure my brain is wired before reviewing a
> patch, sorry :)

No worries, just make things clear :)

>
> [...]
>> >> +             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++);
>> >
>> > Please check these are valid values.
>>
>> Um, It is hard to validate the values for OCV and capacity, because
>> now we do not know each battery's parameters. Any good suggestion?
>>
>
> You know the capacity is between 0 and 100 (since it's an unsigned
> value, checking against 100 is enough), that's a good start.

Yeah, we can validate the percent values.

>
> For the OCV, I'd say it's basically between the minimum and maximum
> voltage a battery can hold. I don't know enough about batteries to
> give the correct bounds unfortunately :(

But in this function, we may can not know the minimum and maximum
voltage of a battery. Some drivers will set the minimum and maximum
voltage in their drivers. So I would like to move the OCV values
validation to drivers.

>
> [...]
>> > What is the use case for letting a user call this function? I can
>> > understand you want to delete the arrays if they're invalid in the
>> > get_function, which could be done thanks to a goto statement or with
>> > this function if you really want (if it does not mess with refs) but I
>> > don't see why you want to export this function.
>>
>> Cause some drivers will copy the OCV tables into their own structures,
>> one helper function to help to free memory of battery information. In
>> future, this can be expanded to clean up more resources of battery
>> information.
>>
>
> Couldn't they only use a pointer to the appropriate table? Or do you
> plan on the drivers directly modifying the table but wanting to keep a
> clean copy on the core side?
>
> I find it weird to free the tables. I'd suppose that a driver wants to
> keep all tables available to be able to chose the appropriate one
> depending on the current temperature of the battery (which is what
> power_supply_batinfo_ocv2cap is for if I understood correctly) and not
> only at definite time (probe function for the driver you attached to the
> patch series IIRC). If you need to keep all tables, why would you want
> to copy them to the driver and not keep them in the core and use a
> pointer to the table in current use?
>
> I'm sure I'm missing something, let me know what it is. Thanks.

Some drivers won't use all of the battery information in struct
power_supply_battery_info, so they can copy the required fields into
their drivers' data structure instead of holding all fields in struct
power_supply_battery_info, which can save some memory (especially we
introduced temperature tables for struct power_supply_battery_info).
So for this case, we only use the OCV table in struct
power_supply_battery_info, I did not use one pointer to the struct
power_supply_battery_info, just copy the required OCV tables into my
data structure and ignore other fields. So I should free the OCV
tables of battery information. Hope I make things clear here.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/power/supply/axp20x_battery.c#L604
[2] https://elixir.bootlin.com/linux/latest/source/drivers/power/supply/bq24190_charger.c#L1673

>
> [...]
>> > I would like to give my specific use case of the OCV on X-Powers PMICs
>> > so that hopefully we can get things sorted out before it's too late to
>> > make modifications that might be needed.
>> >
>> > I'm adding a few people on Cc that work on the X-Power PMICs so that
>> > hopefully we can have all the correct pieces of information and go
>> > towards the right solution.
>> >
>> > In the AXP818/AXP288 datasheet[1] (p.57), we have access to OCV values
>> > and battery RDC register.
>> >
>> > The hardware learns from the battery or from the given OCV and RDC
>> > values and then updates the returned battery capacity accordingly (in
>> > another register).
>> > I've 32 values (see the issue with a max of 20 values?) to be written in
>>
>> I think you misunderstood the 20, it is means we can have max 20 OCV tables now.
>>
>
> Indeed, misread the patch, thanks for clarifying.
>
>> > different registers that represent the battery capacity at a given
>> > voltage. I do not have to do any computation on the kernel side, I just
>> > have to set the registers with the correct values and the battery
>> > capacity will be auto-updated by the PMIC. With this patch series,
>> > should I just call power_supply_get_battery_info, get the OCV tables and
>>
>> I think so.
>>
>> > do my thing in the driver? Could we have a more generic way to do it
>> > (does it make sense to have a generic one?)?
>>
>> Sorry, maybe I did not follow you here. You said your hardware will
>> help to get the capacity automatically if you set the register, so
>> what generic way you want to introduce? Could you elaborate on?
>>
>
> I think I got my thoughts tangled-up, I can't honestly find a generic
> way right now.

OK.

>
>> >
>> > We also definitely need a sysfs entry so that the user can enter the new
>> > values of the RDC/OCV since it changes during the life cycle of the
>> > battery IIRC.
>>
>> Why it changed? Due to different temperatures or other factors? If the
>> factor is temperature, I think we have supplied one generic way:
>> https://lore.kernel.org/patchwork/patch/1002350/
>>
>
> Apparently age is a factor in the OCV curve. I don't know if it's
> substantial enough to care about though.
>
> Anyway, I have a very strong bias towards not having to modify the
> Device Tree or recompile the kernel when a piece of hardware can be
> replaced easily by something different. I see the battery as such a
> piece of hardware. I understand the need to define the battery that
> comes with a product but I also like to let the users switch the battery
> (for a bigger one for example) without getting their hands dirty with
> getting the kernel sources, recompiling, etc.
>
> What if the provided OCV curves are not the best ones and the user has
> found better ones?
>
> For that, I like to let the user configure parameters that impact the
> battery after we've optionaly set the default one.
>
> I'd be mad to have to recompile the Device Tree or kernel when switching
> devices on a USB bus for example.
>
> Does that make sense?

Yes, understood. But I can not add this feature in my patch series,
since we have no this usage for SC27xx FGU. So I think it will be good
to submit one separate patch, which can let other guys focus on your
case and maybe give a better solution. Thanks for your comments.

-- 
Baolin Wang
Best Regards