linux-kernel - Re: [PATCH v7 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:
 <PN2PPFF679F9759E522908216724F696B21F2322@PN2PPFF679F9759.INDP287.PROD.OUTLOOK.COM>
Date: Sat, 7 Dec 2024 14:27:12 +0000
From: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
CC: "christophe.jaillet@...adoo.fr" <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" <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 v7 2/2] power: supply: Add STC3117 fuel gauge unit driver

Hi Sebastian,

Thanks for your feedbacks

> > 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

Sure, i will attach output of the test 
> > +#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

okay 
> > +     data->soc = (value * 10 + 256) / 512;
> > +
> > +     /* cureent in mA*/
>
> typo (cureent -> currrent)

okay
> > +
> > +     /* temp */
>
> Looks like temp is in °C?
okay 
> > +
> > +     /* ocv */
>
> I guess ocv is also in mV?

okay
> > +     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.

yes will remove in next version

> > +{
> > +     int ID, mode, ret;
>
> why is ID in upper case like a constant?

okay will change into lower case
> > +     if (ram_data.reg.state != STC3117_RUNNING) {
> > +             data->batt_current = 0;
> > +             data->temp = 250;
>
> why 250?

temperature range for this sensor is specified in the datasheet
as -40°C to 125°C.
The value 250 lies outside this range, which means that the temperature
data is unavailable or invalid. This approach is consistent with the vendor's
Arduino driver, where 250 is also used in similar conditions to indicate that
the sensor is not in running mode.

> > +     struct stc3117_data *data = container_of(to_delayed_work(work),
> > +                                     struct stc3117_data, update_work);
> > +     stc3117_task(data);
>
> Please run checkpatch before patch submission.

I did this i am not getting any warnings here 
> > +     case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> > +             val->intval = data->voltage;
> 
> This has to be in uV.
okay
> > +             break;
> > +     case POWER_SUPPLY_PROP_CURRENT_NOW:
> > +             val->intval = data->batt_current;
> > +             break;
>
> This has to be in uA.
okay 
> > +     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.

okay 
> > +     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

okay will add this property
> > +             return PTR_ERR(data->regmap);
> > +
> > +     psy_cfg.drv_data = data;
>
> psy_cfg.fwnode = dev_fwnode(dev);
 
okay 
> > +                             "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.

sure 

Best regards,
Bhavin