| Message ID | 20200312222448.25097-1-t.schramm@manjaro.org |
|---|---|
| Headers | show |
| Series | Add support for CellWise cw2015 fuel gauge | expand |
On Thu, Mar 12, 2020 at 11:24:48PM +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. ... > +config BATTERY_CW2015 > + tristate "CW2015 Battery driver" > + depends on I2C > + select REGMAP_I2C > + help > + Say Y here to enable support for the cellwise cw2015 > + battery fuel gauge (used in the Pinebook Pro & others) AFAIK, it's a good tone to include module name in help string. ... > +#include <linux/bits.h> > +#include <linux/delay.h> > +#include <linux/i2c.h> > +#include <linux/init.h> > +#include <linux/gpio/consumer.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/power_supply.h> > +#include <linux/property.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/time.h> > +#include <linux/timekeeping.h> > +#include <linux/workqueue.h> Can you, please, double check if all of this are indeed have users in the driver? ... > + dev_err(cw_bat->dev, > + "Failed to upload battery info\n"); Indentation of the second line. ... > + ret = regmap_raw_read(cw_bat->regmap, CW2015_REG_BATINFO, > + bat_info, CW2015_SIZE_BATINFO); Ditto. > + dev_err(cw_bat->dev, > + "Failed to read stored battery profile\n"); Ditto. > + dev_warn(cw_bat->dev, > + "Can't check current battery profile, no profile provided"); Ditto. ... > + dev_warn(cw_bat->dev, > + "Too many invalid SoC reports, resetting gauge"); Ditto. > + dev_warn(cw_bat->dev, > + "SoC stuck @%u%%, resetting gauge", soc); Ditto. ... > +static void cw_bat_work(struct work_struct *work) > +{ > + struct delayed_work *delay_work; > + struct cw_battery *cw_bat; > + int ret; > + unsigned int reg_val; > + int i = 0; Looks like redundant assignment. > + delay_work = to_delayed_work(work); > + cw_bat = container_of(delay_work, struct cw_battery, battery_delay_work); > + ret = regmap_read(cw_bat->regmap, CW2015_REG_MODE, ®_val); > + if (ret) { > + dev_err(cw_bat->dev, "Failed to read mode from gauge: %d", ret); > + } else { > + if ((reg_val & CW2015_MODE_SLEEP_MASK) == CW2015_MODE_SLEEP) { > + for (i = 0; i < CW2015_RESET_TRIES; i++) { > + if (!cw_power_on_reset(cw_bat)) > + break; > + } > + } > + cw_update_soc(cw_bat); > + cw_update_voltage(cw_bat); > + cw_update_charge_status(cw_bat); > + cw_update_status(cw_bat); > + cw_update_time_to_empty(cw_bat); > + } > + if (cw_bat->battery_changed) > + power_supply_changed(cw_bat->rk_bat); > + cw_bat->battery_changed = false; > + > + queue_delayed_work(cw_bat->battery_workqueue, > + &cw_bat->battery_delay_work, > + msecs_to_jiffies(cw_bat->poll_interval_ms)); > +} ... > +static int cw_battery_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + int ret = 0; Don't see how this is being used. > + struct cw_battery *cw_bat; > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + if (cw_battery_valid_time_to_empty(cw_bat) && > + cw_bat->battery.charge_full_design_uah > 0) { > + /* calculate remaining capacity */ > + val->intval = cw_bat->battery.charge_full_design_uah; > + val->intval = val->intval * cw_bat->soc / 100; > + > + /* estimate current based on time to empty */ > + val->intval = 60 * val->intval / cw_bat->time_to_empty; > + } else { > + val->intval = 0; > + } > + > + break; > + > + default: > + break; > + } > + return ret; > +} ... > +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_count_u8(dev, "cellwise,battery-profile"); > + if (length) { Now it seems we have an issue here because return value can be negative error code. And I'm thinking that we may refactor this function. So, length = ..._count_u8(...); if (length < 0) { dev_warn(...); } else if (length != ...) { dev_err(...); ... } else { ... } > + 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, length, GFP_KERNEL); > + 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, > + length); > + if (ret) > + return ret; > + } else { > + dev_warn(cw_bat->dev, > + "No battery-profile found, rolling with current flash contents"); > + } > + ...and here... > + cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS; > + ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &value); > + if (ret >= 0) { > + dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms", > + value); > + cw_bat->poll_interval_ms = value; > + } ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &cw_bat->poll_interval_ms); if (ret) { dev_dbg(cw_bat->dev, "Use default\n"); cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS; } What do you think? > + > + return 0; > +} ... > +static int cw_bat_probe(struct i2c_client *client, > + const struct i2c_device_id *id) By the way, can you switch to ->probe_new() ? > +{ > + int ret; > + struct cw_battery *cw_bat; > + struct power_supply_config psy_cfg = { }; Don't you need 0 here? > + psy_cfg.drv_data = cw_bat; > + psy_cfg.fwnode = dev_fwnode(cw_bat->dev); > + > + 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 PTR_ERR(cw_bat->rk_bat); > + } > + > + ret = power_supply_get_battery_info(cw_bat->rk_bat, &cw_bat->battery); > + if (ret) { > + dev_warn(cw_bat->dev, > + "No monitored battery, some properties will be missing"); '\n' is missed (please, check all messages. > + } > + > + cw_bat->battery_workqueue = create_singlethread_workqueue("rk_battery"); > + INIT_DELAYED_WORK(&cw_bat->battery_delay_work, cw_bat_work); > + queue_delayed_work(cw_bat->battery_workqueue, > + &cw_bat->battery_delay_work, msecs_to_jiffies(10)); > + return 0; > +}
Hi Andy, thanks for your feedback. Please find my comments inline. > >> + dev_err(cw_bat->dev, >> + "Failed to upload battery info\n"); > > Indentation of the second line. > I've seen quite a few different indentation styles used in kernel source. Personally I'd indent like this: dev_warn(cw_bat->dev, "some long error message"); However coding-style.rst specifies that spaces are never to be used for indentation. May I assume they are ok for alignment though? > And I'm thinking that we may refactor this function. So, > > length = ..._count_u8(...); > if (length < 0) { > dev_warn(...); > } else if (length != ...) { > dev_err(...); > ... > } else { > ... > } > > > >> + 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, length, GFP_KERNEL); >> + 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, >> + length); >> + if (ret) >> + return ret; >> + } else { >> + dev_warn(cw_bat->dev, >> + "No battery-profile found, rolling with current flash contents"); >> + } >> + > > ...and here... > >> + cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS; >> + ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &value); >> + if (ret >= 0) { >> + dev_dbg(cw_bat->dev, "Overriding default monitor-interval with %u ms", >> + value); >> + cw_bat->poll_interval_ms = value; >> + } > > ret = device_property_read_u32(dev, "cellwise,monitor-interval-ms", &cw_bat->poll_interval_ms); > if (ret) { > dev_dbg(cw_bat->dev, "Use default\n"); > cw_bat->poll_interval_ms = CW2015_DEFAULT_POLL_INTERVAL_MS; > } > > What do you think? > Looks good to me. I'll use it just like that. > >> + int ret; >> + struct cw_battery *cw_bat; > >> + struct power_supply_config psy_cfg = { }; > > Don't you need 0 here? > Yeah, that would be better. Think empty initializers are a GNU extension. Best Regards, Tobias
Hi Tobias. On Sun, Mar 15, 2020 at 12:00:35PM +0100, Tobias Schramm wrote: > Hi Andy, > > thanks for your feedback. Please find my comments inline. > > > > >> + dev_err(cw_bat->dev, > >> + "Failed to upload battery info\n"); > > > > Indentation of the second line. > > > I've seen quite a few different indentation styles used in kernel > source. Personally I'd indent like this: > > dev_warn(cw_bat->dev, > "some long error message"); > > However coding-style.rst specifies that spaces are never to be used for > indentation. May I assume they are ok for alignment though? Indent with tabs and align with spaces. So this becomes < tab >< tab >dev_warn(cw_bat->dev, < tab >< tab ><tab >_"some long error message"); Where '_' represents a space. This is the recommend kernel practice. Sam
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 v4 of the patchset. This version incorporates an additional review by Andy and a small spelling fix. I've kept the cellwise,battery-profile 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 comprised of some key parameters of the battery (chemistry, voltages, design capacity) and parameters for tuning the internal state of charge approximation function. Since v2 CellWise has confirmed to me that the only way to obtain the profile blob is to mail them batteries for testing. Thus we will need to keep that property. 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 v3: * Incorporate review by Andy * Add cellwise vendor prefix * Rename cellwise,bat-config-info property to cellwise,battery-profile * Remove most state of charge post-processing * Use fwnode interface * General code cleanup * Lots of code style fixes v4: * Implement additional changes requested by Andy * Use fwnode inline wrappers * Clean up waiting for gauge * Minor code style fixes Tobias Schramm (3): dt-bindings: Document cellwise vendor-prefix dt-bindings: power: supply: add cw2015_battery bindings power: supply: add CellWise cw2015 fuel gauge driver .../bindings/power/supply/cw2015_battery.yaml | 83 ++ .../devicetree/bindings/vendor-prefixes.yaml | 2 + MAINTAINERS | 6 + drivers/power/supply/Kconfig | 8 + drivers/power/supply/Makefile | 1 + drivers/power/supply/cw2015_battery.c | 759 ++++++++++++++++++ 6 files changed, 859 insertions(+) create mode 100644 Documentation/devicetree/bindings/power/supply/cw2015_battery.yaml create mode 100644 drivers/power/supply/cw2015_battery.c