| Message ID | 20200309160346.2203680-1-t.schramm@manjaro.org |
|---|---|
| Headers | show |
| Series | Add support for CellWise cw2015 fuel gauge | expand |
On Mon, Mar 09, 2020 at 05:03:46PM +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. ... > F: arch/powerpc/oprofile/*cell* > F: arch/powerpc/platforms/cell/ > > +CELLWISE CW2015 BATTERY DRIVER > +M: Tobias Schrammm <t.schramm@manjaro.org> > +S: Maintained > +F: drivers/power/supply/cw2015_battery.c > +F: Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml > + > CEPH COMMON CODE (LIBCEPH) > M: Ilya Dryomov <idryomov@gmail.com> > M: Jeff Layton <jlayton@kernel.org> Run parse-mainteiners.pl and fix ordering issue in above. ... > + * Authors: xuhuicong <xhc@rock-chips.com> > + * Authors: Tobias Schramm <tobias@t-sys.eu> > + * Redundant line. ... > +#include <linux/delay.h> > +#include <linux/init.h> > +#include <linux/i2c.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/of_gpio.h> I think you need gpio/consumer.h. > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/slab.h> > +#include <linux/timekeeping.h> > +#include <linux/workqueue.h> > +#include <linux/regmap.h> Perhaps keep above sorted? ... > +#define CW2015_READ_TRIES 30 > +#define CW2015_RESET_TRIES 5 > + > +#define CW2015_REG_VERSION 0x0 > +#define CW2015_REG_VCELL 0x2 > +#define CW2015_REG_SOC 0x4 > +#define CW2015_REG_RRT_ALERT 0x6 > +#define CW2015_REG_CONFIG 0x8 > +#define CW2015_REG_MODE 0xA > +#define CW2015_REG_BATINFO 0x10 Use same width for all register offsets: 0x08 etc. > +#define CW2015_MODE_SLEEP_MASK (0x3<<6) > +#define CW2015_MODE_SLEEP (0x3<<6) > +#define CW2015_MODE_NORMAL (0x0<<6) > +#define CW2015_MODE_QUICK_START (0x3<<4) > +#define CW2015_MODE_RESTART (0xf<<0) > + > +#define CW2015_CONFIG_UPDATE_FLG (0x01<<1) > +#define CW2015_ATHD(x) ((x)<<3) > +#define CW2015_MASK_ATHD (0x1f<<3) > +#define CW2015_MASK_SOC (0x1fff) GENMASK() and somebody missed a lot of white spaces above. > +#define CW2015_BATTERY_UP_MAX_CHANGE (420 * 1000) > +#define CW2015_BATTERY_DOWN_MAX_CHANGE (120 * 1000) > +#define CW2015_BATTERY_DOWN_CHANGE 60 > +#define CW2015_BATTERY_DOWN_MIN_CHANGE_RUN 30 > +#define CW2015_BATTERY_DOWN_MIN_CHANGE_SLEEP 1800 > +#define CW2015_BATTERY_JUMP_TO_ZERO (30 * 1000) > +#define CW2015_BATTERY_CAPACITY_ERROR (40 * 1000) > +#define CW2015_BATTERY_CHARGING_ZERO (1800 * 1000) What these multiplications about? We have time units in time.h (or time64.h). Moreover is a good tone to use units in the name of definitions. And try to shorten them, they are already quite long. > +#define CW2015_DEFAULT_MONITOR_MS 8000 8 seconds? Any comments about this? ... > +struct cw_battery { > + struct i2c_client *client; Do you need this? Perhaps you can keep struct device from which you can easily retrive the client. > +}; ... > +#define PREFIX "cellwise," We have pr_fmt() / dev_fmt() for this. > +#define cw_dbg(cw_bat, ...) dev_dbg(&(cw_bat)->client->dev, __VA_ARGS__) > +#define cw_info(cw_bat, ...) dev_info(&(cw_bat)->client->dev, __VA_ARGS__) > +#define cw_warn(cw_bat, ...) dev_warn(&(cw_bat)->client->dev, __VA_ARGS__) > +#define cw_err(cw_bat, ...) dev_err(&(cw_bat)->client->dev, __VA_ARGS__) No, please don't. It doesn't bring any value and makes this small driver harder to read. If you need shorten lines, simple use temporary variable. ... > +static int cw_read(struct cw_battery *cw_bat, u8 reg, u8 *val) > +{ > + u32 tmp; > + int ret; > + > + ret = regmap_read(cw_bat->regmap, reg, &tmp); > + *val = tmp; > + return ret; > +} > + > +static int cw_write(struct cw_battery *cw_bat, u8 reg, const u8 val) > +{ > + return regmap_write(cw_bat->regmap, reg, val); > +} This two has no value. Why? ... > +static int cw_read_word(struct cw_battery *cw_bat, u8 reg, u16 *val) > +{ > + u8 reg_val[2]; > + int ret; > + > + ret = regmap_raw_read(cw_bat->regmap, reg, reg_val, 2); > + *val = (reg_val[0] << 8) + reg_val[1]; > + return ret; > +} NIH BE type of readings? Can REGMAP_ENDIAN_BIG help? > +static int cw_read_bulk(struct cw_battery *cw_bat, u8 reg, u8 *buf, size_t len) > +{ > + return regmap_raw_read(cw_bat->regmap, reg, buf, len); > +} No value. > +static int cw_write_bulk(struct cw_battery *cw_bat, u8 reg, u8 const *buf, > + size_t len) > +{ > + return regmap_raw_write(cw_bat->regmap, reg, buf, len); > +} Ditto. > +int cw_update_config_info(struct cw_battery *cw_bat) > +{ > + int ret; > + u8 reg_val; > + u8 reset_val; > + > + if (!cw_bat->bat_config_info) { > + cw_err(cw_bat, > + "No battery config info provided, can't update flash contents"); > + return -EINVAL; > + } > + > + /* make sure gauge is not in sleep mode */ > + ret = cw_read(cw_bat, CW2015_REG_MODE, ®_val); > + if (ret < 0) > + return ret; > + > + reset_val = reg_val; > + if ((reg_val & CW2015_MODE_SLEEP_MASK) == CW2015_MODE_SLEEP) { > + cw_err(cw_bat, > + "Device is in sleep mode, can't update battery info"); > + return -EINVAL; > + } > + > + /* write new battery info */ > + ret = cw_write_bulk(cw_bat, CW2015_REG_BATINFO, > + cw_bat->bat_config_info, > + CW2015_SIZE_BATINFO); > + Redundant line. > + if (ret < 0) > + return ret; All above comparison (and a lot below) are against negative return code, have you checked if positive is ever possible (AFAIR no, they are not possible). > + reg_val |= CW2015_CONFIG_UPDATE_FLG; /* set UPDATE_FLAG */ > + reg_val &= ~CW2015_MASK_ATHD; /* clear alert level */ > + reg_val |= CW2015_ATHD(cw_bat->alert_level); /* set alert level */ All comments can be replaced with one on top of three /* Update alert level */ > + ret = cw_write(cw_bat, CW2015_REG_CONFIG, reg_val); > + if (ret < 0) > + return ret; > + > + /* reset */ Useless. > + reset_val &= ~(CW2015_MODE_RESTART); Too many parentheses. Note all my comments should be applied to all similar places in the code, even if I didn't comment them explicitly. > + reg_val = reset_val | CW2015_MODE_RESTART; > + ret = cw_write(cw_bat, CW2015_REG_MODE, reg_val); > + if (ret < 0) > + return ret; > + > + msleep(20); > + ret = cw_write(cw_bat, CW2015_REG_MODE, reset_val); > + if (ret < 0) > + return ret; > + > + cw_dbg(cw_bat, "Battery config updated"); > + > + return 0; > +} ... I didn't review some parts because of style issues. Please, fix all of them and send v2. ... > + msleep(20); This big delay has to be explained. ... > + } else if (charging_5_loop != 0) { Useless conditional. > + charging_5_loop = 0; > + } ... > + voltage = avg * 312 / 1024; All calculations like that better to be explained (like: "Data sheet provides a formula to normalize raw value ... bla bla bla"). ... > + else if (cw_bat->capacity != cw_capacity) { > + cw_bat->capacity = cw_capacity; > + else if (cw_bat->voltage != ret) > + cw_bat->voltage = ret; Too many useless conditionals. ... > + // calculate remaining capacity > + // estimate current based on time to empty (in minutes) You need to choose one comment style over the code. ... > +#ifdef CONFIG_OF You didn't have of.h in the inclusion block, why do you have this? Hint: you can easily drop this dependency by using fwnode / device property API. > +static int cw2015_parse_dt(struct cw_battery *cw_bat) > +{ > + struct device *dev = &cw_bat->client->dev; > + struct device_node *node = dev->of_node; > + struct property *prop; > + int length; > + u32 value; > + int ret; > + > + if (!node) > + return -ENODEV; > + > + prop = of_find_property(node, PREFIX"bat-config-info", &length); No, no, please always spell explicitly name of nodes / properties / etc. > + if (prop) { > + if (length != CW2015_SIZE_BATINFO) { > + cw_err(cw_bat, "bat-config-info must be %d bytes", > + CW2015_SIZE_BATINFO); > + return -EINVAL; > + } > + > + cw_bat->bat_config_info = > + devm_kzalloc(dev, CW2015_SIZE_BATINFO, GFP_KERNEL); > + if (!cw_bat->bat_config_info) { > + cw_err(cw_bat, > + "Failed to allocate memory for battery config info"); > + return -ENOMEM; > + } > + > + ret = of_property_read_u8_array(node, PREFIX"bat-config-info", > + cw_bat->bat_config_info, > + CW2015_SIZE_BATINFO); > + if (ret < 0) > + return ret; > + } else > + cw_warn(cw_bat, > + "No bat-config-info found, rolling with current flash contents"); > + > + cw_bat->monitor_sec = CW2015_DEFAULT_MONITOR_MS; > + > + ret = of_property_read_u32(node, PREFIX"monitor-interval-ms", &value); > + if (ret >= 0) { > + cw_dbg(cw_bat, "Overriding default monitor-interval with %u ms", > + value); > + cw_bat->monitor_sec = value; > + } > + > + return 0; > +} > +#else > +static int cw2015_parse_dt(struct cw_battery *cw_bat) > +{ > + return -ENODEV; > +} > +#endif ... > +static const struct regmap_range regmap_ranges_wr_yes[] = { > + regmap_reg_range(CW2015_REG_RRT_ALERT, CW2015_REG_CONFIG), > + regmap_reg_range(CW2015_REG_MODE, CW2015_REG_MODE), > + regmap_reg_range(CW2015_REG_BATINFO, CW2015_REG_BATINFO + > + CW2015_SIZE_BATINFO - 1), Much better to split differently, i.e. regmap_reg_range(CW2015_REG_BATINFO, CW2015_REG_BATINFO + CW2015_SIZE_BATINFO - 1), > +}; ... > + cw_dbg(cw_bat, "cw2015/cw2013 driver probe success"); Noise. ... > +#ifdef CONFIG_PM Can be replaced by __maybe_unused attribute to the below functions. > +static int cw_bat_suspend(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct cw_battery *cw_bat = i2c_get_clientdata(client); > + > + ktime_get_boottime_ts64(&cw_bat->suspend_time_before); > + cancel_delayed_work_sync(&cw_bat->battery_delay_work); > + return 0; > +} > + > +static int cw_bat_resume(struct device *dev) > +{ > + struct i2c_client *client = to_i2c_client(dev); > + struct cw_battery *cw_bat = i2c_get_clientdata(client); > + > + cw_bat->suspend_resume_mark = 1; > + ktime_get_boottime_ts64(&cw_bat->after); > + cw_bat->after = timespec64_sub(cw_bat->after, > + cw_bat->suspend_time_before); > + queue_delayed_work(cw_bat->battery_workqueue, > + &cw_bat->battery_delay_work, msecs_to_jiffies(2)); > + return 0; > +} > + > +static const struct dev_pm_ops cw_bat_pm_ops = { > + .suspend = cw_bat_suspend, > + .resume = cw_bat_resume, > +}; Use proper helper macro from pm.h instead of above. > +#endif ... > + cw_dbg(cw_bat, "Removing device"); Noise. > + cancel_delayed_work(&cw_bat->battery_delay_work); Are you sure you have no scheduled work after that? ... > +static const struct of_device_id cw2015_of_match[] = { > + { .compatible = PREFIX"cw2015" }, > + { }, No need to have comma for terminator line. > +}; ... > +#ifdef CONFIG_PM > + .pm = &cw_bat_pm_ops, > +#endif Drop this ifdefery and use proper helper macro above.
Hi Andy, thanks for your review. Please find my comments inline. >> +#define CW2015_DEFAULT_MONITOR_MS 8000 > > 8 seconds? Any comments about this? > Default suggested by CellWise Android example code. I'll add a comment explaining that. >> +static int cw_read(struct cw_battery *cw_bat, u8 reg, u8 *val) >> +{ >> + u32 tmp; >> + int ret; >> + >> + ret = regmap_read(cw_bat->regmap, reg, &tmp); >> + *val = tmp; >> + return ret; >> +} >> + [ ... ] > > This two has no value. Why? > This helper translates the memory write size. An u8 * is passed in while regmap_read takes an unsigned int *. I'll drop it and just use an unsigned int in the first place though. >> +static int cw_read_word(struct cw_battery *cw_bat, u8 reg, u16 *val) >> +{ >> + u8 reg_val[2]; >> + int ret; >> + >> + ret = regmap_raw_read(cw_bat->regmap, reg, reg_val, 2); >> + *val = (reg_val[0] << 8) + reg_val[1]; >> + return ret; >> +} > > NIH BE type of readings? Can REGMAP_ENDIAN_BIG help? Not really, or can it? Registers on the cw2015 are a wild mix of single bytes and words. There does not seem to be a per register override for reg_/val_bits. > > I didn't review some parts because of style issues. Please, fix all of them and > send v2. > I'll fix all style issues I find. >> + cancel_delayed_work(&cw_bat->battery_delay_work); > > Are you sure you have no scheduled work after that? > Will replace that with cancel_delayed_work_sync. Best Regards, Tobias
On Tue, Mar 10, 2020 at 07:55:42PM +0100, Tobias Schramm wrote: > >> +static int cw_read_word(struct cw_battery *cw_bat, u8 reg, u16 *val) > >> +{ > >> + u8 reg_val[2]; > >> + int ret; > >> + > >> + ret = regmap_raw_read(cw_bat->regmap, reg, reg_val, 2); > >> + *val = (reg_val[0] << 8) + reg_val[1]; > >> + return ret; > >> +} > > > > NIH BE type of readings? Can REGMAP_ENDIAN_BIG help? > > Not really, or can it? Registers on the cw2015 are a wild mix of single > bytes and words. There does not seem to be a per register override for > reg_/val_bits. I see. Perhaps __be16 value; ret = regmap(..., (...)value, sizeof(value)); if (ret) return ret; // note, you missed this in above. *val = be16_to_cpu(value); return 0;
Hi Andy, was just about to send v3. > > __be16 value; > > > ret = regmap(..., (...)value, sizeof(value)); > if (ret) > return ret; // note, you missed this in above. > > *val = be16_to_cpu(value); > return 0; > That implementation looks pretty clean. I'll take it. Thanks, Tobias
On Wed, Mar 11, 2020 at 09:48:30AM +0100, Tobias Schramm wrote: > was just about to send v3. > > > > > __be16 value; > > > > > > ret = regmap(..., (...)value, sizeof(value)); Just checked this, simple &value works. > > if (ret) > > return ret; // note, you missed this in above. > > > > *val = be16_to_cpu(value); > > return 0; > > > > That implementation looks pretty clean. I'll take it. Thank you.
This patchset adds support for the CellWise cw2015 fuel gauge. The CellWise cw2015 fuel gauge is a shuntless, single-cell Li-Ion fuel gauge. It is used in the pine64 Pinebook Pro laptop. This is v2 of a patchset I sent in late 2019. Took me some time to get around to prepare a v2 but here it is. I've kept the cellwise,bat-config-info property in the device tree. Its content describes characteristics of the battery built into a device. The exact format is unknown and not publicly documented. It is likely composed of some key parameters of the battery (chemistry, voltages, design capacity) and parameters for tuning the internal state of charge approximation function. In general I'm not 100 % sure about my json-schema binding for the gauge. It is my first time ever writing a json-schema binding and I'm not sure whether properties like power-supplies or monitored-battery need to be added to a separate, common schema for power supplies or not. Best regards, Tobias Schramm Changelog: v2: * Change subject to "Add support for CellWise cw2015 fuel gauge" * Rewrite bindings as json-schema * Use default power-supplies handling * Use regmap for register access * Use standard simple-battery node * Replace printk/pr_* by dev_{dbg,info,warn,err} * Use cancel_delayed_work_sync in remove * General code cleanup Tobias Schramm (2): dt-bindings: power: supply: cw2015_battery: add device tree binding documentation power: supply: add CellWise cw2015 fuel gauge driver .../bindings/power/supply/cw2015_battery.yaml | 84 ++ MAINTAINERS | 6 + drivers/power/supply/Kconfig | 6 + drivers/power/supply/Makefile | 1 + drivers/power/supply/cw2015_battery.c | 891 ++++++++++++++++++ 5 files changed, 988 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml create mode 100644 drivers/power/supply/cw2015_battery.c