[v13,08/11] power: supply: bq27xxx_battery: Add power_supply_battery_info support

Commit Message

Liam Breck May 4, 2017, 6:18 a.m. UTC

From: Liam Breck <[e-mail]>

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 <[e-mail]>
Signed-off-by: Liam Breck <[e-mail]>
---
 drivers/power/supply/Kconfig           |   9 ++
 drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
 include/linux/power/bq27xxx_battery.h  |   2 +
 3 files changed, 209 insertions(+), 1 deletion(-)

Comments

Andrew Davis May 4, 2017, 4:52 p.m. UTC | #1

On 05/04/2017 01:18 AM, Liam Breck wrote:
> From: Liam Breck <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Liam Breck <[e-mail]>
> ---
>  drivers/power/supply/Kconfig           |   9 ++
>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>  include/linux/power/bq27xxx_battery.h  |   2 +
>  3 files changed, 209 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
> index 76806a0..85e2fb2 100644
> --- a/drivers/power/supply/Kconfig
> +++ b/drivers/power/supply/Kconfig
> @@ -178,6 +178,15 @@ 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
> +	  be replaced with one of a different type.
> +
>  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 8ab184c..06f15da 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,43 @@ 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",
> +};
> +
> +
> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
> +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 NVM/flash data memory, if present. Default is 1/y.");

As before, the default should not be y, this is a rare case and I can't
see it being useful or wanted by anyone but device manufacturers. I
don't want everyones chip's factory programmed NVM overwritten by
accident when someone compiles and ships a kernel with
BATTERY_BQ27XXX_DT_UPDATES_NVM=y

> +#else
> +static bool bq27xxx_dt_to_nvm = false;
> +#endif
>  
>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>  {
> @@ -1019,6 +1063,51 @@ 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;
> +	}
> +
> +	if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
> +		/* 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;
> +	}
> +
> +	dev_info(di->dev, "update %s to %u\n", str, val);
> +
> +	*prev = cpu_to_be16(val);
> +	buf->dirty = true;
> +}
> +
>  static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
>  {
>  	const int limit = 100;
> @@ -1117,6 +1206,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.
> @@ -1634,6 +1820,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;
> @@ -1667,7 +1860,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);
> @@ -1692,6 +1888,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;
>

Liam Breck May 4, 2017, 6:40 p.m. UTC | #2

On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/04/2017 01:18 AM, Liam Breck wrote:
>> From: Liam Breck <[e-mail]>
>>
>> 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 <[e-mail]>
>> Signed-off-by: Liam Breck <[e-mail]>
>> ---
>>  drivers/power/supply/Kconfig           |   9 ++
>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index 76806a0..85e2fb2 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -178,6 +178,15 @@ 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
>> +       be replaced with one of a different type.
>> +
>>  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 8ab184c..06f15da 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,43 @@ 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",
>> +};
>> +
>> +
>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>> +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 NVM/flash data memory, if present. Default is 1/y.");
>
> As before, the default should not be y, this is a rare case and I can't
> see it being useful or wanted by anyone but device manufacturers. I
> don't want everyones chip's factory programmed NVM overwritten by
> accident when someone compiles and ships a kernel with
> BATTERY_BQ27XXX_DT_UPDATES_NVM=y

Right, the point of the config option is to let a device vendor ship a
kernel + dtb which can field-upgrade NVM. (Setting the option does
nothing absent the necessary dtb.)

Module params are for end-user config; we can't require a device
vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
kernel package update to defeat the end-user config of
dt_monitored_battery_updates_nvm=0 !!

I believe Sebastian agrees, given that he queued this patch :-)

>> +#else
>> +static bool bq27xxx_dt_to_nvm = false;
>> +#endif
>>
>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>  {
>> @@ -1019,6 +1063,51 @@ 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;
>> +     }
>> +
>> +     if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
>> +             /* 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;
>> +     }
>> +
>> +     dev_info(di->dev, "update %s to %u\n", str, val);
>> +
>> +     *prev = cpu_to_be16(val);
>> +     buf->dirty = true;
>> +}
>> +
>>  static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
>>  {
>>       const int limit = 100;
>> @@ -1117,6 +1206,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.
>> @@ -1634,6 +1820,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;
>> @@ -1667,7 +1860,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);
>> @@ -1692,6 +1888,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;
>>

Liam Breck May 8, 2017, 6:16 a.m. UTC | #3

On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>> From: Liam Breck <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Liam Breck <[e-mail]>
>>> ---
>>>  drivers/power/supply/Kconfig           |   9 ++
>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>> index 76806a0..85e2fb2 100644
>>> --- a/drivers/power/supply/Kconfig
>>> +++ b/drivers/power/supply/Kconfig
>>> @@ -178,6 +178,15 @@ 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
>>> +       be replaced with one of a different type.
>>> +
>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>> +};
>>> +
>>> +
>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>
>> As before, the default should not be y, this is a rare case and I can't
>> see it being useful or wanted by anyone but device manufacturers. I
>> don't want everyones chip's factory programmed NVM overwritten by
>> accident when someone compiles and ships a kernel with
>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>
> Right, the point of the config option is to let a device vendor ship a
> kernel + dtb which can field-upgrade NVM. (Setting the option does
> nothing absent the necessary dtb.)
>
> Module params are for end-user config; we can't require a device
> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
> kernel package update to defeat the end-user config of
> dt_monitored_battery_updates_nvm=0 !!
>
> I believe Sebastian agrees, given that he queued this patch :-)

I clarified the rationale for module param default =1 above; could you ack?

That prevents this scenario:
1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
2) vendor ships kernel package with dtb update and
dt_monitored_battery_updates_nvm=1
3) gauge stops working correctly


>>> +#else
>>> +static bool bq27xxx_dt_to_nvm = false;
>>> +#endif
>>>
>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>  {
>>> @@ -1019,6 +1063,51 @@ 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;
>>> +     }
>>> +
>>> +     if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
>>> +             /* 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;
>>> +     }
>>> +
>>> +     dev_info(di->dev, "update %s to %u\n", str, val);
>>> +
>>> +     *prev = cpu_to_be16(val);
>>> +     buf->dirty = true;
>>> +}
>>> +
>>>  static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
>>>  {
>>>       const int limit = 100;
>>> @@ -1117,6 +1206,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.
>>> @@ -1634,6 +1820,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;
>>> @@ -1667,7 +1860,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);
>>> @@ -1692,6 +1888,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;
>>>

Andrew Davis May 8, 2017, 2:54 p.m. UTC | #4

On 05/08/2017 01:16 AM, Liam Breck wrote:
> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>> From: Liam Breck <[e-mail]>
>>>>
>>>> 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 <[e-mail]>
>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>> ---
>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>> index 76806a0..85e2fb2 100644
>>>> --- a/drivers/power/supply/Kconfig
>>>> +++ b/drivers/power/supply/Kconfig
>>>> @@ -178,6 +178,15 @@ 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
>>>> +       be replaced with one of a different type.
>>>> +
>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>> +};
>>>> +
>>>> +
>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>
>>> As before, the default should not be y, this is a rare case and I can't
>>> see it being useful or wanted by anyone but device manufacturers. I
>>> don't want everyones chip's factory programmed NVM overwritten by
>>> accident when someone compiles and ships a kernel with
>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>
>> Right, the point of the config option is to let a device vendor ship a
>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>> nothing absent the necessary dtb.)
>>
>> Module params are for end-user config; we can't require a device
>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>> kernel package update to defeat the end-user config of
>> dt_monitored_battery_updates_nvm=0 !!
>>
>> I believe Sebastian agrees, given that he queued this patch :-)
> 
> I clarified the rationale for module param default =1 above; could you ack?
> 
> That prevents this scenario:
> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
> 2) vendor ships kernel package with dtb update and
> dt_monitored_battery_updates_nvm=1
> 3) gauge stops working correctly
> 

Leaving it =1 will encourage this situation, not prevent it. Updating
NVM is a non-standard operation for these parts. It should only be done
once ideally. It is nice being able to update the NVM, and we have
user-space tools for this, but this is a dangerous operation, improperly
programming can brick the chip, as you have found out.

If you want to add this function into the kernel driver that is fine,
but you *cannot* have this be the default, it needs to be a hidden away
option for people who know what they are doing, else you will end up
accidentally causing damage to countless users' hardware.

> 
>>>> +#else
>>>> +static bool bq27xxx_dt_to_nvm = false;
>>>> +#endif
>>>>
>>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>>  {
>>>> @@ -1019,6 +1063,51 @@ 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;
>>>> +     }
>>>> +
>>>> +     if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
>>>> +             /* 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;
>>>> +     }
>>>> +
>>>> +     dev_info(di->dev, "update %s to %u\n", str, val);
>>>> +
>>>> +     *prev = cpu_to_be16(val);
>>>> +     buf->dirty = true;
>>>> +}
>>>> +
>>>>  static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
>>>>  {
>>>>       const int limit = 100;
>>>> @@ -1117,6 +1206,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.
>>>> @@ -1634,6 +1820,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;
>>>> @@ -1667,7 +1860,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);
>>>> @@ -1692,6 +1888,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;
>>>>

Liam Breck May 8, 2017, 6:39 p.m. UTC | #5

On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/08/2017 01:16 AM, Liam Breck wrote:
>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>> From: Liam Breck <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>> ---
>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>> index 76806a0..85e2fb2 100644
>>>>> --- a/drivers/power/supply/Kconfig
>>>>> +++ b/drivers/power/supply/Kconfig
>>>>> @@ -178,6 +178,15 @@ 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
>>>>> +       be replaced with one of a different type.
>>>>> +
>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>> +};
>>>>> +
>>>>> +
>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>
>>>> As before, the default should not be y, this is a rare case and I can't
>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>> accident when someone compiles and ships a kernel with
>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>
>>> Right, the point of the config option is to let a device vendor ship a
>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>> nothing absent the necessary dtb.)
>>>
>>> Module params are for end-user config; we can't require a device
>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>> kernel package update to defeat the end-user config of
>>> dt_monitored_battery_updates_nvm=0 !!
>>>
>>> I believe Sebastian agrees, given that he queued this patch :-)
>>
>> I clarified the rationale for module param default =1 above; could you ack?
>>
>> That prevents this scenario:
>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>> 2) vendor ships kernel package with dtb update and
>> dt_monitored_battery_updates_nvm=1
>> 3) gauge stops working correctly
>>
>
> Leaving it =1 will encourage this situation, not prevent it. Updating
> NVM is a non-standard operation for these parts. It should only be done
> once ideally. It is nice being able to update the NVM, and we have
> user-space tools for this, but this is a dangerous operation, improperly
> programming can brick the chip, as you have found out.
>
> If you want to add this function into the kernel driver that is fine,
> but you *cannot* have this be the default, it needs to be a hidden away
> option for people who know what they are doing, else you will end up
> accidentally causing damage to countless users' hardware.

I don't think you followed my logic.... It *is* hidden behind a
dedicated kernel config option which only device vendors (and
tinkerers :-) would use. A kernel package with that option and
suitable dtb has to perform the update without manipulation of the
module param. The module param is to let the user prevent any future
update by such a kernel package, since only he knows that he changed
the battery type.

What is your evidence for the assertion of "encourage this situation"?

Andrew Davis May 8, 2017, 6:42 p.m. UTC | #6

On 05/08/2017 01:39 PM, Liam Breck wrote:
> On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/08/2017 01:16 AM, Liam Breck wrote:
>>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>> From: Liam Breck <[e-mail]>
>>>>>>
>>>>>> 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 <[e-mail]>
>>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>>> ---
>>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>>> index 76806a0..85e2fb2 100644
>>>>>> --- a/drivers/power/supply/Kconfig
>>>>>> +++ b/drivers/power/supply/Kconfig
>>>>>> @@ -178,6 +178,15 @@ 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
>>>>>> +       be replaced with one of a different type.
>>>>>> +
>>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>>> +};
>>>>>> +
>>>>>> +
>>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>>
>>>>> As before, the default should not be y, this is a rare case and I can't
>>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>>> accident when someone compiles and ships a kernel with
>>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>>
>>>> Right, the point of the config option is to let a device vendor ship a
>>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>>> nothing absent the necessary dtb.)
>>>>
>>>> Module params are for end-user config; we can't require a device
>>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>>> kernel package update to defeat the end-user config of
>>>> dt_monitored_battery_updates_nvm=0 !!
>>>>
>>>> I believe Sebastian agrees, given that he queued this patch :-)
>>>
>>> I clarified the rationale for module param default =1 above; could you ack?
>>>
>>> That prevents this scenario:
>>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>>> 2) vendor ships kernel package with dtb update and
>>> dt_monitored_battery_updates_nvm=1
>>> 3) gauge stops working correctly
>>>
>>
>> Leaving it =1 will encourage this situation, not prevent it. Updating
>> NVM is a non-standard operation for these parts. It should only be done
>> once ideally. It is nice being able to update the NVM, and we have
>> user-space tools for this, but this is a dangerous operation, improperly
>> programming can brick the chip, as you have found out.
>>
>> If you want to add this function into the kernel driver that is fine,
>> but you *cannot* have this be the default, it needs to be a hidden away
>> option for people who know what they are doing, else you will end up
>> accidentally causing damage to countless users' hardware.
> 
> I don't think you followed my logic.... It *is* hidden behind a
> dedicated kernel config option which only device vendors (and
> tinkerers :-) would use. A kernel package with that option and
> suitable dtb has to perform the update without manipulation of the
> module param. The module param is to let the user prevent any future
> update by such a kernel package, since only he knows that he changed
> the battery type.
> 
> What is your evidence for the assertion of "encourage this situation"?
> 

It is the default value, what more evidence do you want? Someone should
have to enable the functionality in Kconfig *and* enable it as a
module_param. Leaving either on by default will cause it to be
accidentally left enabled.

Liam Breck May 8, 2017, 7:31 p.m. UTC | #7

On Mon, May 8, 2017 at 11:42 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/08/2017 01:39 PM, Liam Breck wrote:
>> On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/08/2017 01:16 AM, Liam Breck wrote:
>>>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>
>>>>>>> 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 <[e-mail]>
>>>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>>>> index 76806a0..85e2fb2 100644
>>>>>>> --- a/drivers/power/supply/Kconfig
>>>>>>> +++ b/drivers/power/supply/Kconfig
>>>>>>> @@ -178,6 +178,15 @@ 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
>>>>>>> +       be replaced with one of a different type.
>>>>>>> +
>>>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>>>> +};
>>>>>>> +
>>>>>>> +
>>>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>>>
>>>>>> As before, the default should not be y, this is a rare case and I can't
>>>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>>>> accident when someone compiles and ships a kernel with
>>>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>>>
>>>>> Right, the point of the config option is to let a device vendor ship a
>>>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>>>> nothing absent the necessary dtb.)
>>>>>
>>>>> Module params are for end-user config; we can't require a device
>>>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>>>> kernel package update to defeat the end-user config of
>>>>> dt_monitored_battery_updates_nvm=0 !!
>>>>>
>>>>> I believe Sebastian agrees, given that he queued this patch :-)
>>>>
>>>> I clarified the rationale for module param default =1 above; could you ack?
>>>>
>>>> That prevents this scenario:
>>>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>>>> 2) vendor ships kernel package with dtb update and
>>>> dt_monitored_battery_updates_nvm=1
>>>> 3) gauge stops working correctly
>>>>
>>>
>>> Leaving it =1 will encourage this situation, not prevent it. Updating
>>> NVM is a non-standard operation for these parts. It should only be done
>>> once ideally. It is nice being able to update the NVM, and we have
>>> user-space tools for this, but this is a dangerous operation, improperly
>>> programming can brick the chip, as you have found out.
>>>
>>> If you want to add this function into the kernel driver that is fine,
>>> but you *cannot* have this be the default, it needs to be a hidden away
>>> option for people who know what they are doing, else you will end up
>>> accidentally causing damage to countless users' hardware.
>>
>> I don't think you followed my logic.... It *is* hidden behind a
>> dedicated kernel config option which only device vendors (and
>> tinkerers :-) would use. A kernel package with that option and
>> suitable dtb has to perform the update without manipulation of the
>> module param. The module param is to let the user prevent any future
>> update by such a kernel package, since only he knows that he changed
>> the battery type.
>>
>> What is your evidence for the assertion of "encourage this situation"?
>>
>
> It is the default value, what more evidence do you want? Someone should
> have to enable the functionality in Kconfig *and* enable it as a
> module_param. Leaving either on by default will cause it to be
> accidentally left enabled.

We could add a second config option to eliminate the possibility of
the first one being set by mistake.

You can't trivially enable a config option. If you set it, you'd
obviously set the module param if that were necessary, so that's no
extra protection against "accidentally left enabled". (And the config
option does nothing without a suitable dtb.)

My scenario is a clear and likely threat. You haven't described a
threat scenario.

The docs for the config option describe its correct use, and I will
mention the risk of overwriting a smart battery. No general purpose
distro will enable it.

Andrew Davis May 8, 2017, 7:50 p.m. UTC | #8

On 05/08/2017 02:31 PM, Liam Breck wrote:
> On Mon, May 8, 2017 at 11:42 AM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/08/2017 01:39 PM, Liam Breck wrote:
>>> On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/08/2017 01:16 AM, Liam Breck wrote:
>>>>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>>>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>
>>>>>>>> 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 <[e-mail]>
>>>>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>>>>> ---
>>>>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>>>>> index 76806a0..85e2fb2 100644
>>>>>>>> --- a/drivers/power/supply/Kconfig
>>>>>>>> +++ b/drivers/power/supply/Kconfig
>>>>>>>> @@ -178,6 +178,15 @@ 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
>>>>>>>> +       be replaced with one of a different type.
>>>>>>>> +
>>>>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +
>>>>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>>>>
>>>>>>> As before, the default should not be y, this is a rare case and I can't
>>>>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>>>>> accident when someone compiles and ships a kernel with
>>>>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>>>>
>>>>>> Right, the point of the config option is to let a device vendor ship a
>>>>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>>>>> nothing absent the necessary dtb.)
>>>>>>
>>>>>> Module params are for end-user config; we can't require a device
>>>>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>>>>> kernel package update to defeat the end-user config of
>>>>>> dt_monitored_battery_updates_nvm=0 !!
>>>>>>
>>>>>> I believe Sebastian agrees, given that he queued this patch :-)
>>>>>
>>>>> I clarified the rationale for module param default =1 above; could you ack?
>>>>>
>>>>> That prevents this scenario:
>>>>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>>>>> 2) vendor ships kernel package with dtb update and
>>>>> dt_monitored_battery_updates_nvm=1
>>>>> 3) gauge stops working correctly
>>>>>
>>>>
>>>> Leaving it =1 will encourage this situation, not prevent it. Updating
>>>> NVM is a non-standard operation for these parts. It should only be done
>>>> once ideally. It is nice being able to update the NVM, and we have
>>>> user-space tools for this, but this is a dangerous operation, improperly
>>>> programming can brick the chip, as you have found out.
>>>>
>>>> If you want to add this function into the kernel driver that is fine,
>>>> but you *cannot* have this be the default, it needs to be a hidden away
>>>> option for people who know what they are doing, else you will end up
>>>> accidentally causing damage to countless users' hardware.
>>>
>>> I don't think you followed my logic.... It *is* hidden behind a
>>> dedicated kernel config option which only device vendors (and
>>> tinkerers :-) would use. A kernel package with that option and
>>> suitable dtb has to perform the update without manipulation of the
>>> module param. The module param is to let the user prevent any future
>>> update by such a kernel package, since only he knows that he changed
>>> the battery type.
>>>
>>> What is your evidence for the assertion of "encourage this situation"?
>>>
>>
>> It is the default value, what more evidence do you want? Someone should
>> have to enable the functionality in Kconfig *and* enable it as a
>> module_param. Leaving either on by default will cause it to be
>> accidentally left enabled.
> 
> We could add a second config option to eliminate the possibility of
> the first one being set by mistake.
> 
> You can't trivially enable a config option. If you set it, you'd
> obviously set the module param if that were necessary, so that's no
> extra protection against "accidentally left enabled". (And the config
> option does nothing without a suitable dtb.)
> 

Why would you "obviously set the module param"? If this config option is
enabled then you still have to set the module param to make this write
to your device's NVM, that is two things vs one. The people who use
kernels often don't build them. Both should need to be set to update the
devices NVM.

> My scenario is a clear and likely threat. You haven't described a
> threat scenario.
> 

Easy, kernel is built with the config option enabled and their DT has
invalid option for their battery. Now everyone bricks their hardware.

Vs. your three step situation that the two people who will ever use this
will never actually find themselves in, that results in a minor
inconvenience as they have to turn a module param back off.

Lets take the safe route and leave this default off.

> The docs for the config option describe its correct use, and I will
> mention the risk of overwriting a smart battery. No general purpose
> distro will enable it.
> 

How can you know this, I'm pretty sure Ubuntu ships with a randconfig
kernel :)

Liam Breck May 8, 2017, 8:34 p.m. UTC | #9

On Mon, May 8, 2017 at 12:50 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/08/2017 02:31 PM, Liam Breck wrote:
>> On Mon, May 8, 2017 at 11:42 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/08/2017 01:39 PM, Liam Breck wrote:
>>>> On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/08/2017 01:16 AM, Liam Breck wrote:
>>>>>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>>>>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>>
>>>>>>>>> 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 <[e-mail]>
>>>>>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>>>>>> index 76806a0..85e2fb2 100644
>>>>>>>>> --- a/drivers/power/supply/Kconfig
>>>>>>>>> +++ b/drivers/power/supply/Kconfig
>>>>>>>>> @@ -178,6 +178,15 @@ 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
>>>>>>>>> +       be replaced with one of a different type.
>>>>>>>>> +
>>>>>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>> +
>>>>>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>>>>>
>>>>>>>> As before, the default should not be y, this is a rare case and I can't
>>>>>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>>>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>>>>>> accident when someone compiles and ships a kernel with
>>>>>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>>>>>
>>>>>>> Right, the point of the config option is to let a device vendor ship a
>>>>>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>>>>>> nothing absent the necessary dtb.)
>>>>>>>
>>>>>>> Module params are for end-user config; we can't require a device
>>>>>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>>>>>> kernel package update to defeat the end-user config of
>>>>>>> dt_monitored_battery_updates_nvm=0 !!
>>>>>>>
>>>>>>> I believe Sebastian agrees, given that he queued this patch :-)
>>>>>>
>>>>>> I clarified the rationale for module param default =1 above; could you ack?
>>>>>>
>>>>>> That prevents this scenario:
>>>>>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>>>>>> 2) vendor ships kernel package with dtb update and
>>>>>> dt_monitored_battery_updates_nvm=1
>>>>>> 3) gauge stops working correctly
>>>>>>
>>>>>
>>>>> Leaving it =1 will encourage this situation, not prevent it. Updating
>>>>> NVM is a non-standard operation for these parts. It should only be done
>>>>> once ideally. It is nice being able to update the NVM, and we have
>>>>> user-space tools for this, but this is a dangerous operation, improperly
>>>>> programming can brick the chip, as you have found out.
>>>>>
>>>>> If you want to add this function into the kernel driver that is fine,
>>>>> but you *cannot* have this be the default, it needs to be a hidden away
>>>>> option for people who know what they are doing, else you will end up
>>>>> accidentally causing damage to countless users' hardware.
>>>>
>>>> I don't think you followed my logic.... It *is* hidden behind a
>>>> dedicated kernel config option which only device vendors (and
>>>> tinkerers :-) would use. A kernel package with that option and
>>>> suitable dtb has to perform the update without manipulation of the
>>>> module param. The module param is to let the user prevent any future
>>>> update by such a kernel package, since only he knows that he changed
>>>> the battery type.
>>>>
>>>> What is your evidence for the assertion of "encourage this situation"?
>>>>
>>>
>>> It is the default value, what more evidence do you want? Someone should
>>> have to enable the functionality in Kconfig *and* enable it as a
>>> module_param. Leaving either on by default will cause it to be
>>> accidentally left enabled.
>>
>> We could add a second config option to eliminate the possibility of
>> the first one being set by mistake.
>>
>> You can't trivially enable a config option. If you set it, you'd
>> obviously set the module param if that were necessary, so that's no
>> extra protection against "accidentally left enabled". (And the config
>> option does nothing without a suitable dtb.)
>>
>
> Why would you "obviously set the module param"? If this config option is
> enabled then you still have to set the module param to make this write
> to your device's NVM, that is two things vs one. The people who use
> kernels often don't build them. Both should need to be set to update the
> devices NVM.

The target of this config option is device vendors. They would
obviously set the module param when setting the config option to
field-upgrade NVM.

>> My scenario is a clear and likely threat. You haven't described a
>> threat scenario.
>>
>
> Easy, kernel is built with the config option enabled and their DT has
> invalid option for their battery. Now everyone bricks their hardware.

A device vendor could misconfigure their hw, sure. They would also fix
it in an update. That's not a threat. In what scenario does someone
other than the vendor or tinkering user misconfigure the gauge?

> Vs. your three step situation that the two people who will ever use this
> will never actually find themselves in, that results in a minor
> inconvenience as they have to turn a module param back off.

I don't think you followed the scenario. User resetting the param =0
after a vendor update sets =1 would not fix the NVM misconfig caused
by the update! The whole point of user setting =0 is to prevent that
update.

> Lets take the safe route and leave this default off.

I offered you a safe route: two config options.

>> The docs for the config option describe its correct use, and I will
>> mention the risk of overwriting a smart battery. No general purpose
>> distro will enable it.
>>
>
> How can you know this, I'm pretty sure Ubuntu ships with a randconfig
> kernel :)

And that includes a rand dtb? :-p

Andrew Davis May 9, 2017, 4:06 p.m. UTC | #10

On 05/08/2017 03:34 PM, Liam Breck wrote:
> On Mon, May 8, 2017 at 12:50 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/08/2017 02:31 PM, Liam Breck wrote:
>>> On Mon, May 8, 2017 at 11:42 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/08/2017 01:39 PM, Liam Breck wrote:
>>>>> On Mon, May 8, 2017 at 7:54 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/08/2017 01:16 AM, Liam Breck wrote:
>>>>>>> On Thu, May 4, 2017 at 11:40 AM, Liam Breck <[e-mail]> wrote:
>>>>>>>> On Thu, May 4, 2017 at 9:52 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>> Signed-off-by: Liam Breck <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/power/supply/Kconfig           |   9 ++
>>>>>>>>>>  drivers/power/supply/bq27xxx_battery.c | 199 ++++++++++++++++++++++++++++++++-
>>>>>>>>>>  include/linux/power/bq27xxx_battery.h  |   2 +
>>>>>>>>>>  3 files changed, 209 insertions(+), 1 deletion(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>>>>>>>>>> index 76806a0..85e2fb2 100644
>>>>>>>>>> --- a/drivers/power/supply/Kconfig
>>>>>>>>>> +++ b/drivers/power/supply/Kconfig
>>>>>>>>>> @@ -178,6 +178,15 @@ 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
>>>>>>>>>> +       be replaced with one of a different type.
>>>>>>>>>> +
>>>>>>>>>>  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 8ab184c..06f15da 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,43 @@ 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",
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>> +
>>>>>>>>>> +#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>>> +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 NVM/flash data memory, if present. Default is 1/y.");
>>>>>>>>>
>>>>>>>>> As before, the default should not be y, this is a rare case and I can't
>>>>>>>>> see it being useful or wanted by anyone but device manufacturers. I
>>>>>>>>> don't want everyones chip's factory programmed NVM overwritten by
>>>>>>>>> accident when someone compiles and ships a kernel with
>>>>>>>>> BATTERY_BQ27XXX_DT_UPDATES_NVM=y
>>>>>>>>
>>>>>>>> Right, the point of the config option is to let a device vendor ship a
>>>>>>>> kernel + dtb which can field-upgrade NVM. (Setting the option does
>>>>>>>> nothing absent the necessary dtb.)
>>>>>>>>
>>>>>>>> Module params are for end-user config; we can't require a device
>>>>>>>> vendor to ship /etc/modprobe.d/xyz. And we certainly don't want a
>>>>>>>> kernel package update to defeat the end-user config of
>>>>>>>> dt_monitored_battery_updates_nvm=0 !!
>>>>>>>>
>>>>>>>> I believe Sebastian agrees, given that he queued this patch :-)
>>>>>>>
>>>>>>> I clarified the rationale for module param default =1 above; could you ack?
>>>>>>>
>>>>>>> That prevents this scenario:
>>>>>>> 1) user changes battery type, sets dt_monitored_battery_updates_nvm=0
>>>>>>> 2) vendor ships kernel package with dtb update and
>>>>>>> dt_monitored_battery_updates_nvm=1
>>>>>>> 3) gauge stops working correctly
>>>>>>>
>>>>>>
>>>>>> Leaving it =1 will encourage this situation, not prevent it. Updating
>>>>>> NVM is a non-standard operation for these parts. It should only be done
>>>>>> once ideally. It is nice being able to update the NVM, and we have
>>>>>> user-space tools for this, but this is a dangerous operation, improperly
>>>>>> programming can brick the chip, as you have found out.
>>>>>>
>>>>>> If you want to add this function into the kernel driver that is fine,
>>>>>> but you *cannot* have this be the default, it needs to be a hidden away
>>>>>> option for people who know what they are doing, else you will end up
>>>>>> accidentally causing damage to countless users' hardware.
>>>>>
>>>>> I don't think you followed my logic.... It *is* hidden behind a
>>>>> dedicated kernel config option which only device vendors (and
>>>>> tinkerers :-) would use. A kernel package with that option and
>>>>> suitable dtb has to perform the update without manipulation of the
>>>>> module param. The module param is to let the user prevent any future
>>>>> update by such a kernel package, since only he knows that he changed
>>>>> the battery type.
>>>>>
>>>>> What is your evidence for the assertion of "encourage this situation"?
>>>>>
>>>>
>>>> It is the default value, what more evidence do you want? Someone should
>>>> have to enable the functionality in Kconfig *and* enable it as a
>>>> module_param. Leaving either on by default will cause it to be
>>>> accidentally left enabled.
>>>
>>> We could add a second config option to eliminate the possibility of
>>> the first one being set by mistake.
>>>
>>> You can't trivially enable a config option. If you set it, you'd
>>> obviously set the module param if that were necessary, so that's no
>>> extra protection against "accidentally left enabled". (And the config
>>> option does nothing without a suitable dtb.)
>>>
>>
>> Why would you "obviously set the module param"? If this config option is
>> enabled then you still have to set the module param to make this write
>> to your device's NVM, that is two things vs one. The people who use
>> kernels often don't build them. Both should need to be set to update the
>> devices NVM.
> 
> The target of this config option is device vendors. They would
> obviously set the module param when setting the config option to
> field-upgrade NVM.
> 

Great, they we don't need to have it default =1 if they will obviously
set it, problem solved.

>>> My scenario is a clear and likely threat. You haven't described a
>>> threat scenario.
>>>
>>
>> Easy, kernel is built with the config option enabled and their DT has
>> invalid option for their battery. Now everyone bricks their hardware.
> 
> A device vendor could misconfigure their hw, sure. They would also fix
> it in an update. That's not a threat. In what scenario does someone
> other than the vendor or tinkering user misconfigure the gauge?
> 
>> Vs. your three step situation that the two people who will ever use this
>> will never actually find themselves in, that results in a minor
>> inconvenience as they have to turn a module param back off.
> 
> I don't think you followed the scenario. User resetting the param =0
> after a vendor update sets =1 would not fix the NVM misconfig caused
> by the update! The whole point of user setting =0 is to prevent that
> update.
> 

Updates are optional, you shouldn't have to manually prevent an update.

>> Lets take the safe route and leave this default off.
> 
> I offered you a safe route: two config options.
> 

How about a default off module param so we don't clutter the kconfig.

>>> The docs for the config option describe its correct use, and I will
>>> mention the risk of overwriting a smart battery. No general purpose
>>> distro will enable it.
>>>
>>
>> How can you know this, I'm pretty sure Ubuntu ships with a randconfig
>> kernel :)
> 
> And that includes a rand dtb? :-p
>

Patch

diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
index 76806a0..85e2fb2 100644
--- a/drivers/power/supply/Kconfig
+++ b/drivers/power/supply/Kconfig
@@ -178,6 +178,15 @@  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
+	  be replaced with one of a different type.
+
 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 8ab184c..06f15da 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,43 @@  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",
+};
+
+
+#ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
+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 NVM/flash data memory, if present. Default is 1/y.");
+#else
+static bool bq27xxx_dt_to_nvm = false;
+#endif
 
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
@@ -1019,6 +1063,51 @@  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;
+	}
+
+	if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
+		/* 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;
+	}
+
+	dev_info(di->dev, "update %s to %u\n", str, val);
+
+	*prev = cpu_to_be16(val);
+	buf->dirty = true;
+}
+
 static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
 {
 	const int limit = 100;
@@ -1117,6 +1206,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.
@@ -1634,6 +1820,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;
@@ -1667,7 +1860,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);
@@ -1692,6 +1888,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;