devicetree battery support and client bq27xxx_battery

From: Liam Breck <[email protected]>

Documentation of static battery characteristics that can be defined for batteries which cannot self-identify. This information is required by fuel-gauge and charger chips for proper handling of the battery.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +21 -0

new file mode 100644
index 0000000..3d916bd
@@ -0,0 +1,42 @@
+Battery Characteristics
+
+Required Properties:
+ - compatible: Must be "fixed-battery"
+
+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity
+
+Future Properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.
+
+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".
+
+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c

Quoted 50 lines · Liam Breck

From: Liam Breck <[email protected]>

Documentation of static battery characteristics that can be defined for batteries which cannot self-identify. This information is required by fuel-gauge and charger chips for proper handling of the battery.

Cc Rob Herring <[email protected]>

Cc [email protected]

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 .../devicetree/bindings/power/supply/battery.txt   | 42 ++++++++++++++++++++++
 1 file changed, 42 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/power/supply/battery.txt

Documentation/devicetree/bindings/power/supply/battery.txt new file +29 -0

new file mode 100644
index 0000000..3d916bd
@@ -0,0 +1,42 @@
+Battery Characteristics
+
+Required Properties:
+ - compatible: Must be "fixed-battery"
+
+Optional Properties:
+ - voltage-min-design-microvolt: drained battery voltage
+ - energy-full-design-microwatt-hours: battery design energy
+ - charge-full-design-microamp-hours: battery design capacity
+
+Future Properties must be named for the corresponding elements in
+enum power_supply_property, defined in include/linux/power_supply.h.
+
+Batteries must be referenced by chargers and/or fuel-gauges
+using a phandle. The phandle's property should be named
+"monitored-battery".
+
+Driver code should call power_supply_get_battery_info() to obtain
+battery properties via monitored-battery. For details see:
+  drivers/power/supply/power_supply_core.c
+  drivers/power/supply/bq27xxx_battery.c
+
+Example:
+
+     bat: battery {
+             compatible = "fixed-battery";
+             voltage-min-design-microvolt = <3200000>;
+             energy-full-design-microwatt-hours = <5290000>;
+             charge-full-design-microamp-hours = <1430000>;

Documentation/devicetree/bindings/power/supply/battery.txt line 31

Quoted 2 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +2 -0

+     };
+

Still not sure what a non-fixed-battery would be. I guess it would be a battery with an integrated fuel-gauge pre-programmed with this design info. So then the charger node would want to point to a fuel-gauge node, and the fuel-gauge can optionally point to a fixed-battery if that info is not pre-programmable.

The issues then is for chargers with an integrated fuel-gauge, or systems without a fuel-gauge, that may get a bit messy, so I have no issues with this current binding.

Quoted 12 lines

Documentation/devicetree/bindings/power/supply/battery.txt new file +11 -0

+     charger: charger@11 {
+             ....
+             monitored-battery = <&bat>;
+             ...
+     };
+
+     fuel_gauge: fuel-gauge@22 {
+             ....
+             monitored-battery = <&bat>;
+             ...
+     };

From: Matt Ranostay <[email protected]>

This patch does not alter the function of the driver. This chip ID is referenced in the devicetree code.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c     | 23 ++++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c |  2 +-
 include/linux/power/bq27xxx_battery.h      |  3 ++-
 3 files changed, 25 insertions(+), 3 deletions(-)

drivers/power/supply/bq27xxx_battery.c +22 -1

index 7272d1e..7475a5f 100644
@@ -259,6 +259,25 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x18,
 	},
+	[BQ27425] = {
+		[BQ27XXX_REG_CTRL] = 0x00,
+		[BQ27XXX_REG_TEMP] = 0x02,
+		[BQ27XXX_REG_INT_TEMP] = 0x1e,
+		[BQ27XXX_REG_VOLT] = 0x04,
+		[BQ27XXX_REG_AI] = 0x10,
+		[BQ27XXX_REG_FLAGS] = 0x06,
+		[BQ27XXX_REG_TTE] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTES] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_NAC] = 0x08,
+		[BQ27XXX_REG_FCC] = 0x0e,
+		[BQ27XXX_REG_CYCT] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_AE] = INVALID_REG_ADDR,
+		[BQ27XXX_REG_SOC] = 0x1c,
+		[BQ27XXX_REG_DCAP] = 0x3c,
+		[BQ27XXX_REG_AP] = 0x18,
+	},
 };
 
 static enum power_supply_property bq27000_battery_props[] = {

@@ -427,6 +446,7 @@ static struct {
 	BQ27XXX_PROP(BQ27541, bq27541_battery_props),
 	BQ27XXX_PROP(BQ27545, bq27545_battery_props),
 	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
+	BQ27XXX_PROP(BQ27425, bq27421_battery_props),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);

@@ -682,6 +702,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
 		return flags & (BQ27XXX_FLAG_OTC | BQ27XXX_FLAG_OTD);
 	case BQ27530:
 	case BQ27421:
+	case BQ27425:
 		return flags & BQ27XXX_FLAG_OT;
 	default:
 		return false;

@@ -693,7 +714,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
  */
 static bool bq27xxx_battery_undertemp(struct bq27xxx_device_info *di, u16 flags)
 {
-	if (di->chip == BQ27530 || di->chip == BQ27421)
+	if (di->chip == BQ27530 || di->chip == BQ27421 || di->chip == BQ27425)
 		return flags & BQ27XXX_FLAG_UT;
 
 	return false;

drivers/power/supply/bq27xxx_battery_i2c.c +1 -1

index 5c5c3a6..d7da535 100644
@@ -159,7 +159,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27742", BQ27541 },
 	{ "bq27545", BQ27545 },
 	{ "bq27421", BQ27421 },
-	{ "bq27425", BQ27421 },
+	{ "bq27425", BQ27425 },
 	{ "bq27441", BQ27421 },
 	{ "bq27621", BQ27421 },
 	{},

include/linux/power/bq27xxx_battery.h +2 -1

index 92df553..4925478 100644
@@ -9,7 +9,8 @@ enum bq27xxx_chip {
 	BQ27530, /* bq27530, bq27531 */
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
 	BQ27545, /* bq27545 */
-	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+	BQ27421, /* bq27421, bq27441, bq27621 */
+	BQ27425, /* bq27425 */
 };
 
 /**

From: Liam Breck <[email protected]>

Previously there was no way to configure chip registers in the event that the defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, and writes battery data to chip RAM or non-volatile memory.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

drivers/power/supply/bq27xxx_battery.c +1 -1

index 7475a5f..8085d4a 100644
@@ -51,7 +51,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION		"1.2.0"
+#define DRIVER_VERSION		"1.3.0"
 
 #define BQ27XXX_MANUFACTURER	"Texas Instruments"
 

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);

drivers/power/supply/bq27xxx_battery.c +8 -0

+
+struct bq27xxx_dm_reg {
+	u8 subclass_id;
+	u8 offset;
+	u8 bytes;
+	u16 min, max;
+};
+

drivers/power/supply/bq27xxx_battery.c +34 -0

+	return true;
+}
+
+enum bq27xxx_dm_reg_id {
+	BQ27XXX_DM_DESIGN_CAPACITY = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+	BQ27XXX_DM_END,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+	[BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+

drivers/power/supply/bq27xxx_battery.c +16 -0

+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
 	struct bq27xxx_device_info *di;

@@ -496,6 +589,297 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 	return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+					  bool state)
+{
+	u32 key = bq27xxx_unseal_keys[di->chip];
+	int ret;
+
+	if (state) {
+		ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SEALED, false);
+		if (ret < 0)
+			goto out;
+	} else {
+		ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)(key >> 16), false);
+		if (ret < 0)

drivers/power/supply/bq27xxx_battery.c +73 -0

+		sum += buf->a[i];
+	sum &= 0xff;
+
+	return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+					 struct bq27xxx_dm_buf *buf)
+{
+	int ret;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out;
+
+	BQ27XXX_MSLEEP(1);
+
+	ret = di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+	if (ret < 0)
+		goto out;
+
+	ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+	if (ret < 0)
+		goto out;
+
+	if ((u8)ret != bq27xxx_battery_checksum(buf)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf->full = true;
+	buf->updt = false;
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+	return ret;
+}
+
+static void bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+	struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+	struct bq27xxx_dm_buf buf = { .class = 0xFF };
+	int i;
+
+	for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+		const char* str = bq27xxx_dm_reg_name[i];
+
+		if (bq27xxx_dm_buf_set(&buf, reg))
+			if (bq27xxx_battery_read_dm_block(di, &buf) < 0)
+				continue;
+
+		if (reg->bytes == 2)
+			dev_info(di->dev, "config register %s is %d\n", str,
+				 be16_to_cpup(BQ27XXX_DM_BUF_PTR(buf, reg)));
+		else
+			dev_warn(di->dev, "unsupported config register %s\n", str);
+	}
+}
+
+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 = &bq27xxx_dm_regs[di->chip][reg_id];
+	u16 *prev;
+
+	if (reg->bytes != 2)

drivers/power/supply/bq27xxx_battery.c +30 -0

+	buf->updt = true;
+	return;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+					  struct bq27xxx_dm_buf *buf)
+{
+	bool cfgup = di->chip == BQ27425 || di->chip == BQ27421; /* || BQ27441 || BQ27621 */
+	int ret;
+
+	if (cfgup) {
+		ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SET_CFGUPDATE, false);
+		if (ret < 0)
+			goto out1;
+	}
+
+	ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+	if (ret < 0)
+		goto out2;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out2;
+
+	ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out2;
+
+	BQ27XXX_MSLEEP(1);
+

drivers/power/supply/bq27xxx_battery.c +148 -1

+	 * If the 350ms delay is insufficient, NVM corruption results
+	 * on the '425 chip, which could damage the chip.
+	 * It was suggested in this TI tool:
+	 *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+	 *
+	 * BQ27XXX_MSLEEP(350);
+	 * ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+	 * BQ27XXX_MSLEEP(1);
+	 * ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+	 * if ((u8)ret != bq27xxx_battery_checksum(buf))
+	 * 	ret = -EINVAL;
+	 */
+
+	if (cfgup) {
+		BQ27XXX_MSLEEP(1);
+		ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+		if (ret < 0)
+			goto out1;
+	}
+
+	buf->updt = false;
+	return 0;
+
+out2:
+	if (cfgup)
+		di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+out1:
+	dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+	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_ENERGY);
+	struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+	if (info->charge_full_design_uah != -EINVAL
+	 && info->energy_full_design_uwh != -EINVAL) {
+		bq27xxx_battery_read_dm_block(di, &bd);
+		if (bd.full) {
+			/* 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.full && bd.class == bt.class && bd.block == bt.block;
+		if (!same)
+			bq27xxx_battery_read_dm_block(di, &bt);
+		if (same ? bd.full : bt.full)
+			bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+					BQ27XXX_DM_TERMINATE_VOLTAGE,
+					info->voltage_min_design_uv / 1000);
+	}
+
+	if (bd.updt)
+		bq27xxx_battery_write_dm_block(di, &bd);
+	if (bt.updt)
+		bq27xxx_battery_write_dm_block(di, &bt);
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+	struct power_supply_battery_info info = {};
+	unsigned int min, max;
+
+	/* functions don't exist for writing data so abort */
+	if (!di->bus.write || !di->bus.write_bulk)
+		return;
+
+	/* no settings to be set for this chipset so abort */
+	if (!bq27xxx_dm_regs[di->chip])
+		return;
+
+	if (bq27xxx_battery_set_seal_state(di, false) < 0)
+		return;
+
+	if (power_supply_get_battery_info(di->bat, &info) < 0)
+		goto out;
+
+	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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+	max = bq27xxx_dm_regs[di->chip][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)
+		goto out;
+
+	bq27xxx_battery_set_config(di, &info);
+
+out:
+	bq27xxx_battery_print_config(di);
+	bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.

@@ -1006,6 +1390,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;

@@ -1039,7 +1430,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);

@@ -1064,6 +1458,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);

Quoted 31 lines · Liam Breck

From: Liam Breck <[email protected]>

Previously there was no way to configure chip registers in the event that the defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, and writes battery data to chip RAM or non-volatile memory.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

drivers/power/supply/bq27xxx_battery.c +1 -1

index 7475a5f..8085d4a 100644
@@ -51,7 +51,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION               "1.2.0"
+#define DRIVER_VERSION               "1.3.0"
 
 #define BQ27XXX_MANUFACTURER "Texas Instruments"
 

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 

drivers/power/supply/bq27xxx_battery.c static struct { · line 462

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+/* writable registers */
+#define BQ27XXX_CONTROL                      0x00
+#define BQ27XXX_DATA_CLASS           0x3E
+#define BQ27XXX_DATA_BLOCK           0x3F
+#define BQ27XXX_BLOCK_DATA           0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM  0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL   0x61
+

These all belong in the bq27xxx_regs array, you can add new register types to the bq27xxx_reg_index enum.

drivers/power/supply/bq27xxx_battery.c static struct { · line 468

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+/* control register params */
+#define BQ27XXX_SEALED                       0x20
+#define BQ27XXX_SET_CFGUPDATE                0x13
+#define BQ27XXX_SOFT_RESET           0x42
+
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)

why?

Quoted 13 lines

drivers/power/supply/bq27xxx_battery.c +13 -0

+
+struct bq27xxx_dm_reg {
+     u8 subclass_id;
+     u8 offset;
+     u8 bytes;
+     u16 min, max;
+};
+
+struct bq27xxx_dm_buf {
+     u8 class;
+     u8 block;
+     u8 a[32];
+     bool full, updt;

drivers/power/supply/bq27xxx_battery.c static struct { · line 486

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+};
+
+#define BQ27XXX_DM_BUF_PTR(b, r) \
+     ( (u16*) &(b).a[ (r)->offset % sizeof (b).a ] )
+

This could compete in the IOCCC, there has to be a more readable way to do this..

drivers/power/supply/bq27xxx_battery.c static struct { · line 496

Quoted 10 lines

drivers/power/supply/bq27xxx_battery.c +10 -0

+#define BQ27XXX_DM_BUF(di, i) { \
+     .class = bq27xxx_dm_regs[(di)->chip][i].subclass_id, \
+     .block = bq27xxx_dm_regs[(di)->chip][i].offset / sizeof ((struct bq27xxx_dm_buf*)0)->a, \
+}
+
+static inline bool bq27xxx_dm_buf_set(struct bq27xxx_dm_buf *buf,
+                                   struct bq27xxx_dm_reg *reg) {
+     if (buf->class == reg->subclass_id
+      && buf->block == reg->offset / sizeof buf->a
+      && buf->full)

Some alignment issues here.

drivers/power/supply/bq27xxx_battery.c static struct { · line 500

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+             return false;
+     buf->class = reg->subclass_id;
+     buf->block = reg->offset / sizeof buf->a;
+     buf->full = buf->updt = false;

newline before return

Quoted 38 lines

drivers/power/supply/bq27xxx_battery.c +38 -0

+     return true;
+}
+
+enum bq27xxx_dm_reg_id {
+     BQ27XXX_DM_DESIGN_CAPACITY = 0,
+     BQ27XXX_DM_DESIGN_ENERGY,
+     BQ27XXX_DM_TERMINATE_VOLTAGE,
+     BQ27XXX_DM_END,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+     [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
+     [BQ27421] = bq27421_dm_regs, /* and BQ27441 */
+     [BQ27425] = bq27425_dm_regs,
+/*   [BQ27621] = */ bq27621_dm_regs,

drivers/power/supply/bq27xxx_battery.c static struct { · line 544

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+};
+
+static u32 bq27xxx_unseal_keys[] = {
+     [BQ27421] = 0x80008000, /* and BQ27441 */
+     [BQ27425] = 0x04143672,
+/*   [BQ27621] = */ 0x80008000,

Why is this commented out.

Quoted 28 lines

drivers/power/supply/bq27xxx_battery.c +21 -0

+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
      struct bq27xxx_device_info *di;

@@ -496,6 +589,297 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
      return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+                                       bool state)
+{
+     u32 key = bq27xxx_unseal_keys[di->chip];
+     int ret;
+
+     if (state) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SEALED, false);
+             if (ret < 0)
+                     goto out;
+     } else {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)(key >> 16), false);
+             if (ret < 0)
+                     goto out;
+
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)key, false);
+             if (ret < 0)
+                     goto out;

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 614

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     }
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);

This is not a debug statement, we don't need the function name, just say where it broke.

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 623

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     return ret;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
+{
+     u16 sum = 0;
+     int i;
+
+     for (i = 0; i < sizeof buf->a; i++)

Linus would like a word with you:
https://lkml.org/lkml/2012/7/11/103

Quoted 78 lines

drivers/power/supply/bq27xxx_battery.c +78 -0

+             sum += buf->a[i];
+     sum &= 0xff;
+
+     return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+                                      struct bq27xxx_dm_buf *buf)
+{
+     int ret;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+     if (ret < 0)
+             goto out;
+
+     if ((u8)ret != bq27xxx_battery_checksum(buf)) {
+             ret = -EINVAL;
+             goto out;
+     }
+
+     buf->full = true;
+     buf->updt = false;
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     return ret;
+}
+
+static void bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+     struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+     struct bq27xxx_dm_buf buf = { .class = 0xFF };
+     int i;
+
+     for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+             const char* str = bq27xxx_dm_reg_name[i];
+
+             if (bq27xxx_dm_buf_set(&buf, reg))
+                     if (bq27xxx_battery_read_dm_block(di, &buf) < 0)
+                             continue;
+
+             if (reg->bytes == 2)
+                     dev_info(di->dev, "config register %s is %d\n", str,
+                              be16_to_cpup(BQ27XXX_DM_BUF_PTR(buf, reg)));
+             else
+                     dev_warn(di->dev, "unsupported config register %s\n", str);
+     }
+}
+
+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 = &bq27xxx_dm_regs[di->chip][reg_id];
+     u16 *prev;
+
+     if (reg->bytes != 2)
+             return;
+
+     prev = BQ27XXX_DM_BUF_PTR(*buf, reg);
+     if (be16_to_cpup(prev) == val)
+             return;

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 706

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     *prev = cpu_to_be16(val);
+
+     dev_info(di->dev, "update chip %d, class %u, block %u, offset %u, value %u\n",
+              di->chip, buf->class, buf->block, reg->offset, val);
+

This should probably be dev_debug, no normal user cares about all this detail.

Quoted 36 lines

drivers/power/supply/bq27xxx_battery.c +36 -0

+     buf->updt = true;
+     return;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+                                       struct bq27xxx_dm_buf *buf)
+{
+     bool cfgup = di->chip == BQ27425 || di->chip == BQ27421; /* || BQ27441 || BQ27621 */
+     int ret;
+
+     if (cfgup) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SET_CFGUPDATE, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out2;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+                         bq27xxx_battery_checksum(buf), true);

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 746

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+     if (ret < 0)
+             goto out2;
+
+     /* THE FOLLOWING CODE IS TOXIC. DO NOT USE!

Then why include it, what is this code supposed to do, and what do we lose without it? If it is really needed we can find a way to guarantee the delay length.

Quoted 174 lines

drivers/power/supply/bq27xxx_battery.c +148 -1

+      * If the 350ms delay is insufficient, NVM corruption results
+      * on the '425 chip, which could damage the chip.
+      * It was suggested in this TI tool:
+      *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+      *
+      * BQ27XXX_MSLEEP(350);
+      * ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+      * BQ27XXX_MSLEEP(1);
+      * ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+      * if ((u8)ret != bq27xxx_battery_checksum(buf))
+      *      ret = -EINVAL;
+      */
+
+     if (cfgup) {
+             BQ27XXX_MSLEEP(1);
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     buf->updt = false;
+     return 0;
+
+out2:
+     if (cfgup)
+             di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+out1:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     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_ENERGY);
+     struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+     if (info->charge_full_design_uah != -EINVAL
+      && info->energy_full_design_uwh != -EINVAL) {
+             bq27xxx_battery_read_dm_block(di, &bd);
+             if (bd.full) {
+                     /* 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.full && bd.class == bt.class && bd.block == bt.block;
+             if (!same)
+                     bq27xxx_battery_read_dm_block(di, &bt);
+             if (same ? bd.full : bt.full)
+                     bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+                                     BQ27XXX_DM_TERMINATE_VOLTAGE,
+                                     info->voltage_min_design_uv / 1000);
+     }
+
+     if (bd.updt)
+             bq27xxx_battery_write_dm_block(di, &bd);
+     if (bt.updt)
+             bq27xxx_battery_write_dm_block(di, &bt);
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+     struct power_supply_battery_info info = {};
+     unsigned int min, max;
+
+     /* functions don't exist for writing data so abort */
+     if (!di->bus.write || !di->bus.write_bulk)
+             return;
+
+     /* no settings to be set for this chipset so abort */
+     if (!bq27xxx_dm_regs[di->chip])
+             return;
+
+     if (bq27xxx_battery_set_seal_state(di, false) < 0)
+             return;
+
+     if (power_supply_get_battery_info(di->bat, &info) < 0)
+             goto out;
+
+     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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+     max = bq27xxx_dm_regs[di->chip][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)
+             goto out;
+
+     bq27xxx_battery_set_config(di, &info);
+
+out:
+     bq27xxx_battery_print_config(di);
+     bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.

@@ -1006,6 +1390,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;

@@ -1039,7 +1430,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);

@@ -1064,6 +1458,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);

Quoted 32 lines · Andrew F. Davis

Quoted 31 lines · Liam Breck

From: Liam Breck <[email protected]>

Previously there was no way to configure chip registers in the event that the defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, and writes battery data to chip RAM or non-volatile memory.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

drivers/power/supply/bq27xxx_battery.c +1 -1

index 7475a5f..8085d4a 100644
@@ -51,7 +51,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION               "1.2.0"
+#define DRIVER_VERSION               "1.3.0"
 
 #define BQ27XXX_MANUFACTURER "Texas Instruments"
 

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 462

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+/* writable registers */
+#define BQ27XXX_CONTROL                      0x00
+#define BQ27XXX_DATA_CLASS           0x3E
+#define BQ27XXX_DATA_BLOCK           0x3F
+#define BQ27XXX_BLOCK_DATA           0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM  0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL   0x61
+

These all belong in the bq27xxx_regs array, you can add new register types to the bq27xxx_reg_index enum.

They're the same on all chips. Let's dup this code 9+ times when we have cause.

Or make a default array of reg numbers and memcopy it onto each chip array, then alter as req'd per chip. But that's outside our scope.

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 468

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+/* control register params */
+#define BQ27XXX_SEALED                       0x20
+#define BQ27XXX_SET_CFGUPDATE                0x13
+#define BQ27XXX_SOFT_RESET           0x42
+
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)

why?

I think this might vary on diff chips. Also I have a pending change which uses MSLEEP(20)

Quoted 13 lines

Quoted 13 lines

drivers/power/supply/bq27xxx_battery.c +13 -0

+
+struct bq27xxx_dm_reg {
+     u8 subclass_id;
+     u8 offset;
+     u8 bytes;
+     u16 min, max;
+};
+
+struct bq27xxx_dm_buf {
+     u8 class;
+     u8 block;
+     u8 a[32];
+     bool full, updt;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 486

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+};
+
+#define BQ27XXX_DM_BUF_PTR(b, r) \
+     ( (u16*) &(b).a[ (r)->offset % sizeof (b).a ] )
+

This could compete in the IOCCC, there has to be a more readable way to do this..

Hence the macro. It didn't seem to merit a function, but I could write an inline fn.

Quoted 59 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 496

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c +10 -0

+#define BQ27XXX_DM_BUF(di, i) { \
+     .class = bq27xxx_dm_regs[(di)->chip][i].subclass_id, \
+     .block = bq27xxx_dm_regs[(di)->chip][i].offset / sizeof ((struct bq27xxx_dm_buf*)0)->a, \
+}
+
+static inline bool bq27xxx_dm_buf_set(struct bq27xxx_dm_buf *buf,
+                                   struct bq27xxx_dm_reg *reg) {
+     if (buf->class == reg->subclass_id
+      && buf->block == reg->offset / sizeof buf->a
+      && buf->full)

Some alignment issues here.

drivers/power/supply/bq27xxx_battery.c static struct { · line 500

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+             return false;
+     buf->class = reg->subclass_id;
+     buf->block = reg->offset / sizeof buf->a;
+     buf->full = buf->updt = false;

newline before return

Quoted 38 lines

drivers/power/supply/bq27xxx_battery.c +38 -0

+     return true;
+}
+
+enum bq27xxx_dm_reg_id {
+     BQ27XXX_DM_DESIGN_CAPACITY = 0,
+     BQ27XXX_DM_DESIGN_ENERGY,
+     BQ27XXX_DM_TERMINATE_VOLTAGE,
+     BQ27XXX_DM_END,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+     [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
+     [BQ27421] = bq27421_dm_regs, /* and BQ27441 */
+     [BQ27425] = bq27425_dm_regs,
+/*   [BQ27621] = */ bq27621_dm_regs,

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 544

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+};
+
+static u32 bq27xxx_unseal_keys[] = {
+     [BQ27421] = 0x80008000, /* and BQ27441 */
+     [BQ27425] = 0x04143672,
+/*   [BQ27621] = */ 0x80008000,

Why is this commented out.

The 621 id isn't defined yet. I didn't want to enable something I can't test.

Quoted 28 lines

Quoted 28 lines

drivers/power/supply/bq27xxx_battery.c +21 -0

+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
      struct bq27xxx_device_info *di;

@@ -496,6 +589,297 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
      return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+                                       bool state)
+{
+     u32 key = bq27xxx_unseal_keys[di->chip];
+     int ret;
+
+     if (state) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SEALED, false);
+             if (ret < 0)
+                     goto out;
+     } else {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)(key >> 16), false);
+             if (ret < 0)
+                     goto out;
+
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)key, false);
+             if (ret < 0)
+                     goto out;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 614

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     }
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);

This is not a debug statement, we don't need the function name, just say where it broke.

What's "where" if not fn name?

Quoted 91 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 623

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     return ret;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
+{
+     u16 sum = 0;
+     int i;
+
+     for (i = 0; i < sizeof buf->a; i++)

Linus would like a word with you:
https://lkml.org/lkml/2012/7/11/103

Quoted 78 lines

drivers/power/supply/bq27xxx_battery.c +78 -0

+             sum += buf->a[i];
+     sum &= 0xff;
+
+     return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+                                      struct bq27xxx_dm_buf *buf)
+{
+     int ret;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+     if (ret < 0)
+             goto out;
+
+     if ((u8)ret != bq27xxx_battery_checksum(buf)) {
+             ret = -EINVAL;
+             goto out;
+     }
+
+     buf->full = true;
+     buf->updt = false;
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     return ret;
+}
+
+static void bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+     struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+     struct bq27xxx_dm_buf buf = { .class = 0xFF };
+     int i;
+
+     for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+             const char* str = bq27xxx_dm_reg_name[i];
+
+             if (bq27xxx_dm_buf_set(&buf, reg))
+                     if (bq27xxx_battery_read_dm_block(di, &buf) < 0)
+                             continue;
+
+             if (reg->bytes == 2)
+                     dev_info(di->dev, "config register %s is %d\n", str,
+                              be16_to_cpup(BQ27XXX_DM_BUF_PTR(buf, reg)));
+             else
+                     dev_warn(di->dev, "unsupported config register %s\n", str);
+     }
+}
+
+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 = &bq27xxx_dm_regs[di->chip][reg_id];
+     u16 *prev;
+
+     if (reg->bytes != 2)
+             return;
+
+     prev = BQ27XXX_DM_BUF_PTR(*buf, reg);
+     if (be16_to_cpup(prev) == val)
+             return;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 706

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     *prev = cpu_to_be16(val);
+
+     dev_info(di->dev, "update chip %d, class %u, block %u, offset %u, value %u\n",
+              di->chip, buf->class, buf->block, reg->offset, val);
+

This should probably be dev_debug, no normal user cares about all this detail.

It's only on change to NVM, which a hw developer (like me) wants to know about. I plan to remove the print_config routine so this is not redundant.

Quoted 36 lines

Quoted 36 lines

drivers/power/supply/bq27xxx_battery.c +36 -0

+     buf->updt = true;
+     return;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+                                       struct bq27xxx_dm_buf *buf)
+{
+     bool cfgup = di->chip == BQ27425 || di->chip == BQ27421; /* || BQ27441 || BQ27621 */
+     int ret;
+
+     if (cfgup) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SET_CFGUPDATE, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out2;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+                         bq27xxx_battery_checksum(buf), true);

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 746

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+     if (ret < 0)
+             goto out2;
+
+     /* THE FOLLOWING CODE IS TOXIC. DO NOT USE!

Then why include it, what is this code supposed to do, and what do we lose without it? If it is really needed we can find a way to guarantee the delay length.

This triggers a chip bug, and future devs need to know to avoid it. I was tempted by bqtool into using it for the extra error check, and now my gauge isn't working, tho I have only reset one NVM block back to factory defaults. Bqtool is easy to find, if poorly written.

Quoted 174 lines

Quoted 174 lines

drivers/power/supply/bq27xxx_battery.c +148 -1

+      * If the 350ms delay is insufficient, NVM corruption results
+      * on the '425 chip, which could damage the chip.
+      * It was suggested in this TI tool:
+      *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+      *
+      * BQ27XXX_MSLEEP(350);
+      * ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+      * BQ27XXX_MSLEEP(1);
+      * ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+      * if ((u8)ret != bq27xxx_battery_checksum(buf))
+      *      ret = -EINVAL;
+      */
+
+     if (cfgup) {
+             BQ27XXX_MSLEEP(1);
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     buf->updt = false;
+     return 0;
+
+out2:
+     if (cfgup)
+             di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+out1:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     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_ENERGY);
+     struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+     if (info->charge_full_design_uah != -EINVAL
+      && info->energy_full_design_uwh != -EINVAL) {
+             bq27xxx_battery_read_dm_block(di, &bd);
+             if (bd.full) {
+                     /* 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.full && bd.class == bt.class && bd.block == bt.block;
+             if (!same)
+                     bq27xxx_battery_read_dm_block(di, &bt);
+             if (same ? bd.full : bt.full)
+                     bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+                                     BQ27XXX_DM_TERMINATE_VOLTAGE,
+                                     info->voltage_min_design_uv / 1000);
+     }
+
+     if (bd.updt)
+             bq27xxx_battery_write_dm_block(di, &bd);
+     if (bt.updt)
+             bq27xxx_battery_write_dm_block(di, &bt);
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+     struct power_supply_battery_info info = {};
+     unsigned int min, max;
+
+     /* functions don't exist for writing data so abort */
+     if (!di->bus.write || !di->bus.write_bulk)
+             return;
+
+     /* no settings to be set for this chipset so abort */
+     if (!bq27xxx_dm_regs[di->chip])
+             return;
+
+     if (bq27xxx_battery_set_seal_state(di, false) < 0)
+             return;
+
+     if (power_supply_get_battery_info(di->bat, &info) < 0)
+             goto out;
+
+     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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+     max = bq27xxx_dm_regs[di->chip][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)
+             goto out;
+
+     bq27xxx_battery_set_config(di, &info);
+
+out:
+     bq27xxx_battery_print_config(di);
+     bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.

@@ -1006,6 +1390,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;

@@ -1039,7 +1430,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);

@@ -1064,6 +1458,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);

Quoted 45 lines · Liam Breck

Quoted 43 lines · Andrew F. Davis

Quoted 29 lines · Liam Breck

From: Liam Breck <[email protected]>

Previously there was no way to configure chip registers in the event that the defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, and writes battery data to chip RAM or non-volatile memory.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

drivers/power/supply/bq27xxx_battery.c +1 -1

index 7475a5f..8085d4a 100644
@@ -51,7 +51,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION               "1.2.0"
+#define DRIVER_VERSION               "1.3.0"
 
 #define BQ27XXX_MANUFACTURER "Texas Instruments"
 

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);

drivers/power/supply/bq27xxx_battery.c static struct { · line 462

Quoted 10 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

 static LIST_HEAD(bq27xxx_battery_devices);
 
+/* writable registers */
+#define BQ27XXX_CONTROL                      0x00
+#define BQ27XXX_DATA_CLASS           0x3E
+#define BQ27XXX_DATA_BLOCK           0x3F
+#define BQ27XXX_BLOCK_DATA           0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM  0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL   0x61
+

These all belong in the bq27xxx_regs array, you can add new register types to the bq27xxx_reg_index enum.

Quoted 2 lines

They're the same on all chips. Let's dup this code 9+ times when we have cause.

They are not the same on all chips, some chips don't have these registers at all. Plus, BQ27XXX_CONTROL is the same for all chips, and we already have it in all the chips bq27xxx_regs.

Quoted 12 lines

Or make a default array of reg numbers and memcopy it onto each chip array, then alter as req'd per chip. But that's outside our scope.

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 468

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+/* control register params */
+#define BQ27XXX_SEALED                       0x20
+#define BQ27XXX_SET_CFGUPDATE                0x13
+#define BQ27XXX_SOFT_RESET           0x42
+
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)

why?

Quoted 3 lines

I think this might vary on diff chips. Also I have a pending change which uses MSLEEP(20)

If they are different we should delay for the largest needed time, a range means some may not get enough delay, we would always be targeting the lowest common denominator chip anyway.

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

drivers/power/supply/bq27xxx_battery.c +13 -0

+
+struct bq27xxx_dm_reg {
+     u8 subclass_id;
+     u8 offset;
+     u8 bytes;
+     u16 min, max;
+};
+
+struct bq27xxx_dm_buf {
+     u8 class;
+     u8 block;
+     u8 a[32];
+     bool full, updt;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 486

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+};
+
+#define BQ27XXX_DM_BUF_PTR(b, r) \
+     ( (u16*) &(b).a[ (r)->offset % sizeof (b).a ] )
+

This could compete in the IOCCC, there has to be a more readable way to do this..

Quoted 3 lines

Hence the macro. It didn't seem to merit a function, but I could write an inline fn.

That would be nice. With C, the whole kernel could be one long line, but just because you can doesn't mean you should, lets keep this readable for all of our sake :)

Quoted 68 lines

Quoted 59 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 496

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c +10 -0

+#define BQ27XXX_DM_BUF(di, i) { \
+     .class = bq27xxx_dm_regs[(di)->chip][i].subclass_id, \
+     .block = bq27xxx_dm_regs[(di)->chip][i].offset / sizeof ((struct bq27xxx_dm_buf*)0)->a, \
+}
+
+static inline bool bq27xxx_dm_buf_set(struct bq27xxx_dm_buf *buf,
+                                   struct bq27xxx_dm_reg *reg) {
+     if (buf->class == reg->subclass_id
+      && buf->block == reg->offset / sizeof buf->a
+      && buf->full)

Some alignment issues here.

drivers/power/supply/bq27xxx_battery.c static struct { · line 500

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+             return false;
+     buf->class = reg->subclass_id;
+     buf->block = reg->offset / sizeof buf->a;
+     buf->full = buf->updt = false;

newline before return

Quoted 38 lines

drivers/power/supply/bq27xxx_battery.c +38 -0

+     return true;
+}
+
+enum bq27xxx_dm_reg_id {
+     BQ27XXX_DM_DESIGN_CAPACITY = 0,
+     BQ27XXX_DM_DESIGN_ENERGY,
+     BQ27XXX_DM_TERMINATE_VOLTAGE,
+     BQ27XXX_DM_END,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+     [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
+     [BQ27421] = bq27421_dm_regs, /* and BQ27441 */
+     [BQ27425] = bq27425_dm_regs,
+/*   [BQ27621] = */ bq27621_dm_regs,

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 544

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+};
+
+static u32 bq27xxx_unseal_keys[] = {
+     [BQ27421] = 0x80008000, /* and BQ27441 */
+     [BQ27425] = 0x04143672,
+/*   [BQ27621] = */ 0x80008000,

Why is this commented out.

Quoted 2 lines

The 621 id isn't defined yet. I didn't want to enable something I can't test.

Yes, but you are only commenting out the index, it still is added to the list. You should wait to add that line when the rest of BQ27621 support is added.

Quoted 37 lines

Quoted 28 lines

Quoted 28 lines

drivers/power/supply/bq27xxx_battery.c +21 -0

+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
      struct bq27xxx_device_info *di;

@@ -496,6 +589,297 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
      return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+                                       bool state)
+{
+     u32 key = bq27xxx_unseal_keys[di->chip];
+     int ret;
+
+     if (state) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SEALED, false);
+             if (ret < 0)
+                     goto out;
+     } else {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)(key >> 16), false);
+             if (ret < 0)
+                     goto out;
+
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)key, false);
+             if (ret < 0)
+                     goto out;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 614

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     }
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);

This is not a debug statement, we don't need the function name, just say where it broke.

Quoted 2 lines

What's "where" if not fn name?

For example:

"Bus error while setting seal state"

The actually function name is unimportant, and the user doesn't care.

Quoted 100 lines

Quoted 91 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 623

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     return ret;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
+{
+     u16 sum = 0;
+     int i;
+
+     for (i = 0; i < sizeof buf->a; i++)

Linus would like a word with you:
https://lkml.org/lkml/2012/7/11/103

Quoted 78 lines

drivers/power/supply/bq27xxx_battery.c +78 -0

+             sum += buf->a[i];
+     sum &= 0xff;
+
+     return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+                                      struct bq27xxx_dm_buf *buf)
+{
+     int ret;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+     if (ret < 0)
+             goto out;
+
+     if ((u8)ret != bq27xxx_battery_checksum(buf)) {
+             ret = -EINVAL;
+             goto out;
+     }
+
+     buf->full = true;
+     buf->updt = false;
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     return ret;
+}
+
+static void bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+     struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+     struct bq27xxx_dm_buf buf = { .class = 0xFF };
+     int i;
+
+     for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+             const char* str = bq27xxx_dm_reg_name[i];
+
+             if (bq27xxx_dm_buf_set(&buf, reg))
+                     if (bq27xxx_battery_read_dm_block(di, &buf) < 0)
+                             continue;
+
+             if (reg->bytes == 2)
+                     dev_info(di->dev, "config register %s is %d\n", str,
+                              be16_to_cpup(BQ27XXX_DM_BUF_PTR(buf, reg)));
+             else
+                     dev_warn(di->dev, "unsupported config register %s\n", str);
+     }
+}
+
+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 = &bq27xxx_dm_regs[di->chip][reg_id];
+     u16 *prev;
+
+     if (reg->bytes != 2)
+             return;
+
+     prev = BQ27XXX_DM_BUF_PTR(*buf, reg);
+     if (be16_to_cpup(prev) == val)
+             return;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 706

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     *prev = cpu_to_be16(val);
+
+     dev_info(di->dev, "update chip %d, class %u, block %u, offset %u, value %u\n",
+              di->chip, buf->class, buf->block, reg->offset, val);
+

This should probably be dev_debug, no normal user cares about all this detail.

Quoted 4 lines

It's only on change to NVM, which a hw developer (like me) wants to know about. I plan to remove the print_config routine so this is not redundant.

HW developers should then have debug enabled for drivers they care about. For everyone else this is just kernel log clutter.

Quoted 45 lines

Quoted 36 lines

Quoted 36 lines

drivers/power/supply/bq27xxx_battery.c +36 -0

+     buf->updt = true;
+     return;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+                                       struct bq27xxx_dm_buf *buf)
+{
+     bool cfgup = di->chip == BQ27425 || di->chip == BQ27421; /* || BQ27441 || BQ27621 */
+     int ret;
+
+     if (cfgup) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SET_CFGUPDATE, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out2;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+                         bq27xxx_battery_checksum(buf), true);

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 746

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+     if (ret < 0)
+             goto out2;
+
+     /* THE FOLLOWING CODE IS TOXIC. DO NOT USE!

Then why include it, what is this code supposed to do, and what do we lose without it? If it is really needed we can find a way to guarantee the delay length.

Quoted 5 lines

This triggers a chip bug, and future devs need to know to avoid it. I was tempted by bqtool into using it for the extra error check, and now my gauge isn't working, tho I have only reset one NVM block back to factory defaults. Bqtool is easy to find, if poorly written.

Just the warning would be enough then, we don't gain anything by writing out the exact code to break your device in the comment also.

Quoted 174 lines

Quoted 174 lines

Quoted 174 lines

drivers/power/supply/bq27xxx_battery.c +148 -1

+      * If the 350ms delay is insufficient, NVM corruption results
+      * on the '425 chip, which could damage the chip.
+      * It was suggested in this TI tool:
+      *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+      *
+      * BQ27XXX_MSLEEP(350);
+      * ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+      * BQ27XXX_MSLEEP(1);
+      * ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+      * if ((u8)ret != bq27xxx_battery_checksum(buf))
+      *      ret = -EINVAL;
+      */
+
+     if (cfgup) {
+             BQ27XXX_MSLEEP(1);
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     buf->updt = false;
+     return 0;
+
+out2:
+     if (cfgup)
+             di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+out1:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     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_ENERGY);
+     struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+     if (info->charge_full_design_uah != -EINVAL
+      && info->energy_full_design_uwh != -EINVAL) {
+             bq27xxx_battery_read_dm_block(di, &bd);
+             if (bd.full) {
+                     /* 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.full && bd.class == bt.class && bd.block == bt.block;
+             if (!same)
+                     bq27xxx_battery_read_dm_block(di, &bt);
+             if (same ? bd.full : bt.full)
+                     bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+                                     BQ27XXX_DM_TERMINATE_VOLTAGE,
+                                     info->voltage_min_design_uv / 1000);
+     }
+
+     if (bd.updt)
+             bq27xxx_battery_write_dm_block(di, &bd);
+     if (bt.updt)
+             bq27xxx_battery_write_dm_block(di, &bt);
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+     struct power_supply_battery_info info = {};
+     unsigned int min, max;
+
+     /* functions don't exist for writing data so abort */
+     if (!di->bus.write || !di->bus.write_bulk)
+             return;
+
+     /* no settings to be set for this chipset so abort */
+     if (!bq27xxx_dm_regs[di->chip])
+             return;
+
+     if (bq27xxx_battery_set_seal_state(di, false) < 0)
+             return;
+
+     if (power_supply_get_battery_info(di->bat, &info) < 0)
+             goto out;
+
+     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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+     max = bq27xxx_dm_regs[di->chip][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)
+             goto out;
+
+     bq27xxx_battery_set_config(di, &info);
+
+out:
+     bq27xxx_battery_print_config(di);
+     bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.

@@ -1006,6 +1390,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;

@@ -1039,7 +1430,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);

@@ -1064,6 +1458,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);

Quoted 49 lines · Andrew F. Davis

Quoted 47 lines · Liam Breck

Quoted 32 lines · Andrew F. Davis

Quoted 31 lines · Liam Breck

From: Liam Breck <[email protected]>

Previously there was no way to configure chip registers in the event that the defaults didn't match the battery in question.

BQ27xxx driver now calls power_supply_get_battery_info, checks the inputs, and writes battery data to chip RAM or non-volatile memory.

Signed-off-by Matt Ranostay <[email protected]>

Signed-off-by Liam Breck <[email protected]>

---
 drivers/power/supply/bq27xxx_battery.c | 399 ++++++++++++++++++++++++++++++++-
 1 file changed, 397 insertions(+), 2 deletions(-)

drivers/power/supply/bq27xxx_battery.c +1 -1

index 7475a5f..8085d4a 100644
@@ -51,7 +51,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION               "1.2.0"
+#define DRIVER_VERSION               "1.3.0"
 
 #define BQ27XXX_MANUFACTURER "Texas Instruments"
 

@@ -452,6 +452,99 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 462

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c +8 -0

+/* writable registers */
+#define BQ27XXX_CONTROL                      0x00
+#define BQ27XXX_DATA_CLASS           0x3E
+#define BQ27XXX_DATA_BLOCK           0x3F
+#define BQ27XXX_BLOCK_DATA           0x40
+#define BQ27XXX_BLOCK_DATA_CHECKSUM  0x60
+#define BQ27XXX_BLOCK_DATA_CONTROL   0x61
+

These all belong in the bq27xxx_regs array, you can add new register types to the bq27xxx_reg_index enum.

They're the same on all chips. Let's dup this code 9+ times when we have cause.

Quoted 3 lines

They are not the same on all chips, some chips don't have these registers at all. Plus, BQ27XXX_CONTROL is the same for all chips, and we already have it in all the chips bq27xxx_regs.

Datasheets? Do those chips even support this feature set? Bqtool doesn't appear to support any other memory update scheme.

Quoted 16 lines

Quoted 15 lines

Or make a default array of reg numbers and memcopy it onto each chip array, then alter as req'd per chip. But that's outside our scope.

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 468

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+/* control register params */
+#define BQ27XXX_SEALED                       0x20
+#define BQ27XXX_SET_CFGUPDATE                0x13
+#define BQ27XXX_SOFT_RESET           0x42
+
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)

why?

I think this might vary on diff chips. Also I have a pending change which uses MSLEEP(20)

Quoted 3 lines

If they are different we should delay for the largest needed time, a range means some may not get enough delay, we would always be targeting the lowest common denominator chip anyway.

That will be a task for someone else who has hw to test :-)

Quoted 487 lines

Quoted 22 lines

Quoted 13 lines

Quoted 13 lines

drivers/power/supply/bq27xxx_battery.c +13 -0

+
+struct bq27xxx_dm_reg {
+     u8 subclass_id;
+     u8 offset;
+     u8 bytes;
+     u16 min, max;
+};
+
+struct bq27xxx_dm_buf {
+     u8 class;
+     u8 block;
+     u8 a[32];
+     bool full, updt;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 486

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+};
+
+#define BQ27XXX_DM_BUF_PTR(b, r) \
+     ( (u16*) &(b).a[ (r)->offset % sizeof (b).a ] )
+

This could compete in the IOCCC, there has to be a more readable way to do this..

Quoted 3 lines

Hence the macro. It didn't seem to merit a function, but I could write an inline fn.

That would be nice. With C, the whole kernel could be one long line, but just because you can doesn't mean you should, lets keep this readable for all of our sake :)

Quoted 68 lines

Quoted 59 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 496

Quoted 11 lines

drivers/power/supply/bq27xxx_battery.c +10 -0

+#define BQ27XXX_DM_BUF(di, i) { \
+     .class = bq27xxx_dm_regs[(di)->chip][i].subclass_id, \
+     .block = bq27xxx_dm_regs[(di)->chip][i].offset / sizeof ((struct bq27xxx_dm_buf*)0)->a, \
+}
+
+static inline bool bq27xxx_dm_buf_set(struct bq27xxx_dm_buf *buf,
+                                   struct bq27xxx_dm_reg *reg) {
+     if (buf->class == reg->subclass_id
+      && buf->block == reg->offset / sizeof buf->a
+      && buf->full)

Some alignment issues here.

drivers/power/supply/bq27xxx_battery.c static struct { · line 500

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+             return false;
+     buf->class = reg->subclass_id;
+     buf->block = reg->offset / sizeof buf->a;
+     buf->full = buf->updt = false;

newline before return

Quoted 38 lines

drivers/power/supply/bq27xxx_battery.c +38 -0

+     return true;
+}
+
+enum bq27xxx_dm_reg_id {
+     BQ27XXX_DM_DESIGN_CAPACITY = 0,
+     BQ27XXX_DM_DESIGN_ENERGY,
+     BQ27XXX_DM_TERMINATE_VOLTAGE,
+     BQ27XXX_DM_END,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+     [BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
+static struct bq27xxx_dm_reg bq27425_dm_regs[] = {
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 14, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 10, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 12, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27621_dm_regs[] = { /* not tested */
+     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 82, 3, 2,    0,  8000 },
+     [BQ27XXX_DM_DESIGN_ENERGY]     = { 82, 5, 2,    0, 32767 },
+     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500,  3700 },
+};
+
+static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = {
+     [BQ27421] = bq27421_dm_regs, /* and BQ27441 */
+     [BQ27425] = bq27425_dm_regs,
+/*   [BQ27621] = */ bq27621_dm_regs,

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static struct { · line 544

Quoted 6 lines

drivers/power/supply/bq27xxx_battery.c +6 -0

+};
+
+static u32 bq27xxx_unseal_keys[] = {
+     [BQ27421] = 0x80008000, /* and BQ27441 */
+     [BQ27425] = 0x04143672,
+/*   [BQ27621] = */ 0x80008000,

Why is this commented out.

Quoted 2 lines

The 621 id isn't defined yet. I didn't want to enable something I can't test.

Yes, but you are only commenting out the index, it still is added to the list. You should wait to add that line when the rest of BQ27621 support is added.

Quoted 37 lines

Quoted 28 lines

Quoted 28 lines

drivers/power/supply/bq27xxx_battery.c +21 -0

+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
      struct bq27xxx_device_info *di;

@@ -496,6 +589,297 @@ static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
      return di->bus.read(di, di->regs[reg_index], single);
 }
 
+static int bq27xxx_battery_set_seal_state(struct bq27xxx_device_info *di,
+                                       bool state)
+{
+     u32 key = bq27xxx_unseal_keys[di->chip];
+     int ret;
+
+     if (state) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SEALED, false);
+             if (ret < 0)
+                     goto out;
+     } else {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)(key >> 16), false);
+             if (ret < 0)
+                     goto out;
+
+             ret = di->bus.write(di, BQ27XXX_CONTROL, (u16)key, false);
+             if (ret < 0)
+                     goto out;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 614

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     }
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);

This is not a debug statement, we don't need the function name, just say where it broke.

Quoted 2 lines

What's "where" if not fn name?

For example:

"Bus error while setting seal state"

The actually function name is unimportant, and the user doesn't care.

Quoted 100 lines

Quoted 91 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 623

Quoted 9 lines

drivers/power/supply/bq27xxx_battery.c +9 -0

+     return ret;
+}
+
+static u8 bq27xxx_battery_checksum(struct bq27xxx_dm_buf *buf)
+{
+     u16 sum = 0;
+     int i;
+
+     for (i = 0; i < sizeof buf->a; i++)

Linus would like a word with you:
https://lkml.org/lkml/2012/7/11/103

Quoted 78 lines

drivers/power/supply/bq27xxx_battery.c +78 -0

+             sum += buf->a[i];
+     sum &= 0xff;
+
+     return 0xff - sum;
+}
+
+static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
+                                      struct bq27xxx_dm_buf *buf)
+{
+     int ret;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.read_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out;
+
+     ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+     if (ret < 0)
+             goto out;
+
+     if ((u8)ret != bq27xxx_battery_checksum(buf)) {
+             ret = -EINVAL;
+             goto out;
+     }
+
+     buf->full = true;
+     buf->updt = false;
+     return 0;
+
+out:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     return ret;
+}
+
+static void bq27xxx_battery_print_config(struct bq27xxx_device_info *di)
+{
+     struct bq27xxx_dm_reg *reg = bq27xxx_dm_regs[di->chip];
+     struct bq27xxx_dm_buf buf = { .class = 0xFF };
+     int i;
+
+     for (i = 0; i < BQ27XXX_DM_END; i++, reg++) {
+             const char* str = bq27xxx_dm_reg_name[i];
+
+             if (bq27xxx_dm_buf_set(&buf, reg))
+                     if (bq27xxx_battery_read_dm_block(di, &buf) < 0)
+                             continue;
+
+             if (reg->bytes == 2)
+                     dev_info(di->dev, "config register %s is %d\n", str,
+                              be16_to_cpup(BQ27XXX_DM_BUF_PTR(buf, reg)));
+             else
+                     dev_warn(di->dev, "unsupported config register %s\n", str);
+     }
+}
+
+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 = &bq27xxx_dm_regs[di->chip][reg_id];
+     u16 *prev;
+
+     if (reg->bytes != 2)
+             return;
+
+     prev = BQ27XXX_DM_BUF_PTR(*buf, reg);
+     if (be16_to_cpup(prev) == val)
+             return;

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 706

Quoted 5 lines

drivers/power/supply/bq27xxx_battery.c +5 -0

+     *prev = cpu_to_be16(val);
+
+     dev_info(di->dev, "update chip %d, class %u, block %u, offset %u, value %u\n",
+              di->chip, buf->class, buf->block, reg->offset, val);
+

This should probably be dev_debug, no normal user cares about all this detail.

Quoted 4 lines

It's only on change to NVM, which a hw developer (like me) wants to know about. I plan to remove the print_config routine so this is not redundant.

HW developers should then have debug enabled for drivers they care about. For everyone else this is just kernel log clutter.

Quoted 45 lines

Quoted 36 lines

Quoted 36 lines

drivers/power/supply/bq27xxx_battery.c +36 -0

+     buf->updt = true;
+     return;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+                                       struct bq27xxx_dm_buf *buf)
+{
+     bool cfgup = di->chip == BQ27425 || di->chip == BQ27421; /* || BQ27441 || BQ27621 */
+     int ret;
+
+     if (cfgup) {
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SET_CFGUPDATE, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CONTROL, 0, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_CLASS, buf->class, true);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+     if (ret < 0)
+             goto out2;
+
+     BQ27XXX_MSLEEP(1);
+
+     ret = di->bus.write_bulk(di, BQ27XXX_BLOCK_DATA, buf->a, sizeof buf->a);
+     if (ret < 0)
+             goto out2;
+
+     ret = di->bus.write(di, BQ27XXX_BLOCK_DATA_CHECKSUM,
+                         bq27xxx_battery_checksum(buf), true);

Quoted 8 lines

drivers/power/supply/bq27xxx_battery.c static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index, · line 746

Quoted 4 lines

drivers/power/supply/bq27xxx_battery.c +4 -0

+     if (ret < 0)
+             goto out2;
+
+     /* THE FOLLOWING CODE IS TOXIC. DO NOT USE!

Then why include it, what is this code supposed to do, and what do we lose without it? If it is really needed we can find a way to guarantee the delay length.

Quoted 5 lines

This triggers a chip bug, and future devs need to know to avoid it. I was tempted by bqtool into using it for the extra error check, and now my gauge isn't working, tho I have only reset one NVM block back to factory defaults. Bqtool is easy to find, if poorly written.

Just the warning would be enough then, we don't gain anything by writing out the exact code to break your device in the comment also.

Quoted 174 lines

Quoted 174 lines

Quoted 174 lines

drivers/power/supply/bq27xxx_battery.c +148 -1

+      * If the 350ms delay is insufficient, NVM corruption results
+      * on the '425 chip, which could damage the chip.
+      * It was suggested in this TI tool:
+      *   http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+      *
+      * BQ27XXX_MSLEEP(350);
+      * ret = di->bus.write(di, BQ27XXX_DATA_BLOCK, buf->block, true);
+      * BQ27XXX_MSLEEP(1);
+      * ret = di->bus.read(di, BQ27XXX_BLOCK_DATA_CHECKSUM, true);
+      * if ((u8)ret != bq27xxx_battery_checksum(buf))
+      *      ret = -EINVAL;
+      */
+
+     if (cfgup) {
+             BQ27XXX_MSLEEP(1);
+             ret = di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+             if (ret < 0)
+                     goto out1;
+     }
+
+     buf->updt = false;
+     return 0;
+
+out2:
+     if (cfgup)
+             di->bus.write(di, BQ27XXX_CONTROL, BQ27XXX_SOFT_RESET, false);
+out1:
+     dev_err(di->dev, "bus error in %s: %d\n", __func__, ret);
+     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_ENERGY);
+     struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+     if (info->charge_full_design_uah != -EINVAL
+      && info->energy_full_design_uwh != -EINVAL) {
+             bq27xxx_battery_read_dm_block(di, &bd);
+             if (bd.full) {
+                     /* 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.full && bd.class == bt.class && bd.block == bt.block;
+             if (!same)
+                     bq27xxx_battery_read_dm_block(di, &bt);
+             if (same ? bd.full : bt.full)
+                     bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+                                     BQ27XXX_DM_TERMINATE_VOLTAGE,
+                                     info->voltage_min_design_uv / 1000);
+     }
+
+     if (bd.updt)
+             bq27xxx_battery_write_dm_block(di, &bd);
+     if (bt.updt)
+             bq27xxx_battery_write_dm_block(di, &bt);
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+     struct power_supply_battery_info info = {};
+     unsigned int min, max;
+
+     /* functions don't exist for writing data so abort */
+     if (!di->bus.write || !di->bus.write_bulk)
+             return;
+
+     /* no settings to be set for this chipset so abort */
+     if (!bq27xxx_dm_regs[di->chip])
+             return;
+
+     if (bq27xxx_battery_set_seal_state(di, false) < 0)
+             return;
+
+     if (power_supply_get_battery_info(di->bat, &info) < 0)
+             goto out;
+
+     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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][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 = bq27xxx_dm_regs[di->chip][BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+     max = bq27xxx_dm_regs[di->chip][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)
+             goto out;
+
+     bq27xxx_battery_set_config(di, &info);
+
+out:
+     bq27xxx_battery_print_config(di);
+     bq27xxx_battery_set_seal_state(di, true);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.

@@ -1006,6 +1390,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;

@@ -1039,7 +1430,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);

@@ -1064,6 +1458,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);