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.