Hi Sebastian,
Thanks for your feedbacks
> > Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@xxxxxxxxxxxxxxxxx>
> > Signed-off-by: Bhavin Sharma <bhavin.sharma@xxxxxxxxxxxxxxxxx>
> > ---
>
> 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
