[RFC v1 0/6] bq27xxx_battery data memory update

[RFC v1 0/6] bq27xxx_battery data memory update

[Liam Breck]

Overview:
* Reorganizes chip data definitions
* Enables features landed in these patches:
  dt-bindings: power: supply: bq27xxx: Add monitored-battery documentation
  power: supply: bq27xxx: Add chip data memory read/write support
  power: supply: bq27xxx: Add power_supply_battery_info support
* Supports the following chips (only BQ27425 is active without #define DEBUG)
  BQ27500, 545, 425, 421, 441, 621

Notes on v1:
* Not fully tested (hence RFC tag)


Liam Breck (6):
  power: supply: bq27xxx: Create single chip data table
  power: supply: bq27xxx: Add chip IDs for previously shadowed chips
  power: supply: bq27xxx: Enable data memory update for certain chips
  power: supply: bq27xxx: Add chip data options for cfgupdate & ram-only
  power: supply: bq27xxx: Flag identical chip data when in debug mode
  power: supply: bq27xxx: Remove duplicate chip data arrays

 drivers/power/supply/bq27xxx_battery.c     | 421 ++++++++++++++++-------------
 drivers/power/supply/bq27xxx_battery_i2c.c |  16 +-
 include/linux/power/bq27xxx_battery.h      |  10 +-
 3 files changed, 245 insertions(+), 202 deletions(-)

-- 
2.13.1

[RFC v1 1/6] power: supply: bq27xxx: Create single chip data table

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

To support new features which require different data for each chip, we
unify the bq27xxx_regs and bq27xxx_battery_props tables into a single one.

No functional changes to the driver.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 121 ++++++++++++++++-----------------
 1 file changed, 60 insertions(+), 61 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index ed44439d..f1c59613 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -132,8 +132,8 @@ enum bq27xxx_reg_index {
 	[BQ27XXX_DM_CKSUM] = 0x60
 
 /* Register mappings */
-static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
-	[BQ27000] = {
+static u8
+	bq27000_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -157,7 +157,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
 		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
-	[BQ27010] = {
+	bq27010_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -181,7 +181,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_DM_DATA] = INVALID_REG_ADDR,
 		[BQ27XXX_DM_CKSUM] = INVALID_REG_ADDR,
 	},
-	[BQ2750X] = {
+	bq2750x_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -201,7 +201,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ2751X] = {
+	bq2751x_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -221,7 +221,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27500] = {
+	bq27500_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -241,7 +241,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27510G1] = {
+	bq27510g1_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -261,7 +261,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27510G2] = {
+	bq27510g2_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -281,7 +281,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27510G3] = {
+	bq27510g3_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -301,7 +301,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27520G1] = {
+	bq27520g1_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = INVALID_REG_ADDR,
@@ -321,7 +321,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27520G2] = {
+	bq27520g2_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x36,
@@ -341,7 +341,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27520G3] = {
+	bq27520g3_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x36,
@@ -361,7 +361,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27520G4] = {
+	bq27520g4_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -381,7 +381,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27530] = {
+	bq27530_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x32,
@@ -401,7 +401,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27541] = {
+	bq27541_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -421,7 +421,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27545] = {
+	bq27545_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
 		[BQ27XXX_REG_INT_TEMP] = 0x28,
@@ -441,7 +441,7 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	[BQ27421] = {
+	bq27421_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x02,
 		[BQ27XXX_REG_INT_TEMP] = 0x1e,
@@ -460,10 +460,9 @@ static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_DCAP] = 0x3c,
 		[BQ27XXX_REG_AP] = 0x18,
 		BQ27XXX_DM_REG_ROWS,
-	},
-};
+	};
 
-static enum power_supply_property bq27000_battery_props[] = {
+static enum power_supply_property bq27000_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -485,7 +484,7 @@ static enum power_supply_property bq27000_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27010_battery_props[] = {
+static enum power_supply_property bq27010_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -505,7 +504,7 @@ static enum power_supply_property bq27010_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq2750x_battery_props[] = {
+static enum power_supply_property bq2750x_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -523,7 +522,7 @@ static enum power_supply_property bq2750x_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq2751x_battery_props[] = {
+static enum power_supply_property bq2751x_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -541,7 +540,7 @@ static enum power_supply_property bq2751x_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27500_battery_props[] = {
+static enum power_supply_property bq27500_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -562,7 +561,7 @@ static enum power_supply_property bq27500_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27510g1_battery_props[] = {
+static enum power_supply_property bq27510g1_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -583,7 +582,7 @@ static enum power_supply_property bq27510g1_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27510g2_battery_props[] = {
+static enum power_supply_property bq27510g2_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -604,7 +603,7 @@ static enum power_supply_property bq27510g2_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27510g3_battery_props[] = {
+static enum power_supply_property bq27510g3_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -622,7 +621,7 @@ static enum power_supply_property bq27510g3_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27520g1_battery_props[] = {
+static enum power_supply_property bq27520g1_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -642,7 +641,7 @@ static enum power_supply_property bq27520g1_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27520g2_battery_props[] = {
+static enum power_supply_property bq27520g2_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -663,7 +662,7 @@ static enum power_supply_property bq27520g2_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27520g3_battery_props[] = {
+static enum power_supply_property bq27520g3_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -683,7 +682,7 @@ static enum power_supply_property bq27520g3_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27520g4_battery_props[] = {
+static enum power_supply_property bq27520g4_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -700,7 +699,7 @@ static enum power_supply_property bq27520g4_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27530_battery_props[] = {
+static enum power_supply_property bq27530_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -718,7 +717,7 @@ static enum power_supply_property bq27530_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27541_battery_props[] = {
+static enum power_supply_property bq27541_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -737,7 +736,7 @@ static enum power_supply_property bq27541_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27545_battery_props[] = {
+static enum power_supply_property bq27545_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -755,7 +754,7 @@ static enum power_supply_property bq27545_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq27421_battery_props[] = {
+static enum power_supply_property bq27421_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
 	POWER_SUPPLY_PROP_PRESENT,
 	POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -770,32 +769,32 @@ static enum power_supply_property bq27421_battery_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-#define BQ27XXX_PROP(_id, _prop)		\
-	[_id] = {				\
-		.props = _prop,			\
-		.size = ARRAY_SIZE(_prop),	\
-	}
+#define BQ27XXX_DATA(ref) {			\
+	.regs  = bq27##ref##_regs,		\
+	.props = bq27##ref##_props,		\
+	.props_size = ARRAY_SIZE(bq27##ref##_props) }
 
 static struct {
+	u8 *regs;
 	enum power_supply_property *props;
-	size_t size;
-} bq27xxx_battery_props[] = {
-	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),
-	BQ27XXX_PROP(BQ27520G3, bq27520g3_battery_props),
-	BQ27XXX_PROP(BQ27520G4, bq27520g4_battery_props),
-	BQ27XXX_PROP(BQ27530, bq27530_battery_props),
-	BQ27XXX_PROP(BQ27541, bq27541_battery_props),
-	BQ27XXX_PROP(BQ27545, bq27545_battery_props),
-	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
+	size_t props_size;
+} bq27xxx_chip_data[] = {
+	[BQ27000]   = BQ27XXX_DATA(000),
+	[BQ27010]   = BQ27XXX_DATA(010),
+	[BQ2750X]   = BQ27XXX_DATA(50x),
+	[BQ2751X]   = BQ27XXX_DATA(51x),
+	[BQ27500]   = BQ27XXX_DATA(500),
+	[BQ27510G1] = BQ27XXX_DATA(510g1),
+	[BQ27510G2] = BQ27XXX_DATA(510g2),
+	[BQ27510G3] = BQ27XXX_DATA(510g3),
+	[BQ27520G1] = BQ27XXX_DATA(520g1),
+	[BQ27520G2] = BQ27XXX_DATA(520g2),
+	[BQ27520G3] = BQ27XXX_DATA(520g3),
+	[BQ27520G4] = BQ27XXX_DATA(520g4),
+	[BQ27530]   = BQ27XXX_DATA(530),
+	[BQ27541]   = BQ27XXX_DATA(541),
+	[BQ27545]   = BQ27XXX_DATA(545),
+	[BQ27421]   = BQ27XXX_DATA(421),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -1884,7 +1883,7 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
-	di->regs = bq27xxx_regs[di->chip];
+	di->regs = bq27xxx_chip_data[di->chip].regs;
 
 	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (!psy_desc)
@@ -1892,8 +1891,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	psy_desc->name = di->name;
 	psy_desc->type = POWER_SUPPLY_TYPE_BATTERY;
-	psy_desc->properties = bq27xxx_battery_props[di->chip].props;
-	psy_desc->num_properties = bq27xxx_battery_props[di->chip].size;
+	psy_desc->properties = bq27xxx_chip_data[di->chip].props;
+	psy_desc->num_properties = bq27xxx_chip_data[di->chip].props_size;
 	psy_desc->get_property = bq27xxx_battery_get_property;
 	psy_desc->external_power_changed = bq27xxx_external_power_changed;
 
-- 
2.13.1

Re: [RFC v1 1/6] power: supply: bq27xxx: Create single chip data table

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

Hi,

On Sat, Jul 08, 2017 at 07:16:55PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> To support new features which require different data for each chip, we
> unify the bq27xxx_regs and bq27xxx_battery_props tables into a single one.
> 
> No functional changes to the driver.

Looks good to me except for one thing.

> [...]
> +#define BQ27XXX_DATA(ref) {			\
> +	.regs  = bq27##ref##_regs,		\
> +	.props = bq27##ref##_props,		\
> +	.props_size = ARRAY_SIZE(bq27##ref##_props) }
>
> [...]
>
> +	[BQ27000]   = BQ27XXX_DATA(000),
> +	[BQ27010]   = BQ27XXX_DATA(010),
> +	[BQ2750X]   = BQ27XXX_DATA(50x),
> +	[BQ2751X]   = BQ27XXX_DATA(51x),
> +	[BQ27500]   = BQ27XXX_DATA(500),
> +	[BQ27510G1] = BQ27XXX_DATA(510g1),
> +	[BQ27510G2] = BQ27XXX_DATA(510g2),
> +	[BQ27510G3] = BQ27XXX_DATA(510g3),
> +	[BQ27520G1] = BQ27XXX_DATA(520g1),
> +	[BQ27520G2] = BQ27XXX_DATA(520g2),
> +	[BQ27520G3] = BQ27XXX_DATA(520g3),
> +	[BQ27520G4] = BQ27XXX_DATA(520g4),
> +	[BQ27530]   = BQ27XXX_DATA(530),
> +	[BQ27541]   = BQ27XXX_DATA(541),
> +	[BQ27545]   = BQ27XXX_DATA(545),
> +	[BQ27421]   = BQ27XXX_DATA(421),
>  };

I would prefer to use ##ref##_regs and ##ref##_props above
and add the bq27 prefix here.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC v1 2/6] power: supply: bq27xxx: Add chip IDs for previously shadowed chips

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

For the existing feature set, these chips act like ones already listed,
so they had been given false but functional IDs. We will be adding features
which obsolete that shadowing, so the following IDs are added:
BQ2752X, 531, 542, 546, 742, 425, 441, 621

So as not to disturb code that relies on the shadowing,
bq27xxx_chip_data[n].acts_like now gives the previous ID for the above
chips. That field will no longer be necessary when the code in question can
test for specific options (to be provided in later patches) with:
(BQ27XXX_O_XYZ & di->opts)

No functional changes to the driver.

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

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index f1c59613..8eafbe2b 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -221,6 +221,7 @@ static u8
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
+#define bq2752x_regs bq2751x_regs
 	bq27500_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -401,6 +402,7 @@ static u8
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
+#define bq27531_regs bq27530_regs
 	bq27541_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -421,6 +423,9 @@ static u8
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
+#define bq27542_regs bq27541_regs
+#define bq27546_regs bq27541_regs
+#define bq27742_regs bq27541_regs
 	bq27545_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -461,6 +466,9 @@ static u8
 		[BQ27XXX_REG_AP] = 0x18,
 		BQ27XXX_DM_REG_ROWS,
 	};
+#define bq27425_regs bq27421_regs
+#define bq27441_regs bq27421_regs
+#define bq27621_regs bq27421_regs
 
 static enum power_supply_property bq27000_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
@@ -539,6 +547,7 @@ static enum power_supply_property bq2751x_props[] = {
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
+#define bq2752x_props bq2751x_props
 
 static enum power_supply_property bq27500_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
@@ -716,6 +725,7 @@ static enum power_supply_property bq27530_props[] = {
 	POWER_SUPPLY_PROP_CYCLE_COUNT,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
+#define bq27531_props bq27530_props
 
 static enum power_supply_property bq27541_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
@@ -735,6 +745,9 @@ static enum power_supply_property bq27541_props[] = {
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
+#define bq27542_props bq27541_props
+#define bq27546_props bq27541_props
+#define bq27742_props bq27541_props
 
 static enum power_supply_property bq27545_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
@@ -768,33 +781,48 @@ static enum power_supply_property bq27421_props[] = {
 	POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
+#define bq27425_props bq27421_props
+#define bq27441_props bq27421_props
+#define bq27621_props bq27421_props
 
-#define BQ27XXX_DATA(ref) {			\
+#define BQ27XXX_DATA(ref, act, opt) {		\
+	.opts = (opt),				\
+	.acts_like = act,			\
 	.regs  = bq27##ref##_regs,		\
 	.props = bq27##ref##_props,		\
 	.props_size = ARRAY_SIZE(bq27##ref##_props) }
 
 static struct {
+	u32 opts;
+	int acts_like; //todo drop this when opts fully implemented
 	u8 *regs;
 	enum power_supply_property *props;
 	size_t props_size;
 } bq27xxx_chip_data[] = {
-	[BQ27000]   = BQ27XXX_DATA(000),
-	[BQ27010]   = BQ27XXX_DATA(010),
-	[BQ2750X]   = BQ27XXX_DATA(50x),
-	[BQ2751X]   = BQ27XXX_DATA(51x),
-	[BQ27500]   = BQ27XXX_DATA(500),
-	[BQ27510G1] = BQ27XXX_DATA(510g1),
-	[BQ27510G2] = BQ27XXX_DATA(510g2),
-	[BQ27510G3] = BQ27XXX_DATA(510g3),
-	[BQ27520G1] = BQ27XXX_DATA(520g1),
-	[BQ27520G2] = BQ27XXX_DATA(520g2),
-	[BQ27520G3] = BQ27XXX_DATA(520g3),
-	[BQ27520G4] = BQ27XXX_DATA(520g4),
-	[BQ27530]   = BQ27XXX_DATA(530),
-	[BQ27541]   = BQ27XXX_DATA(541),
-	[BQ27545]   = BQ27XXX_DATA(545),
-	[BQ27421]   = BQ27XXX_DATA(421),
+	[BQ27000]   = BQ27XXX_DATA(000,   0, 0),
+	[BQ27010]   = BQ27XXX_DATA(010,   0, 0),
+	[BQ2750X]   = BQ27XXX_DATA(50x,   0, 0),
+	[BQ2751X]   = BQ27XXX_DATA(51x,   0, 0),
+	[BQ2752X]   = BQ27XXX_DATA(52x,   BQ2751X, 0),
+	[BQ27500]   = BQ27XXX_DATA(500,   0, 0),
+	[BQ27510G1] = BQ27XXX_DATA(510g1, 0, 0),
+	[BQ27510G2] = BQ27XXX_DATA(510g2, 0, 0),
+	[BQ27510G3] = BQ27XXX_DATA(510g3, 0, 0),
+	[BQ27520G1] = BQ27XXX_DATA(520g1, 0, 0),
+	[BQ27520G2] = BQ27XXX_DATA(520g2, 0, 0),
+	[BQ27520G3] = BQ27XXX_DATA(520g3, 0, 0),
+	[BQ27520G4] = BQ27XXX_DATA(520g4, 0, 0),
+	[BQ27530]   = BQ27XXX_DATA(530,   0, 0),
+	[BQ27531]   = BQ27XXX_DATA(531,   BQ27530, 0),
+	[BQ27541]   = BQ27XXX_DATA(541,   0, 0),
+	[BQ27542]   = BQ27XXX_DATA(542,   BQ27541, 0),
+	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0),
+	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0),
+	[BQ27545]   = BQ27XXX_DATA(545,   0, 0),
+	[BQ27421]   = BQ27XXX_DATA(421,   0, 0),
+	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0),
+	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0),
+	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -1883,7 +1911,9 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
+
 	di->regs = bq27xxx_chip_data[di->chip].regs;
+	di->opts = bq27xxx_chip_data[di->chip].opts;
 
 	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (!psy_desc)
@@ -1896,6 +1926,9 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	psy_desc->get_property = bq27xxx_battery_get_property;
 	psy_desc->external_power_changed = bq27xxx_external_power_changed;
 
+	if (bq27xxx_chip_data[di->chip].acts_like)
+		di->chip = bq27xxx_chip_data[di->chip].acts_like;
+
 	di->bat = power_supply_register_no_ws(di->dev, psy_desc, &psy_cfg);
 	if (IS_ERR(di->bat)) {
 		dev_err(di->dev, "failed to register battery\n");
diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c
index a5972214..0b11ed47 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 11e11685..77fe94f1 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -6,6 +6,7 @@ enum bq27xxx_chip {
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
 	BQ2751X, /* bq27510, bq27520 deprecated alias */
+	BQ2752X,
 	BQ27500, /* bq27500/1 */
 	BQ27510G1, /* bq27510G1 */
 	BQ27510G2, /* bq27510G2 */
@@ -15,9 +16,16 @@ enum bq27xxx_chip {
 	BQ27520G3, /* bq27520G3 */
 	BQ27520G4, /* bq27520G4 */
 	BQ27530, /* bq27530, bq27531 */
+	BQ27531,
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
+	BQ27542,
+	BQ27546,
+	BQ27742,
 	BQ27545, /* bq27545 */
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+	BQ27425,
+	BQ27441,
+	BQ27621,
 };
 
 /**
@@ -64,6 +72,7 @@ struct bq27xxx_device_info {
 	int id;
 	enum bq27xxx_chip chip;
 	bool ram_chip;
+	u32 opts;
 	const char *name;
 	struct bq27xxx_dm_reg *dm_regs;
 	u32 unseal_key;
-- 
2.13.1

Re: [RFC v1 2/6] power: supply: bq27xxx: Add chip IDs for previously shadowed chips

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2968 bytes --]

Hi,

On Sat, Jul 08, 2017 at 07:16:56PM -0700, Liam Breck wrote:
> For the existing feature set, these chips act like ones already listed,
> so they had been given false but functional IDs. We will be adding features
> which obsolete that shadowing, so the following IDs are added:
> BQ2752X, 531, 542, 546, 742, 425, 441, 621
> 
> So as not to disturb code that relies on the shadowing,
> bq27xxx_chip_data[n].acts_like now gives the previous ID for the above
> chips. That field will no longer be necessary when the code in question can
> test for specific options (to be provided in later patches) with:
> (BQ27XXX_O_XYZ & di->opts)

As stated in the other patch it should be (di->opts & BQ27XXX_O_XYZ)

> No functional changes to the driver.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>
> [...]
>
> +	[BQ2752X]   = BQ27XXX_DATA(52x,   BQ2751X, 0),
> +	[BQ27531]   = BQ27XXX_DATA(531,   BQ27530, 0),
> +	[BQ27542]   = BQ27XXX_DATA(542,   BQ27541, 0),
> +	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0),
> +	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0),
> +	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0),
> +	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0),
> +	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0),

Let's get rid of the chip id mapping directly. I deleted everything
but the mapped IDs above. This is what I get for a simple git grep:

drivers/power/supply $ git grep -nE "BQ2751X|BQ27530|BQ27541"
bq27xxx_battery.c:204:  [BQ2751X] = {
bq27xxx_battery.c:384:  [BQ27530] = {
bq27xxx_battery.c:404:  [BQ27541] = {
bq27xxx_battery.c:786:  BQ27XXX_PROP(BQ2751X, bq2751x_battery_props),
bq27xxx_battery.c:795:  BQ27XXX_PROP(BQ27530, bq27530_battery_props),
bq27xxx_battery.c:796:  BQ27XXX_PROP(BQ27541, bq27541_battery_props),
bq27xxx_battery.c:1523: case BQ2751X:
bq27xxx_battery.c:1532: case BQ27541:
bq27xxx_battery.c:1535: case BQ27530:
bq27xxx_battery.c:1548: if (di->chip == BQ27530 || di->chip == BQ27421)
bq27xxx_battery_i2c.c:232:      { "bq27510", BQ2751X },
bq27xxx_battery_i2c.c:233:      { "bq27520", BQ2751X },
bq27xxx_battery_i2c.c:242:      { "bq27530", BQ27530 },
bq27xxx_battery_i2c.c:243:      { "bq27531", BQ27530 },
bq27xxx_battery_i2c.c:244:      { "bq27541", BQ27541 },
bq27xxx_battery_i2c.c:245:      { "bq27542", BQ27541 },
bq27xxx_battery_i2c.c:246:      { "bq27546", BQ27541 },
bq27xxx_battery_i2c.c:247:      { "bq27742", BQ27541 },

Ignoring the chip data defines, that are not affected, this leaves
the following affected lines:

bq27xxx_battery.c:1523: case BQ2751X:
bq27xxx_battery.c:1532: case BQ27541:
bq27xxx_battery.c:1535: case BQ27530:
bq27xxx_battery.c:1548: if (di->chip == BQ27530 || di->chip == BQ27421)

FWIW I do not follow you, that introducing the flags for
those is more error prone, than the ID mapping. If you
don't want to add the flags please just add the new IDs
to the check in bq27xxx_battery_over/undertemp().

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Support data memory update on BQ27500, 545, 425, 421, 441, 621.
With exception of BQ27425, these are only enabled #ifdef DEBUG,
as they are not tested.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 164 ++++++++++++++++++++++++---------
 1 file changed, 123 insertions(+), 41 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 8eafbe2b..0bd46fae 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -58,7 +58,7 @@
 
 #include <linux/power/bq27xxx_battery.h>
 
-#define DRIVER_VERSION		"1.2.0"
+#define DRIVER_VERSION		"1.3.0"
 
 #define BQ27XXX_MANUFACTURER	"Texas Instruments"
 
@@ -785,44 +785,134 @@ static enum power_supply_property bq27421_props[] = {
 #define bq27441_props bq27421_props
 #define bq27621_props bq27421_props
 
-#define BQ27XXX_DATA(ref, act, opt) {		\
+struct bq27xxx_dm_reg {
+	u8 subclass_id;
+	u8 offset;
+	u8 bytes;
+	u16 min, max;
+};
+
+enum bq27xxx_dm_reg_id {
+	BQ27XXX_DM_DESIGN_CAPACITY = 0,
+	BQ27XXX_DM_DESIGN_ENERGY,
+	BQ27XXX_DM_TERMINATE_VOLTAGE,
+};
+
+#define bq27000_dm_regs 0
+#define bq27010_dm_regs 0
+#define bq2750x_dm_regs 0
+#define bq2751x_dm_regs 0
+#define bq2752x_dm_regs 0
+
+#ifdef DEBUG //todo test data memory features on this chip
+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 },
+};
+#else
+#define bq27500_dm_regs 0
+#endif
+
+//todo create data memory definitions from datasheets and test on chips
+#define bq27510g1_dm_regs 0
+#define bq27510g2_dm_regs 0
+#define bq27510g3_dm_regs 0
+#define bq27520g1_dm_regs 0
+#define bq27520g2_dm_regs 0
+#define bq27520g3_dm_regs 0
+#define bq27520g4_dm_regs 0
+#define bq27530_dm_regs 0
+#define bq27531_dm_regs 0
+#define bq27541_dm_regs 0
+#define bq27542_dm_regs 0
+#define bq27546_dm_regs 0
+#define bq27742_dm_regs 0
+
+#ifdef DEBUG //todo test data memory features on this chip
+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 },
+};
+#else
+#define bq27545_dm_regs 0
+#endif
+
+#ifdef DEBUG //todo test data memory features on this chip
+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 },
+};
+#else
+#define bq27421_dm_regs 0
+#endif
+
+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 },
+};
+
+#ifdef DEBUG //todo test data memory features on this chip
+#define bq27441_dm_regs bq27421_dm_regs
+#else
+#define bq27441_dm_regs 0
+#endif
+
+#ifdef DEBUG //todo test data memory features on this chip
+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 },
+};
+#else
+#define bq27621_dm_regs 0
+#endif
+
+#define BQ27XXX_DATA(ref, act, key, opt) {	\
 	.opts = (opt),				\
 	.acts_like = act,			\
+	.unseal_key = key,			\
 	.regs  = bq27##ref##_regs,		\
+	.dm_regs = bq27##ref##_dm_regs,		\
 	.props = bq27##ref##_props,		\
 	.props_size = ARRAY_SIZE(bq27##ref##_props) }
 
 static struct {
 	u32 opts;
 	int acts_like; //todo drop this when opts fully implemented
+	u32 unseal_key;
 	u8 *regs;
+	struct bq27xxx_dm_reg *dm_regs;
 	enum power_supply_property *props;
 	size_t props_size;
 } bq27xxx_chip_data[] = {
-	[BQ27000]   = BQ27XXX_DATA(000,   0, 0),
-	[BQ27010]   = BQ27XXX_DATA(010,   0, 0),
-	[BQ2750X]   = BQ27XXX_DATA(50x,   0, 0),
-	[BQ2751X]   = BQ27XXX_DATA(51x,   0, 0),
-	[BQ2752X]   = BQ27XXX_DATA(52x,   BQ2751X, 0),
-	[BQ27500]   = BQ27XXX_DATA(500,   0, 0),
-	[BQ27510G1] = BQ27XXX_DATA(510g1, 0, 0),
-	[BQ27510G2] = BQ27XXX_DATA(510g2, 0, 0),
-	[BQ27510G3] = BQ27XXX_DATA(510g3, 0, 0),
-	[BQ27520G1] = BQ27XXX_DATA(520g1, 0, 0),
-	[BQ27520G2] = BQ27XXX_DATA(520g2, 0, 0),
-	[BQ27520G3] = BQ27XXX_DATA(520g3, 0, 0),
-	[BQ27520G4] = BQ27XXX_DATA(520g4, 0, 0),
-	[BQ27530]   = BQ27XXX_DATA(530,   0, 0),
-	[BQ27531]   = BQ27XXX_DATA(531,   BQ27530, 0),
-	[BQ27541]   = BQ27XXX_DATA(541,   0, 0),
-	[BQ27542]   = BQ27XXX_DATA(542,   BQ27541, 0),
-	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0),
-	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0),
-	[BQ27545]   = BQ27XXX_DATA(545,   0, 0),
-	[BQ27421]   = BQ27XXX_DATA(421,   0, 0),
-	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0),
-	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0),
-	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0),
+	[BQ27000]   = BQ27XXX_DATA(000,   0, 0, 0),
+	[BQ27010]   = BQ27XXX_DATA(010,   0, 0, 0),
+	[BQ2750X]   = BQ27XXX_DATA(50x,   0, 0, 0),
+	[BQ2751X]   = BQ27XXX_DATA(51x,   0, 0, 0),
+	[BQ2752X]   = BQ27XXX_DATA(52x,   BQ2751X, 0, 0),
+	[BQ27500]   = BQ27XXX_DATA(500,   0,       0x04143672, 0),
+	[BQ27510G1] = BQ27XXX_DATA(510g1, 0, 0, 0),
+	[BQ27510G2] = BQ27XXX_DATA(510g2, 0, 0, 0),
+	[BQ27510G3] = BQ27XXX_DATA(510g3, 0, 0, 0),
+	[BQ27520G1] = BQ27XXX_DATA(520g1, 0, 0, 0),
+	[BQ27520G2] = BQ27XXX_DATA(520g2, 0, 0, 0),
+	[BQ27520G3] = BQ27XXX_DATA(520g3, 0, 0, 0),
+	[BQ27520G4] = BQ27XXX_DATA(520g4, 0, 0, 0),
+	[BQ27530]   = BQ27XXX_DATA(530,   0, 0, 0),
+	[BQ27531]   = BQ27XXX_DATA(531,   BQ27530, 0, 0),
+	[BQ27541]   = BQ27XXX_DATA(541,   0, 0, 0),
+	[BQ27542]   = BQ27XXX_DATA(542,   BQ27541, 0, 0),
+	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0, 0),
+	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0, 0),
+	[BQ27545]   = BQ27XXX_DATA(545,   0,       0x04143672, 0),
+	[BQ27421]   = BQ27XXX_DATA(421,   0,       0x80008000, 0),
+	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0x04143672, 0),
+	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0x80008000, 0),
+	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, 0),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -832,13 +922,6 @@ static LIST_HEAD(bq27xxx_battery_devices);
 
 #define BQ27XXX_DM_SZ	32
 
-struct bq27xxx_dm_reg {
-	u8 subclass_id;
-	u8 offset;
-	u8 bytes;
-	u16 min, max;
-};
-
 /**
  * struct bq27xxx_dm_buf - chip data memory buffer
  * @class: data memory subclass_id
@@ -871,12 +954,6 @@ static inline u16 *bq27xxx_dm_reg_ptr(struct bq27xxx_dm_buf *buf,
 	return NULL;
 }
 
-enum bq27xxx_dm_reg_id {
-	BQ27XXX_DM_DESIGN_CAPACITY = 0,
-	BQ27XXX_DM_DESIGN_ENERGY,
-	BQ27XXX_DM_TERMINATE_VOLTAGE,
-};
-
 static const char * const bq27xxx_dm_reg_name[] = {
 	[BQ27XXX_DM_DESIGN_CAPACITY] = "design-capacity",
 	[BQ27XXX_DM_DESIGN_ENERGY] = "design-energy",
@@ -1912,8 +1989,13 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
 
-	di->regs = bq27xxx_chip_data[di->chip].regs;
-	di->opts = bq27xxx_chip_data[di->chip].opts;
+	di->regs       = bq27xxx_chip_data[di->chip].regs;
+	di->unseal_key = bq27xxx_chip_data[di->chip].unseal_key;
+	di->dm_regs    = bq27xxx_chip_data[di->chip].dm_regs;
+	di->opts       = bq27xxx_chip_data[di->chip].opts;
+	di->ram_chip = di->chip == BQ27421 ||
+		       di->chip == BQ27441 ||
+		       di->chip == BQ27621;
 
 	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (!psy_desc)
-- 
2.13.1

Re: [RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 516 bytes --]

On Sunday 09 July 2017 04:16:57 Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Support data memory update on BQ27500, 545, 425, 421, 441, 621.
> With exception of BQ27425, these are only enabled #ifdef DEBUG,
> as they are not tested.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 164

Why under #ifdef DEBUG? Seems that this is misusing DEBUG which is used 
for other things.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Liam Breck]

Hi Pali,

On Sun, Jul 9, 2017 at 2:07 AM, Pali Rohár <pali.rohar@gmail.com> wrote:
> On Sunday 09 July 2017 04:16:57 Liam Breck wrote:
>> From: Liam Breck <kernel@networkimprov.net>
>>
>> Support data memory update on BQ27500, 545, 425, 421, 441, 621.
>> With exception of BQ27425, these are only enabled #ifdef DEBUG,
>> as they are not tested.
>>
>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c | 164
>
> Why under #ifdef DEBUG? Seems that this is misusing DEBUG which is used
> for other things.

This is a temporary measure, as those definitions are not tested on
their chips. Nothing new happens in debug mode unless
monitored-battery is set in driver's DT config, and it's either a
ram-only chip or the dt_updates_nvm config option is set.

If DEBUG isn't ok in this case, alternative suggestions?

Thanks for your input!

Re: [RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Pali Rohár]

[-- Attachment #1: Type: Text/Plain, Size: 1420 bytes --]

On Sunday 09 July 2017 16:13:59 Liam Breck wrote:
> Hi Pali,
> 
> On Sun, Jul 9, 2017 at 2:07 AM, Pali Rohár <pali.rohar@gmail.com>
> wrote:
> > On Sunday 09 July 2017 04:16:57 Liam Breck wrote:
> >> From: Liam Breck <kernel@networkimprov.net>
> >> 
> >> Support data memory update on BQ27500, 545, 425, 421, 441, 621.
> >> With exception of BQ27425, these are only enabled #ifdef DEBUG,
> >> as they are not tested.
> >> 
> >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> >> ---
> >> 
> >>  drivers/power/supply/bq27xxx_battery.c | 164
> > 
> > Why under #ifdef DEBUG? Seems that this is misusing DEBUG which is
> > used for other things.
> 
> This is a temporary measure, as those definitions are not tested on
> their chips. Nothing new happens in debug mode unless
> monitored-battery is set in driver's DT config, and it's either a
> ram-only chip or the dt_updates_nvm config option is set.
> 
> If DEBUG isn't ok in this case, alternative suggestions?
> 
> Thanks for your input!

DEBUG can be enabled by other kernel option and should provide just more 
debugging functionality. But you are adding non-debug functionality 
under #ifdef DEBUG, which basically is "new experimental untested 
feature".

Better way for "experimental feature" would be introduction of other 
kconfig switch or probably runtime module option...

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC v1 3/6] power: supply: bq27xxx: Enable data memory update for certain chips

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 1766 bytes --]

Hi,

On Sun, Jul 09, 2017 at 05:12:34PM +0200, Pali Rohár wrote:
> On Sunday 09 July 2017 16:13:59 Liam Breck wrote:
> > On Sun, Jul 9, 2017 at 2:07 AM, Pali Rohár <pali.rohar@gmail.com>
> > wrote:
> > > On Sunday 09 July 2017 04:16:57 Liam Breck wrote:
> > >> From: Liam Breck <kernel@networkimprov.net>
> > >> 
> > >> Support data memory update on BQ27500, 545, 425, 421, 441, 621.
> > >> With exception of BQ27425, these are only enabled #ifdef DEBUG,
> > >> as they are not tested.
> > >> 
> > >> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> > >> ---
> > >> 
> > >>  drivers/power/supply/bq27xxx_battery.c | 164
> > > 
> > > Why under #ifdef DEBUG? Seems that this is misusing DEBUG which is
> > > used for other things.
> > 
> > This is a temporary measure, as those definitions are not tested on
> > their chips. Nothing new happens in debug mode unless
> > monitored-battery is set in driver's DT config, and it's either a
> > ram-only chip or the dt_updates_nvm config option is set.
> > 
> > If DEBUG isn't ok in this case, alternative suggestions?
> > 
> > Thanks for your input!
> 
> DEBUG can be enabled by other kernel option and should provide just more 
> debugging functionality. But you are adding non-debug functionality 
> under #ifdef DEBUG, which basically is "new experimental untested 
> feature".
> 
> Better way for "experimental feature" would be introduction of other 
> kconfig switch or probably runtime module option...

As far as I understand it, there is a risk to brick hardware
when internal data is modified. So if these chips are untested,
let's just leave them out.

Testing should be done by a developer anyways and then he can
just add the required values.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC v1 4/6] power: supply: bq27xxx: Add chip data options for cfgupdate & ram-only

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

Add BQ27XXX_O_CFGUP & _O_RAM for use in bq27xxx_chip_data[n].opts
Replace related uses of di->chip == BQ27nnn with (BQ27XXX_O_XYZ & di->opts)

No functional changes to the driver.

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

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 0bd46fae..5d3893a9 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -871,6 +871,9 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
 #define bq27621_dm_regs 0
 #endif
 
+#define BQ27XXX_O_CFGUP  0x00000001
+#define BQ27XXX_O_RAM    0x00000002
+
 #define BQ27XXX_DATA(ref, act, key, opt) {	\
 	.opts = (opt),				\
 	.acts_like = act,			\
@@ -909,10 +912,10 @@ static struct {
 	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0, 0),
 	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0, 0),
 	[BQ27545]   = BQ27XXX_DATA(545,   0,       0x04143672, 0),
-	[BQ27421]   = BQ27XXX_DATA(421,   0,       0x80008000, 0),
-	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0x04143672, 0),
-	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0x80008000, 0),
-	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, 0),
+	[BQ27421]   = BQ27XXX_DATA(421,   0,       0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
+	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0x04143672, BQ27XXX_O_CFGUP),
+	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
+	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
 };
 
 static DEFINE_MUTEX(bq27xxx_list_lock);
@@ -1196,9 +1199,9 @@ static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di,
 	}
 
 #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
-	if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
+	if (!(BQ27XXX_O_RAM & di->opts) && !bq27xxx_dt_to_nvm) {
 #else
-	if (!di->ram_chip) {
+	if (!(BQ27XXX_O_RAM & di->opts)) {
 #endif
 		/* devicetree and NVM differ; defer to NVM */
 		dev_warn(di->dev, "%s has %u; update to %u disallowed "
@@ -1266,7 +1269,7 @@ static inline int bq27xxx_battery_soft_reset(struct bq27xxx_device_info *di)
 static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
 					  struct bq27xxx_dm_buf *buf)
 {
-	bool cfgup = di->chip == BQ27421; /* assume related chips need cfgupdate */
+	bool cfgup = BQ27XXX_O_CFGUP & di->opts;
 	int ret;
 
 	if (!buf->dirty)
@@ -1365,7 +1368,7 @@ static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
 
 	bq27xxx_battery_seal(di);
 
-	if (updated && di->chip != BQ27421) { /* not a cfgupdate chip, so reset */
+	if (updated && !(BQ27XXX_O_CFGUP & di->opts)) {
 		bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false);
 		BQ27XXX_MSLEEP(300); /* reset time is not documented */
 	}
@@ -1993,9 +1996,6 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 	di->unseal_key = bq27xxx_chip_data[di->chip].unseal_key;
 	di->dm_regs    = bq27xxx_chip_data[di->chip].dm_regs;
 	di->opts       = bq27xxx_chip_data[di->chip].opts;
-	di->ram_chip = di->chip == BQ27421 ||
-		       di->chip == BQ27441 ||
-		       di->chip == BQ27621;
 
 	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
 	if (!psy_desc)
diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
index 77fe94f1..8a8a3f61 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -71,7 +71,6 @@ struct bq27xxx_device_info {
 	struct device *dev;
 	int id;
 	enum bq27xxx_chip chip;
-	bool ram_chip;
 	u32 opts;
 	const char *name;
 	struct bq27xxx_dm_reg *dm_regs;
-- 
2.13.1

Re: [RFC v1 4/6] power: supply: bq27xxx: Add chip data options for cfgupdate & ram-only

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 4299 bytes --]

Hi,

On Sat, Jul 08, 2017 at 07:16:58PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> Add BQ27XXX_O_CFGUP & _O_RAM for use in bq27xxx_chip_data[n].opts
> Replace related uses of di->chip == BQ27nnn with (BQ27XXX_O_XYZ & di->opts)

In the kernel we prefer (variable & mask), so it should be (di->opts
& BQ27XXX_O_XYZ). Otherwise looks good.

-- Sebastian

> No functional changes to the driver.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 22 +++++++++++-----------
>  include/linux/power/bq27xxx_battery.h  |  1 -
>  2 files changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 0bd46fae..5d3893a9 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -871,6 +871,9 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
>  #define bq27621_dm_regs 0
>  #endif
>  
> +#define BQ27XXX_O_CFGUP  0x00000001
> +#define BQ27XXX_O_RAM    0x00000002
> +
>  #define BQ27XXX_DATA(ref, act, key, opt) {	\
>  	.opts = (opt),				\
>  	.acts_like = act,			\
> @@ -909,10 +912,10 @@ static struct {
>  	[BQ27546]   = BQ27XXX_DATA(546,   BQ27541, 0, 0),
>  	[BQ27742]   = BQ27XXX_DATA(742,   BQ27541, 0, 0),
>  	[BQ27545]   = BQ27XXX_DATA(545,   0,       0x04143672, 0),
> -	[BQ27421]   = BQ27XXX_DATA(421,   0,       0x80008000, 0),
> -	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0x04143672, 0),
> -	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0x80008000, 0),
> -	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, 0),
> +	[BQ27421]   = BQ27XXX_DATA(421,   0,       0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
> +	[BQ27425]   = BQ27XXX_DATA(425,   BQ27421, 0x04143672, BQ27XXX_O_CFGUP),
> +	[BQ27441]   = BQ27XXX_DATA(441,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
> +	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
>  };
>  
>  static DEFINE_MUTEX(bq27xxx_list_lock);
> @@ -1196,9 +1199,9 @@ static void bq27xxx_battery_update_dm_block(struct bq27xxx_device_info *di,
>  	}
>  
>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
> -	if (!di->ram_chip && !bq27xxx_dt_to_nvm) {
> +	if (!(BQ27XXX_O_RAM & di->opts) && !bq27xxx_dt_to_nvm) {
>  #else
> -	if (!di->ram_chip) {
> +	if (!(BQ27XXX_O_RAM & di->opts)) {
>  #endif
>  		/* devicetree and NVM differ; defer to NVM */
>  		dev_warn(di->dev, "%s has %u; update to %u disallowed "
> @@ -1266,7 +1269,7 @@ static inline int bq27xxx_battery_soft_reset(struct bq27xxx_device_info *di)
>  static int bq27xxx_battery_write_dm_block(struct bq27xxx_device_info *di,
>  					  struct bq27xxx_dm_buf *buf)
>  {
> -	bool cfgup = di->chip == BQ27421; /* assume related chips need cfgupdate */
> +	bool cfgup = BQ27XXX_O_CFGUP & di->opts;
>  	int ret;
>  
>  	if (!buf->dirty)
> @@ -1365,7 +1368,7 @@ static void bq27xxx_battery_set_config(struct bq27xxx_device_info *di,
>  
>  	bq27xxx_battery_seal(di);
>  
> -	if (updated && di->chip != BQ27421) { /* not a cfgupdate chip, so reset */
> +	if (updated && !(BQ27XXX_O_CFGUP & di->opts)) {
>  		bq27xxx_write(di, BQ27XXX_REG_CTRL, BQ27XXX_RESET, false);
>  		BQ27XXX_MSLEEP(300); /* reset time is not documented */
>  	}
> @@ -1993,9 +1996,6 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  	di->unseal_key = bq27xxx_chip_data[di->chip].unseal_key;
>  	di->dm_regs    = bq27xxx_chip_data[di->chip].dm_regs;
>  	di->opts       = bq27xxx_chip_data[di->chip].opts;
> -	di->ram_chip = di->chip == BQ27421 ||
> -		       di->chip == BQ27441 ||
> -		       di->chip == BQ27621;
>  
>  	psy_desc = devm_kzalloc(di->dev, sizeof(*psy_desc), GFP_KERNEL);
>  	if (!psy_desc)
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 77fe94f1..8a8a3f61 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -71,7 +71,6 @@ struct bq27xxx_device_info {
>  	struct device *dev;
>  	int id;
>  	enum bq27xxx_chip chip;
> -	bool ram_chip;
>  	u32 opts;
>  	const char *name;
>  	struct bq27xxx_dm_reg *dm_regs;
> -- 
> 2.13.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC v1 5/6] power: supply: bq27xxx: Flag identical chip data when in debug mode

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

The driver has 13 unique register maps, several of which are shared
by multiple chips. When adding support for a new chip, it's easy to
add a duplicate map by mistake.

In debug mode we now scan bq27xxx_chip_data[n].regs/props/dm_regs for
duplicates.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 36 +++++++++++++++++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 5d3893a9..54755c88 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -883,7 +883,7 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
 	.props = bq27##ref##_props,		\
 	.props_size = ARRAY_SIZE(bq27##ref##_props) }
 
-static struct {
+static struct bq27xxx_chip_datum {
 	u32 opts;
 	int acts_like; //todo drop this when opts fully implemented
 	u32 unseal_key;
@@ -918,6 +918,38 @@ static struct {
 	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
 };
 
+static void __maybe_unused bq27xxx_battery_dbg_dupes(struct bq27xxx_device_info *di)
+{
+	const size_t max = ARRAY_SIZE(bq27xxx_chip_data);
+	const char * const msg = "bq27xxx_chip_data[%d].%s & [%d].%s are identical\n";
+	struct bq27xxx_chip_datum *a, *b;
+	int i, j;
+
+	for (i = 1; i < max-1; i++) {
+		a = bq27xxx_chip_data + i;
+
+		for (j = i+1; j < max; j++) {
+			b = bq27xxx_chip_data + j;
+
+			if (a->regs != b->regs &&
+			    !memcmp(a->regs, b->regs, sizeof(bq27000_regs)))
+				dev_warn(di->dev, msg, i, "regs", j, "regs");
+
+			if (a->props != b->props &&
+			    a->props_size == b->props_size &&
+			    !memcmp(a->props, b->props, a->props_size))
+				dev_warn(di->dev, msg, i, "props", j, "props");
+
+			if (a->dm_regs != b->dm_regs &&
+			    !memcmp(a->dm_regs, b->dm_regs, sizeof(bq27500_dm_regs)))
+				dev_warn(di->dev, msg, i, "dm_regs", j, "dm_regs");
+		}
+	}
+}
+#ifndef DEBUG
+#define bq27xxx_battery_dbg_dupes(di)
+#endif
+
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 
@@ -1989,6 +2021,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 		.drv_data = di,
 	};
 
+	bq27xxx_battery_dbg_dupes(di);
+
 	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
 	mutex_init(&di->lock);
 
-- 
2.13.1

Re: [RFC v1 5/6] power: supply: bq27xxx: Flag identical chip data when in debug mode

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3180 bytes --]

Hi,

On Sat, Jul 08, 2017 at 07:16:59PM -0700, Liam Breck wrote:
> From: Liam Breck <kernel@networkimprov.net>
> 
> The driver has 13 unique register maps, several of which are shared
> by multiple chips. When adding support for a new chip, it's easy to
> add a duplicate map by mistake.

Which is not that bad. Sure, some bytes wasted, but your code also
wastes some bytes. This is something, that should normally tested
@ compile time using static checkers. Independently the patch has
some issues:

> In debug mode we now scan bq27xxx_chip_data[n].regs/props/dm_regs for
> duplicates.
> 
> Signed-off-by: Liam Breck <kernel@networkimprov.net>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 36 +++++++++++++++++++++++++++++++++-
>  1 file changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 5d3893a9..54755c88 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -883,7 +883,7 @@ static struct bq27xxx_dm_reg bq27621_dm_regs[] = {
>  	.props = bq27##ref##_props,		\
>  	.props_size = ARRAY_SIZE(bq27##ref##_props) }
>  
> -static struct {
> +static struct bq27xxx_chip_datum {
>  	u32 opts;
>  	int acts_like; //todo drop this when opts fully implemented
>  	u32 unseal_key;
> @@ -918,6 +918,38 @@ static struct {
>  	[BQ27621]   = BQ27XXX_DATA(621,   BQ27421, 0x80008000, BQ27XXX_O_CFGUP | BQ27XXX_O_RAM),
>  };
>  
> +static void __maybe_unused bq27xxx_battery_dbg_dupes(struct bq27xxx_device_info *di)

If there are multiple bq27xxx batteries installed in the system the
code is executed multiple times, so let's modify this a bit:

> +{
> +	const size_t max = ARRAY_SIZE(bq27xxx_chip_data);
> +	const char * const msg = "bq27xxx_chip_data[%d].%s & [%d].%s are identical\n";
> +	struct bq27xxx_chip_datum *a, *b;
> +	int i, j;

static checked = false;

if (checked)
    return;

> +	for (i = 1; i < max-1; i++) {
> +		a = bq27xxx_chip_data + i;
> +
> +		for (j = i+1; j < max; j++) {
> +			b = bq27xxx_chip_data + j;
> +
> +			if (a->regs != b->regs &&
> +			    !memcmp(a->regs, b->regs, sizeof(bq27000_regs)))
> +				dev_warn(di->dev, msg, i, "regs", j, "regs");
> +
> +			if (a->props != b->props &&
> +			    a->props_size == b->props_size &&
> +			    !memcmp(a->props, b->props, a->props_size))
> +				dev_warn(di->dev, msg, i, "props", j, "props");
> +
> +			if (a->dm_regs != b->dm_regs &&
> +			    !memcmp(a->dm_regs, b->dm_regs, sizeof(bq27500_dm_regs)))
> +				dev_warn(di->dev, msg, i, "dm_regs", j, "dm_regs");
> +		}
> +	}

checked = true;

> +}
> +#ifndef DEBUG
> +#define bq27xxx_battery_dbg_dupes(di)
> +#endif
> +
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>  
> @@ -1989,6 +2021,8 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  		.drv_data = di,
>  	};
>  
> +	bq27xxx_battery_dbg_dupes(di);

Add #ifdef DEBUG here and drop the above.

>  	INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll);
>  	mutex_init(&di->lock);
>  
> -- 
> 2.13.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[RFC v1 6/6] power: supply: bq27xxx: Remove duplicate chip data arrays

[Liam Breck]

From: Liam Breck <kernel@networkimprov.net>

BQ2751X & BQ27510G3 have identical regs & props.
BQ27500 & BQ27510G1 & BQ27510G2 have identical regs & props.
Remove the duplicate arrays.

No functional changes to the driver.

Signed-off-by: Liam Breck <kernel@networkimprov.net>
---
 drivers/power/supply/bq27xxx_battery.c | 129 ++-------------------------------
 1 file changed, 8 insertions(+), 121 deletions(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 54755c88..3632784e 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -201,27 +201,8 @@ static u8
 		[BQ27XXX_REG_AP] = INVALID_REG_ADDR,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	bq2751x_regs[BQ27XXX_REG_MAX] = {
-		[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_REG_ROWS,
-	},
-#define bq2752x_regs bq2751x_regs
+#define bq2751x_regs bq27510g3_regs
+#define bq2752x_regs bq27510g3_regs
 	bq27500_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -242,46 +223,8 @@ static u8
 		[BQ27XXX_REG_AP] = 0x24,
 		BQ27XXX_DM_REG_ROWS,
 	},
-	bq27510g1_regs[BQ27XXX_REG_MAX] = {
-		[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_REG_ROWS,
-	},
-	bq27510g2_regs[BQ27XXX_REG_MAX] = {
-		[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_REG_ROWS,
-	},
+#define bq27510g1_regs bq27500_regs
+#define bq27510g2_regs bq27500_regs
 	bq27510g3_regs[BQ27XXX_REG_MAX] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -530,24 +473,8 @@ static enum power_supply_property bq2750x_props[] = {
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
 
-static enum power_supply_property bq2751x_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,
-};
-#define bq2752x_props bq2751x_props
+#define bq2751x_props bq27510g3_props
+#define bq2752x_props bq27510g3_props
 
 static enum power_supply_property bq27500_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
@@ -569,48 +496,8 @@ static enum power_supply_property bq27500_props[] = {
 	POWER_SUPPLY_PROP_HEALTH,
 	POWER_SUPPLY_PROP_MANUFACTURER,
 };
-
-static enum power_supply_property bq27510g1_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_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,
-};
+#define bq27510g1_props bq27500_props
+#define bq27510g2_props bq27500_props
 
 static enum power_supply_property bq27510g3_props[] = {
 	POWER_SUPPLY_PROP_STATUS,
-- 
2.13.1