[PATCH v12.2 0/5] bq27xxx_battery partial series

[PATCH v12.2 0/5] bq27xxx_battery partial series

[Liam Breck]

Changes in this rev:

Removed macro for if (...) dev_dbg(...) from I/O helper functions.

Moved update_dm_block() to patch "Add power_supply_battery_info support".
No more refactoring please; it's not really improving the code itself :-)

set_cfgupdate is required for DM write on certain chips, see this Q&A
https://e2e.ti.com/support/power_management/battery_management/f/180/p/577798/2121489#2121489

Renamed dm_buf .full => .has_data, .updt => .dirty
dm_buf has 3 states: !has_data, has_data && !dirty, has_data && dirty
I considered a single field and enum values, but I think this is clearer.

Renamed set_cfgupdate(..., u16 flag => state)
We call set_cfgupdate(di, BQ27XXX_FLAG_CFGUP or 0)
We !!state to print the intended flag state in error msg.


  power: bq27xxx_battery: Add bulk transfer bus methods
  power: bq27xxx_battery: Add chip data memory read/write support
  power: bq27xxx_battery: Add power_supply_battery_info support
  power: bq27xxx_battery: Enable chip data memory update for certain chips
  power: bq27xxx_battery: Remove duplicate register arrays

 drivers/power/supply/bq27xxx_battery.c     | 700 +++++++++++++++++++++++------
 drivers/power/supply/bq27xxx_battery_i2c.c |  98 +++-
 include/linux/power/bq27xxx_battery.h      |  26 +-
 3 files changed, 682 insertions(+), 142 deletions(-)

-- 
2.9.3

[PATCH v12.2 5/9] power: bq27xxx_battery: Add bulk transfer bus methods

[Liam Breck]

From: Matt Ranostay <matt@ranostay.consulting>

Declare bus.write/read_bulk/write_bulk().
Add I2C write/read_bulk/write_bulk() to implement the above.
Add bq27xxx_write/read_block/write_block() helper functions to call the above.

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c     | 58 ++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c | 82 +++++++++++++++++++++++++++++-
 include/linux/power/bq27xxx_battery.h      |  3 ++
 3 files changed, 140 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 398801a..e3472c3 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -794,11 +794,65 @@ MODULE_PARM_DESC(poll_interval,
 static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
 			       bool single)
 {
-	/* Reports EINVAL for invalid/missing registers */
+	int ret;
+
 	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
 		return -EINVAL;
 
-	return di->bus.read(di, di->regs[reg_index], single);
+	ret = di->bus.read(di, di->regs[reg_index], single);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
+		        di->regs[reg_index], reg_index);
+
+	return ret;
+}
+
+static inline int bq27xxx_write(struct bq27xxx_device_info *di, int reg_index,
+				u16 value, bool single)
+{
+	int ret;
+
+	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write)
+		return -EINVAL;
+
+	ret = di->bus.write(di, di->regs[reg_index], value, single);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to write register 0x%02x (index %d)\n",
+		        di->regs[reg_index], reg_index);
+
+	return ret;
+}
+
+static inline int bq27xxx_read_block(struct bq27xxx_device_info *di, int reg_index,
+				     u8 *data, int len)
+{
+	int ret;
+
+	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.read_bulk)
+		return -EINVAL;
+
+	ret = di->bus.read_bulk(di, di->regs[reg_index], data, len);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to read_bulk register 0x%02x (index %d)\n",
+		        di->regs[reg_index], reg_index);
+
+	return ret;
+}
+
+static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_index,
+				      u8 *data, int len)
+{
+	int ret;
+
+	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write_bulk)
+		return -EINVAL;
+
+	ret = di->bus.write_bulk(di, di->regs[reg_index], data, len);
+	if (ret < 0)
+		dev_dbg(di->dev, "failed to write_bulk register 0x%02x (index %d)\n",
+		        di->regs[reg_index], reg_index);
+
+	return ret;
 }
 
 /*
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index c68fbc3..a597221 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -38,7 +38,7 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 {
 	struct i2c_client *client = to_i2c_client(di->dev);
 	struct i2c_msg msg[2];
-	unsigned char data[2];
+	u8 data[2];
 	int ret;
 
 	if (!client->adapter)
@@ -68,6 +68,82 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
 	return ret;
 }
 
+static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
+				     int value, bool single)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	u8 data[4];
+	int ret;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	data[0] = reg;
+	if (single) {
+		data[1] = (u8) value;
+		msg.len = 2;
+	} else {
+		put_unaligned_le16(value, &data[1]);
+		msg.len = 3;
+	}
+
+	msg.buf = data;
+	msg.addr = client->addr;
+	msg.flags = 0;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EINVAL;
+	return 0;
+}
+
+static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
+					 u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	int ret;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	ret = i2c_smbus_read_i2c_block_data(client, reg, len, data);
+	if (ret < 0)
+		return ret;
+	if (ret != len)
+		return -EINVAL;
+	return 0;
+}
+
+static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
+					  u8 reg, u8 *data, int len)
+{
+	struct i2c_client *client = to_i2c_client(di->dev);
+	struct i2c_msg msg;
+	u8 buf[33];
+	int ret;
+
+	if (!client->adapter)
+		return -ENODEV;
+
+	buf[0] = reg;
+	memcpy(&buf[1], data, len);
+
+	msg.buf = buf;
+	msg.addr = client->addr;
+	msg.flags = 0;
+	msg.len = len + 1;
+
+	ret = i2c_transfer(client->adapter, &msg, 1);
+	if (ret < 0)
+		return ret;
+	if (ret != 1)
+		return -EINVAL;
+	return 0;
+}
+
 static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 				     const struct i2c_device_id *id)
 {
@@ -95,7 +171,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
 	di->dev = &client->dev;
 	di->chip = id->driver_data;
 	di->name = name;
+
 	di->bus.read = bq27xxx_battery_i2c_read;
+	di->bus.write = bq27xxx_battery_i2c_write;
+	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
+	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
 
 	ret = bq27xxx_battery_setup(di);
 	if (ret)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b312bce..c3369fa 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -40,6 +40,9 @@ struct bq27xxx_platform_data {
 struct bq27xxx_device_info;
 struct bq27xxx_access_methods {
 	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
+	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
+	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
+	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
 };
 
 struct bq27xxx_reg_cache {
-- 
2.9.3

[PATCH v12.2 6/9] power: bq27xxx_battery: Add chip data memory read/write support

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Add the following to enable read/write of chip data memory (DM) RAM/NVM/flash:
  bq27xxx_battery_seal()
  bq27xxx_battery_unseal()
  bq27xxx_battery_set_cfgupdate()
  bq27xxx_battery_read_dm_block()
  bq27xxx_battery_write_dm_block()
  bq27xxx_battery_checksum_dm_block()

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 285 +++++++++++++++++++++++++++++++++
 include/linux/power/bq27xxx_battery.h  |   1 +
 2 files changed, 286 insertions(+)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index e3472c3..dbcf14a 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -65,6 +65,7 @@
 #define BQ27XXX_FLAG_DSC	BIT(0)
 #define BQ27XXX_FLAG_SOCF	BIT(1) /* State-of-Charge threshold final */
 #define BQ27XXX_FLAG_SOC1	BIT(2) /* State-of-Charge threshold 1 */
+#define BQ27XXX_FLAG_CFGUP	BIT(4)
 #define BQ27XXX_FLAG_FC		BIT(9)
 #define BQ27XXX_FLAG_OTD	BIT(14)
 #define BQ27XXX_FLAG_OTC	BIT(15)
@@ -78,6 +79,12 @@
 #define BQ27000_FLAG_FC		BIT(5)
 #define BQ27000_FLAG_CHGS	BIT(7) /* Charge state flag */
 
+/* control register params */
+#define BQ27XXX_SEALED			0x20
+#define BQ27XXX_SET_CFGUPDATE		0x13
+#define BQ27XXX_SOFT_RESET		0x42
+#define BQ27XXX_RESET			0x41
+
 #define BQ27XXX_RS			(20) /* Resistor sense mOhm */
 #define BQ27XXX_POWER_CONSTANT		(29200) /* 29.2 µV^2 * 1000 */
 #define BQ27XXX_CURRENT_CONSTANT	(3570) /* 3.57 µV * 1000 */
@@ -108,6 +115,11 @@ enum bq27xxx_reg_index {
 	BQ27XXX_REG_SOC,	/* State-of-Charge */
 	BQ27XXX_REG_DCAP,	/* Design Capacity */
 	BQ27XXX_REG_AP,		/* Average Power */
+	BQ27XXX_DM_CTRL,	/* Block Data Control */
+	BQ27XXX_DM_CLASS,	/* Data Class */
+	BQ27XXX_DM_BLOCK,	/* Data Block */
+	BQ27XXX_DM_DATA,	/* Block Data */
+	BQ27XXX_DM_CKSUM,	/* Block Data Checksum */
 	BQ27XXX_REG_MAX,	/* sentinel */
 };
 
@@ -131,6 +143,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x0b,
 		[BQ27XXX_REG_DCAP] = 0x76,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
 	[BQ27010] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -150,6 +167,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x0b,
 		[BQ27XXX_REG_DCAP] = 0x76,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
 	[BQ2750X] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -169,6 +191,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ2751X] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -188,6 +215,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = 0x2e,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27500] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -207,6 +239,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G1] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -226,6 +263,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G2] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -245,6 +287,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27510G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -264,6 +311,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = 0x2e,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G1] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -283,6 +335,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G2] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -302,6 +359,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -321,6 +383,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27520G4] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -340,6 +407,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x20,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27530] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -359,6 +431,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27541] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -378,6 +455,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27545] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -397,6 +479,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x2c,
 		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
 		[BQ27XXX_REG_AP] = 0x24,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 	[BQ27421] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
@@ -416,6 +503,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_SOC] = 0x1c,
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x18,
+		[BQ27XXX_DM_CTRL] = 0x61,
+		[BQ27XXX_DM_CLASS] = 0x3e,
+		[BQ27XXX_DM_BLOCK] = 0x3f,
+		[BQ27XXX_DM_DATA] = 0x40,
+		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
 };
 
@@ -757,6 +849,18 @@ static struct {
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 
+#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)
+
+#define BQ27XXX_DM_SZ	32
+
+struct bq27xxx_dm_buf {
+	u8 class;
+	u8 block;
+	u8 data[BQ27XXX_DM_SZ];
+	bool has_data, dirty;
+};
+
+
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
 	struct bq27xxx_device_info *di;
@@ -855,6 +959,187 @@ static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_in
 	return ret;
 }
 
+static int bq27xxx_battery_seal(struct bq27xxx_device_info *di)
+{
+	int ret;
+
+	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_SEALED, false);
+	if (ret >= 0)
+		return 0;
+
+	dev_err(di->dev, "bus error on seal: %d\n", ret);
+	return ret;
+}
+
+static int bq27xxx_battery_unseal(struct bq27xxx_device_info *di)
+{
+	int ret;
+
+	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)(di->unseal_key >> 16), false);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)di->unseal_key, false);
+	if (ret < 0)
+		goto out;
+
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error on unseal: %d\n", ret);
+	return ret;
+}
+
+static u8 bq27xxx_battery_checksum_dm_block(struct bq27xxx_dm_buf *buf)
+{
+	u16 sum = 0;
+	int i;
+
+	for (i = 0; i < BQ27XXX_DM_SZ; i++)
+		sum += buf->data[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;
+
+	buf->has_data = false;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out;
+
+	BQ27XXX_MSLEEP(1);
+
+	ret = bq27xxx_read_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_read(di, BQ27XXX_DM_CKSUM, true);
+	if (ret < 0)
+		goto out;
+
+	if ((u8)ret != bq27xxx_battery_checksum_dm_block(buf)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
+	buf->has_data = true;
+	buf->dirty = false;
+
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error reading chip memory: %d\n", ret);
+	return ret;
+}
+
+static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
+{
+	const int limit = 100;
+	int ret, try = limit;
+
+	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL,
+			    state ? BQ27XXX_SET_CFGUPDATE : BQ27XXX_SOFT_RESET,
+			    false);
+	if (ret < 0)
+		goto out;
+
+	do {
+		BQ27XXX_MSLEEP(25);
+		ret = di->bus.read(di, di->regs[BQ27XXX_REG_FLAGS], false);
+		if (ret < 0)
+			goto out;
+	} while ((ret & BQ27XXX_FLAG_CFGUP) != state && --try);
+
+	if (!try) {
+		dev_err(di->dev, "timed out waiting for cfgupdate flag %d\n", !!state);
+		return -EINVAL;
+	}
+
+	if (limit-try > 3)
+		dev_warn(di->dev, "cfgupdate %d, retries %d\n", !!state, limit-try);
+
+	return 0;
+
+out:
+	dev_err(di->dev, "bus error on %s: %d\n", state ? "set_cfgupdate" : "soft_reset", ret);
+	return ret;
+}
+
+static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
+					  struct bq27xxx_dm_buf *buf)
+{
+	bool cfgup = di->chip == BQ27421; /* assume group supports cfgupdate */
+	int ret;
+
+	if (!buf->dirty)
+		return 0;
+
+	if (cfgup) {
+		ret = bq27xxx_battery_set_cfgupdate(di, BQ27XXX_FLAG_CFGUP);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CTRL, 0, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
+	if (ret < 0)
+		goto out;
+
+	BQ27XXX_MSLEEP(1);
+
+	ret = bq27xxx_write_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
+	if (ret < 0)
+		goto out;
+
+	ret = bq27xxx_write(di, BQ27XXX_DM_CKSUM,
+			    bq27xxx_battery_checksum_dm_block(buf), true);
+	if (ret < 0)
+		goto out;
+
+	/* DO NOT read BQ27XXX_DM_CKSUM here to verify it! That may cause NVM
+	 * corruption on the '425 chip (and perhaps others), which can damage
+	 * the chip. See TI bqtool for what not to do:
+	 * http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
+	 */
+
+	if (cfgup) {
+		BQ27XXX_MSLEEP(1);
+		ret = bq27xxx_battery_set_cfgupdate(di, 0);
+		if (ret < 0)
+			return ret;
+	} else {
+		BQ27XXX_MSLEEP(100); /* flash DM updates in <100ms */
+	}
+
+	buf->dirty = false;
+
+	return 0;
+
+out:
+	if (cfgup)
+		bq27xxx_battery_set_cfgupdate(di, 0);
+
+	dev_err(di->dev, "bus error writing chip memory: %d\n", ret);
+	return ret;
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index c3369fa..b1defb8 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -64,6 +64,7 @@ struct bq27xxx_device_info {
 	int id;
 	enum bq27xxx_chip chip;
 	const char *name;
+	u32 unseal_key;
 	struct bq27xxx_access_methods bus;
 	struct bq27xxx_reg_cache cache;
 	int charge_design_full;
-- 
2.9.3

[PATCH v12.2 7/9] power: bq27xxx_battery: Add power_supply_battery_info support

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

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

We now call power_supply_get_battery_info(), check its values, and write
battery data to chip data memory (flash/RAM/NVM).

Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 172 ++++++++++++++++++++++++++++++++-
 include/linux/power/bq27xxx_battery.h  |   1 +
 2 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index dbcf14a..4c7a13e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -853,6 +853,13 @@ static LIST_HEAD(bq27xxx_battery_devices);
 
 #define BQ27XXX_DM_SZ	32
 
+struct bq27xxx_dm_reg {
+	u8 subclass_id;
+	u8 offset;
+	u8 bytes;
+	u16 min, max;
+};
+
 struct bq27xxx_dm_buf {
 	u8 class;
 	u8 block;
@@ -860,6 +867,33 @@ struct bq27xxx_dm_buf {
 	bool has_data, dirty;
 };
 
+#define BQ27XXX_DM_BUF(di, i) { \
+	.class = (di)->dm_regs[i].subclass_id, \
+	.block = (di)->dm_regs[i].offset / BQ27XXX_DM_SZ, \
+}
+
+static inline u16* bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf,
+				      struct bq27xxx_dm_reg *reg)
+{
+	if (buf->class == reg->subclass_id &&
+	    buf->block == reg->offset / BQ27XXX_DM_SZ)
+		return (u16*) (buf->data + reg->offset % BQ27XXX_DM_SZ);
+
+	return NULL;
+}
+
+enum bq27xxx_dm_reg_id {
+	BQ27XXX_DM_DESIGN_CAPACITY = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+};
+
+static const char* bq27xxx_dm_reg_name[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
+	[BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
+};
+
 
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
@@ -1042,6 +1076,39 @@ static int bq27xxx_battery_read_dm_block(struct bq27xxx_device_info *di,
 	return ret;
 }
 
+static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di,
+					    struct bq27xxx_dm_buf *buf,
+					    enum bq27xxx_dm_reg_id reg_id,
+					    unsigned int val)
+{
+	struct bq27xxx_dm_reg *reg = &di->dm_regs[reg_id];
+	const char *str = bq27xxx_dm_reg_name[reg_id];
+	u16 *prev = bq27xxx_dm_reg_ptr(buf, reg);
+
+	if (prev == NULL) {
+		dev_warn(di->dev, "buffer does not match %s dm spec\n", str);
+		return;
+	}
+
+	if (reg->bytes != 2) {
+		dev_warn(di->dev, "%s dm spec has unsupported byte size\n", str);
+		return;
+	}
+
+	if (!buf->has_data)
+		return;
+
+	if (be16_to_cpup(prev) == val) {
+		dev_info(di->dev, "%s has %u\n", str, val);
+		return;
+	}
+
+	dev_info(di->dev, "update %s to %u\n", str, val);
+
+	*prev = cpu_to_be16(val);
+	buf->dirty = true;
+}
+
 static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
 {
 	const int limit = 100;
@@ -1140,6 +1207,98 @@ static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
 	return ret;
 }
 
+static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
+				       struct power_supply_battery_info *info)
+{
+	struct bq27xxx_dm_buf bd = BQ27XXX_DM_BUF(di, BQ27XXX_DM_DESIGN_CAPACITY);
+	struct bq27xxx_dm_buf bt = BQ27XXX_DM_BUF(di, BQ27XXX_DM_TERMINATE_VOLTAGE);
+
+	if (bq27xxx_battery_unseal(di) < 0)
+		return;
+
+	if (info->charge_full_design_uah != -EINVAL &&
+	    info->energy_full_design_uwh != -EINVAL) {
+		bq27xxx_battery_read_dm_block(di, &bd);
+		/* assume design energy & capacity are in same block */
+		bq27xxx_battery_update_dm_block(di, &bd,
+					BQ27XXX_DM_DESIGN_CAPACITY,
+					info->charge_full_design_uah / 1000);
+		bq27xxx_battery_update_dm_block(di, &bd,
+					BQ27XXX_DM_DESIGN_ENERGY,
+					info->energy_full_design_uwh / 1000);
+	}
+
+	if (info->voltage_min_design_uv != -EINVAL) {
+		bool same = bd.class == bt.class && bd.block == bt.block;
+		if (!same)
+			bq27xxx_battery_read_dm_block(di, &bt);
+		bq27xxx_battery_update_dm_block(di, same ? &bd : &bt,
+					BQ27XXX_DM_TERMINATE_VOLTAGE,
+					info->voltage_min_design_uv / 1000);
+	}
+
+	bq27xxx_battery_write_dm_block(di, &bd);
+	bq27xxx_battery_write_dm_block(di, &bt);
+
+	bq27xxx_battery_seal(di);
+
+	if (di->chip != BQ27421) { /* not a cfgupdate chip, so reset */
+		bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false);
+		BQ27XXX_MSLEEP(300); /* reset time is not documented */
+	}
+	/* assume bq27xxx_battery_update() is called hereafter */
+}
+
+void bq27xxx_battery_settings(struct bq27xxx_device_info *di)
+{
+	struct power_supply_battery_info info = {};
+	unsigned int min, max;
+
+	if (!di->dm_regs)
+		return;
+
+	if (power_supply_get_battery_info(di->bat, &info) < 0)
+		return;
+
+	if (info.energy_full_design_uwh != info.charge_full_design_uah) {
+		if (info.energy_full_design_uwh == -EINVAL)
+			dev_warn(di->dev, "missing battery:energy-full-design-microwatt-hours\n");
+		else if (info.charge_full_design_uah == -EINVAL)
+			dev_warn(di->dev, "missing battery:charge-full-design-microamp-hours\n");
+	}
+
+	/* assume min == 0 */
+	max = di->dm_regs[BQ27XXX_DM_DESIGN_ENERGY].max;
+	if (info.energy_full_design_uwh > max * 1000) {
+		dev_err(di->dev, "invalid battery:energy-full-design-microwatt-hours %d\n",
+			info.energy_full_design_uwh);
+		info.energy_full_design_uwh = -EINVAL;
+	}
+
+	/* assume min == 0 */
+	max = di->dm_regs[BQ27XXX_DM_DESIGN_CAPACITY].max;
+	if (info.charge_full_design_uah > max * 1000) {
+		dev_err(di->dev, "invalid battery:charge-full-design-microamp-hours %d\n",
+			info.charge_full_design_uah);
+		info.charge_full_design_uah = -EINVAL;
+	}
+
+	min = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].min;
+	max = di->dm_regs[BQ27XXX_DM_TERMINATE_VOLTAGE].max;
+	if ((info.voltage_min_design_uv < min * 1000 ||
+	     info.voltage_min_design_uv > max * 1000) &&
+	     info.voltage_min_design_uv != -EINVAL) {
+		dev_err(di->dev, "invalid battery:voltage-min-design-microvolt %d\n",
+			info.voltage_min_design_uv);
+		info.voltage_min_design_uv = -EINVAL;
+	}
+
+	if ((info.energy_full_design_uwh != -EINVAL &&
+	     info.charge_full_design_uah != -EINVAL) ||
+	     info.voltage_min_design_uv  != -EINVAL)
+		bq27xxx_battery_set_config(di, &info);
+}
+
 /*
  * Return the battery State-of-Charge
  * Or < 0 if something fails.
@@ -1657,6 +1816,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;
@@ -1690,7 +1856,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);
@@ -1715,6 +1884,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	dev_info(di->dev, "support ver. %s enabled\n", DRIVER_VERSION);
 
+	bq27xxx_battery_settings(di);
 	bq27xxx_battery_update(di);
 
 	mutex_lock(&bq27xxx_list_lock);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index b1defb8..227eb08 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -64,6 +64,7 @@ struct bq27xxx_device_info {
 	int id;
 	enum bq27xxx_chip chip;
 	const char *name;
+	struct bq27xxx_dm_reg *dm_regs;
 	u32 unseal_key;
 	struct bq27xxx_access_methods bus;
 	struct bq27xxx_reg_cache cache;
-- 
2.9.3

[PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Support data memory update of BQ27500, 545, 421, 425, 441, 621.

Create IDs for for previously unID'd chips, to index arrays for unseal keys
and data memory register tables.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
 include/linux/power/bq27xxx_battery.h      | 11 +++++
 3 files changed, 90 insertions(+), 10 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 4c7a13e..70b8d1c 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -57,7 +57,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION		"1.2.0"
+#define DRIVER_VERSION		"1.3.0"
 
 #define BQ27XXX_MANUFACTURER	"Texas Instruments"
 
@@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
 	[BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
 };
 
+static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
+};
+
+static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
+	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
+	[BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
+	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
+};
+
+static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
+	[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 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 bq27621_dm_regs[] = {
+	[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[] = {
+	[BQ27500] = bq27500_dm_regs,
+	[BQ27545] = bq27545_dm_regs,
+	[BQ27421] = bq27421_dm_regs,
+	[BQ27425] = bq27425_dm_regs,
+	[BQ27441] = bq27421_dm_regs,
+	[BQ27621] = bq27621_dm_regs,
+};
+
+static u32 bq27xxx_unseal_keys[] = {
+	[BQ27500] = 0x04143672,
+	[BQ27545] = 0x04143672,
+	[BQ27421] = 0x80008000,
+	[BQ27425] = 0x04143672,
+	[BQ27441] = 0x80008000,
+	[BQ27621] = 0x80008000,
+};
+
 
 static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
 {
@@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 		.drv_data = di,
 	};
 
+	di->unseal_key = bq27xxx_unseal_keys[di->chip];
+	di->dm_regs = bq27xxx_dm_regs[di->chip];
+
+	switch(di->chip) {  /* translate members of categories */
+	case BQ2752X:
+		di->chip = BQ27510G3; break;
+	case BQ27531:
+		di->chip = BQ27530;   break;
+	case BQ27542:
+	case BQ27546:
+	case BQ27742:
+		di->chip = BQ27541;   break;
+	case BQ27425:
+	case BQ27441:
+	case BQ27621:
+		di->chip = BQ27421;   break;
+	default:
+		break;
+	}
+
+	di->regs = bq27xxx_regs[di->chip];
+
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
-	di->regs = bq27xxx_regs[di->chip];
 
 	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (!psy_desc)
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index a597221..0b11ed4 100644
--- a/drivers/power/supply/bq27xxx_battery_i2c.c
+++ b/drivers/power/supply/bq27xxx_battery_i2c.c
@@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27210", BQ27010 },
 	{ "bq27500", BQ2750X },
 	{ "bq27510", BQ2751X },
-	{ "bq27520", BQ2751X },
+	{ "bq27520", BQ2752X },
 	{ "bq27500-1", BQ27500 },
 	{ "bq27510g1", BQ27510G1 },
 	{ "bq27510g2", BQ27510G2 },
@@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
 	{ "bq27520g3", BQ27520G3 },
 	{ "bq27520g4", BQ27520G4 },
 	{ "bq27530", BQ27530 },
-	{ "bq27531", BQ27530 },
+	{ "bq27531", BQ27531 },
 	{ "bq27541", BQ27541 },
-	{ "bq27542", BQ27541 },
-	{ "bq27546", BQ27541 },
-	{ "bq27742", BQ27541 },
+	{ "bq27542", BQ27542 },
+	{ "bq27546", BQ27546 },
+	{ "bq27742", BQ27742 },
 	{ "bq27545", BQ27545 },
 	{ "bq27421", BQ27421 },
-	{ "bq27425", BQ27421 },
-	{ "bq27441", BQ27421 },
-	{ "bq27621", BQ27421 },
+	{ "bq27425", BQ27425 },
+	{ "bq27441", BQ27441 },
+	{ "bq27621", BQ27621 },
 	{},
 };
 MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 227eb08..014e59f 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -2,6 +2,7 @@
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+	/* categories; index for bq27xxx_regs[] */
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
@@ -18,6 +19,16 @@ enum bq27xxx_chip {
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
 	BQ27545, /* bq27545 */
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+
+	/* members of categories; translate these to category in _setup() */
+	BQ2752X, /* deprecated alias */
+	BQ27531,
+	BQ27542,
+	BQ27546,
+	BQ27742,
+	BQ27425,
+	BQ27441,
+	BQ27621,
 };
 
 /**
-- 
2.9.3

[PATCH v12.2 9/9] power: bq27xxx_battery: Remove duplicate register arrays

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

BQ2751X & BQ27510G3 are identical.
BQ27500 & BQ27510G1 & BQ27510G2 are identical.
Make BQ2751X & BQ27510G1/2 category members.
Remove the duplicate arrays, etc.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 142 +--------------------------------
 include/linux/power/bq27xxx_battery.h  |  12 +--
 2 files changed, 10 insertions(+), 144 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 70b8d1c..6cf0178 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -197,30 +197,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = 0x40,
 		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
-	[BQ2751X] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = 0x28,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_TTES] = 0x1a,
-		[BQ27XXX_REG_TTECP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x1e,
-		[BQ27XXX_REG_AE] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_SOC] = 0x20,
-		[BQ27XXX_REG_DCAP] = 0x2e,
-		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
 	[BQ27500] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -245,54 +221,6 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = 0x40,
 		[BQ27XXX_DM_CKSUM] = 0x60,
 	},
-	[BQ27510G1] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = 0x18,
-		[BQ27XXX_REG_TTES] = 0x1c,
-		[BQ27XXX_REG_TTECP] = 0x26,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x2a,
-		[BQ27XXX_REG_AE] = 0x22,
-		[BQ27XXX_REG_SOC] = 0x2c,
-		[BQ27XXX_REG_DCAP] = 0x3c,
-		[BQ27XXX_REG_AP] = 0x24,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
-	[BQ27510G2] = {
-		[BQ27XXX_REG_CTRL] = 0x00,
-		[BQ27XXX_REG_TEMP] = 0x06,
-		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
-		[BQ27XXX_REG_VOLT] = 0x08,
-		[BQ27XXX_REG_AI] = 0x14,
-		[BQ27XXX_REG_FLAGS] = 0x0a,
-		[BQ27XXX_REG_TTE] = 0x16,
-		[BQ27XXX_REG_TTF] = 0x18,
-		[BQ27XXX_REG_TTES] = 0x1c,
-		[BQ27XXX_REG_TTECP] = 0x26,
-		[BQ27XXX_REG_NAC] = 0x0c,
-		[BQ27XXX_REG_FCC] = 0x12,
-		[BQ27XXX_REG_CYCT] = 0x2a,
-		[BQ27XXX_REG_AE] = 0x22,
-		[BQ27XXX_REG_SOC] = 0x2c,
-		[BQ27XXX_REG_DCAP] = 0x3c,
-		[BQ27XXX_REG_AP] = 0x24,
-		[BQ27XXX_DM_CTRL] = 0x61,
-		[BQ27XXX_DM_CLASS] = 0x3e,
-		[BQ27XXX_DM_BLOCK] = 0x3f,
-		[BQ27XXX_DM_DATA] = 0x40,
-		[BQ27XXX_DM_CKSUM] = 0x60,
-	},
 	[BQ27510G3] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -571,24 +499,6 @@ static enum power_supply_property bq2750x_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq2751x_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
 static enum power_supply_property bq27500_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -610,48 +520,6 @@ static enum power_supply_property bq27500_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27510g1_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_ENERGY_NOW,
-	POWER_SUPPLY_PROP_POWER_AVG,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
-static enum power_supply_property bq27510g2_battery_props[] = {
-	POWER_SUPPLY_PROP_STATUS,
-	POWER_SUPPLY_PROP_PRESENT,
-	POWER_SUPPLY_PROP_VOLTAGE_NOW,
-	POWER_SUPPLY_PROP_CURRENT_NOW,
-	POWER_SUPPLY_PROP_CAPACITY,
-	POWER_SUPPLY_PROP_CAPACITY_LEVEL,
-	POWER_SUPPLY_PROP_TEMP,
-	POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW,
-	POWER_SUPPLY_PROP_TIME_TO_FULL_NOW,
-	POWER_SUPPLY_PROP_TECHNOLOGY,
-	POWER_SUPPLY_PROP_CHARGE_FULL,
-	POWER_SUPPLY_PROP_CHARGE_NOW,
-	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
-	POWER_SUPPLY_PROP_CYCLE_COUNT,
-	POWER_SUPPLY_PROP_ENERGY_NOW,
-	POWER_SUPPLY_PROP_POWER_AVG,
-	POWER_SUPPLY_PROP_HEALTH,
-	POWER_SUPPLY_PROP_MANUFACTURER,
-};
-
 static enum power_supply_property bq27510g3_battery_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
@@ -831,10 +699,7 @@ static struct {
 	BQ27XXX_PROP(BQ27000, bq27000_battery_props),
 	BQ27XXX_PROP(BQ27010, bq27010_battery_props),
 	BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
-	BQ27XXX_PROP(BQ2751X, bq2751x_battery_props),
 	BQ27XXX_PROP(BQ27500, bq27500_battery_props),
-	BQ27XXX_PROP(BQ27510G1, bq27510g1_battery_props),
-	BQ27XXX_PROP(BQ27510G2, bq27510g2_battery_props),
 	BQ27XXX_PROP(BQ27510G3, bq27510g3_battery_props),
 	BQ27XXX_PROP(BQ27520G1, bq27520g1_battery_props),
 	BQ27XXX_PROP(BQ27520G2, bq27520g2_battery_props),
@@ -1547,10 +1412,7 @@ static bool bq27xxx_battery_overtemp(struct bq27xxx_device_info *di, u16 flags)
 {
 	switch (di->chip) {
 	case BQ2750X:
-	case BQ2751X:
 	case BQ27500:
-	case BQ27510G1:
-	case BQ27510G2:
 	case BQ27510G3:
 	case BQ27520G1:
 	case BQ27520G2:
@@ -1913,8 +1775,12 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	di->dm_regs = bq27xxx_dm_regs[di->chip];
 
 	switch(di->chip) {  /* translate members of categories */
+	case BQ2751X:
 	case BQ2752X:
 		di->chip = BQ27510G3; break;
+	case BQ27510G1:
+	case BQ27510G2:
+		di->chip = BQ27500;   break;
 	case BQ27531:
 		di->chip = BQ27530;   break;
 	case BQ27542:
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 014e59f..81f7ffa 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -6,11 +6,8 @@ enum bq27xxx_chip {
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
-	BQ2751X, /* bq27510, bq27520 deprecated alias */
-	BQ27500, /* bq27500/1 */
-	BQ27510G1, /* bq27510G1 */
-	BQ27510G2, /* bq27510G2 */
-	BQ27510G3, /* bq27510G3 */
+	BQ27500, /* bq27500/1, bq27510G1, bq27510G2 */
+	BQ27510G3, /* bq27510G3, bq27510, bq27520 */
 	BQ27520G1, /* bq27520G1 */
 	BQ27520G2, /* bq27520G2 */
 	BQ27520G3, /* bq27520G3 */
@@ -21,7 +18,10 @@ enum bq27xxx_chip {
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
 
 	/* members of categories; translate these to category in _setup() */
-	BQ2752X, /* deprecated alias */
+	BQ2751X, /* bq27510 deprecated alias */
+	BQ2752X, /* bq27520 deprecated alias */
+	BQ27510G1,
+	BQ27510G2,
 	BQ27531,
 	BQ27542,
 	BQ27546,
-- 
2.9.3

Re: [PATCH v12.2 0/5] bq27xxx_battery partial series

[Andrew F. Davis]

On 03/30/2017 04:02 AM, Liam Breck wrote:
> Changes in this rev:
> 
> Removed macro for if (...) dev_dbg(...) from I/O helper functions.
> 
> Moved update_dm_block() to patch "Add power_supply_battery_info support".
> No more refactoring please; it's not really improving the code itself :-)
> 

Thats a big no can do :), refactoring is important for patch review,
each patch needs to be a single logical change that stands on its own. I
hate refactoring as much as the next guy, but no one said working on
Linux would be easy!

> set_cfgupdate is required for DM write on certain chips, see this Q&A
> https://e2e.ti.com/support/power_management/battery_management/f/180/p/577798/2121489#2121489
> 

So lets add the functionality for those chips in a separate patch. I
don't like the way you did set_cfgupdate() so lets not block a patch I
do like (read/write_block) by including that change in it.

> Renamed dm_buf .full => .has_data, .updt => .dirty
> dm_buf has 3 states: !has_data, has_data && !dirty, has_data && dirty
> I considered a single field and enum values, but I think this is clearer.
> 
> Renamed set_cfgupdate(..., u16 flag => state)
> We call set_cfgupdate(di, BQ27XXX_FLAG_CFGUP or 0)
> We !!state to print the intended flag state in error msg.
> 

Just print that it is a flag, or map from state to string. Personally I
would just put that in a debug print and not worry if it is pretty.

> 
>   power: bq27xxx_battery: Add bulk transfer bus methods
>   power: bq27xxx_battery: Add chip data memory read/write support
>   power: bq27xxx_battery: Add power_supply_battery_info support
>   power: bq27xxx_battery: Enable chip data memory update for certain chips
>   power: bq27xxx_battery: Remove duplicate register arrays
> 
>  drivers/power/supply/bq27xxx_battery.c     | 700 +++++++++++++++++++++++------
>  drivers/power/supply/bq27xxx_battery_i2c.c |  98 +++-
>  include/linux/power/bq27xxx_battery.h      |  26 +-
>  3 files changed, 682 insertions(+), 142 deletions(-)
> 

Re: [PATCH v12.2 5/9] power: bq27xxx_battery: Add bulk transfer bus methods

[Andrew F. Davis]

On 03/30/2017 04:02 AM, Liam Breck wrote:
> From: Matt Ranostay <matt@ranostay.consulting>
> 
> Declare bus.write/read_bulk/write_bulk().
> Add I2C write/read_bulk/write_bulk() to implement the above.
> Add bq27xxx_write/read_block/write_block() helper functions to call the above.
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 58 ++++++++++++++++++++-
>  drivers/power/supply/bq27xxx_battery_i2c.c | 82 +++++++++++++++++++++++++++++-
>  include/linux/power/bq27xxx_battery.h      |  3 ++
>  3 files changed, 140 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 398801a..e3472c3 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -794,11 +794,65 @@ MODULE_PARM_DESC(poll_interval,
>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>  			       bool single)
>  {
> -	/* Reports EINVAL for invalid/missing registers */
> +	int ret;
> +
>  	if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>  		return -EINVAL;
>  
> -	return di->bus.read(di, di->regs[reg_index], single);
> +	ret = di->bus.read(di, di->regs[reg_index], single);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
> +		        di->regs[reg_index], reg_index);
> +
> +	return ret;
> +}
> +
> +static inline int bq27xxx_write(struct bq27xxx_device_info *di, int reg_index,
> +				u16 value, bool single)
> +{
> +	int ret;
> +
> +	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write)
> +		return -EINVAL;

Perhaps (!di || !di->bus.write) should return "not implemented" or some
similar separate error code.

Otherwise,
Acked-by: Andrew F. Davis <afd@ti.com>

> +
> +	ret = di->bus.write(di, di->regs[reg_index], value, single);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to write register 0x%02x (index %d)\n",
> +		        di->regs[reg_index], reg_index);
> +
> +	return ret;
> +}
> +
> +static inline int bq27xxx_read_block(struct bq27xxx_device_info *di, int reg_index,
> +				     u8 *data, int len)
> +{
> +	int ret;
> +
> +	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.read_bulk)
> +		return -EINVAL;
> +
> +	ret = di->bus.read_bulk(di, di->regs[reg_index], data, len);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to read_bulk register 0x%02x (index %d)\n",
> +		        di->regs[reg_index], reg_index);
> +
> +	return ret;
> +}
> +
> +static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_index,
> +				      u8 *data, int len)
> +{
> +	int ret;
> +
> +	if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write_bulk)
> +		return -EINVAL;
> +
> +	ret = di->bus.write_bulk(di, di->regs[reg_index], data, len);
> +	if (ret < 0)
> +		dev_dbg(di->dev, "failed to write_bulk register 0x%02x (index %d)\n",
> +		        di->regs[reg_index], reg_index);
> +
> +	return ret;
>  }
>  
>  /*
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index c68fbc3..a597221 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -38,7 +38,7 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>  {
>  	struct i2c_client *client = to_i2c_client(di->dev);
>  	struct i2c_msg msg[2];
> -	unsigned char data[2];
> +	u8 data[2];
>  	int ret;
>  
>  	if (!client->adapter)
> @@ -68,6 +68,82 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>  	return ret;
>  }
>  
> +static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
> +				     int value, bool single)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +	struct i2c_msg msg;
> +	u8 data[4];
> +	int ret;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	data[0] = reg;
> +	if (single) {
> +		data[1] = (u8) value;
> +		msg.len = 2;
> +	} else {
> +		put_unaligned_le16(value, &data[1]);
> +		msg.len = 3;
> +	}
> +
> +	msg.buf = data;
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
> +					 u8 *data, int len)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +	int ret;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, reg, len, data);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != len)
> +		return -EINVAL;
> +	return 0;
> +}
> +
> +static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
> +					  u8 reg, u8 *data, int len)
> +{
> +	struct i2c_client *client = to_i2c_client(di->dev);
> +	struct i2c_msg msg;
> +	u8 buf[33];
> +	int ret;
> +
> +	if (!client->adapter)
> +		return -ENODEV;
> +
> +	buf[0] = reg;
> +	memcpy(&buf[1], data, len);
> +
> +	msg.buf = buf;
> +	msg.addr = client->addr;
> +	msg.flags = 0;
> +	msg.len = len + 1;
> +
> +	ret = i2c_transfer(client->adapter, &msg, 1);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != 1)
> +		return -EINVAL;
> +	return 0;
> +}
> +
>  static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  				     const struct i2c_device_id *id)
>  {
> @@ -95,7 +171,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>  	di->dev = &client->dev;
>  	di->chip = id->driver_data;
>  	di->name = name;
> +
>  	di->bus.read = bq27xxx_battery_i2c_read;
> +	di->bus.write = bq27xxx_battery_i2c_write;
> +	di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
> +	di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
>  
>  	ret = bq27xxx_battery_setup(di);
>  	if (ret)
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index b312bce..c3369fa 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -40,6 +40,9 @@ struct bq27xxx_platform_data {
>  struct bq27xxx_device_info;
>  struct bq27xxx_access_methods {
>  	int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
> +	int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
> +	int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
> +	int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>  };
>  
>  struct bq27xxx_reg_cache {
> 

Re: [PATCH v12.2 6/9] power: bq27xxx_battery: Add chip data memory read/write support

[Andrew F. Davis]

On 03/30/2017 04:02 AM, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Add the following to enable read/write of chip data memory (DM) RAM/NVM/flash:
>   bq27xxx_battery_seal()
>   bq27xxx_battery_unseal()
>   bq27xxx_battery_set_cfgupdate()
>   bq27xxx_battery_read_dm_block()
>   bq27xxx_battery_write_dm_block()
>   bq27xxx_battery_checksum_dm_block()
> 
> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 285 +++++++++++++++++++++++++++++++++
>  include/linux/power/bq27xxx_battery.h  |   1 +
>  2 files changed, 286 insertions(+)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index e3472c3..dbcf14a 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -65,6 +65,7 @@
>  #define BQ27XXX_FLAG_DSC	BIT(0)
>  #define BQ27XXX_FLAG_SOCF	BIT(1) /* State-of-Charge threshold final */
>  #define BQ27XXX_FLAG_SOC1	BIT(2) /* State-of-Charge threshold 1 */
> +#define BQ27XXX_FLAG_CFGUP	BIT(4)
>  #define BQ27XXX_FLAG_FC		BIT(9)
>  #define BQ27XXX_FLAG_OTD	BIT(14)
>  #define BQ27XXX_FLAG_OTC	BIT(15)
> @@ -78,6 +79,12 @@
>  #define BQ27000_FLAG_FC		BIT(5)
>  #define BQ27000_FLAG_CHGS	BIT(7) /* Charge state flag */
>  
> +/* control register params */
> +#define BQ27XXX_SEALED			0x20
> +#define BQ27XXX_SET_CFGUPDATE		0x13
> +#define BQ27XXX_SOFT_RESET		0x42
> +#define BQ27XXX_RESET			0x41
> +
>  #define BQ27XXX_RS			(20) /* Resistor sense mOhm */
>  #define BQ27XXX_POWER_CONSTANT		(29200) /* 29.2 µV^2 * 1000 */
>  #define BQ27XXX_CURRENT_CONSTANT	(3570) /* 3.57 µV * 1000 */
> @@ -108,6 +115,11 @@ enum bq27xxx_reg_index {
>  	BQ27XXX_REG_SOC,	/* State-of-Charge */
>  	BQ27XXX_REG_DCAP,	/* Design Capacity */
>  	BQ27XXX_REG_AP,		/* Average Power */
> +	BQ27XXX_DM_CTRL,	/* Block Data Control */
> +	BQ27XXX_DM_CLASS,	/* Data Class */
> +	BQ27XXX_DM_BLOCK,	/* Data Block */
> +	BQ27XXX_DM_DATA,	/* Block Data */
> +	BQ27XXX_DM_CKSUM,	/* Block Data Checksum */
>  	BQ27XXX_REG_MAX,	/* sentinel */
>  };
>  
> @@ -131,6 +143,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x0b,
>  		[BQ27XXX_REG_DCAP] = 0x76,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
>  	},
>  	[BQ27010] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -150,6 +167,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x0b,
>  		[BQ27XXX_REG_DCAP] = 0x76,
>  		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CTRL] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CLASS] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_BLOCK] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
>  	},
>  	[BQ2750X] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -169,6 +191,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ2751X] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -188,6 +215,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x20,
>  		[BQ27XXX_REG_DCAP] = 0x2e,
>  		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27500] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -207,6 +239,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27510G1] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -226,6 +263,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27510G2] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -245,6 +287,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27510G3] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -264,6 +311,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x20,
>  		[BQ27XXX_REG_DCAP] = 0x2e,
>  		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27520G1] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -283,6 +335,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27520G2] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -302,6 +359,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27520G3] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -321,6 +383,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27520G4] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -340,6 +407,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x20,
>  		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
>  		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27530] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -359,6 +431,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27541] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -378,6 +455,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27545] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -397,6 +479,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x2c,
>  		[BQ27XXX_REG_DCAP] = INVALID_REG_ADDR,
>  		[BQ27XXX_REG_AP] = 0x24,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  	[BQ27421] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
> @@ -416,6 +503,11 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>  		[BQ27XXX_REG_SOC] = 0x1c,
>  		[BQ27XXX_REG_DCAP] = 0x3c,
>  		[BQ27XXX_REG_AP] = 0x18,
> +		[BQ27XXX_DM_CTRL] = 0x61,
> +		[BQ27XXX_DM_CLASS] = 0x3e,
> +		[BQ27XXX_DM_BLOCK] = 0x3f,
> +		[BQ27XXX_DM_DATA] = 0x40,
> +		[BQ27XXX_DM_CKSUM] = 0x60,
>  	},
>  };
>  
> @@ -757,6 +849,18 @@ static struct {
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>  
> +#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)
> +
> +#define BQ27XXX_DM_SZ	32
> +
> +struct bq27xxx_dm_buf {
> +	u8 class;
> +	u8 block;
> +	u8 data[BQ27XXX_DM_SZ];
> +	bool has_data, dirty;
> +};
> +

kernel-doc this struct

> +
>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>  {
>  	struct bq27xxx_device_info *di;
> @@ -855,6 +959,187 @@ static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_in
>  	return ret;
>  }
>  
> +static int bq27xxx_battery_seal(struct bq27xxx_device_info *di)
> +{
> +	int ret;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_SEALED, false);
> +	if (ret >= 0)
> +		return 0;
> +

This logic is reversed, return on error first.

> +	dev_err(di->dev, "bus error on seal: %d\n", ret);
> +	return ret;
> +}
> +
> +static int bq27xxx_battery_unseal(struct bq27xxx_device_info *di)
> +{
> +	int ret;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)(di->unseal_key >> 16), false);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)di->unseal_key, false);
> +	if (ret < 0)
> +		goto out;
> +
> +	return 0;
> +
> +out:
> +	dev_err(di->dev, "bus error on unseal: %d\n", ret);
> +	return ret;
> +}
> +
> +static u8 bq27xxx_battery_checksum_dm_block(struct bq27xxx_dm_buf *buf)
> +{
> +	u16 sum = 0;

I haven't tested this, but maybe you could:

u8 sum = 0xff;

for (i = 0; i < BQ27XXX_DM_SZ; i++)
	sum -= buf->data[i];

return sum;

> +	int i;
> +
> +	for (i = 0; i < BQ27XXX_DM_SZ; i++)
> +		sum += buf->data[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;
> +
> +	buf->has_data = false;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	BQ27XXX_MSLEEP(1);
> +
> +	ret = bq27xxx_read_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_read(di, BQ27XXX_DM_CKSUM, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	if ((u8)ret != bq27xxx_battery_checksum_dm_block(buf)) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	buf->has_data = true;
> +	buf->dirty = false;
> +
> +	return 0;
> +
> +out:
> +	dev_err(di->dev, "bus error reading chip memory: %d\n", ret);
> +	return ret;
> +}
> +
> +static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
> +{
> +	const int limit = 100;
> +	int ret, try = limit;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_REG_CTRL,
> +			    state ? BQ27XXX_SET_CFGUPDATE : BQ27XXX_SOFT_RESET,
> +			    false);
> +	if (ret < 0)
> +		goto out;
> +
> +	do {
> +		BQ27XXX_MSLEEP(25);
> +		ret = di->bus.read(di, di->regs[BQ27XXX_REG_FLAGS], false);
> +		if (ret < 0)
> +			goto out;
> +	} while ((ret & BQ27XXX_FLAG_CFGUP) != state && --try);
> +
> +	if (!try) {
> +		dev_err(di->dev, "timed out waiting for cfgupdate flag %d\n", !!state);
> +		return -EINVAL;
> +	}
> +
> +	if (limit-try > 3)
> +		dev_warn(di->dev, "cfgupdate %d, retries %d\n", !!state, limit-try);
> +
> +	return 0;
> +
> +out:
> +	dev_err(di->dev, "bus error on %s: %d\n", state ? "set_cfgupdate" : "soft_reset", ret);
> +	return ret;
> +}
> +
> +static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
> +					  struct bq27xxx_dm_buf *buf)
> +{
> +	bool cfgup = di->chip == BQ27421; /* assume group supports cfgupdate */
> +	int ret;
> +
> +	if (!buf->dirty)
> +		return 0;
> +
> +	if (cfgup) {
> +		ret = bq27xxx_battery_set_cfgupdate(di, BQ27XXX_FLAG_CFGUP);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_CTRL, 0, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
> +	if (ret < 0)
> +		goto out;
> +
> +	BQ27XXX_MSLEEP(1);
> +
> +	ret = bq27xxx_write_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
> +	if (ret < 0)
> +		goto out;
> +
> +	ret = bq27xxx_write(di, BQ27XXX_DM_CKSUM,
> +			    bq27xxx_battery_checksum_dm_block(buf), true);
> +	if (ret < 0)
> +		goto out;
> +
> +	/* DO NOT read BQ27XXX_DM_CKSUM here to verify it! That may cause NVM
> +	 * corruption on the '425 chip (and perhaps others), which can damage
> +	 * the chip. See TI bqtool for what not to do:

The bqtool does work, it even says delay needs to be long enough.

> +	 * http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
> +	 */
> +
> +	if (cfgup) {
> +		BQ27XXX_MSLEEP(1);
> +		ret = bq27xxx_battery_set_cfgupdate(di, 0);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		BQ27XXX_MSLEEP(100); /* flash DM updates in <100ms */
> +	}
> +
> +	buf->dirty = false;
> +
> +	return 0;
> +
> +out:
> +	if (cfgup)
> +		bq27xxx_battery_set_cfgupdate(di, 0);
> +
> +	dev_err(di->dev, "bus error writing chip memory: %d\n", ret);
> +	return ret;
> +}
> +
>  /*
>   * Return the battery State-of-Charge
>   * Or < 0 if something fails.
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index c3369fa..b1defb8 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -64,6 +64,7 @@ struct bq27xxx_device_info {
>  	int id;
>  	enum bq27xxx_chip chip;
>  	const char *name;
> +	u32 unseal_key;
>  	struct bq27xxx_access_methods bus;
>  	struct bq27xxx_reg_cache cache;
>  	int charge_design_full;
> 

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Andrew F. Davis]

On 03/30/2017 04:02 AM, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
> 
> Create IDs for for previously unID'd chips, to index arrays for unseal keys
> and data memory register tables.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>  3 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 4c7a13e..70b8d1c 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -57,7 +57,7 @@
>  
>  #include <linux/power/bq27xxx_battery.h>
>  
> -#define DRIVER_VERSION		"1.2.0"
> +#define DRIVER_VERSION		"1.3.0"
>  
>  #define BQ27XXX_MANUFACTURER	"Texas Instruments"
>  
> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>  	[BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>  };
>  
> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
> +	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
> +	[BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
> +	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
> +};
> +
> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
> +	[BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
> +	[BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
> +	[BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
> +};
> +
> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
> +	[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 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 bq27621_dm_regs[] = {
> +	[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[] = {
> +	[BQ27500] = bq27500_dm_regs,
> +	[BQ27545] = bq27545_dm_regs,
> +	[BQ27421] = bq27421_dm_regs,
> +	[BQ27425] = bq27425_dm_regs,
> +	[BQ27441] = bq27421_dm_regs,
> +	[BQ27621] = bq27621_dm_regs,
> +};
> +
> +static u32 bq27xxx_unseal_keys[] = {
> +	[BQ27500] = 0x04143672,
> +	[BQ27545] = 0x04143672,
> +	[BQ27421] = 0x80008000,
> +	[BQ27425] = 0x04143672,
> +	[BQ27441] = 0x80008000,
> +	[BQ27621] = 0x80008000,
> +};
> +
>  
>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>  {
> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  		.drv_data = di,
>  	};
>  
> +	di->unseal_key = bq27xxx_unseal_keys[di->chip];
> +	di->dm_regs = bq27xxx_dm_regs[di->chip];
> +
> +	switch(di->chip) {  /* translate members of categories */
> +	case BQ2752X:
> +		di->chip = BQ27510G3; break;
> +	case BQ27531:
> +		di->chip = BQ27530;   break;
> +	case BQ27542:
> +	case BQ27546:
> +	case BQ27742:
> +		di->chip = BQ27541;   break;
> +	case BQ27425:
> +	case BQ27441:
> +	case BQ27621:
> +		di->chip = BQ27421;   break;
> +	default:
> +		break;
> +	}
> +
> +	di->regs = bq27xxx_regs[di->chip];
> +
>  	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>  	mutex_init(&di->lock);
> -	di->regs = bq27xxx_regs[di->chip];
>  
>  	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>  	if (!psy_desc)
> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
> index a597221..0b11ed4 100644
> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>  	{ "bq27210", BQ27010 },
>  	{ "bq27500", BQ2750X },
>  	{ "bq27510", BQ2751X },
> -	{ "bq27520", BQ2751X },
> +	{ "bq27520", BQ2752X },
>  	{ "bq27500-1", BQ27500 },
>  	{ "bq27510g1", BQ27510G1 },
>  	{ "bq27510g2", BQ27510G2 },
> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>  	{ "bq27520g3", BQ27520G3 },
>  	{ "bq27520g4", BQ27520G4 },
>  	{ "bq27530", BQ27530 },
> -	{ "bq27531", BQ27530 },
> +	{ "bq27531", BQ27531 },
>  	{ "bq27541", BQ27541 },
> -	{ "bq27542", BQ27541 },
> -	{ "bq27546", BQ27541 },
> -	{ "bq27742", BQ27541 },
> +	{ "bq27542", BQ27542 },
> +	{ "bq27546", BQ27546 },
> +	{ "bq27742", BQ27742 },
>  	{ "bq27545", BQ27545 },
>  	{ "bq27421", BQ27421 },
> -	{ "bq27425", BQ27421 },
> -	{ "bq27441", BQ27421 },
> -	{ "bq27621", BQ27421 },
> +	{ "bq27425", BQ27425 },
> +	{ "bq27441", BQ27441 },
> +	{ "bq27621", BQ27621 },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 227eb08..014e59f 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -2,6 +2,7 @@
>  #define __LINUX_BQ27X00_BATTERY_H__
>  
>  enum bq27xxx_chip {
> +	/* categories; index for bq27xxx_regs[] */
>  	BQ27000 = 1, /* bq27000, bq27200 */
>  	BQ27010, /* bq27010, bq27210 */
>  	BQ2750X, /* bq27500 deprecated alias */
> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>  	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>  	BQ27545, /* bq27545 */
>  	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
> +
> +	/* members of categories; translate these to category in _setup() */

These don't need to be separate from the others, these are valid chip
IDs, we may use another compatible ID for register array index right now
but these are not "categories".

> +	BQ2752X, /* deprecated alias */
> +	BQ27531,
> +	BQ27542,
> +	BQ27546,
> +	BQ27742,
> +	BQ27425,
> +	BQ27441,
> +	BQ27621,
>  };
>  
>  /**
> 

Re: [PATCH v12.2 0/5] bq27xxx_battery partial series

[Liam Breck]

On Thu, Mar 30, 2017 at 5:39 AM, Andrew F. Davis <afd@ti.com> wrote:
>
> On 03/30/2017 04:02 AM, Liam Breck wrote:
> > Changes in this rev:
> >
> > Removed macro for if (...) dev_dbg(...) from I/O helper functions.
> >
> > Moved update_dm_block() to patch "Add power_supply_battery_info support".
> > No more refactoring please; it's not really improving the code itself :-)
> >
>
> Thats a big no can do :), refactoring is important for patch review,
> each patch needs to be a single logical change that stands on its own. I
> hate refactoring as much as the next guy, but no one said working on
> Linux would be easy!

In the past two years 3 other patchsets on linux-pm have reached 14
versions (where we are counting partial series). One of those is a new
kernel framework. You suggested one additional patch in your fixup
pass, and I did that. It's enough.

> > set_cfgupdate is required for DM write on certain chips, see this Q&A
> > https://e2e.ti.com/support/power_management/battery_management/f/180/p/577798/2121489#2121489
> >
>
> So lets add the functionality for those chips in a separate patch. I
> don't like the way you did set_cfgupdate() so lets not block a patch I
> do like (read/write_block) by including that change in it.

The chips that most need this patchset are the RAM-only ones; they all
require set_cfgupdate. Our hardware has such a chip. I have spent
considerable time making this code compatible with any bq27 part. I
can't donate more time to chips I can't test. set_cfgupdate is
implemented correctly. It doesn't help anyone to patch it separately.

> > Renamed dm_buf .full => .has_data, .updt => .dirty
> > dm_buf has 3 states: !has_data, has_data && !dirty, has_data && dirty
> > I considered a single field and enum values, but I think this is clearer.
> >
> > Renamed set_cfgupdate(..., u16 flag => state)
> > We call set_cfgupdate(di, BQ27XXX_FLAG_CFGUP or 0)
> > We !!state to print the intended flag state in error msg.
> >
>
> Just print that it is a flag, or map from state to string. Personally I
> would just put that in a debug print and not worry if it is pretty.

One msg flags an error condition; it's not a dev_dbg case. We need to
print the expected flag value, anything else is confusing.

> >   power: bq27xxx_battery: Add bulk transfer bus methods
> >   power: bq27xxx_battery: Add chip data memory read/write support
> >   power: bq27xxx_battery: Add power_supply_battery_info support
> >   power: bq27xxx_battery: Enable chip data memory update for certain chips
> >   power: bq27xxx_battery: Remove duplicate register arrays
> >
> >  drivers/power/supply/bq27xxx_battery.c     | 700 +++++++++++++++++++++++------
> >  drivers/power/supply/bq27xxx_battery_i2c.c |  98 +++-
> >  include/linux/power/bq27xxx_battery.h      |  26 +-
> >  3 files changed, 682 insertions(+), 142 deletions(-)
> >

Re: [PATCH v12.2 5/9] power: bq27xxx_battery: Add bulk transfer bus methods

[Liam Breck]

On Thu, Mar 30, 2017 at 5:45 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/30/2017 04:02 AM, Liam Breck wrote:
>> From: Matt Ranostay <matt@ranostay.consulting>
>>
>> Declare bus.write/read_bulk/write_bulk().
>> Add I2C write/read_bulk/write_bulk() to implement the above.
>> Add bq27xxx_write/read_block/write_block() helper functions to call the above.
>>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c     | 58 ++++++++++++++++++++-
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 82 +++++++++++++++++++++++++++++-
>>  include/linux/power/bq27xxx_battery.h      |  3 ++
>>  3 files changed, 140 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 398801a..e3472c3 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c
>> @@ -794,11 +794,65 @@ MODULE_PARM_DESC(poll_interval,
>>  static inline int bq27xxx_read(struct bq27xxx_device_info *di, int reg_index,
>>                              bool single)
>>  {
>> -     /* Reports EINVAL for invalid/missing registers */
>> +     int ret;
>> +
>>       if (!di || di->regs[reg_index] == INVALID_REG_ADDR)
>>               return -EINVAL;
>>
>> -     return di->bus.read(di, di->regs[reg_index], single);
>> +     ret = di->bus.read(di, di->regs[reg_index], single);
>> +     if (ret < 0)
>> +             dev_dbg(di->dev, "failed to read register 0x%02x (index %d)\n",
>> +                     di->regs[reg_index], reg_index);
>> +
>> +     return ret;
>> +}
>> +
>> +static inline int bq27xxx_write(struct bq27xxx_device_info *di, int reg_index,
>> +                             u16 value, bool single)
>> +{
>> +     int ret;
>> +
>> +     if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write)
>> +             return -EINVAL;
>
> Perhaps (!di || !di->bus.write) should return "not implemented" or some
> similar separate error code.

That would be -ENOSYS. Is OK?

> Otherwise,
> Acked-by: Andrew F. Davis <afd@ti.com>
>
>> +
>> +     ret = di->bus.write(di, di->regs[reg_index], value, single);
>> +     if (ret < 0)
>> +             dev_dbg(di->dev, "failed to write register 0x%02x (index %d)\n",
>> +                     di->regs[reg_index], reg_index);
>> +
>> +     return ret;
>> +}
>> +
>> +static inline int bq27xxx_read_block(struct bq27xxx_device_info *di, int reg_index,
>> +                                  u8 *data, int len)
>> +{
>> +     int ret;
>> +
>> +     if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.read_bulk)
>> +             return -EINVAL;
>> +
>> +     ret = di->bus.read_bulk(di, di->regs[reg_index], data, len);
>> +     if (ret < 0)
>> +             dev_dbg(di->dev, "failed to read_bulk register 0x%02x (index %d)\n",
>> +                     di->regs[reg_index], reg_index);
>> +
>> +     return ret;
>> +}
>> +
>> +static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_index,
>> +                                   u8 *data, int len)
>> +{
>> +     int ret;
>> +
>> +     if (!di || di->regs[reg_index] == INVALID_REG_ADDR || !di->bus.write_bulk)
>> +             return -EINVAL;
>> +
>> +     ret = di->bus.write_bulk(di, di->regs[reg_index], data, len);
>> +     if (ret < 0)
>> +             dev_dbg(di->dev, "failed to write_bulk register 0x%02x (index %d)\n",
>> +                     di->regs[reg_index], reg_index);
>> +
>> +     return ret;
>>  }
>>
>>  /*
>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>> index c68fbc3..a597221 100644
>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>> @@ -38,7 +38,7 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>>  {
>>       struct i2c_client *client = to_i2c_client(di->dev);
>>       struct i2c_msg msg[2];
>> -     unsigned char data[2];
>> +     u8 data[2];
>>       int ret;
>>
>>       if (!client->adapter)
>> @@ -68,6 +68,82 @@ static int bq27xxx_battery_i2c_read(struct bq27xxx_device_info *di, u8 reg,
>>       return ret;
>>  }
>>
>> +static int bq27xxx_battery_i2c_write(struct bq27xxx_device_info *di, u8 reg,
>> +                                  int value, bool single)
>> +{
>> +     struct i2c_client *client = to_i2c_client(di->dev);
>> +     struct i2c_msg msg;
>> +     u8 data[4];
>> +     int ret;
>> +
>> +     if (!client->adapter)
>> +             return -ENODEV;
>> +
>> +     data[0] = reg;
>> +     if (single) {
>> +             data[1] = (u8) value;
>> +             msg.len = 2;
>> +     } else {
>> +             put_unaligned_le16(value, &data[1]);
>> +             msg.len = 3;
>> +     }
>> +
>> +     msg.buf = data;
>> +     msg.addr = client->addr;
>> +     msg.flags = 0;
>> +
>> +     ret = i2c_transfer(client->adapter, &msg, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != 1)
>> +             return -EINVAL;
>> +     return 0;
>> +}
>> +
>> +static int bq27xxx_battery_i2c_bulk_read(struct bq27xxx_device_info *di, u8 reg,
>> +                                      u8 *data, int len)
>> +{
>> +     struct i2c_client *client = to_i2c_client(di->dev);
>> +     int ret;
>> +
>> +     if (!client->adapter)
>> +             return -ENODEV;
>> +
>> +     ret = i2c_smbus_read_i2c_block_data(client, reg, len, data);
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != len)
>> +             return -EINVAL;
>> +     return 0;
>> +}
>> +
>> +static int bq27xxx_battery_i2c_bulk_write(struct bq27xxx_device_info *di,
>> +                                       u8 reg, u8 *data, int len)
>> +{
>> +     struct i2c_client *client = to_i2c_client(di->dev);
>> +     struct i2c_msg msg;
>> +     u8 buf[33];
>> +     int ret;
>> +
>> +     if (!client->adapter)
>> +             return -ENODEV;
>> +
>> +     buf[0] = reg;
>> +     memcpy(&buf[1], data, len);
>> +
>> +     msg.buf = buf;
>> +     msg.addr = client->addr;
>> +     msg.flags = 0;
>> +     msg.len = len + 1;
>> +
>> +     ret = i2c_transfer(client->adapter, &msg, 1);
>> +     if (ret < 0)
>> +             return ret;
>> +     if (ret != 1)
>> +             return -EINVAL;
>> +     return 0;
>> +}
>> +
>>  static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>>                                    const struct i2c_device_id *id)
>>  {
>> @@ -95,7 +171,11 @@ static int bq27xxx_battery_i2c_probe(struct i2c_client *client,
>>       di->dev = &client->dev;
>>       di->chip = id->driver_data;
>>       di->name = name;
>> +
>>       di->bus.read = bq27xxx_battery_i2c_read;
>> +     di->bus.write = bq27xxx_battery_i2c_write;
>> +     di->bus.read_bulk = bq27xxx_battery_i2c_bulk_read;
>> +     di->bus.write_bulk = bq27xxx_battery_i2c_bulk_write;
>>
>>       ret = bq27xxx_battery_setup(di);
>>       if (ret)
>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> index b312bce..c3369fa 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -40,6 +40,9 @@ struct bq27xxx_platform_data {
>>  struct bq27xxx_device_info;
>>  struct bq27xxx_access_methods {
>>       int (*read)(struct bq27xxx_device_info *di, u8 reg, bool single);
>> +     int (*write)(struct bq27xxx_device_info *di, u8 reg, int value, bool single);
>> +     int (*read_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>> +     int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
>>  };
>>
>>  struct bq27xxx_reg_cache {
>>

Re: [PATCH v12.2 6/9] power: bq27xxx_battery: Add chip data memory read/write support

[Liam Breck]

On Thu, Mar 30, 2017 at 5:55 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/30/2017 04:02 AM, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Add the following to enable read/write of chip data memory (DM) RAM/NVM/flash:
>>   bq27xxx_battery_seal()
>>   bq27xxx_battery_unseal()
>>   bq27xxx_battery_set_cfgupdate()
>>   bq27xxx_battery_read_dm_block()
>>   bq27xxx_battery_write_dm_block()
>>   bq27xxx_battery_checksum_dm_block()
>>
>> Signed-off-by: Matt Ranostay <matt@ranostay.consulting>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c | 285 +++++++++++++++++++++++++++++++++
>>  include/linux/power/bq27xxx_battery.h  |   1 +
>>  2 files changed, 286 insertions(+)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index e3472c3..dbcf14a 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c

>> @@ -757,6 +849,18 @@ static struct {
>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>  static LIST_HEAD(bq27xxx_battery_devices);
>>
>> +#define BQ27XXX_MSLEEP(i) usleep_range((i)*1000, (i)*1000+500)
>> +
>> +#define BQ27XXX_DM_SZ        32
>> +
>> +struct bq27xxx_dm_buf {
>> +     u8 class;
>> +     u8 block;
>> +     u8 data[BQ27XXX_DM_SZ];
>> +     bool has_data, dirty;
>> +};
>> +
>
> kernel-doc this struct
>
>> +
>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>  {
>>       struct bq27xxx_device_info *di;
>> @@ -855,6 +959,187 @@ static inline int bq27xxx_write_block(struct bq27xxx_device_info *di, int reg_in
>>       return ret;
>>  }
>>
>> +static int bq27xxx_battery_seal(struct bq27xxx_device_info *di)
>> +{
>> +     int ret;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_SEALED, false);
>> +     if (ret >= 0)
>> +             return 0;
>> +
>
> This logic is reversed, return on error first.

It mirrors the structure of all the other functions, with an out:
error case at the end. Can I use goto-out here too?

>> +     dev_err(di->dev, "bus error on seal: %d\n", ret);
>> +     return ret;
>> +}
>> +
>> +static int bq27xxx_battery_unseal(struct bq27xxx_device_info *di)
>> +{
>> +     int ret;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)(di->unseal_key >> 16), false);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_REG_CTRL, (u16)di->unseal_key, false);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     return 0;
>> +
>> +out:
>> +     dev_err(di->dev, "bus error on unseal: %d\n", ret);
>> +     return ret;
>> +}
>> +
>> +static u8 bq27xxx_battery_checksum_dm_block(struct bq27xxx_dm_buf *buf)
>> +{
>> +     u16 sum = 0;
>
> I haven't tested this, but maybe you could:
>
> u8 sum = 0xff;
>
> for (i = 0; i < BQ27XXX_DM_SZ; i++)
>         sum -= buf->data[i];
>
> return sum;

I gracefully decline. I prefer the bqtool algorithm. We will cause
confusion with distinct logic. Also using subtraction in a checkSUM is
odd.

>> +     int i;
>> +
>> +     for (i = 0; i < BQ27XXX_DM_SZ; i++)
>> +             sum += buf->data[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;
>> +
>> +     buf->has_data = false;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     BQ27XXX_MSLEEP(1);
>> +
>> +     ret = bq27xxx_read_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_read(di, BQ27XXX_DM_CKSUM, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     if ((u8)ret != bq27xxx_battery_checksum_dm_block(buf)) {
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     buf->has_data = true;
>> +     buf->dirty = false;
>> +
>> +     return 0;
>> +
>> +out:
>> +     dev_err(di->dev, "bus error reading chip memory: %d\n", ret);
>> +     return ret;
>> +}
>> +
>> +static int bq27xxx_battery_set_cfgupdate(struct bq27xxx_device_info *di, u16 state)
>> +{
>> +     const int limit = 100;
>> +     int ret, try = limit;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_REG_CTRL,
>> +                         state ? BQ27XXX_SET_CFGUPDATE : BQ27XXX_SOFT_RESET,
>> +                         false);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     do {
>> +             BQ27XXX_MSLEEP(25);
>> +             ret = di->bus.read(di, di->regs[BQ27XXX_REG_FLAGS], false);
>> +             if (ret < 0)
>> +                     goto out;
>> +     } while ((ret & BQ27XXX_FLAG_CFGUP) != state && --try);
>> +
>> +     if (!try) {
>> +             dev_err(di->dev, "timed out waiting for cfgupdate flag %d\n", !!state);
>> +             return -EINVAL;
>> +     }
>> +
>> +     if (limit-try > 3)
>> +             dev_warn(di->dev, "cfgupdate %d, retries %d\n", !!state, limit-try);
>> +
>> +     return 0;
>> +
>> +out:
>> +     dev_err(di->dev, "bus error on %s: %d\n", state ? "set_cfgupdate" : "soft_reset", ret);
>> +     return ret;
>> +}
>> +
>> +static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
>> +                                       struct bq27xxx_dm_buf *buf)
>> +{
>> +     bool cfgup = di->chip == BQ27421; /* assume group supports cfgupdate */
>> +     int ret;
>> +
>> +     if (!buf->dirty)
>> +             return 0;
>> +
>> +     if (cfgup) {
>> +             ret = bq27xxx_battery_set_cfgupdate(di, BQ27XXX_FLAG_CFGUP);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_CTRL, 0, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_CLASS, buf->class, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_BLOCK, buf->block, true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     BQ27XXX_MSLEEP(1);
>> +
>> +     ret = bq27xxx_write_block(di, BQ27XXX_DM_DATA, buf->data, BQ27XXX_DM_SZ);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     ret = bq27xxx_write(di, BQ27XXX_DM_CKSUM,
>> +                         bq27xxx_battery_checksum_dm_block(buf), true);
>> +     if (ret < 0)
>> +             goto out;
>> +
>> +     /* DO NOT read BQ27XXX_DM_CKSUM here to verify it! That may cause NVM
>> +      * corruption on the '425 chip (and perhaps others), which can damage
>> +      * the chip. See TI bqtool for what not to do:
>
> The bqtool does work, it even says delay needs to be long enough.

It doesn't say the result is catastrophic if the delay isn't
sufficient. It does increase the delay and retry on failure, implying
that the starting value is flexible.

>> +      * http://git.ti.com/bms-linux/bqtool/blobs/master/gauge.c#line328
>> +      */
>> +
>> +     if (cfgup) {
>> +             BQ27XXX_MSLEEP(1);
>> +             ret = bq27xxx_battery_set_cfgupdate(di, 0);
>> +             if (ret < 0)
>> +                     return ret;
>> +     } else {
>> +             BQ27XXX_MSLEEP(100); /* flash DM updates in <100ms */
>> +     }
>> +
>> +     buf->dirty = false;
>> +
>> +     return 0;
>> +
>> +out:
>> +     if (cfgup)
>> +             bq27xxx_battery_set_cfgupdate(di, 0);
>> +
>> +     dev_err(di->dev, "bus error writing chip memory: %d\n", ret);
>> +     return ret;
>> +}
>> +
>>  /*
>>   * Return the battery State-of-Charge
>>   * Or < 0 if something fails.
>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> index c3369fa..b1defb8 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -64,6 +64,7 @@ struct bq27xxx_device_info {
>>       int id;
>>       enum bq27xxx_chip chip;
>>       const char *name;
>> +     u32 unseal_key;
>>       struct bq27xxx_access_methods bus;
>>       struct bq27xxx_reg_cache cache;
>>       int charge_design_full;
>>

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Liam Breck]

On Thu, Mar 30, 2017 at 5:58 AM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/30/2017 04:02 AM, Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
>>
>> Create IDs for for previously unID'd chips, to index arrays for unseal keys
>> and data memory register tables.
>>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>>  3 files changed, 90 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 4c7a13e..70b8d1c 100644
>> --- a/drivers/power/supply/bq27xxx_battery.c
>> +++ b/drivers/power/supply/bq27xxx_battery.c
>> @@ -57,7 +57,7 @@
>>
>>  #include <linux/power/bq27xxx_battery.h>
>>
>> -#define DRIVER_VERSION               "1.2.0"
>> +#define DRIVER_VERSION               "1.3.0"
>>
>>  #define BQ27XXX_MANUFACTURER "Texas Instruments"
>>
>> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>>       [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>>  };
>>
>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
>> +};
>> +
>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
>> +};
>> +
>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>> +     [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 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 bq27621_dm_regs[] = {
>> +     [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[] = {
>> +     [BQ27500] = bq27500_dm_regs,
>> +     [BQ27545] = bq27545_dm_regs,
>> +     [BQ27421] = bq27421_dm_regs,
>> +     [BQ27425] = bq27425_dm_regs,
>> +     [BQ27441] = bq27421_dm_regs,
>> +     [BQ27621] = bq27621_dm_regs,
>> +};
>> +
>> +static u32 bq27xxx_unseal_keys[] = {
>> +     [BQ27500] = 0x04143672,
>> +     [BQ27545] = 0x04143672,
>> +     [BQ27421] = 0x80008000,
>> +     [BQ27425] = 0x04143672,
>> +     [BQ27441] = 0x80008000,
>> +     [BQ27621] = 0x80008000,
>> +};
>> +
>>
>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>  {
>> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>               .drv_data = di,
>>       };
>>
>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>> +
>> +     switch(di->chip) {  /* translate members of categories */
>> +     case BQ2752X:
>> +             di->chip = BQ27510G3; break;
>> +     case BQ27531:
>> +             di->chip = BQ27530;   break;
>> +     case BQ27542:
>> +     case BQ27546:
>> +     case BQ27742:
>> +             di->chip = BQ27541;   break;
>> +     case BQ27425:
>> +     case BQ27441:
>> +     case BQ27621:
>> +             di->chip = BQ27421;   break;
>> +     default:
>> +             break;
>> +     }
>> +
>> +     di->regs = bq27xxx_regs[di->chip];
>> +
>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>       mutex_init(&di->lock);
>> -     di->regs = bq27xxx_regs[di->chip];
>>
>>       psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>>       if (!psy_desc)
>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>> index a597221..0b11ed4 100644
>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>       { "bq27210", BQ27010 },
>>       { "bq27500", BQ2750X },
>>       { "bq27510", BQ2751X },
>> -     { "bq27520", BQ2751X },
>> +     { "bq27520", BQ2752X },
>>       { "bq27500-1", BQ27500 },
>>       { "bq27510g1", BQ27510G1 },
>>       { "bq27510g2", BQ27510G2 },
>> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>       { "bq27520g3", BQ27520G3 },
>>       { "bq27520g4", BQ27520G4 },
>>       { "bq27530", BQ27530 },
>> -     { "bq27531", BQ27530 },
>> +     { "bq27531", BQ27531 },
>>       { "bq27541", BQ27541 },
>> -     { "bq27542", BQ27541 },
>> -     { "bq27546", BQ27541 },
>> -     { "bq27742", BQ27541 },
>> +     { "bq27542", BQ27542 },
>> +     { "bq27546", BQ27546 },
>> +     { "bq27742", BQ27742 },
>>       { "bq27545", BQ27545 },
>>       { "bq27421", BQ27421 },
>> -     { "bq27425", BQ27421 },
>> -     { "bq27441", BQ27421 },
>> -     { "bq27621", BQ27421 },
>> +     { "bq27425", BQ27425 },
>> +     { "bq27441", BQ27441 },
>> +     { "bq27621", BQ27621 },
>>       {},
>>  };
>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>> index 227eb08..014e59f 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -2,6 +2,7 @@
>>  #define __LINUX_BQ27X00_BATTERY_H__
>>
>>  enum bq27xxx_chip {
>> +     /* categories; index for bq27xxx_regs[] */
>>       BQ27000 = 1, /* bq27000, bq27200 */
>>       BQ27010, /* bq27010, bq27210 */
>>       BQ2750X, /* bq27500 deprecated alias */
>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>       BQ27545, /* bq27545 */
>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>> +
>> +     /* members of categories; translate these to category in _setup() */
>
> These don't need to be separate from the others, these are valid chip
> IDs, we may use another compatible ID for register array index right now
> but these are not "categories".

The above were indeed treated like categories, as the I2C ID table
mapped other chips to them. We could call them category|class
"leaders" (prototypes? models? patterns?) instead.

The below are not valid in di->chip, they must be converted. So it
makes sense to group them.

>> +     BQ2752X, /* deprecated alias */
>> +     BQ27531,
>> +     BQ27542,
>> +     BQ27546,
>> +     BQ27742,
>> +     BQ27425,
>> +     BQ27441,
>> +     BQ27621,
>>  };
>>
>>  /**
>>

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Andrew F. Davis]

On 03/30/2017 03:14 PM, Liam Breck wrote:
> On Thu, Mar 30, 2017 at 5:58 AM, Andrew F. Davis <afd@ti.com> wrote:
>> On 03/30/2017 04:02 AM, Liam Breck wrote:
>>> From: Liam Breck <kernel@networkimprov.net>
>>>
>>> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
>>>
>>> Create IDs for for previously unID'd chips, to index arrays for unseal keys
>>> and data memory register tables.
>>>
>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>> ---
>>>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>>>  3 files changed, 90 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>> index 4c7a13e..70b8d1c 100644
>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>> @@ -57,7 +57,7 @@
>>>
>>>  #include <linux/power/bq27xxx_battery.h>
>>>
>>> -#define DRIVER_VERSION               "1.2.0"
>>> +#define DRIVER_VERSION               "1.3.0"
>>>
>>>  #define BQ27XXX_MANUFACTURER "Texas Instruments"
>>>
>>> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>>>       [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>>>  };
>>>
>>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
>>> +};
>>> +
>>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
>>> +};
>>> +
>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>> +     [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 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 bq27621_dm_regs[] = {
>>> +     [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[] = {
>>> +     [BQ27500] = bq27500_dm_regs,
>>> +     [BQ27545] = bq27545_dm_regs,
>>> +     [BQ27421] = bq27421_dm_regs,
>>> +     [BQ27425] = bq27425_dm_regs,
>>> +     [BQ27441] = bq27421_dm_regs,
>>> +     [BQ27621] = bq27621_dm_regs,
>>> +};
>>> +
>>> +static u32 bq27xxx_unseal_keys[] = {
>>> +     [BQ27500] = 0x04143672,
>>> +     [BQ27545] = 0x04143672,
>>> +     [BQ27421] = 0x80008000,
>>> +     [BQ27425] = 0x04143672,
>>> +     [BQ27441] = 0x80008000,
>>> +     [BQ27621] = 0x80008000,
>>> +};
>>> +
>>>
>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>  {
>>> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>               .drv_data = di,
>>>       };
>>>
>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>> +
>>> +     switch(di->chip) {  /* translate members of categories */
>>> +     case BQ2752X:
>>> +             di->chip = BQ27510G3; break;
>>> +     case BQ27531:
>>> +             di->chip = BQ27530;   break;
>>> +     case BQ27542:
>>> +     case BQ27546:
>>> +     case BQ27742:
>>> +             di->chip = BQ27541;   break;
>>> +     case BQ27425:
>>> +     case BQ27441:
>>> +     case BQ27621:
>>> +             di->chip = BQ27421;   break;
>>> +     default:
>>> +             break;
>>> +     }
>>> +
>>> +     di->regs = bq27xxx_regs[di->chip];
>>> +
>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>       mutex_init(&di->lock);
>>> -     di->regs = bq27xxx_regs[di->chip];
>>>
>>>       psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>>>       if (!psy_desc)
>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>> index a597221..0b11ed4 100644
>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>       { "bq27210", BQ27010 },
>>>       { "bq27500", BQ2750X },
>>>       { "bq27510", BQ2751X },
>>> -     { "bq27520", BQ2751X },
>>> +     { "bq27520", BQ2752X },
>>>       { "bq27500-1", BQ27500 },
>>>       { "bq27510g1", BQ27510G1 },
>>>       { "bq27510g2", BQ27510G2 },
>>> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>       { "bq27520g3", BQ27520G3 },
>>>       { "bq27520g4", BQ27520G4 },
>>>       { "bq27530", BQ27530 },
>>> -     { "bq27531", BQ27530 },
>>> +     { "bq27531", BQ27531 },
>>>       { "bq27541", BQ27541 },
>>> -     { "bq27542", BQ27541 },
>>> -     { "bq27546", BQ27541 },
>>> -     { "bq27742", BQ27541 },
>>> +     { "bq27542", BQ27542 },
>>> +     { "bq27546", BQ27546 },
>>> +     { "bq27742", BQ27742 },
>>>       { "bq27545", BQ27545 },
>>>       { "bq27421", BQ27421 },
>>> -     { "bq27425", BQ27421 },
>>> -     { "bq27441", BQ27421 },
>>> -     { "bq27621", BQ27421 },
>>> +     { "bq27425", BQ27425 },
>>> +     { "bq27441", BQ27441 },
>>> +     { "bq27621", BQ27621 },
>>>       {},
>>>  };
>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>> index 227eb08..014e59f 100644
>>> --- a/include/linux/power/bq27xxx_battery.h
>>> +++ b/include/linux/power/bq27xxx_battery.h
>>> @@ -2,6 +2,7 @@
>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>
>>>  enum bq27xxx_chip {
>>> +     /* categories; index for bq27xxx_regs[] */
>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>       BQ27010, /* bq27010, bq27210 */
>>>       BQ2750X, /* bq27500 deprecated alias */
>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>       BQ27545, /* bq27545 */
>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>> +
>>> +     /* members of categories; translate these to category in _setup() */
>>
>> These don't need to be separate from the others, these are valid chip
>> IDs, we may use another compatible ID for register array index right now
>> but these are not "categories".
> 
> The above were indeed treated like categories, as the I2C ID table
> mapped other chips to them. We could call them category|class
> "leaders" (prototypes? models? patterns?) instead.
> 
> The below are not valid in di->chip, they must be converted. So it
> makes sense to group them.
> 

They are only "not valid" because you did not add tables for them. If
you don't want to add register tables then think of a better way to map
chip to register table. This is overly complex and will cause problems
for people adding more chips.

>>> +     BQ2752X, /* deprecated alias */
>>> +     BQ27531,
>>> +     BQ27542,
>>> +     BQ27546,
>>> +     BQ27742,
>>> +     BQ27425,
>>> +     BQ27441,
>>> +     BQ27621,
>>>  };
>>>
>>>  /**
>>>

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Liam Breck]

On Tue, Apr 4, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 03/30/2017 03:14 PM, Liam Breck wrote:
>> On Thu, Mar 30, 2017 at 5:58 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 03/30/2017 04:02 AM, Liam Breck wrote:
>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>
>>>> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
>>>>
>>>> Create IDs for for previously unID'd chips, to index arrays for unseal keys
>>>> and data memory register tables.
>>>>
>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>> ---
>>>>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>>>>  3 files changed, 90 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>> index 4c7a13e..70b8d1c 100644
>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>> @@ -57,7 +57,7 @@
>>>>
>>>>  #include <linux/power/bq27xxx_battery.h>
>>>>
>>>> -#define DRIVER_VERSION               "1.2.0"
>>>> +#define DRIVER_VERSION               "1.3.0"
>>>>
>>>>  #define BQ27XXX_MANUFACTURER "Texas Instruments"
>>>>
>>>> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>>>>       [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>>>>  };
>>>>
>>>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
>>>> +};
>>>> +
>>>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
>>>> +};
>>>> +
>>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>> +     [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 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 bq27621_dm_regs[] = {
>>>> +     [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[] = {
>>>> +     [BQ27500] = bq27500_dm_regs,
>>>> +     [BQ27545] = bq27545_dm_regs,
>>>> +     [BQ27421] = bq27421_dm_regs,
>>>> +     [BQ27425] = bq27425_dm_regs,
>>>> +     [BQ27441] = bq27421_dm_regs,
>>>> +     [BQ27621] = bq27621_dm_regs,
>>>> +};
>>>> +
>>>> +static u32 bq27xxx_unseal_keys[] = {
>>>> +     [BQ27500] = 0x04143672,
>>>> +     [BQ27545] = 0x04143672,
>>>> +     [BQ27421] = 0x80008000,
>>>> +     [BQ27425] = 0x04143672,
>>>> +     [BQ27441] = 0x80008000,
>>>> +     [BQ27621] = 0x80008000,
>>>> +};
>>>> +
>>>>
>>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>>  {
>>>> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>               .drv_data = di,
>>>>       };
>>>>
>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>> +
>>>> +     switch(di->chip) {  /* translate members of categories */
>>>> +     case BQ2752X:
>>>> +             di->chip = BQ27510G3; break;
>>>> +     case BQ27531:
>>>> +             di->chip = BQ27530;   break;
>>>> +     case BQ27542:
>>>> +     case BQ27546:
>>>> +     case BQ27742:
>>>> +             di->chip = BQ27541;   break;
>>>> +     case BQ27425:
>>>> +     case BQ27441:
>>>> +     case BQ27621:
>>>> +             di->chip = BQ27421;   break;
>>>> +     default:
>>>> +             break;
>>>> +     }
>>>> +
>>>> +     di->regs = bq27xxx_regs[di->chip];
>>>> +
>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>       mutex_init(&di->lock);
>>>> -     di->regs = bq27xxx_regs[di->chip];
>>>>
>>>>       psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>>>>       if (!psy_desc)
>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> index a597221..0b11ed4 100644
>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>       { "bq27210", BQ27010 },
>>>>       { "bq27500", BQ2750X },
>>>>       { "bq27510", BQ2751X },
>>>> -     { "bq27520", BQ2751X },
>>>> +     { "bq27520", BQ2752X },
>>>>       { "bq27500-1", BQ27500 },
>>>>       { "bq27510g1", BQ27510G1 },
>>>>       { "bq27510g2", BQ27510G2 },
>>>> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>       { "bq27520g3", BQ27520G3 },
>>>>       { "bq27520g4", BQ27520G4 },
>>>>       { "bq27530", BQ27530 },
>>>> -     { "bq27531", BQ27530 },
>>>> +     { "bq27531", BQ27531 },
>>>>       { "bq27541", BQ27541 },
>>>> -     { "bq27542", BQ27541 },
>>>> -     { "bq27546", BQ27541 },
>>>> -     { "bq27742", BQ27541 },
>>>> +     { "bq27542", BQ27542 },
>>>> +     { "bq27546", BQ27546 },
>>>> +     { "bq27742", BQ27742 },
>>>>       { "bq27545", BQ27545 },
>>>>       { "bq27421", BQ27421 },
>>>> -     { "bq27425", BQ27421 },
>>>> -     { "bq27441", BQ27421 },
>>>> -     { "bq27621", BQ27421 },
>>>> +     { "bq27425", BQ27425 },
>>>> +     { "bq27441", BQ27441 },
>>>> +     { "bq27621", BQ27621 },
>>>>       {},
>>>>  };
>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>> index 227eb08..014e59f 100644
>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>> @@ -2,6 +2,7 @@
>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>
>>>>  enum bq27xxx_chip {
>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>       BQ27010, /* bq27010, bq27210 */
>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>       BQ27545, /* bq27545 */
>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>> +
>>>> +     /* members of categories; translate these to category in _setup() */
>>>
>>> These don't need to be separate from the others, these are valid chip
>>> IDs, we may use another compatible ID for register array index right now
>>> but these are not "categories".
>>
>> The above were indeed treated like categories, as the I2C ID table
>> mapped other chips to them. We could call them category|class
>> "leaders" (prototypes? models? patterns?) instead.
>>
>> The below are not valid in di->chip, they must be converted. So it
>> makes sense to group them.
>>
>
> They are only "not valid" because you did not add tables for them. If
> you don't want to add register tables then think of a better way to map
> chip to register table. This is overly complex and will cause problems
> for people adding more chips.

This very mapping from category member -> prototype ID was done by the
I2C table previously. How is doing it in a setup() switch any more
complex?

The alternative mapping from chip to regs is this, which you said was
there but replaced by a 2D array a while ago. Shall we revert to this?
  bq27xxx_regs[] = {
    [BQ27991] = bq27991_regs, // prototype
    [BQ27992] = bq27991_regs, // member
    ...
  };


>>>> +     BQ2752X, /* deprecated alias */
>>>> +     BQ27531,
>>>> +     BQ27542,
>>>> +     BQ27546,
>>>> +     BQ27742,
>>>> +     BQ27425,
>>>> +     BQ27441,
>>>> +     BQ27621,
>>>>  };
>>>>
>>>>  /**
>>>>

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Andrew F. Davis]

On 04/04/2017 04:37 PM, Liam Breck wrote:
> On Tue, Apr 4, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>> On 03/30/2017 03:14 PM, Liam Breck wrote:
>>> On Thu, Mar 30, 2017 at 5:58 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>> On 03/30/2017 04:02 AM, Liam Breck wrote:
>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>
>>>>> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
>>>>>
>>>>> Create IDs for for previously unID'd chips, to index arrays for unseal keys
>>>>> and data memory register tables.
>>>>>
>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>> ---
>>>>>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>>>>>  3 files changed, 90 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>> index 4c7a13e..70b8d1c 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>> @@ -57,7 +57,7 @@
>>>>>
>>>>>  #include <linux/power/bq27xxx_battery.h>
>>>>>
>>>>> -#define DRIVER_VERSION               "1.2.0"
>>>>> +#define DRIVER_VERSION               "1.3.0"
>>>>>
>>>>>  #define BQ27XXX_MANUFACTURER "Texas Instruments"
>>>>>
>>>>> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>>>>>       [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>>>>>  };
>>>>>
>>>>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
>>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
>>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
>>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
>>>>> +};
>>>>> +
>>>>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
>>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
>>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
>>>>> +};
>>>>> +
>>>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>> +     [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 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 bq27621_dm_regs[] = {
>>>>> +     [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[] = {
>>>>> +     [BQ27500] = bq27500_dm_regs,
>>>>> +     [BQ27545] = bq27545_dm_regs,
>>>>> +     [BQ27421] = bq27421_dm_regs,
>>>>> +     [BQ27425] = bq27425_dm_regs,
>>>>> +     [BQ27441] = bq27421_dm_regs,
>>>>> +     [BQ27621] = bq27621_dm_regs,
>>>>> +};
>>>>> +
>>>>> +static u32 bq27xxx_unseal_keys[] = {
>>>>> +     [BQ27500] = 0x04143672,
>>>>> +     [BQ27545] = 0x04143672,
>>>>> +     [BQ27421] = 0x80008000,
>>>>> +     [BQ27425] = 0x04143672,
>>>>> +     [BQ27441] = 0x80008000,
>>>>> +     [BQ27621] = 0x80008000,
>>>>> +};
>>>>> +
>>>>>
>>>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>>>  {
>>>>> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>               .drv_data = di,
>>>>>       };
>>>>>
>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>> +
>>>>> +     switch(di->chip) {  /* translate members of categories */
>>>>> +     case BQ2752X:
>>>>> +             di->chip = BQ27510G3; break;
>>>>> +     case BQ27531:
>>>>> +             di->chip = BQ27530;   break;
>>>>> +     case BQ27542:
>>>>> +     case BQ27546:
>>>>> +     case BQ27742:
>>>>> +             di->chip = BQ27541;   break;
>>>>> +     case BQ27425:
>>>>> +     case BQ27441:
>>>>> +     case BQ27621:
>>>>> +             di->chip = BQ27421;   break;
>>>>> +     default:
>>>>> +             break;
>>>>> +     }
>>>>> +
>>>>> +     di->regs = bq27xxx_regs[di->chip];
>>>>> +
>>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>>       mutex_init(&di->lock);
>>>>> -     di->regs = bq27xxx_regs[di->chip];
>>>>>
>>>>>       psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>>>>>       if (!psy_desc)
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> index a597221..0b11ed4 100644
>>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>       { "bq27210", BQ27010 },
>>>>>       { "bq27500", BQ2750X },
>>>>>       { "bq27510", BQ2751X },
>>>>> -     { "bq27520", BQ2751X },
>>>>> +     { "bq27520", BQ2752X },
>>>>>       { "bq27500-1", BQ27500 },
>>>>>       { "bq27510g1", BQ27510G1 },
>>>>>       { "bq27510g2", BQ27510G2 },
>>>>> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>       { "bq27520g3", BQ27520G3 },
>>>>>       { "bq27520g4", BQ27520G4 },
>>>>>       { "bq27530", BQ27530 },
>>>>> -     { "bq27531", BQ27530 },
>>>>> +     { "bq27531", BQ27531 },
>>>>>       { "bq27541", BQ27541 },
>>>>> -     { "bq27542", BQ27541 },
>>>>> -     { "bq27546", BQ27541 },
>>>>> -     { "bq27742", BQ27541 },
>>>>> +     { "bq27542", BQ27542 },
>>>>> +     { "bq27546", BQ27546 },
>>>>> +     { "bq27742", BQ27742 },
>>>>>       { "bq27545", BQ27545 },
>>>>>       { "bq27421", BQ27421 },
>>>>> -     { "bq27425", BQ27421 },
>>>>> -     { "bq27441", BQ27421 },
>>>>> -     { "bq27621", BQ27421 },
>>>>> +     { "bq27425", BQ27425 },
>>>>> +     { "bq27441", BQ27441 },
>>>>> +     { "bq27621", BQ27621 },
>>>>>       {},
>>>>>  };
>>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>>> index 227eb08..014e59f 100644
>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>> @@ -2,6 +2,7 @@
>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>
>>>>>  enum bq27xxx_chip {
>>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>       BQ27545, /* bq27545 */
>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>> +
>>>>> +     /* members of categories; translate these to category in _setup() */
>>>>
>>>> These don't need to be separate from the others, these are valid chip
>>>> IDs, we may use another compatible ID for register array index right now
>>>> but these are not "categories".
>>>
>>> The above were indeed treated like categories, as the I2C ID table
>>> mapped other chips to them. We could call them category|class
>>> "leaders" (prototypes? models? patterns?) instead.
>>>
>>> The below are not valid in di->chip, they must be converted. So it
>>> makes sense to group them.
>>>
>>
>> They are only "not valid" because you did not add tables for them. If
>> you don't want to add register tables then think of a better way to map
>> chip to register table. This is overly complex and will cause problems
>> for people adding more chips.
> 
> This very mapping from category member -> prototype ID was done by the
> I2C table previously. How is doing it in a setup() switch any more
> complex?
> 
> The alternative mapping from chip to regs is this, which you said was
> there but replaced by a 2D array a while ago. Shall we revert to this?
>   bq27xxx_regs[] = {
>     [BQ27991] = bq27991_regs, // prototype
>     [BQ27992] = bq27991_regs, // member
>     ...
>   };
> 

It's either that or add a second index into di, up to you. The easiest
way is still, and always has been, to just add the extra tables. It
would get this series reviewed faster, and we can remove redundant ones
later when we pick a clean way of doing that.

> 
>>>>> +     BQ2752X, /* deprecated alias */
>>>>> +     BQ27531,
>>>>> +     BQ27542,
>>>>> +     BQ27546,
>>>>> +     BQ27742,
>>>>> +     BQ27425,
>>>>> +     BQ27441,
>>>>> +     BQ27621,
>>>>>  };
>>>>>
>>>>>  /**
>>>>>

Re: [PATCH v12.2 8/9] power: bq27xxx_battery: Enable chip data memory update for certain chips

[Liam Breck]

On Tue, Apr 4, 2017 at 2:42 PM, Andrew F. Davis <afd@ti.com> wrote:
> On 04/04/2017 04:37 PM, Liam Breck wrote:
>> On Tue, Apr 4, 2017 at 1:12 PM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 03/30/2017 03:14 PM, Liam Breck wrote:
>>>> On Thu, Mar 30, 2017 at 5:58 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 03/30/2017 04:02 AM, Liam Breck wrote:
>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>
>>>>>> Support data memory update of BQ27500, 545, 421, 425, 441, 621.
>>>>>>
>>>>>> Create IDs for for previously unID'd chips, to index arrays for unseal keys
>>>>>> and data memory register tables.
>>>>>>
>>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>>> ---
>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 73 +++++++++++++++++++++++++++++-
>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++----
>>>>>>  include/linux/power/bq27xxx_battery.h      | 11 +++++
>>>>>>  3 files changed, 90 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>> index 4c7a13e..70b8d1c 100644
>>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>>> @@ -57,7 +57,7 @@
>>>>>>
>>>>>>  #include <linux/power/bq27xxx_battery.h>
>>>>>>
>>>>>> -#define DRIVER_VERSION               "1.2.0"
>>>>>> +#define DRIVER_VERSION               "1.3.0"
>>>>>>
>>>>>>  #define BQ27XXX_MANUFACTURER "Texas Instruments"
>>>>>>
>>>>>> @@ -894,6 +894,54 @@ static const char* bq27xxx_dm_reg_name[] = {
>>>>>>       [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>>>>>>  };
>>>>>>
>>>>>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = {
>>>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 10, 2,    0, 65535 },
>>>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { }, /* missing on chip */
>>>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 },
>>>>>> +};
>>>>>> +
>>>>>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = {
>>>>>> +     [BQ27XXX_DM_DESIGN_CAPACITY]   = { 48, 23, 2,    0, 32767 },
>>>>>> +     [BQ27XXX_DM_DESIGN_ENERGY]     = { 48, 25, 2,    0, 32767 },
>>>>>> +     [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800,  3700 },
>>>>>> +};
>>>>>> +
>>>>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = {
>>>>>> +     [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 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 bq27621_dm_regs[] = {
>>>>>> +     [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[] = {
>>>>>> +     [BQ27500] = bq27500_dm_regs,
>>>>>> +     [BQ27545] = bq27545_dm_regs,
>>>>>> +     [BQ27421] = bq27421_dm_regs,
>>>>>> +     [BQ27425] = bq27425_dm_regs,
>>>>>> +     [BQ27441] = bq27421_dm_regs,
>>>>>> +     [BQ27621] = bq27621_dm_regs,
>>>>>> +};
>>>>>> +
>>>>>> +static u32 bq27xxx_unseal_keys[] = {
>>>>>> +     [BQ27500] = 0x04143672,
>>>>>> +     [BQ27545] = 0x04143672,
>>>>>> +     [BQ27421] = 0x80008000,
>>>>>> +     [BQ27425] = 0x04143672,
>>>>>> +     [BQ27441] = 0x80008000,
>>>>>> +     [BQ27621] = 0x80008000,
>>>>>> +};
>>>>>> +
>>>>>>
>>>>>>  static int poll_interval_param_set(const char *val, const struct kernel_param *kp)
>>>>>>  {
>>>>>> @@ -1861,9 +1909,30 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>               .drv_data = di,
>>>>>>       };
>>>>>>
>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>> +
>>>>>> +     switch(di->chip) {  /* translate members of categories */
>>>>>> +     case BQ2752X:
>>>>>> +             di->chip = BQ27510G3; break;
>>>>>> +     case BQ27531:
>>>>>> +             di->chip = BQ27530;   break;
>>>>>> +     case BQ27542:
>>>>>> +     case BQ27546:
>>>>>> +     case BQ27742:
>>>>>> +             di->chip = BQ27541;   break;
>>>>>> +     case BQ27425:
>>>>>> +     case BQ27441:
>>>>>> +     case BQ27621:
>>>>>> +             di->chip = BQ27421;   break;
>>>>>> +     default:
>>>>>> +             break;
>>>>>> +     }
>>>>>> +
>>>>>> +     di->regs = bq27xxx_regs[di->chip];
>>>>>> +
>>>>>>       INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>>>>>>       mutex_init(&di->lock);
>>>>>> -     di->regs = bq27xxx_regs[di->chip];
>>>>>>
>>>>>>       psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>>>>>>       if (!psy_desc)
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> index a597221..0b11ed4 100644
>>>>>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c
>>>>>> @@ -230,7 +230,7 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>>       { "bq27210", BQ27010 },
>>>>>>       { "bq27500", BQ2750X },
>>>>>>       { "bq27510", BQ2751X },
>>>>>> -     { "bq27520", BQ2751X },
>>>>>> +     { "bq27520", BQ2752X },
>>>>>>       { "bq27500-1", BQ27500 },
>>>>>>       { "bq27510g1", BQ27510G1 },
>>>>>>       { "bq27510g2", BQ27510G2 },
>>>>>> @@ -240,16 +240,16 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>>       { "bq27520g3", BQ27520G3 },
>>>>>>       { "bq27520g4", BQ27520G4 },
>>>>>>       { "bq27530", BQ27530 },
>>>>>> -     { "bq27531", BQ27530 },
>>>>>> +     { "bq27531", BQ27531 },
>>>>>>       { "bq27541", BQ27541 },
>>>>>> -     { "bq27542", BQ27541 },
>>>>>> -     { "bq27546", BQ27541 },
>>>>>> -     { "bq27742", BQ27541 },
>>>>>> +     { "bq27542", BQ27542 },
>>>>>> +     { "bq27546", BQ27546 },
>>>>>> +     { "bq27742", BQ27742 },
>>>>>>       { "bq27545", BQ27545 },
>>>>>>       { "bq27421", BQ27421 },
>>>>>> -     { "bq27425", BQ27421 },
>>>>>> -     { "bq27441", BQ27421 },
>>>>>> -     { "bq27621", BQ27421 },
>>>>>> +     { "bq27425", BQ27425 },
>>>>>> +     { "bq27441", BQ27441 },
>>>>>> +     { "bq27621", BQ27621 },
>>>>>>       {},
>>>>>>  };
>>>>>>  MODULE_DEVICE_TABLE(i2c, bq27xxx_i2c_id_table);
>>>>>> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
>>>>>> index 227eb08..014e59f 100644
>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>> @@ -2,6 +2,7 @@
>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>
>>>>>>  enum bq27xxx_chip {
>>>>>> +     /* categories; index for bq27xxx_regs[] */
>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>> @@ -18,6 +19,16 @@ enum bq27xxx_chip {
>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>       BQ27545, /* bq27545 */
>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>> +
>>>>>> +     /* members of categories; translate these to category in _setup() */
>>>>>
>>>>> These don't need to be separate from the others, these are valid chip
>>>>> IDs, we may use another compatible ID for register array index right now
>>>>> but these are not "categories".
>>>>
>>>> The above were indeed treated like categories, as the I2C ID table
>>>> mapped other chips to them. We could call them category|class
>>>> "leaders" (prototypes? models? patterns?) instead.
>>>>
>>>> The below are not valid in di->chip, they must be converted. So it
>>>> makes sense to group them.
>>>>
>>>
>>> They are only "not valid" because you did not add tables for them. If
>>> you don't want to add register tables then think of a better way to map
>>> chip to register table. This is overly complex and will cause problems
>>> for people adding more chips.
>>
>> This very mapping from category member -> prototype ID was done by the
>> I2C table previously. How is doing it in a setup() switch any more
>> complex?
>>
>> The alternative mapping from chip to regs is this, which you said was
>> there but replaced by a 2D array a while ago. Shall we revert to this?
>>   bq27xxx_regs[] = {
>>     [BQ27991] = bq27991_regs, // prototype
>>     [BQ27992] = bq27991_regs, // member
>>     ...
>>   };
>>
>
> It's either that or add a second index into di, up to you. The easiest
> way is still, and always has been, to just add the extra tables. It
> would get this series reviewed faster, and we can remove redundant ones
> later when we pick a clean way of doing that.

Oh, I think what you want is a new table mapping orig ID to chip ID,
instead of a switch?

>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>> +     BQ27531,
>>>>>> +     BQ27542,
>>>>>> +     BQ27546,
>>>>>> +     BQ27742,
>>>>>> +     BQ27425,
>>>>>> +     BQ27441,
>>>>>> +     BQ27621,
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>>