| 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:
<MA0P287MB1178B165FAFE680FDBD14301F2292@MA0P287MB1178.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 28 Nov 2024 08:44:53 +0000
From: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
To: Krzysztof Kozlowski <krzk@...nel.org>, "sre@...nel.org" <sre@...nel.org>,
"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "robh@...nel.org"
<robh@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>
CC: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
"linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 2/2] power: supply: Add STC3117 fuel gauge unit driver
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@...nel.org>
Sent: Wednesday, November 27, 2024 11:54 PM
To: Bhavin Sharma <bhavin.sharma@...iconsignals.io>; sre@...nel.org <sre@...nel.org>; krzk+dt@...nel.org <krzk+dt@...nel.org>; robh@...nel.org <robh@...nel.org>; conor+dt@...nel.org <conor+dt@...nel.org>
Cc: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>; linux-pm@...r.kernel.org <linux-pm@...r.kernel.org>; devicetree@...r.kernel.org <devicetree@...r.kernel.org>; linux-kernel@...r.kernel.org <linux-kernel@...r.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@...iconsignals.io>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@...iconsignals.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@...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 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@...iconsignals.io>
> + * Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.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
Powered by blists - more mailing lists