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

[Krzysztof Kozlowski <krzk@kernel.org>]

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