From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liam Breck Subject: Re: [PATCH v13.2 08/11] power: supply: bq27xxx_battery: Add power_supply_battery_info support Date: Tue, 9 May 2017 03:18:30 -0700 Message-ID: References: <20170509082123.27592-1-liam@networkimprov.net> <20170509082123.27592-3-liam@networkimprov.net> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: Received: from mail-it0-f68.google.com ([209.85.214.68]:33098 "EHLO mail-it0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751115AbdEIKSb (ORCPT ); Tue, 9 May 2017 06:18:31 -0400 Received: by mail-it0-f68.google.com with SMTP id v135so9936790itv.0 for ; Tue, 09 May 2017 03:18:31 -0700 (PDT) In-Reply-To: <20170509082123.27592-3-liam@networkimprov.net> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Sebastian Reichel , "Andrew F. Davis" , linux-pm@vger.kernel.org Cc: Liam Breck Hi Sebastian, This patch only allows update of fuel gauge flash/NVM data if the kernel is built with CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM=y and a dtb with monitored-battery config is packaged with the kernel. We expect some device vendors to field-upgrade the fuel gauge this way. The patch provides a module param dt_monitored_battery_updates_nvm (default true) so the user can disable flash/NVM updates if he changes the battery type. The param has no effect if config option is not =y. Andrew is concerned that a distro could "by accident" enable the config option and ship a dtb which causes some smart batteries to be misconfigured. He believes that the module param should default false to prevent this. However, that can cause the following scenario: 1) user changes battery type, sets param dt_monitored_battery_updates_nvm=0 2) device vendor ships gauge update in kernel package with /etc/modprobe.d/xyz containing dt_monitored_battery_updates_nvm=1 3) gauge stops working correctly due to incompatible update So making the module param default false is a bug. I think the likelihood of a distro shipping a kernel package with config option =y and a dtb with monitored-battery config where it does not belong is vanishingly small. But to prevent a config accident we could add a second option which also must be =y. Some more comments inline below... What do you think? On Tue, May 9, 2017 at 1:21 AM, Liam Breck wrote: > From: Liam Breck > > Previously there was no way to configure these chips in the event that the > defaults didn't match the battery in question. > > For chips with RAM data memory (and also those with flash/NVM data memory > if CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM is defined and the user has not > set module param dt_monitored_battery_updates_nvm=0) we now call > power_supply_get_battery_info(), check its values, and write battery > properties to chip data memory if there is a dm_regs table for the chip. > > Signed-off-by: Matt Ranostay > Signed-off-by: Liam Breck > --- > drivers/power/supply/Kconfig | 11 ++ > drivers/power/supply/bq27xxx_battery.c | 204 ++++++++++++++++++++++++++++++++- > include/linux/power/bq27xxx_battery.h | 2 + > 3 files changed, 216 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 76806a0..38fae10 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -178,6 +178,17 @@ config BATTERY_BQ27XXX_I2C > Say Y here to enable support for batteries with BQ27xxx chips > connected over an I2C bus. > > +config BATTERY_BQ27XXX_DT_UPDATES_NVM > + bool "BQ27xxx support for update of NVM/flash data memory" > + depends on BATTERY_BQ27XXX_I2C > + help > + Say Y here to enable devicetree monitored-battery config to update > + NVM/flash data memory. Only enable this option for devices with a > + fuel gauge mounted on the circuit board, and a battery that cannot > + easily be replaced with one of a different type. Not for > + general-purpose kernels, as this can cause misconfiguration of a > + smart battery with embedded NVM/flash. Above I have documented why distros should not set this option. > config BATTERY_DA9030 > tristate "DA9030 battery driver" > depends on PMIC_DA903X > diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c > index 6a4ac14..ed44439 100644 > --- a/drivers/power/supply/bq27xxx_battery.c > +++ b/drivers/power/supply/bq27xxx_battery.c > @@ -805,6 +805,13 @@ static LIST_HEAD(bq27xxx_battery_devices); > > #define BQ27XXX_DM_SZ 32 > > +struct bq27xxx_dm_reg { > + u8 subclass_id; > + u8 offset; > + u8 bytes; > + u16 min, max; > +}; > + > /** > * struct bq27xxx_dm_buf - chip data memory buffer > * @class: data memory subclass_id > @@ -822,6 +829,44 @@ struct bq27xxx_dm_buf { > bool has_data, dirty; > }; > > +#define BQ27XXX_DM_BUF(di, i) { \ > + .class = (di)->dm_regs[i].subclass_id, \ > + .block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \ > +} > + > +static inline u16 *bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf, > + struct bq27xxx_dm_reg *reg) > +{ > + if (buf->class == reg->subclass_id && > + buf->block == reg->offset / BQ27XXX_DM_SZ) > + return (u16 *) (buf->data + reg->offset % BQ27XXX_DM_SZ); > + > + return NULL; > +} > + > +enum bq27xxx_dm_reg_id { > + BQ27XXX_DM_DESIGN_CAPACITY = 0, > + BQ27XXX_DM_DESIGN_ENERGY, > + BQ27XXX_DM_TERMINATE_VOLTAGE, > +}; > + > +static const char * const bq27xxx_dm_reg_name[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity", > + [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy", > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", > +}; > + > + > +static bool bq27xxx_dt_to_nvm = true; > +module_param_named(dt_monitored_battery_updates_nvm, bq27xxx_dt_to_nvm, bool, 0444); > +MODULE_PARM_DESC(dt_monitored_battery_updates_nvm, > + "Devicetree monitored-battery config updates data memory on NVM/flash chips.\n" > + "Users must set this =0 when installing a different type of battery!\n" > + "Default is =1." > +#ifndef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM > + "\nSetting this affects future kernel updates, not the current configuration." > +#endif > +); The module param has no effect when config option is not set, but I define the param so the user can see the docs for it with modinfo either way. > static int poll_interval_param_set(const char *val, const struct kernel_param *kp) > { > @@ -1019,6 +1064,55 @@ static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di, > return ret; > } > > +static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di, > + struct bq27xxx_dm_buf *buf, > + enum bq27xxx_dm_reg_id reg_id, > + unsigned int val) > +{ > + struct bq27xxx_dm_reg *reg = &di->dm_regs[reg_id]; > + const char *str = bq27xxx_dm_reg_name[reg_id]; > + u16 *prev = bq27xxx_dm_reg_ptr(buf, reg); > + > + if (prev == NULL) { > + dev_warn(di->dev, "buffer does not match %s dm spec\n", str); > + return; > + } > + > + if (reg->bytes != 2) { > + dev_warn(di->dev, "%s dm spec has unsupported byte size\n", str); > + return; > + } > + > + if (!buf->has_data) > + return; > + > + if (be16_to_cpup(prev) == val) { > + dev_info(di->dev, "%s has %u\n", str, val); > + return; > + } > + > +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM > + if (!di->ram_chip && !bq27xxx_dt_to_nvm) { > +#else > + if (!di->ram_chip) { > +#endif > + /* devicetree and NVM differ; defer to NVM */ > + dev_warn(di->dev, "%s has %u; update to %u disallowed " > +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM > + "by dt_monitored_battery_updates_nvm=0" > +#else > + "for flash/NVM data memory" > +#endif > + "\n", str, be16_to_cpup(prev), val); > + return; > + } The above is how the config option and module param control NVM update. > + dev_info(di->dev, "update %s to %u\n", str, val); > + > + *prev = cpu_to_be16(val); > + buf->dirty = true; > +} > + > static int bq27xxx_battery_cfgupdate_priv(struct bq27xxx_device_info *di, bool active) > { > const int limit = 100; > @@ -1129,6 +1223,103 @@ static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di, > return ret; > } > > +static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di, > + struct power_supply_battery_info *info) > +{ > + struct bq27xxx_dm_buf bd = BQ27XXX_DM_BUF(di, BQ27XXX_DM_DESIGN_CAPACITY); > + struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE); > + bool updated; > + > + if (bq27xxx_battery_unseal(di) < 0) > + return; > + > + if (info->charge_full_design_uah != -EINVAL && > + info->energy_full_design_uwh != -EINVAL) { > + bq27xxx_battery_read_dm_block(di, &bd); > + /* assume design energy & capacity are in same block */ > + bq27xxx_battery_update_dm_block(di, &bd, > + BQ27XXX_DM_DESIGN_CAPACITY, > + info->charge_full_design_uah / 1000); > + bq27xxx_battery_update_dm_block(di, &bd, > + BQ27XXX_DM_DESIGN_ENERGY, > + info->energy_full_design_uwh / 1000); > + } > + > + if (info->voltage_min_design_uv != -EINVAL) { > + bool same = bd.class == bt.class && bd.block == bt.block; > + if (!same) > + bq27xxx_battery_read_dm_block(di, &bt); > + bq27xxx_battery_update_dm_block(di, same ? &bd : &bt, > + BQ27XXX_DM_TERMINATE_VOLTAGE, > + info->voltage_min_design_uv / 1000); > + } > + > + updated = bd.dirty || bt.dirty; > + > + bq27xxx_battery_write_dm_block(di, &bd); > + bq27xxx_battery_write_dm_block(di, &bt); > + > + bq27xxx_battery_seal(di); > + > + if (updated && di->chip != BQ27421) { /* not a cfgupdate chip, so reset */ > + bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false); > + BQ27XXX_MSLEEP(300); /* reset time is not documented */ > + } > + /* assume bq27xxx_battery_update() is called hereafter */ > +} > + > +static void bq27xxx_battery_settings(struct bq27xxx_device_info *di) > +{ > + struct power_supply_battery_info info = {}; > + unsigned int min, max; > + > + if (power_supply_get_battery_info(di->bat, &info) < 0) > + return; > + > + if (!di->dm_regs) { > + dev_warn(di->dev, "data memory update not supported for chip\n"); > + return; > + } > + > + if (info.energy_full_design_uwh != info.charge_full_design_uah) { > + if (info.energy_full_design_uwh == -EINVAL) > + dev_warn(di->dev, "missing battery:energy-full-design-microwatt-hours\n"); > + else if (info.charge_full_design_uah == -EINVAL) > + dev_warn(di->dev, "missing battery:charge-full-design-microamp-hours\n"); > + } > + > + /* assume min == 0 */ > + max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max; > + if (info.energy_full_design_uwh > max * 1000) { > + dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n", > + info.energy_full_design_uwh); > + info.energy_full_design_uwh = -EINVAL; > + } > + > + /* assume min == 0 */ > + max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max; > + if (info.charge_full_design_uah > max * 1000) { > + dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n", > + info.charge_full_design_uah); > + info.charge_full_design_uah = -EINVAL; > + } > + > + min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min; > + max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max; > + if ((info.voltage_min_design_uv < min * 1000 || > + info.voltage_min_design_uv > max * 1000) && > + info.voltage_min_design_uv != -EINVAL) { > + dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n", > + info.voltage_min_design_uv); > + info.voltage_min_design_uv = -EINVAL; > + } > + > + if ((info.energy_full_design_uwh != -EINVAL && > + info.charge_full_design_uah != -EINVAL) || > + info.voltage_min_design_uv != -EINVAL) > + bq27xxx_battery_set_config(di, &info); > +} > + > /* > * Return the battery State-of-Charge > * Or < 0 if something fails. > @@ -1646,6 +1837,13 @@ static int bq27xxx_battery_get_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: > ret = bq27xxx_simple_value(di->charge_design_full, val); > break; > + /* > + * TODO: Implement these to make registers set from > + * power_supply_battery_info visible in sysfs. > + */ > + case POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN: > + case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN: > + return -EINVAL; > case POWER_SUPPLY_PROP_CYCLE_COUNT: > ret = bq27xxx_simple_value(di->cache.cycle_count, val); > break; > @@ -1679,7 +1877,10 @@ static void bq27xxx_external_power_changed(struct power_supply *psy) > int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > { > struct power_supply_desc *psy_desc; > - struct power_supply_config psy_cfg = { .drv_data = di, }; > + struct power_supply_config psy_cfg = { > + .of_node = di->dev->of_node, > + .drv_data = di, > + }; > > INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll); > mutex_init(&di->lock); > @@ -1704,6 +1905,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > > dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION); > > + bq27xxx_battery_settings(di); > bq27xxx_battery_update(di); > > mutex_lock(&bq27xxx_list_lock); > diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h > index b1defb8..11e1168 100644 > --- a/include/linux/power/bq27xxx_battery.h > +++ b/include/linux/power/bq27xxx_battery.h > @@ -63,7 +63,9 @@ struct bq27xxx_device_info { > struct device *dev; > int id; > enum bq27xxx_chip chip; > + bool ram_chip; > const char *name; > + struct bq27xxx_dm_reg *dm_regs; > u32 unseal_key; > struct bq27xxx_access_methods bus; > struct bq27xxx_reg_cache cache; > -- > 2.9.3 >