[PATCH v4 0/2] power: supply: Add STC3117 Fuel Gauge

[PATCH v4 0/2] power: supply: Add STC3117 Fuel Gauge

[Bhavin Sharma]

From: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>

Adds initial support for the STC3117 fuel gauge.

v3 -> v4

- Added support for current, soc, temp, and status properties.
- Addressed comments and feedback provided by Krzysztof and Sebastian.
 
Link for v3: https://lore.kernel.org/linux-pm/20240205051321.4079933-1-bhavin.sharma@siliconsignals.io/T/#t

v2 -> v3

- Resolved DTC warnings and errors
- Formatted the changelogs
- Added monitored battery properties
- Replaced 'additionalProperties' with 'unevaluatedProperties'
- Replaced '&i2c6' with 'i2c'

Link for v2: https://lore.kernel.org/linux-pm/202401080530.0hMWnrIg-lkp@intel.com/T/#t

v1 -> v2

- String value is redundantly quoted with any quotes (quoted-strings)
- Found character '\t' that cannot start any token

Link for v1: https://lore.kernel.org/linux-pm/46bba29c-330d-417d-ad84-ceb5207fdb55@wanadoo.fr/T/#t

Hardevsinh Palaniya(1):
  dt-bindings: power: supply: Add STC3117 Fuel Gauge

Bhavin Sharma(2):
  power: supply: Add STC3117 fuel gauge unit driver

 .../bindings/power/supply/st,stc3117.yaml     |  53 ++
 MAINTAINERS                                   |   8 +
 drivers/power/supply/Kconfig                  |   7 +
 drivers/power/supply/Makefile                 |   1 +
 drivers/power/supply/stc3117_fuel_gauge.c     | 625 ++++++++++++++++++
 5 files changed, 694 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
 create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c

-- 
2.43.0

[PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Bhavin Sharma]

The STC3117 provides a simple fuel gauge via I2C.
Add a DT schema to describe how to set it up in the device tree.

Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
---
 .../bindings/power/supply/st,stc3117.yaml     | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/st,stc3117.yaml

diff --git a/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml b/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
new file mode 100644
index 000000000000..06e53534ad76
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/power/supply/st,stc3117.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STMicroelectronics STC3117 Fuel Gauge Unit Power Supply
+
+maintainers:
+  - Bhavin Sharma <bhavin.sharma@siliconsignals.io>
+  - Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
+
+description: |
+  The STC3117 includes the STMicroelectronics OptimGauge algorithm.
+  It provides accurate battery state-of-charge (SOC) monitoring, tracks
+  battery parameter changes with operation conditions, temperature,
+  and aging, and allows the application to get a battery state-of-health
+  (SOH) indication.
+
+  An alarm output signals low SOC or low voltage conditions and also
+  indicates fault conditions like a missing or swapped battery.
+
+  Datasheet is available at
+  https://www.st.com/resource/en/datasheet/stc3117.pdf
+
+allOf:
+  - $ref: power-supply.yaml#
+
+properties:
+  compatible:
+    enum:
+      - st,stc3117
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      battery@70 {
+        compatible = "st,stc3117";
+        reg = <0x70>;
+      };
+    };
-- 
2.43.0

[PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver

[Bhavin Sharma]

Adds initial support for the STC3117 fuel gauge.

The driver provides functionality to monitor key parameters including:
- Voltage
- Current
- State of Charge (SOC)
- Temperature
- Status

Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
---
 MAINTAINERS                               |   8 +
 drivers/power/supply/Kconfig              |   7 +
 drivers/power/supply/Makefile             |   1 +
 drivers/power/supply/stc3117_fuel_gauge.c | 625 ++++++++++++++++++++++
 4 files changed, 641 insertions(+)
 create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 82161bc70b51..42c1af29eddb 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21855,6 +21855,14 @@ T:	git git://linuxtv.org/media_tree.git
 F:	Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml
 F:	drivers/media/i2c/st-mipid02.c
 
+ST STC3117 FUEL GAUGE DRIVER
+M:	Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
+M:	Bhavin Sharma <bhavin.sharma@siliconsignals.io>
+L:	linux-pm@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
+F:	drivers/power/supply/stc3117_fuel_gauge.c
+
 ST STM32 FIREWALL
 M:	Gatien Chevallier <gatien.chevallier@foss.st.com>
 S:	Maintained
diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index bcfa63fb9f1e..6ad968fa1f69 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -908,6 +908,13 @@ config FUEL_GAUGE_SC27XX
 	  Say Y here to enable support for fuel gauge with SC27XX
 	  PMIC chips.
 
+config FUEL_GAUGE_STC3117
+	tristate "STMicroelectronics STC3117 fuel gauge driver"
+	depends on I2C
+	help
+	  Say Y here to enable support for fuel gauge with STC3117
+	  PMIC chips.
+
 config CHARGER_UCS1002
 	tristate "Microchip UCS1002 USB Port Power Controller"
 	depends on I2C
diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
index 8dcb41545317..aea3d35f27f3 100644
--- a/drivers/power/supply/Makefile
+++ b/drivers/power/supply/Makefile
@@ -107,6 +107,7 @@ obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
 obj-$(CONFIG_CHARGER_CROS_PCHG)	+= cros_peripheral_charger.o
 obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
 obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
+obj-$(CONFIG_FUEL_GAUGE_STC3117)	+= stc3117_fuel_gauge.o
 obj-$(CONFIG_CHARGER_UCS1002)	+= ucs1002_power.o
 obj-$(CONFIG_CHARGER_BD99954)	+= bd99954-charger.o
 obj-$(CONFIG_CHARGER_WILCO)	+= wilco-charger.o
diff --git a/drivers/power/supply/stc3117_fuel_gauge.c b/drivers/power/supply/stc3117_fuel_gauge.c
new file mode 100644
index 000000000000..99291bb9250f
--- /dev/null
+++ b/drivers/power/supply/stc3117_fuel_gauge.c
@@ -0,0 +-2,622 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * stc3117_fuel_gauge.c - STMicroelectronics STC3117 Fuel Gauge Driver
+ *
+ * Copyright (c) 2024 Silicon Signals Pvt Ltd.
+ * Author:      Bhavin Sharma <bhavin.sharma@siliconsignals.io>
+ *              Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/workqueue.h>
+#include <linux/power_supply.h>
+#include <linux/regmap.h>
+#include <linux/crc8.h>
+
+#define STC3117_ADDR_MODE                       0x00
+#define STC3117_ADDR_CTRL                       0x01
+#define STC3117_ADDR_SOC_L                      0x02
+#define STC3117_ADDR_SOC_H                      0x03
+#define STC3117_ADDR_COUNTER_L                  0x04
+#define STC3117_ADDR_COUNTER_H                  0x05
+#define STC3117_ADDR_CURRENT_L                  0x06
+#define STC3117_ADDR_CURRENT_H                  0x07
+#define STC3117_ADDR_VOLTAGE_L                  0x08
+#define STC3117_ADDR_VOLTAGE_H                  0x09
+#define STC3117_ADDR_TEMPERATURE                0x0A
+#define STC3117_ADDR_AVG_CURRENT_L              0X0B
+#define STC3117_ADDR_AVG_CURRENT_H              0X0C
+#define STC3117_ADDR_OCV_L                      0X0D
+#define STC3117_ADDR_OCV_H                      0X0E
+#define STC3117_ADDR_CC_CNF_L                   0X0F
+#define STC3117_ADDR_CC_CNF_H                   0X10
+#define STC3117_ADDR_VM_CNF_L                   0X11
+#define STC3117_ADDR_VM_CNF_H                   0X12
+#define STC3117_ADDR_ALARM_SOC                  0X13
+#define STC3117_ADDR_ALARM_VOLTAGE              0X14
+#define STC3117_ADDR_ID                         0X18
+#define STC3117_ADDR_CC_ADJ_L			0X1B
+#define STC3117_ADDR_CC_ADJ_H			0X1C
+#define STC3117_ADDR_VM_ADJ_L			0X1D
+#define STC3117_ADDR_VM_ADJ_H			0X1E
+#define STC3117_ADDR_RAM			0x20
+#define STC3117_ADDR_OCV_TABLE			0x30
+#define STC3117_ADDR_SOC_TABLE			0x30
+
+/************ Bit mask definition ************/
+#define STC3117_ID			        0x16
+#define STC3117_MIXED_MODE			0x00
+#define STC3117_VMODE				BIT(0)
+#define STC3117_GG_RUN				BIT(4)
+#define STC3117_CC_MODE				BIT(5)
+#define STC3117_BATFAIL			BIT(3)
+#define STC3117_PORDET				BIT(4)
+
+#define STC3117_RAM_SIZE			16
+#define STC3117_OCV_TABLE_SIZE			16
+#define STC3117_RAM_TESTWORD			0x53A9
+#define STC3117_SOFT_RESET                      0x11
+#define STC3117_NOMINAL_CAPACITY		2600
+
+#define VOLTAGE_LSB_VALUE			9011
+#define CURRENT_LSB_VALUE			24084
+#define RSENSE					10
+#define BATT_CAPACITY				1800
+#define BATTERY_RINT				60
+#define APP_CUTOFF_VOLTAGE			2500
+#define BATT_CHG_VOLTAGE			4250
+#define BATT_MIN_VOLTAGE			3300
+#define MAX_HRSOC				51200
+#define MAX_SOC				1000
+#define CHG_MIN_CURRENT			200
+#define CHG_END_CURRENT			20
+#define APP_MIN_CURRENT			(-5)
+#define BATTERY_FULL				95
+#define CRC8_POLYNOMIAL			0x07
+#define CRC8_INIT				0x00
+#define CRC8_TABLE_SIZE			256
+
+DECLARE_CRC8_TABLE(stc3117_crc_table);
+
+enum stc3117_state {
+	STC3117_INIT,
+	STC3117_RUNNING,
+	STC3117_POWERDN,
+};
+
+enum stc3117_status {
+	BATT_LOWBATT = -2,
+	BATT_DISCHARG,
+	BATT_IDLE,
+	BATT_FULCHARG,
+	BATT_ENDCHARG,
+	BATT_CHARGING,
+};
+
+/* Default OCV curve Li-ion battery */
+static const int OCVValue[16] = {
+    3400, 3582, 3669, 3676, 3699, 3737, 3757, 3774,
+    3804, 3844, 3936, 3984, 4028, 4131, 4246, 4320
+};
+
+static union stc3117_internal_ram {
+	u8 ram_bytes[STC3117_RAM_SIZE];
+	struct {
+	u16 TestWord;   /* 0-1    Bytes */
+	u16 HRSOC;      /* 2-3    Bytes */
+	u16 CC_cnf;     /* 4-5    Bytes */
+	u16 VM_cnf;     /* 6-7    Bytes */
+	u8 soc;         /* 8      Byte  */
+	u8 state;       /* 9      Byte  */
+	u8 unused[5];   /* 10-14  Bytes */
+	u8 CRC;         /* 15     Byte  */
+	} reg;
+} ram_data;
+
+struct stc3117_data {
+	struct i2c_client	*client;
+	struct regmap		*regmap;
+	struct delayed_work update_work;
+	struct power_supply	*battery;
+
+	u8 SOCValue[16];
+	int CC_cnf;
+	int VM_cnf;
+	int CC_adj;
+	int VM_adj;
+	int AvgCurrent;
+	int AvgVoltage;
+	int Current;
+	int Voltage;
+	int Temp;
+	int SOC;
+	int OCV;
+	int HRSOC;
+	int Presence;
+	int Battery_state;
+};
+
+static int STC3117_convert(int value, int factor)
+{
+	value = (value * factor) / 4096;
+	return value;
+}
+
+static int stc3117_get_battery_data(struct stc3117_data *data)
+{
+	u8 reg_list[16];
+	u8 data_adjust[4];
+	int value, mode;
+
+	regmap_bulk_read(data->regmap, STC3117_ADDR_MODE, reg_list, sizeof(reg_list));
+
+	/* SOC */
+	value = (reg_list[3] << 8) + reg_list[2];
+	data->HRSOC = value;
+	data->SOC = (value * 10 + 256) / 512;
+
+	/* cureent */
+	value = (reg_list[7] << 8) + reg_list[6];
+	data->Current = STC3117_convert(value, CURRENT_LSB_VALUE / 10);
+
+	/* Voltage */
+	value = (reg_list[9] << 8) + reg_list[8];
+	data->Voltage = STC3117_convert(value, VOLTAGE_LSB_VALUE);  /* result in mV */
+
+	/* Temp */
+	data->Temp = reg_list[10];
+
+	/* Avg current */
+	value = (reg_list[12] << 8) + reg_list[11];
+	regmap_read(data->regmap, STC3117_ADDR_MODE, &mode);
+	if (!(mode & STC3117_VMODE)) {
+		value = STC3117_convert(value, CURRENT_LSB_VALUE / 10);
+		value = value / 4;
+	} else {
+		value = STC3117_convert(value, 36 * STC3117_NOMINAL_CAPACITY);
+	}
+	data->AvgCurrent = value;  /* result in mA */
+
+	/* OCV */
+	value = (reg_list[14] << 8) + reg_list[13];
+	value = STC3117_convert(value, VOLTAGE_LSB_VALUE);  /* result in mV */
+	value = (value + 2) / 4;
+	data->OCV = value;
+
+	/* CC & VM adjustment counters */
+	regmap_bulk_read(data->regmap, STC3117_ADDR_CC_ADJ_L, data_adjust, sizeof(data_adjust));
+	value = (data_adjust[1] << 8) + data_adjust[0];
+	data->CC_adj = value;
+
+	value = (data_adjust[3] << 8) + data_adjust[2];
+	data->VM_adj = value;
+
+	return 0;
+}
+
+static int ram_write(struct stc3117_data *data)
+{
+	int ret;
+
+	ret = regmap_bulk_write(data->regmap, STC3117_ADDR_RAM, ram_data.ram_bytes, STC3117_RAM_SIZE);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+};
+
+static int stc3117_update_battery_status(struct stc3117_data *data)
+{
+	switch (data->Battery_state) {
+	case BATT_CHARGING:
+		if (data->AvgCurrent < CHG_MIN_CURRENT)
+			data->Battery_state = BATT_ENDCHARG;
+		break;
+
+	case BATT_ENDCHARG:
+		if (data->Current > CHG_MIN_CURRENT)
+			data->Battery_state = BATT_CHARGING;
+		else if (data->AvgCurrent < CHG_END_CURRENT)
+			data->Battery_state = BATT_IDLE;
+		else if ((data->Current > CHG_END_CURRENT) && (data->Voltage > BATT_CHG_VOLTAGE))
+			data->Battery_state = BATT_FULCHARG;
+		break;
+
+	case BATT_FULCHARG:
+		if ((data->Current > CHG_MIN_CURRENT))
+			data->Battery_state = BATT_CHARGING;
+		else if (data->AvgCurrent < CHG_END_CURRENT) {
+			if (data->AvgVoltage > BATT_CHG_VOLTAGE)
+			{
+				regmap_write(data->regmap, STC3117_ADDR_SOC_H, (MAX_HRSOC >> 8 & 0xFF));
+				regmap_write(data->regmap, STC3117_ADDR_SOC_L, (MAX_HRSOC & 0xFF));
+	       			data->SOC = MAX_SOC;
+			}
+			data->Battery_state = BATT_IDLE;
+		}
+		break;
+
+	case BATT_IDLE:
+		if (data->Current > CHG_END_CURRENT)
+			data->Battery_state = BATT_CHARGING;
+		else if (data->Current < APP_MIN_CURRENT)
+			data->Battery_state = BATT_DISCHARG;
+		break;
+
+	case BATT_DISCHARG:
+		if (data->Current > APP_MIN_CURRENT)
+			data->Battery_state = BATT_IDLE;
+		else if (data->AvgVoltage < BATT_MIN_VOLTAGE)
+			data->Battery_state = BATT_LOWBATT;
+		break;
+
+	case BATT_LOWBATT:
+	  	if (data->AvgVoltage > (BATT_MIN_VOLTAGE + 50))
+			data->Battery_state = BATT_IDLE;
+		break;
+
+	default:
+		data->Battery_state = BATT_IDLE;
+	}
+
+	return 0;
+}
+
+static int ram_read(struct stc3117_data *data)
+{
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, STC3117_ADDR_RAM, ram_data.ram_bytes, STC3117_RAM_SIZE);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+};
+
+static int stc3117_set_para(struct stc3117_data *data)
+{
+	int ret;
+
+	ret = regmap_write(data->regmap, STC3117_ADDR_MODE, STC3117_VMODE);
+
+	for (int i = 0; i < STC3117_OCV_TABLE_SIZE; i++)
+		regmap_write(data->regmap, STC3117_ADDR_OCV_TABLE + i, OCVValue[i] * 100 / 55);
+
+	if (data->SOCValue[1] != 0)
+		regmap_bulk_write(data->regmap, STC3117_ADDR_SOC_TABLE, data->SOCValue, STC3117_OCV_TABLE_SIZE);
+	ret |= regmap_write(data->regmap, STC3117_ADDR_CC_CNF_H,
+						(ram_data.reg.CC_cnf >> 8) & 0xFF);
+	ret |= regmap_write(data->regmap, STC3117_ADDR_CC_CNF_L,
+						ram_data.reg.CC_cnf & 0xFF);
+	ret |= regmap_write(data->regmap, STC3117_ADDR_VM_CNF_H,
+						(ram_data.reg.VM_cnf >> 8) & 0xFF);
+	ret |= regmap_write(data->regmap, STC3117_ADDR_VM_CNF_L,
+						ram_data.reg.VM_cnf & 0xFF);
+	ret |= regmap_write(data->regmap, STC3117_ADDR_CTRL, 0x03);
+
+	ret |= regmap_write(data->regmap, STC3117_ADDR_MODE, STC3117_MIXED_MODE | STC3117_GG_RUN);
+
+	return ret;
+};
+
+static int stc3117_init(struct stc3117_data *data)
+{
+	int ID, ret;
+	int ctrl;
+	int ocv_m, ocv_l;
+
+	regmap_read(data->regmap, STC3117_ADDR_ID, &ID);
+	if (ID != STC3117_ID)
+		return -EINVAL;
+
+	data->CC_cnf = (BATT_CAPACITY * RSENSE * 250 + 6194) / 12389;
+	data->VM_cnf = (BATT_CAPACITY * 200 * 50 + 24444) / 48889;
+
+	/* Battery has not been removed */
+
+	data->Presence = 1;
+
+	/* Read RAM data */
+	ret = ram_read(data);
+	if (ret)
+		return -EINVAL;
+
+	if ((ram_data.reg.TestWord != STC3117_RAM_TESTWORD) ||
+                (crc8(stc3117_crc_table, ram_data.ram_bytes, STC3117_RAM_SIZE, CRC8_INIT)) != 0)
+	{
+		ram_data.reg.TestWord = STC3117_RAM_TESTWORD;
+		ram_data.reg.CC_cnf = data->CC_cnf;
+		ram_data.reg.VM_cnf = data->VM_cnf;
+		ram_data.reg.CRC = crc8(stc3117_crc_table, ram_data.ram_bytes,
+                	                        STC3117_RAM_SIZE - 1, CRC8_INIT);
+
+		ret = regmap_read(data->regmap, STC3117_ADDR_OCV_H, &ocv_m);
+
+		ret |= regmap_read(data->regmap, STC3117_ADDR_OCV_L, &ocv_l);
+
+		ret |= stc3117_set_para(data);
+
+		ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_H, ocv_m);
+
+		ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_L, ocv_l);
+		if (ret)
+			return -EINVAL;
+	} else {
+		ret = regmap_read(data->regmap, STC3117_ADDR_CTRL, &ctrl);
+		if ((ctrl & STC3117_BATFAIL) != 0  || (ctrl & STC3117_PORDET) != 0)
+		{
+			ret = regmap_read(data->regmap, STC3117_ADDR_OCV_H, &ocv_m);
+
+			ret |= regmap_read(data->regmap, STC3117_ADDR_OCV_L, &ocv_l);
+
+			ret |= stc3117_set_para(data);
+
+			ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_H, ocv_m);
+
+			ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_L, ocv_l);
+			if (ret)
+				return -EINVAL;
+		} else {
+			ret = stc3117_set_para(data);
+			if (ret)
+				return -EINVAL;
+			regmap_write(data->regmap, STC3117_ADDR_SOC_H, (ram_data.reg.HRSOC >> 8 & 0xFF));
+			regmap_write(data->regmap, STC3117_ADDR_SOC_L, (ram_data.reg.HRSOC & 0xFF));
+		}
+	}
+
+	ram_data.reg.state = STC3117_INIT;
+	ram_data.reg.CRC = crc8(stc3117_crc_table, ram_data.ram_bytes,
+						STC3117_RAM_SIZE - 1, CRC8_INIT);
+	ret = ram_write(data);
+	if (ret)
+		return -EINVAL;
+
+	data->Battery_state = BATT_IDLE;
+
+	return 0;
+};
+
+static int stc3117_task(struct stc3117_data *data)
+{
+	int ID, mode, ret;
+	int count_l, count_m;
+	int ocv_l, ocv_m;
+
+	regmap_read(data->regmap, STC3117_ADDR_ID, &ID);
+	if (ID != STC3117_ID) {
+		data->Presence = 0;
+		return -EINVAL;
+	}
+
+	stc3117_get_battery_data(data);
+
+	/* Read RAM data */
+	ret = ram_read(data);
+	if (ret)
+		return -EINVAL;
+
+	if ((ram_data.reg.TestWord != STC3117_RAM_TESTWORD) ||
+			(crc8(stc3117_crc_table, ram_data.ram_bytes, STC3117_RAM_SIZE, CRC8_INIT) != 0))
+	{
+		ram_data.reg.TestWord = STC3117_RAM_TESTWORD; 
+		ram_data.reg.CC_cnf = data->CC_cnf;
+		ram_data.reg.VM_cnf = data->VM_cnf;
+		ram_data.reg.CRC = crc8(stc3117_crc_table, ram_data.ram_bytes,
+                                STC3117_RAM_SIZE - 1, CRC8_INIT);
+		ram_data.reg.state = STC3117_INIT;
+	}
+
+	/* check battery presence status */
+	regmap_read(data->regmap, STC3117_ADDR_CTRL, &mode);
+	if ((mode & STC3117_BATFAIL) != 0)
+	{
+		data->Presence = 0;
+		ram_data.reg.TestWord = 0;
+		ram_data.reg.state = STC3117_INIT;
+		ret = ram_write(data);
+		if (ret)
+			return -EINVAL;
+		regmap_write(data->regmap, STC3117_ADDR_CTRL, STC3117_PORDET);
+	}
+
+	data->Presence = 1;
+
+	 regmap_read(data->regmap, STC3117_ADDR_MODE, &mode);
+	if ((mode & STC3117_GG_RUN) == 0)
+	{
+		if (ram_data.reg.state > STC3117_INIT) {
+			ret = stc3117_set_para(data);
+			if (ret)
+				return -EINVAL;
+			regmap_write(data->regmap, STC3117_ADDR_SOC_L, (ram_data.reg.HRSOC & 0xFF));
+			regmap_write(data->regmap, STC3117_ADDR_SOC_H, (ram_data.reg.HRSOC >> 8 & 0xFF));
+		} else {
+			ret = regmap_read(data->regmap, STC3117_ADDR_OCV_H, &ocv_m);
+
+			ret |= regmap_read(data->regmap, STC3117_ADDR_OCV_L, &ocv_l);
+
+			ret |= stc3117_set_para(data);
+
+			ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_H, ocv_m);
+
+			ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_L, ocv_l);
+			if (ret)
+				return -EINVAL;
+		}
+		ram_data.reg.state = STC3117_INIT;
+	}
+
+	regmap_read(data->regmap, STC3117_ADDR_COUNTER_L, &count_l);
+	regmap_read(data->regmap, STC3117_ADDR_COUNTER_H, &count_m);
+
+	count_m = (count_m << 8) + count_l;
+
+	/* INIT state, wait for current & temperature value available: */
+	if (ram_data.reg.state == STC3117_INIT && count_m > 4) {
+		data->AvgVoltage = data->Voltage;
+		data->AvgCurrent = data->Current;
+		ram_data.reg.state = STC3117_RUNNING;
+	}
+
+	if (ram_data.reg.state != STC3117_RUNNING)
+	{
+    		data->Current = 0;
+        	data->Temp = 250;
+	} else {
+		if (data->Voltage < APP_CUTOFF_VOLTAGE)
+			data->SOC = 0;
+
+		if (mode & STC3117_VMODE) {
+			data->AvgCurrent = 0;
+			data->Current = 0;
+		} else {
+		       stc3117_update_battery_status(data);
+		}
+	}
+
+	ram_data.reg.HRSOC = data->HRSOC;
+	ram_data.reg.soc = (data->SOC + 5) / 10;
+	ram_data.reg.CRC = crc8(stc3117_crc_table, ram_data.ram_bytes,
+        	        STC3117_RAM_SIZE - 1, CRC8_INIT);
+
+	ret = ram_write(data);
+	if (ret)
+		return -EINVAL;
+	return 0;
+};
+
+static void fuel_gauge_update_work(struct work_struct *work)
+{
+	struct stc3117_data *data = container_of(to_delayed_work(work), struct stc3117_data, update_work);
+
+	stc3117_task(data);
+
+	/* Schedule the work to run again in 2 seconds */
+	schedule_delayed_work(&data->update_work, msecs_to_jiffies(2000));
+}
+
+static int stc3117_get_property(struct power_supply *psy,
+				enum power_supply_property psp, union power_supply_propval *val)
+{
+	struct stc3117_data *data = power_supply_get_drvdata(psy);
+
+	switch (psp) {
+	case POWER_SUPPLY_PROP_STATUS:
+		if (data->SOC > BATTERY_FULL)
+			val->intval = POWER_SUPPLY_STATUS_FULL;
+		if (data->Current < 0)
+			val->intval = POWER_SUPPLY_STATUS_CHARGING;
+		else if (data->Current > 0)
+			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
+		else
+			val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
+		break;
+	case POWER_SUPPLY_PROP_VOLTAGE_NOW:
+		val->intval = data->Voltage;
+		break;
+	case POWER_SUPPLY_PROP_CURRENT_NOW:
+		val->intval = data->Current;
+		break;
+	case POWER_SUPPLY_PROP_CAPACITY:
+		val->intval = data->SOC;
+		break;
+	case POWER_SUPPLY_PROP_TEMP:
+		val->intval = data->Temp;
+		break;
+	case POWER_SUPPLY_PROP_PRESENT:
+		val->intval = data->Presence;
+		break;
+	default:
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static enum power_supply_property stc3117_battery_props[] = {
+	POWER_SUPPLY_PROP_STATUS,
+	POWER_SUPPLY_PROP_VOLTAGE_NOW,
+	POWER_SUPPLY_PROP_CURRENT_NOW,
+	POWER_SUPPLY_PROP_CAPACITY,
+	POWER_SUPPLY_PROP_TEMP,
+	POWER_SUPPLY_PROP_PRESENT,
+};
+
+static const struct power_supply_desc stc3117_battery_desc = {
+	.name = "stc3117-battery",
+	.type = POWER_SUPPLY_TYPE_BATTERY,
+	.get_property = stc3117_get_property,
+	.properties = stc3117_battery_props,
+	.num_properties = ARRAY_SIZE(stc3117_battery_props),
+};
+
+static const struct regmap_config stc3117_regmap_config = {
+	.reg_bits       = 8,
+	.val_bits       = 8,
+};
+
+static int stc3117_probe(struct i2c_client *client)
+{
+	struct stc3117_data *data;
+	struct power_supply_config psy_cfg = {};
+	int ret;
+
+	data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	data->client = client;
+	data->regmap = devm_regmap_init_i2c(client, &stc3117_regmap_config);
+	if (IS_ERR(data->regmap))
+		return PTR_ERR(data->regmap);
+
+	i2c_set_clientdata(client, data);
+	psy_cfg.drv_data = data;
+
+	crc8_populate_msb(stc3117_crc_table, CRC8_POLYNOMIAL);
+
+	ret = stc3117_init(data);
+	if (ret)
+		dev_err_probe(&client->dev, ret, "failed to initialization of stc3117\n");
+
+	INIT_DELAYED_WORK(&data->update_work, fuel_gauge_update_work);
+
+	schedule_delayed_work(&data->update_work, 0);
+
+	data->battery = devm_power_supply_register(&client->dev,
+						   &stc3117_battery_desc, &psy_cfg);
+	if (IS_ERR(data->battery))
+		dev_err_probe(&client->dev, PTR_ERR(data->battery), "failed to register battery\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id stc3117_id[] = {
+	{"stc3117", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, stc3117_id);
+
+static const struct of_device_id stc3117_of_match[] = {
+	{ .compatible = "st,stc3117" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, stc3117_of_match);
+
+static struct i2c_driver stc3117_i2c_driver = {
+	.driver = {
+		.name = "stc3117_i2c_driver",
+		.of_match_table = stc3117_of_match,
+	},
+	.probe = stc3117_probe,
+	.id_table = stc3117_id,
+};
+
+module_i2c_driver(stc3117_i2c_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Bhavin Sharma <bhavin.sharma@siliconsignals.io>");
+MODULE_AUTHOR("Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>");
+MODULE_DESCRIPTION("STC3117 Fuel Gauge Driver");
-- 
2.43.0

Re: [PATCH v4 0/2] power: supply: Add STC3117 Fuel Gauge

[Krzysztof Kozlowski]

On 27/11/2024 16:19, Bhavin Sharma wrote:
> From: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> 
> Adds initial support for the STC3117 fuel gauge.
> 
> v3 -> v4
> 
> - Added support for current, soc, temp, and status properties.
> - Addressed comments and feedback provided by Krzysztof and Sebastian.

Which comments? What changed exactly? This has to be precise.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Krzysztof Kozlowski]

On 27/11/2024 16:19, Bhavin Sharma wrote:
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,stc3117
> +
> +  reg:
> +    maxItems: 1

I asked you some questions on v2, then on v3 and no responses.

You implemented some changes but still did not answer my question. I am
not going to ask again, obviously expecting different result on the same
makes little sense.

No ack from me.

Best regards,
Krzysztof

Re: [PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver

[Krzysztof Kozlowski]

On 27/11/2024 16:19, Bhavin Sharma wrote:
> Adds initial support for the STC3117 fuel gauge.
> 
> The driver provides functionality to monitor key parameters including:
> - Voltage
> - Current
> - State of Charge (SOC)
> - Temperature
> - Status
> 
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> ---
>  MAINTAINERS                               |   8 +
>  drivers/power/supply/Kconfig              |   7 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/stc3117_fuel_gauge.c | 625 ++++++++++++++++++++++
>  4 files changed, 641 insertions(+)
>  create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 82161bc70b51..42c1af29eddb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21855,6 +21855,14 @@ T:	git git://linuxtv.org/media_tree.git
>  F:	Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml
>  F:	drivers/media/i2c/st-mipid02.c
>  
> +ST STC3117 FUEL GAUGE DRIVER
> +M:	Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> +M:	Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> +L:	linux-pm@vger.kernel.org
> +S:	Maintained
> +F:	Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
> +F:	drivers/power/supply/stc3117_fuel_gauge.c
> +
>  ST STM32 FIREWALL
>  M:	Gatien Chevallier <gatien.chevallier@foss.st.com>
>  S:	Maintained
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index bcfa63fb9f1e..6ad968fa1f69 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -908,6 +908,13 @@ config FUEL_GAUGE_SC27XX
>  	  Say Y here to enable support for fuel gauge with SC27XX
>  	  PMIC chips.
>  
> +config FUEL_GAUGE_STC3117
> +	tristate "STMicroelectronics STC3117 fuel gauge driver"
> +	depends on I2C
> +	help
> +	  Say Y here to enable support for fuel gauge with STC3117
> +	  PMIC chips.
> +
>  config CHARGER_UCS1002
>  	tristate "Microchip UCS1002 USB Port Power Controller"
>  	depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 8dcb41545317..aea3d35f27f3 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_CHARGER_CROS_USBPD)	+= cros_usbpd-charger.o
>  obj-$(CONFIG_CHARGER_CROS_PCHG)	+= cros_peripheral_charger.o
>  obj-$(CONFIG_CHARGER_SC2731)	+= sc2731_charger.o
>  obj-$(CONFIG_FUEL_GAUGE_SC27XX)	+= sc27xx_fuel_gauge.o
> +obj-$(CONFIG_FUEL_GAUGE_STC3117)	+= stc3117_fuel_gauge.o
>  obj-$(CONFIG_CHARGER_UCS1002)	+= ucs1002_power.o
>  obj-$(CONFIG_CHARGER_BD99954)	+= bd99954-charger.o
>  obj-$(CONFIG_CHARGER_WILCO)	+= wilco-charger.o
> diff --git a/drivers/power/supply/stc3117_fuel_gauge.c b/drivers/power/supply/stc3117_fuel_gauge.c
> new file mode 100644
> index 000000000000..99291bb9250f
> --- /dev/null
> +++ b/drivers/power/supply/stc3117_fuel_gauge.c
> @@ -0,0 +-2,622 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * stc3117_fuel_gauge.c - STMicroelectronics STC3117 Fuel Gauge Driver
> + *
> + * Copyright (c) 2024 Silicon Signals Pvt Ltd.
> + * Author:      Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> + *              Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/workqueue.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/crc8.h>
> +
> +#define STC3117_ADDR_MODE                       0x00
> +#define STC3117_ADDR_CTRL                       0x01
> +#define STC3117_ADDR_SOC_L                      0x02
> +#define STC3117_ADDR_SOC_H                      0x03
> +#define STC3117_ADDR_COUNTER_L                  0x04
> +#define STC3117_ADDR_COUNTER_H                  0x05
> +#define STC3117_ADDR_CURRENT_L                  0x06
> +#define STC3117_ADDR_CURRENT_H                  0x07
> +#define STC3117_ADDR_VOLTAGE_L                  0x08
> +#define STC3117_ADDR_VOLTAGE_H                  0x09
> +#define STC3117_ADDR_TEMPERATURE                0x0A
> +#define STC3117_ADDR_AVG_CURRENT_L              0X0B
> +#define STC3117_ADDR_AVG_CURRENT_H              0X0C
> +#define STC3117_ADDR_OCV_L                      0X0D
> +#define STC3117_ADDR_OCV_H                      0X0E
> +#define STC3117_ADDR_CC_CNF_L                   0X0F
> +#define STC3117_ADDR_CC_CNF_H                   0X10
> +#define STC3117_ADDR_VM_CNF_L                   0X11
> +#define STC3117_ADDR_VM_CNF_H                   0X12
> +#define STC3117_ADDR_ALARM_SOC                  0X13
> +#define STC3117_ADDR_ALARM_VOLTAGE              0X14
> +#define STC3117_ADDR_ID                         0X18
> +#define STC3117_ADDR_CC_ADJ_L			0X1B
> +#define STC3117_ADDR_CC_ADJ_H			0X1C
> +#define STC3117_ADDR_VM_ADJ_L			0X1D
> +#define STC3117_ADDR_VM_ADJ_H			0X1E
> +#define STC3117_ADDR_RAM			0x20
> +#define STC3117_ADDR_OCV_TABLE			0x30
> +#define STC3117_ADDR_SOC_TABLE			0x30
> +
> +/************ Bit mask definition ************/
> +#define STC3117_ID			        0x16
> +#define STC3117_MIXED_MODE			0x00
> +#define STC3117_VMODE				BIT(0)
> +#define STC3117_GG_RUN				BIT(4)
> +#define STC3117_CC_MODE				BIT(5)
> +#define STC3117_BATFAIL			BIT(3)
> +#define STC3117_PORDET				BIT(4)
> +
> +#define STC3117_RAM_SIZE			16
> +#define STC3117_OCV_TABLE_SIZE			16
> +#define STC3117_RAM_TESTWORD			0x53A9
> +#define STC3117_SOFT_RESET                      0x11
> +#define STC3117_NOMINAL_CAPACITY		2600
> +
> +#define VOLTAGE_LSB_VALUE			9011
> +#define CURRENT_LSB_VALUE			24084
> +#define RSENSE					10
> +#define BATT_CAPACITY				1800
> +#define BATTERY_RINT				60
> +#define APP_CUTOFF_VOLTAGE			2500
> +#define BATT_CHG_VOLTAGE			4250
> +#define BATT_MIN_VOLTAGE			3300
> +#define MAX_HRSOC				51200
> +#define MAX_SOC				1000
> +#define CHG_MIN_CURRENT			200
> +#define CHG_END_CURRENT			20
> +#define APP_MIN_CURRENT			(-5)
> +#define BATTERY_FULL				95
> +#define CRC8_POLYNOMIAL			0x07
> +#define CRC8_INIT				0x00
> +#define CRC8_TABLE_SIZE			256
> +
> +DECLARE_CRC8_TABLE(stc3117_crc_table);
> +
> +enum stc3117_state {
> +	STC3117_INIT,
> +	STC3117_RUNNING,
> +	STC3117_POWERDN,
> +};
> +
> +enum stc3117_status {
> +	BATT_LOWBATT = -2,
> +	BATT_DISCHARG,
> +	BATT_IDLE,
> +	BATT_FULCHARG,
> +	BATT_ENDCHARG,
> +	BATT_CHARGING,
> +};
> +
> +/* Default OCV curve Li-ion battery */
> +static const int OCVValue[16] = {
> +    3400, 3582, 3669, 3676, 3699, 3737, 3757, 3774,
> +    3804, 3844, 3936, 3984, 4028, 4131, 4246, 4320
> +};
> +
> +static union stc3117_internal_ram {
> +	u8 ram_bytes[STC3117_RAM_SIZE];
> +	struct {
> +	u16 TestWord;   /* 0-1    Bytes */
> +	u16 HRSOC;      /* 2-3    Bytes */
> +	u16 CC_cnf;     /* 4-5    Bytes */
> +	u16 VM_cnf;     /* 6-7    Bytes */
> +	u8 soc;         /* 8      Byte  */
> +	u8 state;       /* 9      Byte  */
> +	u8 unused[5];   /* 10-14  Bytes */
> +	u8 CRC;         /* 15     Byte  */
> +	} reg;
> +} ram_data;
> +
> +struct stc3117_data {
> +	struct i2c_client	*client;
> +	struct regmap		*regmap;
> +	struct delayed_work update_work;
> +	struct power_supply	*battery;
> +
> +	u8 SOCValue[16];
> +	int CC_cnf;
> +	int VM_cnf;
> +	int CC_adj;
> +	int VM_adj;
> +	int AvgCurrent;
> +	int AvgVoltage;
> +	int Current;
> +	int Voltage;
> +	int Temp;
> +	int SOC;
> +	int OCV;
> +	int HRSOC;
> +	int Presence;
> +	int Battery_state;


That's some Windows coding style... You need to clean up everything here
to match Linux Coding style.


> +};
> +




> +	i2c_set_clientdata(client, data);
> +	psy_cfg.drv_data = data;
> +
> +	crc8_populate_msb(stc3117_crc_table, CRC8_POLYNOMIAL);
> +
> +	ret = stc3117_init(data);
> +	if (ret)
> +		dev_err_probe(&client->dev, ret, "failed to initialization of stc3117\n");
> +
> +	INIT_DELAYED_WORK(&data->update_work, fuel_gauge_update_work);
> +
> +	schedule_delayed_work(&data->update_work, 0);
> +
> +	data->battery = devm_power_supply_register(&client->dev,
> +						   &stc3117_battery_desc, &psy_cfg);
> +	if (IS_ERR(data->battery))
> +		dev_err_probe(&client->dev, PTR_ERR(data->battery), "failed to register battery\n");
> +
You ignored (again!) received comments. In multiple places. Go back to
previous email and carefully read commetns.

One more thing:

Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).


Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Krzysztof Kozlowski]

On 27/11/2024 19:22, Krzysztof Kozlowski wrote:
> On 27/11/2024 16:19, Bhavin Sharma wrote:
>> +
>> +allOf:
>> +  - $ref: power-supply.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - st,stc3117
>> +
>> +  reg:
>> +    maxItems: 1
> 
> I asked you some questions on v2, then on v3 and no responses.
> 
> You implemented some changes but still did not answer my question. I am
> not going to ask again, obviously expecting different result on the same
> makes little sense.
> 
> No ack from me.
> 

You responded privately - I am not going to do any talks under NDA. I
also do not provide some sort of personal support service. Keep *ALL*
discussions public.

Explaining what you asked:

Some of these are from monitored-battery. Sense resistor should be
separate property. But different question is about missing resources,
like supplies (VCC) and interrupts. Just look at datasheet.

Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Bhavin Sharma]

Hi Krzysztof,
 
Thank you for your review.
 
> On 27/11/2024 16:19, Bhavin Sharma wrote:
> > +
> > +allOf:
> > +  - $ref: power-supply.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - st,stc3117
> > +
> > +  reg:
> > +    maxItems: 1
>
> I asked you some questions on v2, then on v3 and no responses.
 
I sincerely apologize for not addressing your questions in versions 2 and 3 
of the patch.
 
Regarding the battery configuration, we need the following information:
- Battery capacity
- Battery impedance
- OCV curve
- Minimum and maximum voltage
- Current sense register
 
Currently, I have set the battery capacity and impedance directly in the 
driver as default values. However, I understand these should ideally be 
defined in the device tree source (DTS). I will update the DTS accordingly
to include these parameters.
 
Additionally, since the OCV curve, minimum/maximum voltage, and sense
register values are fixed, I would like your opinion on whether these should
also be defined in the DTS or if it is acceptable to keep them in the driver itself.
 
Once again, I apologize for the oversight and thank you for your understanding.
 
Best regards,
Bhavin
________________________________________
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Wednesday, November 27, 2024 11:52 PM
To: Bhavin Sharma <bhavin.sharma@siliconsignals.io>; sre@kernel.org <sre@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>
Cc: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; linux-pm@vger.kernel.org <linux-pm@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge
 
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On 27/11/2024 16:19, Bhavin Sharma wrote:
> +
> +allOf:
> +  - $ref: power-supply.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - st,stc3117
> +
> +  reg:
> +    maxItems: 1

I asked you some questions on v2, then on v3 and no responses.

You implemented some changes but still did not answer my question. I am
not going to ask again, obviously expecting different result on the same
makes little sense.

No ack from me.

Best regards,
Krzysztof

Re: [PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver

[Bhavin Sharma]

Hi Krzysztof,
 
Thank you for your review and feedback.
 
> > +struct stc3117_data {
> > +     struct i2c_client       *client;
> > +     struct regmap           *regmap;
> > +     struct delayed_work update_work;
> > +     struct power_supply     *battery;
> > +
> > +     u8 SOCValue[16];
> > +     int CC_cnf;
> > +     int VM_cnf;
> > +     int CC_adj;
> > +     int VM_adj;
> > +     int AvgCurrent;
> > +     int AvgVoltage;
> > +     int Current;
> > +     int Voltage;
> > +     int Temp;
> > +     int SOC;
> > +     int OCV;
> > +     int HRSOC;
> > +     int Presence;
> > +     int Battery_state;
>
> That's some Windows coding style... You need to clean up everything here
> to match Linux Coding style.
 
Could you clarify what specific changes are required here to align with the Linux
coding style?
 
I am not sure what exactly needs to be changed here.
 
> > +     data->battery = devm_power_supply_register(&client->dev,
> > +                                                &stc3117_battery_desc, &psy_cfg);
> > +     if (IS_ERR(data->battery))
> > +             dev_err_probe(&client->dev, PTR_ERR(data->battery), "failed to register battery\n");
> > +
> You ignored (again!) received comments. In multiple places. Go back to
> previous email and carefully read commetns.
 
Sebastian suggested using dev_err_probe, while you mentioned using dev_err.
so what should i follow ?
 
> One more thing:
> 
> Please wrap code according to coding style (checkpatch is not a coding
> style description, but only a tool).
 
Could you recommend an example driver from the kernel source tree that 
follows the expected coding style? This would help me ensure compliance.
 
Best Regards,
Bhavin
 







________________________________________
From: Krzysztof Kozlowski <krzk@kernel.org>
Sent: Wednesday, November 27, 2024 11:54 PM
To: Bhavin Sharma <bhavin.sharma@siliconsignals.io>; sre@kernel.org <sre@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>
Cc: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; linux-pm@vger.kernel.org <linux-pm@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver
 
CAUTION: This email originated from outside the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

On 27/11/2024 16:19, Bhavin Sharma wrote:
> Adds initial support for the STC3117 fuel gauge.
>
> The driver provides functionality to monitor key parameters including:
> - Voltage
> - Current
> - State of Charge (SOC)
> - Temperature
> - Status
>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> ---
>  MAINTAINERS                               |   8 +
>  drivers/power/supply/Kconfig              |   7 +
>  drivers/power/supply/Makefile             |   1 +
>  drivers/power/supply/stc3117_fuel_gauge.c | 625 ++++++++++++++++++++++
>  4 files changed, 641 insertions(+)
>  create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 82161bc70b51..42c1af29eddb 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21855,6 +21855,14 @@ T:   git git://linuxtv.org/media_tree.git
>  F:   Documentation/devicetree/bindings/media/i2c/st,st-mipid02.yaml
>  F:   drivers/media/i2c/st-mipid02.c
>
> +ST STC3117 FUEL GAUGE DRIVER
> +M:   Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>
> +M:   Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> +L:   linux-pm@vger.kernel.org
> +S:   Maintained
> +F:   Documentation/devicetree/bindings/power/supply/st,stc3117.yaml
> +F:   drivers/power/supply/stc3117_fuel_gauge.c
> +
>  ST STM32 FIREWALL
>  M:   Gatien Chevallier <gatien.chevallier@foss.st.com>
>  S:   Maintained
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index bcfa63fb9f1e..6ad968fa1f69 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -908,6 +908,13 @@ config FUEL_GAUGE_SC27XX
>         Say Y here to enable support for fuel gauge with SC27XX
>         PMIC chips.
>
> +config FUEL_GAUGE_STC3117
> +     tristate "STMicroelectronics STC3117 fuel gauge driver"
> +     depends on I2C
> +     help
> +       Say Y here to enable support for fuel gauge with STC3117
> +       PMIC chips.
> +
>  config CHARGER_UCS1002
>       tristate "Microchip UCS1002 USB Port Power Controller"
>       depends on I2C
> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile
> index 8dcb41545317..aea3d35f27f3 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -107,6 +107,7 @@ obj-$(CONFIG_CHARGER_CROS_USBPD)  += cros_usbpd-charger.o
>  obj-$(CONFIG_CHARGER_CROS_PCHG)      += cros_peripheral_charger.o
>  obj-$(CONFIG_CHARGER_SC2731) += sc2731_charger.o
>  obj-$(CONFIG_FUEL_GAUGE_SC27XX)      += sc27xx_fuel_gauge.o
> +obj-$(CONFIG_FUEL_GAUGE_STC3117)     += stc3117_fuel_gauge.o
>  obj-$(CONFIG_CHARGER_UCS1002)        += ucs1002_power.o
>  obj-$(CONFIG_CHARGER_BD99954)        += bd99954-charger.o
>  obj-$(CONFIG_CHARGER_WILCO)  += wilco-charger.o
> diff --git a/drivers/power/supply/stc3117_fuel_gauge.c b/drivers/power/supply/stc3117_fuel_gauge.c
> new file mode 100644
> index 000000000000..99291bb9250f
> --- /dev/null
> +++ b/drivers/power/supply/stc3117_fuel_gauge.c
> @@ -0,0 +-2,622 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * stc3117_fuel_gauge.c - STMicroelectronics STC3117 Fuel Gauge Driver
> + *
> + * Copyright (c) 2024 Silicon Signals Pvt Ltd.
> + * Author:      Bhavin Sharma <bhavin.sharma@siliconsignals.io>
> + *              Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.com>
> + */
> +
> +#include <linux/i2c.h>
> +#include <linux/workqueue.h>
> +#include <linux/power_supply.h>
> +#include <linux/regmap.h>
> +#include <linux/crc8.h>
> +
> +#define STC3117_ADDR_MODE                       0x00
> +#define STC3117_ADDR_CTRL                       0x01
> +#define STC3117_ADDR_SOC_L                      0x02
> +#define STC3117_ADDR_SOC_H                      0x03
> +#define STC3117_ADDR_COUNTER_L                  0x04
> +#define STC3117_ADDR_COUNTER_H                  0x05
> +#define STC3117_ADDR_CURRENT_L                  0x06
> +#define STC3117_ADDR_CURRENT_H                  0x07
> +#define STC3117_ADDR_VOLTAGE_L                  0x08
> +#define STC3117_ADDR_VOLTAGE_H                  0x09
> +#define STC3117_ADDR_TEMPERATURE                0x0A
> +#define STC3117_ADDR_AVG_CURRENT_L              0X0B
> +#define STC3117_ADDR_AVG_CURRENT_H              0X0C
> +#define STC3117_ADDR_OCV_L                      0X0D
> +#define STC3117_ADDR_OCV_H                      0X0E
> +#define STC3117_ADDR_CC_CNF_L                   0X0F
> +#define STC3117_ADDR_CC_CNF_H                   0X10
> +#define STC3117_ADDR_VM_CNF_L                   0X11
> +#define STC3117_ADDR_VM_CNF_H                   0X12
> +#define STC3117_ADDR_ALARM_SOC                  0X13
> +#define STC3117_ADDR_ALARM_VOLTAGE              0X14
> +#define STC3117_ADDR_ID                         0X18
> +#define STC3117_ADDR_CC_ADJ_L                        0X1B
> +#define STC3117_ADDR_CC_ADJ_H                        0X1C
> +#define STC3117_ADDR_VM_ADJ_L                        0X1D
> +#define STC3117_ADDR_VM_ADJ_H                        0X1E
> +#define STC3117_ADDR_RAM                     0x20
> +#define STC3117_ADDR_OCV_TABLE                       0x30
> +#define STC3117_ADDR_SOC_TABLE                       0x30
> +
> +/************ Bit mask definition ************/
> +#define STC3117_ID                           0x16
> +#define STC3117_MIXED_MODE                   0x00
> +#define STC3117_VMODE                                BIT(0)
> +#define STC3117_GG_RUN                               BIT(4)
> +#define STC3117_CC_MODE                              BIT(5)
> +#define STC3117_BATFAIL                      BIT(3)
> +#define STC3117_PORDET                               BIT(4)
> +
> +#define STC3117_RAM_SIZE                     16
> +#define STC3117_OCV_TABLE_SIZE                       16
> +#define STC3117_RAM_TESTWORD                 0x53A9
> +#define STC3117_SOFT_RESET                      0x11
> +#define STC3117_NOMINAL_CAPACITY             2600
> +
> +#define VOLTAGE_LSB_VALUE                    9011
> +#define CURRENT_LSB_VALUE                    24084
> +#define RSENSE                                       10
> +#define BATT_CAPACITY                                1800
> +#define BATTERY_RINT                         60
> +#define APP_CUTOFF_VOLTAGE                   2500
> +#define BATT_CHG_VOLTAGE                     4250
> +#define BATT_MIN_VOLTAGE                     3300
> +#define MAX_HRSOC                            51200
> +#define MAX_SOC                              1000
> +#define CHG_MIN_CURRENT                      200
> +#define CHG_END_CURRENT                      20
> +#define APP_MIN_CURRENT                      (-5)
> +#define BATTERY_FULL                         95
> +#define CRC8_POLYNOMIAL                      0x07
> +#define CRC8_INIT                            0x00
> +#define CRC8_TABLE_SIZE                      256
> +
> +DECLARE_CRC8_TABLE(stc3117_crc_table);
> +
> +enum stc3117_state {
> +     STC3117_INIT,
> +     STC3117_RUNNING,
> +     STC3117_POWERDN,
> +};
> +
> +enum stc3117_status {
> +     BATT_LOWBATT = -2,
> +     BATT_DISCHARG,
> +     BATT_IDLE,
> +     BATT_FULCHARG,
> +     BATT_ENDCHARG,
> +     BATT_CHARGING,
> +};
> +
> +/* Default OCV curve Li-ion battery */
> +static const int OCVValue[16] = {
> +    3400, 3582, 3669, 3676, 3699, 3737, 3757, 3774,
> +    3804, 3844, 3936, 3984, 4028, 4131, 4246, 4320
> +};
> +
> +static union stc3117_internal_ram {
> +     u8 ram_bytes[STC3117_RAM_SIZE];
> +     struct {
> +     u16 TestWord;   /* 0-1    Bytes */
> +     u16 HRSOC;      /* 2-3    Bytes */
> +     u16 CC_cnf;     /* 4-5    Bytes */
> +     u16 VM_cnf;     /* 6-7    Bytes */
> +     u8 soc;         /* 8      Byte  */
> +     u8 state;       /* 9      Byte  */
> +     u8 unused[5];   /* 10-14  Bytes */
> +     u8 CRC;         /* 15     Byte  */
> +     } reg;
> +} ram_data;
> +
> +struct stc3117_data {
> +     struct i2c_client       *client;
> +     struct regmap           *regmap;
> +     struct delayed_work update_work;
> +     struct power_supply     *battery;
> +
> +     u8 SOCValue[16];
> +     int CC_cnf;
> +     int VM_cnf;
> +     int CC_adj;
> +     int VM_adj;
> +     int AvgCurrent;
> +     int AvgVoltage;
> +     int Current;
> +     int Voltage;
> +     int Temp;
> +     int SOC;
> +     int OCV;
> +     int HRSOC;
> +     int Presence;
> +     int Battery_state;


That's some Windows coding style... You need to clean up everything here
to match Linux Coding style.


> +};
> +




> +     i2c_set_clientdata(client, data);
> +     psy_cfg.drv_data = data;
> +
> +     crc8_populate_msb(stc3117_crc_table, CRC8_POLYNOMIAL);
> +
> +     ret = stc3117_init(data);
> +     if (ret)
> +             dev_err_probe(&client->dev, ret, "failed to initialization of stc3117\n");
> +
> +     INIT_DELAYED_WORK(&data->update_work, fuel_gauge_update_work);
> +
> +     schedule_delayed_work(&data->update_work, 0);
> +
> +     data->battery = devm_power_supply_register(&client->dev,
> +                                                &stc3117_battery_desc, &psy_cfg);
> +     if (IS_ERR(data->battery))
> +             dev_err_probe(&client->dev, PTR_ERR(data->battery), "failed to register battery\n");
> +
You ignored (again!) received comments. In multiple places. Go back to
previous email and carefully read commetns.

One more thing:

Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).


Best regards,
Krzysztof

Re: [PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver

[Krzysztof Kozlowski]

On 28/11/2024 09:44, Bhavin Sharma wrote:
> Hi Krzysztof,
>  
> Thank you for your review and feedback.
>  
>>> +struct stc3117_data {
>>> +     struct i2c_client       *client;
>>> +     struct regmap           *regmap;
>>> +     struct delayed_work update_work;
>>> +     struct power_supply     *battery;
>>> +
>>> +     u8 SOCValue[16];
>>> +     int CC_cnf;
>>> +     int VM_cnf;
>>> +     int CC_adj;
>>> +     int VM_adj;
>>> +     int AvgCurrent;
>>> +     int AvgVoltage;
>>> +     int Current;
>>> +     int Voltage;
>>> +     int Temp;
>>> +     int SOC;
>>> +     int OCV;
>>> +     int HRSOC;
>>> +     int Presence;
>>> +     int Battery_state;
>>
>> That's some Windows coding style... You need to clean up everything here
>> to match Linux Coding style.
>  
> Could you clarify what specific changes are required here to align with the Linux
> coding style?


Entire. Go one by one: "Breaking long lines and strings" - not
implemented. "Naming" - not implemented. Then go with every point. You
are making here some sort of shortcut - ignoring coding style, not
reading it and insisting on me to provide you exact things to change.
No, that's way too many things. You are supposed to read the coding style.

>  
> I am not sure what exactly needs to be changed here.
>  
>>> +     data->battery = devm_power_supply_register(&client->dev,
>>> +                                                &stc3117_battery_desc, &psy_cfg);
>>> +     if (IS_ERR(data->battery))
>>> +             dev_err_probe(&client->dev, PTR_ERR(data->battery), "failed to register battery\n");
>>> +
>> You ignored (again!) received comments. In multiple places. Go back to
>> previous email and carefully read commetns.
>  
> Sebastian suggested using dev_err_probe, while you mentioned using dev_err.
> so what should i follow ?


No. That's not true. Read comments again. I am not happy that after
pointing out you still insist and force me to re-iterate the same.
That's my last reply in this matter:

comment was:
"return dev_err_probe(dev, PTR_ERR(stc_sply), "failed to register
battery\n");"

Where do you have "return" statement?

What about all my other comments? You are supposed to reply inline and
acknowledge each of such comment. That's the only way I believe you will
really do what we ask you to do.

> 
>> One more thing:
>>  
>> Please wrap code according to coding style (checkpatch is not a coding
>> style description, but only a tool).
>  
> Could you recommend an example driver from the kernel source tree that 
> follows the expected coding style? This would help me ensure compliance.


`git log -- path` will tell give you the latest drivers..


>  
> Best Regards,
> Bhavin
>  


Trim your replies and do not top-post. All this copied stuff below is
making things just difficult to read.

> 
> 
> 
> 
> 
> 
> 
> ________________________________________
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Wednesday, November 27, 2024 11:54 PM
> To: Bhavin Sharma <bhavin.sharma@siliconsignals.io>; sre@kernel.org <sre@kernel.org>; krzk+dt@kernel.org <krzk+dt@kernel.org>; robh@kernel.org <robh@kernel.org>; conor+dt@kernel.org <conor+dt@kernel.org>
> Cc: Hardevsinh Palaniya <hardevsinh.palaniya@siliconsignals.io>; linux-pm@vger.kernel.org <linux-pm@vger.kernel.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>; linux-


All this must be removed.


Best regards,
Krzysztof

Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Bhavin Sharma]

Hi Krzysztof,

>>> +  reg:
>>> +    maxItems: 1
>>
>> I asked you some questions on v2, then on v3 and no responses.
>>
>> You implemented some changes but still did not answer my question. I am
>> not going to ask again, obviously expecting different result on the same
>> makes little sense.
>>
>> No ack from me.
>>
>
> You responded privately - I am not going to do any talks under NDA. I
> also do not provide some sort of personal support service. Keep *ALL*
> discussions public.

My apologies, that was unintentional.

> Explaining what you asked:
>
> Some of these are from monitored-battery. Sense resistor should be
> separate property. But different question is about missing resources,
> like supplies (VCC) and interrupts. Just look at datasheet.

since battery itself serves as the power supply for fuel gauge no additional supplies are required. 

Regarding interrupts yes, datasheet specifies an alarm pin that signals low State of Charge or
Voltage conditions. However, I haven’t incorporated interrupts in the driver to handle this. Should I
list the alarm pin as a resource in the device tree (or relevant configuration) even if it’s not 
implemented in the driver yet?

Best Regards,
Bhavin

Re: [PATCH v4 1/2] dt-bindings: power: supply: Add STC3117 Fuel Gauge

[Krzysztof Kozlowski]

On 28/11/2024 10:22, Bhavin Sharma wrote:
> 
>> Explaining what you asked:
>>
>> Some of these are from monitored-battery. Sense resistor should be
>> separate property. But different question is about missing resources,
>> like supplies (VCC) and interrupts. Just look at datasheet.
> 
> since battery itself serves as the power supply for fuel gauge no additional supplies are required. 
> 
> Regarding interrupts yes, datasheet specifies an alarm pin that signals low State of Charge or
> Voltage conditions. However, I haven’t incorporated interrupts in the driver to handle this. Should I
> list the alarm pin as a resource in the device tree (or relevant configuration) even if it’s not 
> implemented in the driver yet?

Yes, bindings should be complete.
Best regards,
Krzysztof