linux-kernel - Re: [PATCH v12 2/2] power: supply: Add STC3117 fuel gauge unit driver

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <6degfnykilymnfxv2ly4gc24bkt5jrtneyrwv2bw5v2zrpgurf@bra77oalcysd>
Date: Fri, 20 Dec 2024 00:32:04 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
Cc: 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 v12 2/2] power: supply: Add STC3117 fuel gauge unit driver

Hi,

On Thu, Dec 19, 2024 at 03:19:12PM +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
> 
> Co-developed-by: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
> ---

I missed one thing in my review. Apart from that things look good to
me now.

> [...]
> +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;
> +};
> +
> +struct stc3117_battery_info {
> +	int voltage_min_mv;
> +	int voltage_max_mv;
> +	int battery_capacity_mah;
> +	int sense_resistor;
> +} battery_info;

You may not use a static variable for holding the battery
information and RAM data. A system could have two stc3117
fuel gauges with different data. Instead you can add them
into struct stc3117_data, which is allocated per instance.

Greetings,

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)