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

[Sebastian Reichel]

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@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Bhavin Sharma <bhavin.sharma@xxxxxxxxxxxxxxxxx>
> ---

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

Attachment: signature.asc
Description: PGP signature