| 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: <20241130174819.19551-1-christophe.jaillet@wanadoo.fr>
Date: Sat, 30 Nov 2024 18:47:57 +0100
From: Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:
Cc: conor+dt@...nel.org,
devicetree@...r.kernel.org,
krzk+dt@...nel.org,
linux-kernel@...r.kernel.org,
linux-pm@...r.kernel.org,
robh@...nel.org,
sre@...nel.org
Subject: Re: [PATCH v5 2/2] power: supply: Add STC3117 fuel gauge unit driver
My mailer refuses adesses at @siliconsignals.io. So not every one is in this reply
Le 29/11/2024 à 12:40, Bhavin Sharma a écrit :
> 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: Bhavin Sharma <bhavin.sharma@...iconsignals.io>
> Signed-off-by: Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>
> ---
...
> +/* 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)
One unneeded extra tab?
> +#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
...
> +static int stc3117_set_para(struct stc3117_data *data)
> +{
> + int ret;
> +
> + ret = regmap_write(data->regmap, STC3117_ADDR_MODE, STC3117_VMODE);
> +
> + for (int i = 0; i < STC3117_OCV_TABLE_SIZE; i++)
> + regmap_write(data->regmap, STC3117_ADDR_OCV_TABLE + i,
> + ocvValue[i] * 100 / 55);
Should there be a ret |= ?
> + if (data->soc_tab[1] != 0)
> + regmap_bulk_write(data->regmap, STC3117_ADDR_SOC_TABLE,
> + data->soc_tab, STC3117_OCV_TABLE_SIZE);
Should there be a ret |= ?
If it is needed, some other places in the driver may alos need it.
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_CC_CNF_H,
> + (ram_data.reg.cc_cnf >> 8) & 0xFF);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_CC_CNF_L,
> + ram_data.reg.cc_cnf & 0xFF);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_VM_CNF_H,
> + (ram_data.reg.vm_cnf >> 8) & 0xFF);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_VM_CNF_L,
> + ram_data.reg.vm_cnf & 0xFF);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_CTRL, 0x03);
> +
> + ret |= regmap_write(data->regmap, STC3117_ADDR_MODE,
> + STC3117_MIXED_MODE | STC3117_GG_RUN);
> +
> + return ret;
> +};
...
> +static int stc3117_get_property(struct power_supply *psy,
> + enum power_supply_property psp, union power_supply_propval *val)
> +{
> + struct stc3117_data *data = power_supply_get_drvdata(psy);
> +
> + switch (psp) {
> + case POWER_SUPPLY_PROP_STATUS:
> + if (data->soc > BATTERY_FULL)
> + val->intval = POWER_SUPPLY_STATUS_FULL;
This is dead-code. "val->intval" is over-written in ALL paths below.
The logic looks broken.
> + if (data->batt_current < 0)
> + val->intval = POWER_SUPPLY_STATUS_CHARGING;
> + else if (data->batt_current > 0)
> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> + else
> + val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> + break;
> + case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> + val->intval = data->voltage;
> + break;
> + case POWER_SUPPLY_PROP_CURRENT_NOW:
> + val->intval = data->batt_current;
> + break;
> + case POWER_SUPPLY_PROP_CAPACITY:
> + val->intval = data->soc;
> + break;
> + case POWER_SUPPLY_PROP_TEMP:
> + val->intval = data->temp;
> + break;
> + case POWER_SUPPLY_PROP_PRESENT:
> + val->intval = data->presence;
> + break;
> + default:
> + return -EINVAL;
> + }
> + return 0;
> +}
...
> +static int stc3117_probe(struct i2c_client *client)
> +{
> + struct stc3117_data *data;
> + struct power_supply_config psy_cfg = {};
> + struct power_supply_battery_info *info;
> + int ret;
> +
> + data = devm_kzalloc(&client->dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + return -ENOMEM;
> +
> + data->client = client;
> + data->regmap = devm_regmap_init_i2c(client, &stc3117_regmap_config);
> + if (IS_ERR(data->regmap))
> + return PTR_ERR(data->regmap);
> +
> + i2c_set_clientdata(client, data);
Is it needed?
(there is no i2c_get_clientdata() in the code)
> + psy_cfg.drv_data = data;
> +
> + crc8_populate_msb(stc3117_crc_table, CRC8_POLYNOMIAL);
> +
> + data->battery = devm_power_supply_register(&client->dev,
> + &stc3117_battery_desc, &psy_cfg);> + if (IS_ERR(data->battery))
> + return dev_err_probe(&client->dev, PTR_ERR(data->battery),
> + "failed to register battery\n");
> +
> + ret = device_property_read_u32(&client->dev, "sense-resistor",
> + &battery_info.sense_resistor);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to get sense-register\n");
Should it be "failed to get sense-resistor\n"?
> +
> + ret = power_supply_get_battery_info(data->battery, &info);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to get battery information\n");
> +
> + battery_info.battery_capacity = info->charge_full_design_uah * 1000;
> + battery_info.voltage_min = info->voltage_min_design_uv * 1000;
> + battery_info.voltage_max = info->voltage_min_design_uv * 1000;
Should it be voltage_max_design_uv?
> +
> + ret = stc3117_init(data);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "failed to initialization of stc3117\n");
"failed initialization" of "failed to initialize"?
> +
> + INIT_DELAYED_WORK(&data->update_work, fuel_gauge_update_work);
> +
> + schedule_delayed_work(&data->update_work, 0);
> +
> + return 0;
> +}
> +
> +static const struct i2c_device_id stc3117_id[] = {
> + {"stc3117", 0},
Spaces sould be added to match stc3117_of_match below.
> + {},
Unneeded ending comma after a terminator entry.
> +};
> +MODULE_DEVICE_TABLE(i2c, stc3117_id);
> +
> +static const struct of_device_id stc3117_of_match[] = {
> + { .compatible = "st,stc3117" },
> + {},
Unneeded ending comma after a terminator entry.
> +};
> +MODULE_DEVICE_TABLE(of, stc3117_of_match);
...
CJ
Powered by blists - more mailing lists