| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <6ccn5xnsfwxeepsft7tfozbhsvs3w3m2qpmcj74xua74qzkny3@s5rg3ytmcgzn>
Date: Fri, 6 Dec 2024 13:26:09 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
Cc: christophe.jaillet@...adoo.fr,
Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 2/2] power: supply: Add STC3117 fuel gauge unit driver
Hi,
On Mon, Dec 02, 2024 at 03:11:53PM +0530, 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@...iconsignals.io>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
> ---
Please add the output from running the following script on a system
without your fuel gauge to the cover letter in the next submission.
It helps to check that the values are properly converted:
./tools/testing/selftests/power_supply/test_power_supply_properties.sh
> MAINTAINERS | 8 +
> drivers/power/supply/Kconfig | 7 +
> drivers/power/supply/Makefile | 1 +
> drivers/power/supply/stc3117_fuel_gauge.c | 665 ++++++++++++++++++++++
> 4 files changed, 681 insertions(+)
> create mode 100644 drivers/power/supply/stc3117_fuel_gauge.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index c1fcc56bf2fb..10ead03660d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -22125,6 +22125,14 @@ T: git git://linuxtv.org/media.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@...iconsignals.io>
> +M: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
> +L: linux-pm@...r.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@...s.st.com>
> S: Maintained
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 9f2eef6787f7..9f34a2a6bc00 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -918,6 +918,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
> + chip.
> +
> 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 59c4a9f40d28..b55cc48a4c86 100644
> --- a/drivers/power/supply/Makefile
> +++ b/drivers/power/supply/Makefile
> @@ -108,6 +108,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..ffa7e0d35474
> --- /dev/null
> +++ b/drivers/power/supply/stc3117_fuel_gauge.c
> @@ -0,0 +1,665 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * stc3117_fuel_gauge.c - STMicroelectronics STC3117 Fuel Gauge Driver
> + *
> + * Copyright (c) 2024 Silicon Signals Pvt Ltd.
> + * Author: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.com>
> + * Bhavin Sharma <bhavin.sharma@...iconsignals.io>
> + */
> +
> +#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
Please keep the x in 0X lower case. It suddenly changes after 0x0A
> +#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 APP_CUTOFF_VOLTAGE 2500
> +#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
> +
> +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 ocv_value[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 soc_tab[16];
> + int cc_cnf;
> + int vm_cnf;
> + int cc_adj;
> + int vm_adj;
> + int avg_current;
> + int avg_voltage;
> + int batt_current;
> + int voltage;
> + int temp;
> + int soc;
> + int ocv;
> + int hrsoc;
> + int presence;
> + int battery_state;
> +};
> +
> +struct stc3117_battery_info {
> + int voltage_min_mv;
> + int voltage_max_mv;
> + int battery_capacity_mah;
> + int sense_resistor;
> +} battery_info;
> +
> +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 in mA*/
typo (cureent -> currrent)
> + value = (reg_list[7] << 8) + reg_list[6];
> + data->batt_current = stc3117_convert(value,
> + CURRENT_LSB_VALUE / battery_info.sense_resistor);
> +
> + /* voltage in mV */
> + value = (reg_list[9] << 8) + reg_list[8];
> + data->voltage = stc3117_convert(value, VOLTAGE_LSB_VALUE);
> +
> + /* temp */
Looks like temp is in °C?
> + data->temp = reg_list[10];
> +
> + /* Avg batt_current in mA */
> + 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 / battery_info.sense_resistor);
> + value = value / 4;
> + } else {
> + value = stc3117_convert(value, 36 * STC3117_NOMINAL_CAPACITY);
> + }
> + data->avg_current = value;
> +
> + /* ocv */
I guess ocv is also in mV?
> + value = (reg_list[14] << 8) + reg_list[13];
> + value = stc3117_convert(value, VOLTAGE_LSB_VALUE);
> + 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 stc3117_update_battery_status(struct stc3117_data *data)
> +{
> + switch (data->battery_state) {
> + case BATT_CHARGING:
> + if (data->avg_current < CHG_MIN_CURRENT)
> + data->battery_state = BATT_ENDCHARG;
> + break;
> +
> + case BATT_ENDCHARG:
> + if (data->batt_current > CHG_MIN_CURRENT)
> + data->battery_state = BATT_CHARGING;
> + else if (data->avg_current < CHG_END_CURRENT)
> + data->battery_state = BATT_IDLE;
> + else if ((data->batt_current > CHG_END_CURRENT) &&
> + (data->voltage > battery_info.voltage_max_mv))
> + data->battery_state = BATT_FULCHARG;
> + break;
> +
> + case BATT_FULCHARG:
> + if (data->batt_current > CHG_MIN_CURRENT) {
> + data->battery_state = BATT_CHARGING;
> + } else if (data->avg_current < CHG_END_CURRENT) {
> + if (data->avg_voltage > battery_info.voltage_max_mv) {
> + 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->batt_current > CHG_END_CURRENT)
> + data->battery_state = BATT_CHARGING;
> + else if (data->batt_current < APP_MIN_CURRENT)
> + data->battery_state = BATT_DISCHARG;
> + break;
> +
> + case BATT_DISCHARG:
> + if (data->batt_current > APP_MIN_CURRENT)
> + data->battery_state = BATT_IDLE;
> + else if (data->avg_voltage < battery_info.voltage_min_mv)
> + data->battery_state = BATT_LOWBATT;
> + break;
> +
> + case BATT_LOWBATT:
> + if (data->avg_voltage > (battery_info.voltage_min_mv + 50))
> + data->battery_state = BATT_IDLE;
> + break;
> +
> + default:
> + data->battery_state = BATT_IDLE;
> + }
> +
> + return 0;
> +}
You are never using data->battery_state, so the whole function can
be removed and battery_state can be removed from the data struct.
> +
> +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 ret;
> +
> + 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 ret;
> +
> + 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++)
> + ret |= regmap_write(data->regmap, STC3117_ADDR_OCV_TABLE + i,
> + ocv_value[i] * 100 / 55);
> + if (data->soc_tab[1] != 0)
> + ret |= regmap_bulk_write(data->regmap, STC3117_ADDR_SOC_TABLE,
> + data->soc_tab, 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 = (battery_info.battery_capacity_mah *
> + battery_info.sense_resistor * 250 + 6194) / 12389;
> + data->vm_cnf = (battery_info.battery_capacity_mah
> + * 200 * 50 + 24444) / 48889;
> +
> + /* Battery has not been removed */
> + data->presence = 1;
> +
> + /* Read RAM data */
> + ret = ram_read(data);
> + if (ret)
> + return ret;
> +
> + 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 ret;
> + } else {
> + ret = regmap_read(data->regmap, STC3117_ADDR_CTRL, &ctrl);
> + if (ret)
> + return ret;
> +
> + 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 ret;
> + } else {
> + ret = stc3117_set_para(data);
> + ret |= regmap_write(data->regmap, STC3117_ADDR_SOC_H,
> + (ram_data.reg.hrsoc >> 8 & 0xFF));
> + ret |= regmap_write(data->regmap, STC3117_ADDR_SOC_L,
> + (ram_data.reg.hrsoc & 0xFF));
> + if (ret)
> + return ret;
> + }
> + }
> +
> + 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 ret;
> +
> + data->battery_state = BATT_IDLE;
> +
> + return 0;
> +};
> +
> +static int stc3117_task(struct stc3117_data *data)
> +{
> + int ID, mode, ret;
why is ID in upper case like a constant?
> + 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 ret;
> +
> + 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 */
> + ret = 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);
> + ret |= regmap_write(data->regmap, STC3117_ADDR_CTRL, STC3117_PORDET);
> + if (ret)
> + return ret;
> + }
> +
> + data->presence = 1;
> +
> + ret = regmap_read(data->regmap, STC3117_ADDR_MODE, &mode);
> + if (ret)
> + return ret;
> + if ((mode & STC3117_GG_RUN) == 0) {
> + if (ram_data.reg.state > STC3117_INIT) {
> + ret = stc3117_set_para(data);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_SOC_H,
> + (ram_data.reg.hrsoc >> 8 & 0xFF));
> + ret |= regmap_write(data->regmap, STC3117_ADDR_SOC_L,
> + (ram_data.reg.hrsoc & 0xFF));
> + if (ret)
> + return ret;
> + } 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 ret;
> + }
> + 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 batt_current & temperature value available: */
> + if (ram_data.reg.state == STC3117_INIT && count_m > 4) {
> + data->avg_voltage = data->voltage;
> + data->avg_current = data->batt_current;
> + ram_data.reg.state = STC3117_RUNNING;
> + }
> +
> + if (ram_data.reg.state != STC3117_RUNNING) {
> + data->batt_current = 0;
> + data->temp = 250;
why 250?
> + } else {
> + if (data->voltage < APP_CUTOFF_VOLTAGE)
> + data->soc = 0;
> +
> + if (mode & STC3117_VMODE) {
> + data->avg_current = 0;
> + data->batt_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 ret;
> + 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);
Please run checkpatch before patch submission.
> +
> + /* 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;
> + else if (data->batt_current < 0)
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + else if (data->batt_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;
This has to be in uV.
> + break;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = data->batt_current;
> + break;
This has to be in uA.
> + case POWER_SUPPLY_PROP_CAPACITY:
> + val->intval = data->soc;
> + break;
> + case POWER_SUPPLY_PROP_TEMP:
> + val->intval = data->temp;
> + break;
This has to be in 1/10 of °C.
> + 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,
> +};
During the read process you are also getting OCV and average
current. Why are you not exporting them via the following
properties (in uV and uA) when you are getting them anyways?
POWER_SUPPLY_PROP_VOLTAGE_OCV
POWER_SUPPLY_PROP_CURRENT_AVG
> +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 = {};
> + struct power_supply_battery_info *info;
> + 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);
> +
> + psy_cfg.drv_data = data;
psy_cfg.fwnode = dev_fwnode(dev);
> +
> + crc8_populate_msb(stc3117_crc_table, CRC8_POLYNOMIAL);
> +
> + data->battery = devm_power_supply_register(&client->dev,
> + &stc3117_battery_desc, &psy_cfg);
> + if (IS_ERR(data->battery))
> + return dev_err_probe(&client->dev, PTR_ERR(data->battery),
> + "failed to register battery\n");
> +
> + ret = device_property_read_u32(&client->dev, "shunt-resistor-micro-ohms",
> + &battery_info.sense_resistor);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to get sense-registor\n");
> + battery_info.sense_resistor = battery_info.sense_resistor / 1000;
> +
> + ret = power_supply_get_battery_info(data->battery, &info);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to get battery information\n");
> +
> + battery_info.battery_capacity_mah = info->charge_full_design_uah / 1000;
> + battery_info.voltage_min_mv = info->voltage_min_design_uv / 1000;
> + battery_info.voltage_max_mv = info->voltage_max_design_uv / 1000;
> +
> + ret = stc3117_init(data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to initialize of stc3117\n");
> +
> + INIT_DELAYED_WORK(&data->update_work, fuel_gauge_update_work);
This is not being stopped on module removal. You need
devm_delayed_work_autocancel() instead of INIT_DELAYED_WORK.
Greetings,
-- Sebastian
> + schedule_delayed_work(&data->update_work, 0);
> +
> + 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("Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>");
> +MODULE_AUTHOR("Bhavin Sharma <bhavin.sharma@...iconsignals.io>");
> +MODULE_DESCRIPTION("STC3117 Fuel Gauge Driver");
> --
> 2.43.0
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists