| 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
| ||
|
Date: Wed, 11 Mar 2020 12:18:30 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Tobias Schramm <t.schramm@...jaro.org>
Cc: Sebastian Reichel <sre@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Maxime Ripard <mripard@...nel.org>,
Sam Ravnborg <sam@...nborg.org>,
Heiko Stuebner <heiko.stuebner@...obroma-systems.com>,
Stephan Gerhold <stephan@...hold.net>,
Mark Brown <broonie@...nel.org>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Jonathan Cameron <Jonathan.Cameron@...wei.com>,
linux-pm@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] power: supply: add CellWise cw2015 fuel gauge
driver
On Wed, Mar 11, 2020 at 10:30:43AM +0100, Tobias Schramm wrote:
> This patch adds a driver for the CellWise cw2015 fuel gauge.
>
> The CellWise cw2015 is a shuntless, single-cell Li-Ion fuel gauge used
> in the pine64 Pinebook Pro laptop and some Raspberry Pi UPS HATs.
Thank you for an update!
My comments below.
...
> + /* wait for gauge to become ready */
> + for (i = 0; i < CW2015_READ_TRIES; i++) {
> + ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, ®_val);
> + if (ret)
> + return ret;
> + /* SoC must not be more than 100% */
> + else if (reg_val <= 100)
> + break;
> +
> + msleep(100);
> + }
Have you considered to use regmap_read_poll_timeout()?
> +
> + if (i >= CW2015_READ_TRIES) {
> + reg_val = CW2015_MODE_SLEEP;
> + regmap_write(cw_bat->regmap, CW2015_REG_MODE, reg_val);
> + dev_err(cw_bat->dev,
> + "Gauge did not become ready after profile upload");
> + return -ETIMEDOUT;
> + }
...
> + if (memcmp(bat_info, cw_bat->bat_profile,
> + CW2015_SIZE_BATINFO)) {
I think it's pretty much okay to have this on one line, disregard 80 limit
(it's only 1 extra).
...
> +static int cw_get_soc(struct cw_battery *cw_bat)
> +{
> + unsigned int soc;
> + int ret;
> +
> + ret = regmap_read(cw_bat->regmap, CW2015_REG_SOC, &soc);
> + if (ret)
> + return ret;
> +
> + if (soc > 100) {
> + int max_error_cycles = CW2015_BAT_SOC_ERROR_MS /
> + cw_bat->poll_interval_ms;
The following looks better
int max_error_cycles =
CW2015_BAT_SOC_ERROR_MS / cw_bat->poll_interval_ms;
Applies to all similar places in the code.
> + dev_err(cw_bat->dev, "Invalid SoC %d%%", soc);
> + cw_bat->read_errors++;
> + if (cw_bat->read_errors > max_error_cycles) {
> + dev_warn(cw_bat->dev,
> + "Too many invalid SoC reports, resetting gauge");
> + cw_power_on_reset(cw_bat);
> + cw_bat->read_errors = 0;
> + }
> + return cw_bat->soc;
> + }
> + cw_bat->read_errors = 0;
> +
> + /* Reset gauge if stuck while charging */
> + if (cw_bat->status == POWER_SUPPLY_STATUS_CHARGING &&
> + soc == cw_bat->soc) {
A bit strange indentation, and honestly I would leave it on one line, but it's up to you.
> + int max_stuck_cycles = CW2015_BAT_CHARGING_STUCK_MS /
> + cw_bat->poll_interval_ms;
> +
> + cw_bat->charge_stuck_cnt++;
> + if (cw_bat->charge_stuck_cnt > max_stuck_cycles) {
> + dev_warn(cw_bat->dev,
> + "SoC stuck @%u%%, resetting gauge", soc);
> + cw_power_on_reset(cw_bat);
> + cw_bat->charge_stuck_cnt = 0;
> + }
> + } else {
> + cw_bat->charge_stuck_cnt = 0;
> + }
> +
> + /* Ignore voltage dips during charge */
> + if (cw_bat->charger_attached &&
> + HYSTERESIS(soc, cw_bat->soc, 0, 3)) {
This is pretty much one line (77), check your editor settings and update all
similar places in the code.
> + soc = cw_bat->soc;
> + }
> +
> + /* Ignore voltage spikes during discharge */
> + if (!cw_bat->charger_attached &&
> + HYSTERESIS(soc, cw_bat->soc, 3, 0)) {
> + soc = cw_bat->soc;
> + }
> +
> + return soc;
> +}
...
> + cw_bat =
> + container_of(delay_work, struct cw_battery, battery_delay_work);
It will be better to read if it would be one line.
...
> +static bool cw_battery_valid_time_to_empty(struct cw_battery *cw_bat)
> +{
> + return cw_bat->time_to_empty > 0 &&
> + cw_bat->time_to_empty < CW2015_MASK_SOC &&
> + cw_bat->status == POWER_SUPPLY_STATUS_DISCHARGING;
Fix indentation to be all 'c':s in one column.
> +}
...
> +static int cw2015_parse_properties(struct cw_battery *cw_bat)
> +{
> + struct device *dev = cw_bat->dev;
> + int length;
> + u32 value;
> + int ret;
> +
> + length = device_property_read_u8_array(dev, "cellwise,battery-profile",
> + NULL, 0);
device_property_count_u8();
> + if (length) {
> + if (length != CW2015_SIZE_BATINFO) {
> + dev_err(cw_bat->dev, "battery-profile must be %d bytes",
> + CW2015_SIZE_BATINFO);
> + return -EINVAL;
> + }
> +
> + cw_bat->bat_profile =
> + devm_kzalloc(dev, CW2015_SIZE_BATINFO, GFP_KERNEL);
Replace with length (so, you will have one point of validation), and put on
one line.
> + if (!cw_bat->bat_profile) {
> + dev_err(cw_bat->dev,
> + "Failed to allocate memory for battery config info");
> + return -ENOMEM;
> + }
> +
> + ret = device_property_read_u8_array(dev,
> + "cellwise,battery-profile",
> + cw_bat->bat_profile,
> + CW2015_SIZE_BATINFO);
length.
> + if (ret)
> + return ret;
> + } else {
> + dev_warn(cw_bat->dev,
> + "No battery-profile found, rolling with current flash contents");
> + }
> +
> + cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS;
> + ret = device_property_read_u32_array(dev,
> + "cellwise,monitor-interval-ms",
It's fine to have it on one line.
> + &value, 1);
> + if (ret >= 0) {
> + dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms",
> + value);
> + cw_bat->poll_interval_ms = value;
> + }
> +
> + return 0;
> +}
...
> + regmap_reg_range(CW2015_REG_BATINFO,
> + CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1),
Indentation issue. Check all similar places.
...
> + cw_bat->rk_bat = devm_power_supply_register(&client->dev,
> + &cw2015_bat_desc, &psy_cfg);
> + if (IS_ERR(cw_bat->rk_bat)) {
> + dev_err(cw_bat->dev, "Failed to register power supply");
> + return -1;
Do not shadow an error code.
> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists