[v13,09/11] power: supply: bq27xxx_battery: Enable data memory update for certain chips

Commit Message

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

From: Liam Breck <[e-mail]>

Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
---
 drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
 drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
 include/linux/power/bq27xxx_battery.h      | 13 +++++
 3 files changed, 107 insertions(+), 12 deletions(-)

Comments

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

On 05/04/2017 01:18 AM, Liam Breck wrote:
> From: Liam Breck <[e-mail]>
> 
> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>  3 files changed, 107 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 06f15da..0aecd41 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"
>  
> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>  	[BQ27XXX_DM_CKSUM] = 0x60
>  
>  /* Register mappings */
> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>  	[BQ27000] = {
>  		[BQ27XXX_REG_CTRL] = 0x00,
>  		[BQ27XXX_REG_TEMP] = 0x06,
> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>  static struct {
>  	enum power_supply_property *props;
>  	size_t size;
> -} bq27xxx_battery_props[] = {
> +} bq27xxx_battery_props[BQ27MAX] = {
>  	BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>  	BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>  	BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
> @@ -798,6 +798,33 @@ static struct {
>  	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>  };
>  
> +static enum bq27xxx_chip bq27xxx_chips[] = {
> +	[BQ27000]   = BQ27000,
> +	[BQ27010]   = BQ27010,
> +	[BQ2750X]   = BQ2750X,
> +	[BQ2751X]   = BQ2751X,
> +	[BQ2752X]   = BQ2751X,
> +	[BQ27500]   = BQ27500,
> +	[BQ27510G1] = BQ27510G1,
> +	[BQ27510G2] = BQ27510G2,
> +	[BQ27510G3] = BQ27510G3,
> +	[BQ27520G1] = BQ27520G1,
> +	[BQ27520G2] = BQ27520G2,
> +	[BQ27520G3] = BQ27520G3,
> +	[BQ27520G4] = BQ27520G4,
> +	[BQ27530]   = BQ27530,
> +	[BQ27531]   = BQ27530,
> +	[BQ27541]   = BQ27541,
> +	[BQ27542]   = BQ27541,
> +	[BQ27546]   = BQ27541,
> +	[BQ27742]   = BQ27541,
> +	[BQ27545]   = BQ27545,
> +	[BQ27421]   = BQ27421,
> +	[BQ27425]   = BQ27421,
> +	[BQ27441]   = BQ27421,
> +	[BQ27621]   = BQ27421,
> +};
> +
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>  
> @@ -856,6 +883,54 @@ static const char * const 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,
> +};
> +
>  
>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>  static bool bq27xxx_dt_to_nvm = true;
> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>  		.drv_data = di,
>  	};
>  
> +	di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
> +
> +	di->unseal_key = bq27xxx_unseal_keys[di->chip];
> +	di->dm_regs = bq27xxx_dm_regs[di->chip];
> +	di->chip = bq27xxx_chips[di->chip];

NACK, this is a mess, you should not be using a table to change what
chip was passed in, it may be needed later. The chip is still the same,
it should have the same one correct ID, use a different index or array
if you would like, but this is very hacky.

Just stop trying to hack around it, add the extra tables, even if they
are clones, so we can be done with this series already. I'm sure you
want that more than you want this "clever" work-around, right?

Also fix all the checkpatch --strict warnings.

> +
> +	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 11e1168..543c10e 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -2,6 +2,8 @@
>  #define __LINUX_BQ27X00_BATTERY_H__
>  
>  enum bq27xxx_chip {
> +	/* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
> +	/* and map to themselves in bq27xxx_chips[]             */
>  	BQ27000 = 1, /* bq27000, bq27200 */
>  	BQ27010, /* bq27010, bq27210 */
>  	BQ2750X, /* bq27500 deprecated alias */
> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>  	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>  	BQ27545, /* bq27545 */
>  	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
> +	BQ27MAX,
> +
> +	/* these map to above in bq27xxx_chips[] */
> +	BQ2752X, /* deprecated alias */
> +	BQ27531,
> +	BQ27542,
> +	BQ27546,
> +	BQ27742,
> +	BQ27425,
> +	BQ27441,
> +	BQ27621,
>  };
>  
>  /**
>

Liam Breck May 4, 2017, 7:12 p.m. UTC | #2

On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/04/2017 01:18 AM, Liam Breck wrote:
>> From: Liam Breck <[e-mail]>
>>
>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 06f15da..0aecd41 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"
>>
>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>       [BQ27XXX_DM_CKSUM] = 0x60
>>
>>  /* Register mappings */
>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>       [BQ27000] = {
>>               [BQ27XXX_REG_CTRL] = 0x00,
>>               [BQ27XXX_REG_TEMP] = 0x06,
>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>  static struct {
>>       enum power_supply_property *props;
>>       size_t size;
>> -} bq27xxx_battery_props[] = {
>> +} bq27xxx_battery_props[BQ27MAX] = {
>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>> @@ -798,6 +798,33 @@ static struct {
>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>  };
>>
>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>> +     [BQ27000]   = BQ27000,
>> +     [BQ27010]   = BQ27010,
>> +     [BQ2750X]   = BQ2750X,
>> +     [BQ2751X]   = BQ2751X,
>> +     [BQ2752X]   = BQ2751X,
>> +     [BQ27500]   = BQ27500,
>> +     [BQ27510G1] = BQ27510G1,
>> +     [BQ27510G2] = BQ27510G2,
>> +     [BQ27510G3] = BQ27510G3,
>> +     [BQ27520G1] = BQ27520G1,
>> +     [BQ27520G2] = BQ27520G2,
>> +     [BQ27520G3] = BQ27520G3,
>> +     [BQ27520G4] = BQ27520G4,
>> +     [BQ27530]   = BQ27530,
>> +     [BQ27531]   = BQ27530,
>> +     [BQ27541]   = BQ27541,
>> +     [BQ27542]   = BQ27541,
>> +     [BQ27546]   = BQ27541,
>> +     [BQ27742]   = BQ27541,
>> +     [BQ27545]   = BQ27545,
>> +     [BQ27421]   = BQ27421,
>> +     [BQ27425]   = BQ27421,
>> +     [BQ27441]   = BQ27421,
>> +     [BQ27621]   = BQ27421,
>> +};
>> +
>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>  static LIST_HEAD(bq27xxx_battery_devices);
>>
>> @@ -856,6 +883,54 @@ static const char * const 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,
>> +};
>> +
>>
>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>  static bool bq27xxx_dt_to_nvm = true;
>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>               .drv_data = di,
>>       };
>>
>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>> +
>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>> +     di->chip = bq27xxx_chips[di->chip];
>
> NACK, this is a mess, you should not be using a table to change what
> chip was passed in, it may be needed later. The chip is still the same,
> it should have the same one correct ID, use a different index or array
> if you would like, but this is very hacky.

You forget that the original I2C table did this exact translation.
This isn't any more hacky than that. I do not see a better
alternative. If you do, pls draft a little code to illustrate it.

> Just stop trying to hack around it, add the extra tables, even if they
> are clones, so we can be done with this series already. I'm sure you
> want that more than you want this "clever" work-around, right?

I have simply replicated the original design, which avoided dupes in
the reg & properties tables by translating IDs in the I2C table. It's
not a work-around on my part, and I believe it's the correct scheme.

> Also fix all the checkpatch --strict warnings.

OK, but some lines are more readable over 80 chars, so I will leave them as is.

Can you take another pass or two to illuminate any remaining issues
before my next rev? I'd like it to be the last one :-)

>> +
>> +     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 11e1168..543c10e 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -2,6 +2,8 @@
>>  #define __LINUX_BQ27X00_BATTERY_H__
>>
>>  enum bq27xxx_chip {
>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>> +     /* and map to themselves in bq27xxx_chips[]             */
>>       BQ27000 = 1, /* bq27000, bq27200 */
>>       BQ27010, /* bq27010, bq27210 */
>>       BQ2750X, /* bq27500 deprecated alias */
>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>       BQ27545, /* bq27545 */
>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>> +     BQ27MAX,
>> +
>> +     /* these map to above in bq27xxx_chips[] */
>> +     BQ2752X, /* deprecated alias */
>> +     BQ27531,
>> +     BQ27542,
>> +     BQ27546,
>> +     BQ27742,
>> +     BQ27425,
>> +     BQ27441,
>> +     BQ27621,
>>  };
>>
>>  /**
>>

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

On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/04/2017 01:18 AM, Liam Breck wrote:
>> From: Liam Breck <[e-mail]>
>>
>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>> ---
>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>> index 06f15da..0aecd41 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"
>>
>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>       [BQ27XXX_DM_CKSUM] = 0x60
>>
>>  /* Register mappings */
>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>       [BQ27000] = {
>>               [BQ27XXX_REG_CTRL] = 0x00,
>>               [BQ27XXX_REG_TEMP] = 0x06,
>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>  static struct {
>>       enum power_supply_property *props;
>>       size_t size;
>> -} bq27xxx_battery_props[] = {
>> +} bq27xxx_battery_props[BQ27MAX] = {
>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>> @@ -798,6 +798,33 @@ static struct {
>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>  };
>>
>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>> +     [BQ27000]   = BQ27000,
>> +     [BQ27010]   = BQ27010,
>> +     [BQ2750X]   = BQ2750X,
>> +     [BQ2751X]   = BQ2751X,
>> +     [BQ2752X]   = BQ2751X,
>> +     [BQ27500]   = BQ27500,
>> +     [BQ27510G1] = BQ27510G1,
>> +     [BQ27510G2] = BQ27510G2,
>> +     [BQ27510G3] = BQ27510G3,
>> +     [BQ27520G1] = BQ27520G1,
>> +     [BQ27520G2] = BQ27520G2,
>> +     [BQ27520G3] = BQ27520G3,
>> +     [BQ27520G4] = BQ27520G4,
>> +     [BQ27530]   = BQ27530,
>> +     [BQ27531]   = BQ27530,
>> +     [BQ27541]   = BQ27541,
>> +     [BQ27542]   = BQ27541,
>> +     [BQ27546]   = BQ27541,
>> +     [BQ27742]   = BQ27541,
>> +     [BQ27545]   = BQ27545,
>> +     [BQ27421]   = BQ27421,
>> +     [BQ27425]   = BQ27421,
>> +     [BQ27441]   = BQ27421,
>> +     [BQ27621]   = BQ27421,
>> +};
>> +
>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>  static LIST_HEAD(bq27xxx_battery_devices);
>>
>> @@ -856,6 +883,54 @@ static const char * const 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,
>> +};
>> +
>>
>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>  static bool bq27xxx_dt_to_nvm = true;
>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>               .drv_data = di,
>>       };
>>
>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>> +
>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>> +     di->chip = bq27xxx_chips[di->chip];
>
> NACK, this is a mess, you should not be using a table to change what
> chip was passed in, it may be needed later. The chip is still the same,
> it should have the same one correct ID, use a different index or array
> if you would like, but this is very hacky.

The only way I see to make the I2C subsystem deliver multiple IDs for
a device is to treat i2c_device_id.driver_data as a u16[2] instead of
its normal u32. What do you think of that?


> Just stop trying to hack around it, add the extra tables, even if they
> are clones, so we can be done with this series already. I'm sure you
> want that more than you want this "clever" work-around, right?
>
> Also fix all the checkpatch --strict warnings.
>
>> +
>> +     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 11e1168..543c10e 100644
>> --- a/include/linux/power/bq27xxx_battery.h
>> +++ b/include/linux/power/bq27xxx_battery.h
>> @@ -2,6 +2,8 @@
>>  #define __LINUX_BQ27X00_BATTERY_H__
>>
>>  enum bq27xxx_chip {
>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>> +     /* and map to themselves in bq27xxx_chips[]             */
>>       BQ27000 = 1, /* bq27000, bq27200 */
>>       BQ27010, /* bq27010, bq27210 */
>>       BQ2750X, /* bq27500 deprecated alias */
>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>       BQ27545, /* bq27545 */
>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>> +     BQ27MAX,
>> +
>> +     /* these map to above in bq27xxx_chips[] */
>> +     BQ2752X, /* deprecated alias */
>> +     BQ27531,
>> +     BQ27542,
>> +     BQ27546,
>> +     BQ27742,
>> +     BQ27425,
>> +     BQ27441,
>> +     BQ27621,
>>  };
>>
>>  /**
>>

Andrew Davis May 5, 2017, 7:45 p.m. UTC | #4

On 05/05/2017 02:31 PM, Liam Breck wrote:
> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>> From: Liam Breck <[e-mail]>
>>>
>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>> ---
>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>> index 06f15da..0aecd41 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"
>>>
>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>
>>>  /* Register mappings */
>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>       [BQ27000] = {
>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>  static struct {
>>>       enum power_supply_property *props;
>>>       size_t size;
>>> -} bq27xxx_battery_props[] = {
>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>> @@ -798,6 +798,33 @@ static struct {
>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>  };
>>>
>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>> +     [BQ27000]   = BQ27000,
>>> +     [BQ27010]   = BQ27010,
>>> +     [BQ2750X]   = BQ2750X,
>>> +     [BQ2751X]   = BQ2751X,
>>> +     [BQ2752X]   = BQ2751X,
>>> +     [BQ27500]   = BQ27500,
>>> +     [BQ27510G1] = BQ27510G1,
>>> +     [BQ27510G2] = BQ27510G2,
>>> +     [BQ27510G3] = BQ27510G3,
>>> +     [BQ27520G1] = BQ27520G1,
>>> +     [BQ27520G2] = BQ27520G2,
>>> +     [BQ27520G3] = BQ27520G3,
>>> +     [BQ27520G4] = BQ27520G4,
>>> +     [BQ27530]   = BQ27530,
>>> +     [BQ27531]   = BQ27530,
>>> +     [BQ27541]   = BQ27541,
>>> +     [BQ27542]   = BQ27541,
>>> +     [BQ27546]   = BQ27541,
>>> +     [BQ27742]   = BQ27541,
>>> +     [BQ27545]   = BQ27545,
>>> +     [BQ27421]   = BQ27421,
>>> +     [BQ27425]   = BQ27421,
>>> +     [BQ27441]   = BQ27421,
>>> +     [BQ27621]   = BQ27421,
>>> +};
>>> +
>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>
>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>> +};
>>> +
>>>
>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>  static bool bq27xxx_dt_to_nvm = true;
>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>               .drv_data = di,
>>>       };
>>>
>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>> +
>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>> +     di->chip = bq27xxx_chips[di->chip];
>>
>> NACK, this is a mess, you should not be using a table to change what
>> chip was passed in, it may be needed later. The chip is still the same,
>> it should have the same one correct ID, use a different index or array
>> if you would like, but this is very hacky.
> 
> The only way I see to make the I2C subsystem deliver multiple IDs for
> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
> its normal u32. What do you think of that?
> 

Why would it need multiple IDs for one device? Just pass in the one
correct device ID that it is. No reason to make this so complicated.
From the one ID you can then use tables to lookup any other info about
that device, you don't have to have ever ID populated in every table,
you can even have a table of tables if you would like:

static const struct chip_lookup lookup_table = {
	[bq27425] = {
		.regs = &bq27xxx_regs[bq27425],
		.dm_regs = &bq27xxx_dm_regs[bq274xx],
		.unseal_key = INVALID_FOR_THIS_DEVICE,
	},
	[bq27343] = {
...

Not sure if that is valid C but I think you can get the idea.

> 
>> Just stop trying to hack around it, add the extra tables, even if they
>> are clones, so we can be done with this series already. I'm sure you
>> want that more than you want this "clever" work-around, right?
>>
>> Also fix all the checkpatch --strict warnings.
>>
>>> +
>>> +     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 11e1168..543c10e 100644
>>> --- a/include/linux/power/bq27xxx_battery.h
>>> +++ b/include/linux/power/bq27xxx_battery.h
>>> @@ -2,6 +2,8 @@
>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>
>>>  enum bq27xxx_chip {
>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>       BQ27010, /* bq27010, bq27210 */
>>>       BQ2750X, /* bq27500 deprecated alias */
>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>       BQ27545, /* bq27545 */
>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>> +     BQ27MAX,
>>> +
>>> +     /* these map to above in bq27xxx_chips[] */
>>> +     BQ2752X, /* deprecated alias */
>>> +     BQ27531,
>>> +     BQ27542,
>>> +     BQ27546,
>>> +     BQ27742,
>>> +     BQ27425,
>>> +     BQ27441,
>>> +     BQ27621,
>>>  };
>>>
>>>  /**
>>>

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

On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/05/2017 02:31 PM, Liam Breck wrote:
>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>> From: Liam Breck <[e-mail]>
>>>>
>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>> ---
>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>> index 06f15da..0aecd41 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"
>>>>
>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>
>>>>  /* Register mappings */
>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>       [BQ27000] = {
>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>  static struct {
>>>>       enum power_supply_property *props;
>>>>       size_t size;
>>>> -} bq27xxx_battery_props[] = {
>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>> @@ -798,6 +798,33 @@ static struct {
>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>  };
>>>>
>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>> +     [BQ27000]   = BQ27000,
>>>> +     [BQ27010]   = BQ27010,
>>>> +     [BQ2750X]   = BQ2750X,
>>>> +     [BQ2751X]   = BQ2751X,
>>>> +     [BQ2752X]   = BQ2751X,
>>>> +     [BQ27500]   = BQ27500,
>>>> +     [BQ27510G1] = BQ27510G1,
>>>> +     [BQ27510G2] = BQ27510G2,
>>>> +     [BQ27510G3] = BQ27510G3,
>>>> +     [BQ27520G1] = BQ27520G1,
>>>> +     [BQ27520G2] = BQ27520G2,
>>>> +     [BQ27520G3] = BQ27520G3,
>>>> +     [BQ27520G4] = BQ27520G4,
>>>> +     [BQ27530]   = BQ27530,
>>>> +     [BQ27531]   = BQ27530,
>>>> +     [BQ27541]   = BQ27541,
>>>> +     [BQ27542]   = BQ27541,
>>>> +     [BQ27546]   = BQ27541,
>>>> +     [BQ27742]   = BQ27541,
>>>> +     [BQ27545]   = BQ27545,
>>>> +     [BQ27421]   = BQ27421,
>>>> +     [BQ27425]   = BQ27421,
>>>> +     [BQ27441]   = BQ27421,
>>>> +     [BQ27621]   = BQ27421,
>>>> +};
>>>> +
>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>
>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>> +};
>>>> +
>>>>
>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>               .drv_data = di,
>>>>       };
>>>>
>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>> +
>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>
>>> NACK, this is a mess, you should not be using a table to change what
>>> chip was passed in, it may be needed later. The chip is still the same,
>>> it should have the same one correct ID, use a different index or array
>>> if you would like, but this is very hacky.
>>
>> The only way I see to make the I2C subsystem deliver multiple IDs for
>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>> its normal u32. What do you think of that?
>>
>
> Why would it need multiple IDs for one device? Just pass in the one
> correct device ID that it is. No reason to make this so complicated.

Take a look at bq27xxx_i2c_id_table[] here:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148

Notice how this table maps certain chips to the same IDs. I am going
to preserve that scheme, as changing it is outside the scope of this
series.

So the only question is how to deliver the "real" ID to the driver. In
v13, I change the I2C table to deliver real-ID, and replicate the
mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
make the I2C table carry two values, chip-ID and real-ID. Then we can
drop bq27xxx_chips[].

Hope that clarifies the issue...

> From the one ID you can then use tables to lookup any other info about
> that device, you don't have to have ever ID populated in every table,
> you can even have a table of tables if you would like:
>
> static const struct chip_lookup lookup_table = {
>         [bq27425] = {
>                 .regs = &bq27xxx_regs[bq27425],
>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>         },
>         [bq27343] = {
> ...
>
> Not sure if that is valid C but I think you can get the idea.
>
>>
>>> Just stop trying to hack around it, add the extra tables, even if they
>>> are clones, so we can be done with this series already. I'm sure you
>>> want that more than you want this "clever" work-around, right?
>>>
>>> Also fix all the checkpatch --strict warnings.
>>>
>>>> +
>>>> +     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 11e1168..543c10e 100644
>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>> @@ -2,6 +2,8 @@
>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>
>>>>  enum bq27xxx_chip {
>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>       BQ27010, /* bq27010, bq27210 */
>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>       BQ27545, /* bq27545 */
>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>> +     BQ27MAX,
>>>> +
>>>> +     /* these map to above in bq27xxx_chips[] */
>>>> +     BQ2752X, /* deprecated alias */
>>>> +     BQ27531,
>>>> +     BQ27542,
>>>> +     BQ27546,
>>>> +     BQ27742,
>>>> +     BQ27425,
>>>> +     BQ27441,
>>>> +     BQ27621,
>>>>  };
>>>>
>>>>  /**
>>>>

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

On 05/05/2017 03:14 PM, Liam Breck wrote:
> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>> From: Liam Breck <[e-mail]>
>>>>>
>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>> ---
>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>> index 06f15da..0aecd41 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"
>>>>>
>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>
>>>>>  /* Register mappings */
>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>       [BQ27000] = {
>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>  static struct {
>>>>>       enum power_supply_property *props;
>>>>>       size_t size;
>>>>> -} bq27xxx_battery_props[] = {
>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>  };
>>>>>
>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>> +     [BQ27000]   = BQ27000,
>>>>> +     [BQ27010]   = BQ27010,
>>>>> +     [BQ2750X]   = BQ2750X,
>>>>> +     [BQ2751X]   = BQ2751X,
>>>>> +     [BQ2752X]   = BQ2751X,
>>>>> +     [BQ27500]   = BQ27500,
>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>> +     [BQ27530]   = BQ27530,
>>>>> +     [BQ27531]   = BQ27530,
>>>>> +     [BQ27541]   = BQ27541,
>>>>> +     [BQ27542]   = BQ27541,
>>>>> +     [BQ27546]   = BQ27541,
>>>>> +     [BQ27742]   = BQ27541,
>>>>> +     [BQ27545]   = BQ27545,
>>>>> +     [BQ27421]   = BQ27421,
>>>>> +     [BQ27425]   = BQ27421,
>>>>> +     [BQ27441]   = BQ27421,
>>>>> +     [BQ27621]   = BQ27421,
>>>>> +};
>>>>> +
>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>
>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>> +};
>>>>> +
>>>>>
>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>               .drv_data = di,
>>>>>       };
>>>>>
>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>> +
>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>
>>>> NACK, this is a mess, you should not be using a table to change what
>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>> it should have the same one correct ID, use a different index or array
>>>> if you would like, but this is very hacky.
>>>
>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>> its normal u32. What do you think of that?
>>>
>>
>> Why would it need multiple IDs for one device? Just pass in the one
>> correct device ID that it is. No reason to make this so complicated.
> 
> Take a look at bq27xxx_i2c_id_table[] here:
> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
> 
> Notice how this table maps certain chips to the same IDs. I am going
> to preserve that scheme, as changing it is outside the scope of this
> series.
> 

You can't just say doing something the right is "outside the scope of
this series" then hack something together...

Fixing this should be trivial anyway, add the new IDs, pass the real IDs
in, then add the translation table so you don't have to add any new
entries to bq27xxx_regs.

> So the only question is how to deliver the "real" ID to the driver. In
> v13, I change the I2C table to deliver real-ID, and replicate the
> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
> make the I2C table carry two values, chip-ID and real-ID. Then we can
> drop bq27xxx_chips[].
> 
> Hope that clarifies the issue...
> 
>> From the one ID you can then use tables to lookup any other info about
>> that device, you don't have to have ever ID populated in every table,
>> you can even have a table of tables if you would like:
>>
>> static const struct chip_lookup lookup_table = {
>>         [bq27425] = {
>>                 .regs = &bq27xxx_regs[bq27425],
>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>         },
>>         [bq27343] = {
>> ...
>>
>> Not sure if that is valid C but I think you can get the idea.
>>
>>>
>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>> are clones, so we can be done with this series already. I'm sure you
>>>> want that more than you want this "clever" work-around, right?
>>>>
>>>> Also fix all the checkpatch --strict warnings.
>>>>
>>>>> +
>>>>> +     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 11e1168..543c10e 100644
>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>> @@ -2,6 +2,8 @@
>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>
>>>>>  enum bq27xxx_chip {
>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>       BQ27545, /* bq27545 */
>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>> +     BQ27MAX,
>>>>> +
>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>> +     BQ2752X, /* deprecated alias */
>>>>> +     BQ27531,
>>>>> +     BQ27542,
>>>>> +     BQ27546,
>>>>> +     BQ27742,
>>>>> +     BQ27425,
>>>>> +     BQ27441,
>>>>> +     BQ27621,
>>>>>  };
>>>>>
>>>>>  /**
>>>>>

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

On Fri, May 5, 2017 at 1:44 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/05/2017 03:14 PM, Liam Breck wrote:
>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>> From: Liam Breck <[e-mail]>
>>>>>>
>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>> ---
>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>> index 06f15da..0aecd41 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"
>>>>>>
>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>
>>>>>>  /* Register mappings */
>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>       [BQ27000] = {
>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>  static struct {
>>>>>>       enum power_supply_property *props;
>>>>>>       size_t size;
>>>>>> -} bq27xxx_battery_props[] = {
>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>  };
>>>>>>
>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>> +     [BQ27000]   = BQ27000,
>>>>>> +     [BQ27010]   = BQ27010,
>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>> +     [BQ27500]   = BQ27500,
>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>> +     [BQ27530]   = BQ27530,
>>>>>> +     [BQ27531]   = BQ27530,
>>>>>> +     [BQ27541]   = BQ27541,
>>>>>> +     [BQ27542]   = BQ27541,
>>>>>> +     [BQ27546]   = BQ27541,
>>>>>> +     [BQ27742]   = BQ27541,
>>>>>> +     [BQ27545]   = BQ27545,
>>>>>> +     [BQ27421]   = BQ27421,
>>>>>> +     [BQ27425]   = BQ27421,
>>>>>> +     [BQ27441]   = BQ27421,
>>>>>> +     [BQ27621]   = BQ27421,
>>>>>> +};
>>>>>> +
>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>
>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>> +};
>>>>>> +
>>>>>>
>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>               .drv_data = di,
>>>>>>       };
>>>>>>
>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>> +
>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>
>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>> it should have the same one correct ID, use a different index or array
>>>>> if you would like, but this is very hacky.
>>>>
>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>> its normal u32. What do you think of that?
>>>>
>>>
>>> Why would it need multiple IDs for one device? Just pass in the one
>>> correct device ID that it is. No reason to make this so complicated.
>>
>> Take a look at bq27xxx_i2c_id_table[] here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
>>
>> Notice how this table maps certain chips to the same IDs. I am going
>> to preserve that scheme, as changing it is outside the scope of this
>> series.
>>
>
> You can't just say doing something the right is "outside the scope of
> this series" then hack something together...
>
> Fixing this should be trivial anyway, add the new IDs, pass the real IDs
> in, then add the translation table so you don't have to add any new
> entries to bq27xxx_regs.

That is *precisely* what I do in v13. Except I don't store real-ID in
a di field because there isn't a use for it. Add that later if you
need it.

bq27xxx_chips[] is the translation table. However we could unify that
with the I2C table by placing two values in its driver_data field, and
I think that's cleaner.


>> So the only question is how to deliver the "real" ID to the driver. In
>> v13, I change the I2C table to deliver real-ID, and replicate the
>> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
>> make the I2C table carry two values, chip-ID and real-ID. Then we can
>> drop bq27xxx_chips[].
>>
>> Hope that clarifies the issue...
>>
>>> From the one ID you can then use tables to lookup any other info about
>>> that device, you don't have to have ever ID populated in every table,
>>> you can even have a table of tables if you would like:
>>>
>>> static const struct chip_lookup lookup_table = {
>>>         [bq27425] = {
>>>                 .regs = &bq27xxx_regs[bq27425],
>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>         },
>>>         [bq27343] = {
>>> ...
>>>
>>> Not sure if that is valid C but I think you can get the idea.
>>>
>>>>
>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>> want that more than you want this "clever" work-around, right?
>>>>>
>>>>> Also fix all the checkpatch --strict warnings.
>>>>>
>>>>>> +
>>>>>> +     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 11e1168..543c10e 100644
>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>> @@ -2,6 +2,8 @@
>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>
>>>>>>  enum bq27xxx_chip {
>>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>       BQ27545, /* bq27545 */
>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>> +     BQ27MAX,
>>>>>> +
>>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>> +     BQ27531,
>>>>>> +     BQ27542,
>>>>>> +     BQ27546,
>>>>>> +     BQ27742,
>>>>>> +     BQ27425,
>>>>>> +     BQ27441,
>>>>>> +     BQ27621,
>>>>>>  };
>>>>>>
>>>>>>  /**
>>>>>>

Andrew Davis May 5, 2017, 9:04 p.m. UTC | #8

On 05/05/2017 03:55 PM, Liam Breck wrote:
> On Fri, May 5, 2017 at 1:44 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/05/2017 03:14 PM, Liam Breck wrote:
>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>
>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>
>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>
>>>>>>>  /* Register mappings */
>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>       [BQ27000] = {
>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>  static struct {
>>>>>>>       enum power_supply_property *props;
>>>>>>>       size_t size;
>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>  };
>>>>>>>
>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>> +};
>>>>>>> +
>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>
>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>> +};
>>>>>>> +
>>>>>>>
>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>               .drv_data = di,
>>>>>>>       };
>>>>>>>
>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>> +
>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>
>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>> it should have the same one correct ID, use a different index or array
>>>>>> if you would like, but this is very hacky.
>>>>>
>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>> its normal u32. What do you think of that?
>>>>>
>>>>
>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>> correct device ID that it is. No reason to make this so complicated.
>>>
>>> Take a look at bq27xxx_i2c_id_table[] here:
>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
>>>
>>> Notice how this table maps certain chips to the same IDs. I am going
>>> to preserve that scheme, as changing it is outside the scope of this
>>> series.
>>>
>>
>> You can't just say doing something the right is "outside the scope of
>> this series" then hack something together...
>>
>> Fixing this should be trivial anyway, add the new IDs, pass the real IDs
>> in, then add the translation table so you don't have to add any new
>> entries to bq27xxx_regs.
> 
> That is *precisely* what I do in v13. Except I don't store real-ID in
> a di field because there isn't a use for it. Add that later if you
> need it.
> 

The first two yes, but it's the type of translation that doesn't look
good, we should go from lookup_table[realID]->whatever_values. With this
we are mapping realID -> fakeID then later whatever_values_table[fakeID].

So if you barrow the "struct chip_lookup lookup_table" below then we end
up with only two mapping types (table_lookup and values tables) and no
translations needed.

> bq27xxx_chips[] is the translation table. However we could unify that
> with the I2C table by placing two values in its driver_data field, and
> I think that's cleaner.
> 
> 
>>> So the only question is how to deliver the "real" ID to the driver. In
>>> v13, I change the I2C table to deliver real-ID, and replicate the
>>> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
>>> make the I2C table carry two values, chip-ID and real-ID. Then we can
>>> drop bq27xxx_chips[].
>>>
>>> Hope that clarifies the issue...
>>>
>>>> From the one ID you can then use tables to lookup any other info about
>>>> that device, you don't have to have ever ID populated in every table,
>>>> you can even have a table of tables if you would like:
>>>>
>>>> static const struct chip_lookup lookup_table = {
>>>>         [bq27425] = {
>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>         },
>>>>         [bq27343] = {
>>>> ...
>>>>
>>>> Not sure if that is valid C but I think you can get the idea.
>>>>
>>>>>
>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>
>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>
>>>>>>> +
>>>>>>> +     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 11e1168..543c10e 100644
>>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>>
>>>>>>>  enum bq27xxx_chip {
>>>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>>       BQ27545, /* bq27545 */
>>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>>> +     BQ27MAX,
>>>>>>> +
>>>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>>> +     BQ27531,
>>>>>>> +     BQ27542,
>>>>>>> +     BQ27546,
>>>>>>> +     BQ27742,
>>>>>>> +     BQ27425,
>>>>>>> +     BQ27441,
>>>>>>> +     BQ27621,
>>>>>>>  };
>>>>>>>
>>>>>>>  /**
>>>>>>>

Liam Breck May 5, 2017, 9:27 p.m. UTC | #9

On Fri, May 5, 2017 at 2:04 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/05/2017 03:55 PM, Liam Breck wrote:
>> On Fri, May 5, 2017 at 1:44 PM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/05/2017 03:14 PM, Liam Breck wrote:
>>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>
>>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>>
>>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>>
>>>>>>>>  /* Register mappings */
>>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>>       [BQ27000] = {
>>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>>  static struct {
>>>>>>>>       enum power_supply_property *props;
>>>>>>>>       size_t size;
>>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>>
>>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>
>>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>>               .drv_data = di,
>>>>>>>>       };
>>>>>>>>
>>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>>> +
>>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>>
>>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>>> it should have the same one correct ID, use a different index or array
>>>>>>> if you would like, but this is very hacky.
>>>>>>
>>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>>> its normal u32. What do you think of that?
>>>>>>
>>>>>
>>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>>> correct device ID that it is. No reason to make this so complicated.
>>>>
>>>> Take a look at bq27xxx_i2c_id_table[] here:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
>>>>
>>>> Notice how this table maps certain chips to the same IDs. I am going
>>>> to preserve that scheme, as changing it is outside the scope of this
>>>> series.
>>>>
>>>
>>> You can't just say doing something the right is "outside the scope of
>>> this series" then hack something together...
>>>
>>> Fixing this should be trivial anyway, add the new IDs, pass the real IDs
>>> in, then add the translation table so you don't have to add any new
>>> entries to bq27xxx_regs.
>>
>> That is *precisely* what I do in v13. Except I don't store real-ID in
>> a di field because there isn't a use for it. Add that later if you
>> need it.
>>
>
> The first two yes, but it's the type of translation that doesn't look
> good, we should go from lookup_table[realID]->whatever_values. With this
> we are mapping realID -> fakeID then later whatever_values_table[fakeID].
>
> So if you barrow the "struct chip_lookup lookup_table" below then we end
> up with only two mapping types (table_lookup and values tables) and no
> translations needed.

If we deliver two values in I2C driver_data, the lookup code will be
this, and we don't need an extra table:

       di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
       di->dm_regs = bq27xxx_dm_regs[di->real_chip];

       di->regs = bq27xxx_regs[di->chip];


>> bq27xxx_chips[] is the translation table. However we could unify that
>> with the I2C table by placing two values in its driver_data field, and
>> I think that's cleaner.
>>
>>
>>>> So the only question is how to deliver the "real" ID to the driver. In
>>>> v13, I change the I2C table to deliver real-ID, and replicate the
>>>> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
>>>> make the I2C table carry two values, chip-ID and real-ID. Then we can
>>>> drop bq27xxx_chips[].
>>>>
>>>> Hope that clarifies the issue...
>>>>
>>>>> From the one ID you can then use tables to lookup any other info about
>>>>> that device, you don't have to have ever ID populated in every table,
>>>>> you can even have a table of tables if you would like:
>>>>>
>>>>> static const struct chip_lookup lookup_table = {
>>>>>         [bq27425] = {
>>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>>         },
>>>>>         [bq27343] = {
>>>>> ...
>>>>>
>>>>> Not sure if that is valid C but I think you can get the idea.
>>>>>
>>>>>>
>>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>>
>>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>>
>>>>>>>> +
>>>>>>>> +     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 11e1168..543c10e 100644
>>>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>>>
>>>>>>>>  enum bq27xxx_chip {
>>>>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>>>       BQ27545, /* bq27545 */
>>>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>>>> +     BQ27MAX,
>>>>>>>> +
>>>>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>>>> +     BQ27531,
>>>>>>>> +     BQ27542,
>>>>>>>> +     BQ27546,
>>>>>>>> +     BQ27742,
>>>>>>>> +     BQ27425,
>>>>>>>> +     BQ27441,
>>>>>>>> +     BQ27621,
>>>>>>>>  };
>>>>>>>>
>>>>>>>>  /**
>>>>>>>>

Andrew Davis May 5, 2017, 9:29 p.m. UTC | #10

On 05/05/2017 04:27 PM, Liam Breck wrote:
> On Fri, May 5, 2017 at 2:04 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/05/2017 03:55 PM, Liam Breck wrote:
>>> On Fri, May 5, 2017 at 1:44 PM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/05/2017 03:14 PM, Liam Breck wrote:
>>>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>>
>>>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>>>
>>>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>>>
>>>>>>>>>  /* Register mappings */
>>>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>>>       [BQ27000] = {
>>>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>>>  static struct {
>>>>>>>>>       enum power_supply_property *props;
>>>>>>>>>       size_t size;
>>>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>>>
>>>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>>>               .drv_data = di,
>>>>>>>>>       };
>>>>>>>>>
>>>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>>>> +
>>>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>>>
>>>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>>>> it should have the same one correct ID, use a different index or array
>>>>>>>> if you would like, but this is very hacky.
>>>>>>>
>>>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>>>> its normal u32. What do you think of that?
>>>>>>>
>>>>>>
>>>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>>>> correct device ID that it is. No reason to make this so complicated.
>>>>>
>>>>> Take a look at bq27xxx_i2c_id_table[] here:
>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
>>>>>
>>>>> Notice how this table maps certain chips to the same IDs. I am going
>>>>> to preserve that scheme, as changing it is outside the scope of this
>>>>> series.
>>>>>
>>>>
>>>> You can't just say doing something the right is "outside the scope of
>>>> this series" then hack something together...
>>>>
>>>> Fixing this should be trivial anyway, add the new IDs, pass the real IDs
>>>> in, then add the translation table so you don't have to add any new
>>>> entries to bq27xxx_regs.
>>>
>>> That is *precisely* what I do in v13. Except I don't store real-ID in
>>> a di field because there isn't a use for it. Add that later if you
>>> need it.
>>>
>>
>> The first two yes, but it's the type of translation that doesn't look
>> good, we should go from lookup_table[realID]->whatever_values. With this
>> we are mapping realID -> fakeID then later whatever_values_table[fakeID].
>>
>> So if you barrow the "struct chip_lookup lookup_table" below then we end
>> up with only two mapping types (table_lookup and values tables) and no
>> translations needed.
> 
> If we deliver two values in I2C driver_data, the lookup code will be
> this, and we don't need an extra table:
> 

Sure, but why should the I2C driver have to know anything about how the
core works or what tables it does and does not have index IDs for? It
should only pass in the real ID.

>        di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>        di->dm_regs = bq27xxx_dm_regs[di->real_chip];
> 
>        di->regs = bq27xxx_regs[di->chip];
> 
> 
>>> bq27xxx_chips[] is the translation table. However we could unify that
>>> with the I2C table by placing two values in its driver_data field, and
>>> I think that's cleaner.
>>>
>>>
>>>>> So the only question is how to deliver the "real" ID to the driver. In
>>>>> v13, I change the I2C table to deliver real-ID, and replicate the
>>>>> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
>>>>> make the I2C table carry two values, chip-ID and real-ID. Then we can
>>>>> drop bq27xxx_chips[].
>>>>>
>>>>> Hope that clarifies the issue...
>>>>>
>>>>>> From the one ID you can then use tables to lookup any other info about
>>>>>> that device, you don't have to have ever ID populated in every table,
>>>>>> you can even have a table of tables if you would like:
>>>>>>
>>>>>> static const struct chip_lookup lookup_table = {
>>>>>>         [bq27425] = {
>>>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>>>         },
>>>>>>         [bq27343] = {
>>>>>> ...
>>>>>>
>>>>>> Not sure if that is valid C but I think you can get the idea.
>>>>>>
>>>>>>>
>>>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>>>
>>>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +     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 11e1168..543c10e 100644
>>>>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>>>>
>>>>>>>>>  enum bq27xxx_chip {
>>>>>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>>>>       BQ27545, /* bq27545 */
>>>>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>>>>> +     BQ27MAX,
>>>>>>>>> +
>>>>>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>>>>> +     BQ27531,
>>>>>>>>> +     BQ27542,
>>>>>>>>> +     BQ27546,
>>>>>>>>> +     BQ27742,
>>>>>>>>> +     BQ27425,
>>>>>>>>> +     BQ27441,
>>>>>>>>> +     BQ27621,
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>>  /**
>>>>>>>>>

Liam Breck May 5, 2017, 9:38 p.m. UTC | #11

On Fri, May 5, 2017 at 2:29 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/05/2017 04:27 PM, Liam Breck wrote:
>> On Fri, May 5, 2017 at 2:04 PM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/05/2017 03:55 PM, Liam Breck wrote:
>>>> On Fri, May 5, 2017 at 1:44 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/05/2017 03:14 PM, Liam Breck wrote:
>>>>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>>>
>>>>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>>>>
>>>>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>>>>
>>>>>>>>>>  /* Register mappings */
>>>>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>>>>       [BQ27000] = {
>>>>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>>>>  static struct {
>>>>>>>>>>       enum power_supply_property *props;
>>>>>>>>>>       size_t size;
>>>>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>>>>  };
>>>>>>>>>>
>>>>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>>>>
>>>>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>>>>> +};
>>>>>>>>>> +
>>>>>>>>>>
>>>>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>>>>               .drv_data = di,
>>>>>>>>>>       };
>>>>>>>>>>
>>>>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>>>>> +
>>>>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>>>>
>>>>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>>>>> it should have the same one correct ID, use a different index or array
>>>>>>>>> if you would like, but this is very hacky.
>>>>>>>>
>>>>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>>>>> its normal u32. What do you think of that?
>>>>>>>>
>>>>>>>
>>>>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>>>>> correct device ID that it is. No reason to make this so complicated.
>>>>>>
>>>>>> Take a look at bq27xxx_i2c_id_table[] here:
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git/tree/drivers/power/supply/bq27xxx_battery_i2c.c?h=linux-4.11.y#n148
>>>>>>
>>>>>> Notice how this table maps certain chips to the same IDs. I am going
>>>>>> to preserve that scheme, as changing it is outside the scope of this
>>>>>> series.
>>>>>>
>>>>>
>>>>> You can't just say doing something the right is "outside the scope of
>>>>> this series" then hack something together...
>>>>>
>>>>> Fixing this should be trivial anyway, add the new IDs, pass the real IDs
>>>>> in, then add the translation table so you don't have to add any new
>>>>> entries to bq27xxx_regs.
>>>>
>>>> That is *precisely* what I do in v13. Except I don't store real-ID in
>>>> a di field because there isn't a use for it. Add that later if you
>>>> need it.
>>>>
>>>
>>> The first two yes, but it's the type of translation that doesn't look
>>> good, we should go from lookup_table[realID]->whatever_values. With this
>>> we are mapping realID -> fakeID then later whatever_values_table[fakeID].
>>>
>>> So if you barrow the "struct chip_lookup lookup_table" below then we end
>>> up with only two mapping types (table_lookup and values tables) and no
>>> translations needed.
>>
>> If we deliver two values in I2C driver_data, the lookup code will be
>> this, and we don't need an extra table:
>>
>
> Sure, but why should the I2C driver have to know anything about how the
> core works or what tables it does and does not have index IDs for? It
> should only pass in the real ID.

It's already mapping certain IDs to others. We need to leave that as
is. Remember, IDs are not just table indexes, they also select
behavior, so di->chip has to keep its current value.

It's easy to extend it to deliver a real-ID along with the orig value.
That will simplify the patch.




>>        di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>>        di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>>
>>        di->regs = bq27xxx_regs[di->chip];
>>
>>
>>>> bq27xxx_chips[] is the translation table. However we could unify that
>>>> with the I2C table by placing two values in its driver_data field, and
>>>> I think that's cleaner.
>>>>
>>>>
>>>>>> So the only question is how to deliver the "real" ID to the driver. In
>>>>>> v13, I change the I2C table to deliver real-ID, and replicate the
>>>>>> mapping of the orig I2C table in bq27xxx_chips[]. Now I propose to
>>>>>> make the I2C table carry two values, chip-ID and real-ID. Then we can
>>>>>> drop bq27xxx_chips[].
>>>>>>
>>>>>> Hope that clarifies the issue...
>>>>>>
>>>>>>> From the one ID you can then use tables to lookup any other info about
>>>>>>> that device, you don't have to have ever ID populated in every table,
>>>>>>> you can even have a table of tables if you would like:
>>>>>>>
>>>>>>> static const struct chip_lookup lookup_table = {
>>>>>>>         [bq27425] = {
>>>>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>>>>         },
>>>>>>>         [bq27343] = {
>>>>>>> ...
>>>>>>>
>>>>>>> Not sure if that is valid C but I think you can get the idea.
>>>>>>>
>>>>>>>>
>>>>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>>>>
>>>>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +     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 11e1168..543c10e 100644
>>>>>>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>>>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>>>>>>
>>>>>>>>>>  enum bq27xxx_chip {
>>>>>>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>>>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>>>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>>>>>>       BQ27545, /* bq27545 */
>>>>>>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>>>>>>> +     BQ27MAX,
>>>>>>>>>> +
>>>>>>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>>>>>>> +     BQ2752X, /* deprecated alias */
>>>>>>>>>> +     BQ27531,
>>>>>>>>>> +     BQ27542,
>>>>>>>>>> +     BQ27546,
>>>>>>>>>> +     BQ27742,
>>>>>>>>>> +     BQ27425,
>>>>>>>>>> +     BQ27441,
>>>>>>>>>> +     BQ27621,
>>>>>>>>>>  };
>>>>>>>>>>
>>>>>>>>>>  /**
>>>>>>>>>>

Liam Breck May 8, 2017, 6:40 a.m. UTC | #12

On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/05/2017 02:31 PM, Liam Breck wrote:
>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>> From: Liam Breck <[e-mail]>
>>>>
>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>> ---
>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>> index 06f15da..0aecd41 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"
>>>>
>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>
>>>>  /* Register mappings */
>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>       [BQ27000] = {
>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>  static struct {
>>>>       enum power_supply_property *props;
>>>>       size_t size;
>>>> -} bq27xxx_battery_props[] = {
>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>> @@ -798,6 +798,33 @@ static struct {
>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>  };
>>>>
>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>> +     [BQ27000]   = BQ27000,
>>>> +     [BQ27010]   = BQ27010,
>>>> +     [BQ2750X]   = BQ2750X,
>>>> +     [BQ2751X]   = BQ2751X,
>>>> +     [BQ2752X]   = BQ2751X,
>>>> +     [BQ27500]   = BQ27500,
>>>> +     [BQ27510G1] = BQ27510G1,
>>>> +     [BQ27510G2] = BQ27510G2,
>>>> +     [BQ27510G3] = BQ27510G3,
>>>> +     [BQ27520G1] = BQ27520G1,
>>>> +     [BQ27520G2] = BQ27520G2,
>>>> +     [BQ27520G3] = BQ27520G3,
>>>> +     [BQ27520G4] = BQ27520G4,
>>>> +     [BQ27530]   = BQ27530,
>>>> +     [BQ27531]   = BQ27530,
>>>> +     [BQ27541]   = BQ27541,
>>>> +     [BQ27542]   = BQ27541,
>>>> +     [BQ27546]   = BQ27541,
>>>> +     [BQ27742]   = BQ27541,
>>>> +     [BQ27545]   = BQ27545,
>>>> +     [BQ27421]   = BQ27421,
>>>> +     [BQ27425]   = BQ27421,
>>>> +     [BQ27441]   = BQ27421,
>>>> +     [BQ27621]   = BQ27421,
>>>> +};
>>>> +
>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>
>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>> +};
>>>> +
>>>>
>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>               .drv_data = di,
>>>>       };
>>>>
>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>> +
>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>
>>> NACK, this is a mess, you should not be using a table to change what
>>> chip was passed in, it may be needed later. The chip is still the same,
>>> it should have the same one correct ID, use a different index or array
>>> if you would like, but this is very hacky.
>>
>> The only way I see to make the I2C subsystem deliver multiple IDs for
>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>> its normal u32. What do you think of that?
>>
>
> Why would it need multiple IDs for one device? Just pass in the one
> correct device ID that it is. No reason to make this so complicated.
> From the one ID you can then use tables to lookup any other info about
> that device, you don't have to have ever ID populated in every table,
> you can even have a table of tables if you would like:
>
> static const struct chip_lookup lookup_table = {
>         [bq27425] = {
>                 .regs = &bq27xxx_regs[bq27425],
>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>         },
>         [bq27343] = {
> ...
>
> Not sure if that is valid C but I think you can get the idea.

I considered a master lookup_table, but its ref to
bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
The latter is still necessary as di->chip is used to select
conditional behaviors; we can't change its value in this patch.

So instead lets amend the I2C table to carry both its original value
(fake-ID) and the real-ID. That looks like:

static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
    /* dest.    di->real_chip       di->chip      */
    { "bq27200",   (BQ27000   << 16) |  BQ27000   },
    { "bq27210",   (BQ27010   << 16) |  BQ27010   },
    ...
    { "bq27441",   (BQ27441   << 16) |  BQ27421   },
    { "bq27621",   (BQ27621   << 16) |  BQ27421   },
    {},
};

In probe, retrieve two values:

    di->real_chip = id->driver_data >> 16;
    di->chip = (u16) id->driver_data;

In setup, use two values:

    di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
    di->dm_regs = bq27xxx_dm_regs[di->real_chip];
    di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src

That yields the most compact implementation of this patch, and you get
di->real_chip in the bargain :-)


>>> Just stop trying to hack around it, add the extra tables, even if they
>>> are clones, so we can be done with this series already. I'm sure you
>>> want that more than you want this "clever" work-around, right?
>>>
>>> Also fix all the checkpatch --strict warnings.
>>>
>>>> +
>>>> +     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 11e1168..543c10e 100644
>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>> @@ -2,6 +2,8 @@
>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>
>>>>  enum bq27xxx_chip {
>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>       BQ27010, /* bq27010, bq27210 */
>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>       BQ27545, /* bq27545 */
>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>> +     BQ27MAX,
>>>> +
>>>> +     /* these map to above in bq27xxx_chips[] */
>>>> +     BQ2752X, /* deprecated alias */
>>>> +     BQ27531,
>>>> +     BQ27542,
>>>> +     BQ27546,
>>>> +     BQ27742,
>>>> +     BQ27425,
>>>> +     BQ27441,
>>>> +     BQ27621,
>>>>  };
>>>>
>>>>  /**
>>>>

Andrew Davis May 8, 2017, 3 p.m. UTC | #13

On 05/08/2017 01:40 AM, Liam Breck wrote:
> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>> From: Liam Breck <[e-mail]>
>>>>>
>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>> ---
>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>> index 06f15da..0aecd41 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"
>>>>>
>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>
>>>>>  /* Register mappings */
>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>       [BQ27000] = {
>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>  static struct {
>>>>>       enum power_supply_property *props;
>>>>>       size_t size;
>>>>> -} bq27xxx_battery_props[] = {
>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>  };
>>>>>
>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>> +     [BQ27000]   = BQ27000,
>>>>> +     [BQ27010]   = BQ27010,
>>>>> +     [BQ2750X]   = BQ2750X,
>>>>> +     [BQ2751X]   = BQ2751X,
>>>>> +     [BQ2752X]   = BQ2751X,
>>>>> +     [BQ27500]   = BQ27500,
>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>> +     [BQ27530]   = BQ27530,
>>>>> +     [BQ27531]   = BQ27530,
>>>>> +     [BQ27541]   = BQ27541,
>>>>> +     [BQ27542]   = BQ27541,
>>>>> +     [BQ27546]   = BQ27541,
>>>>> +     [BQ27742]   = BQ27541,
>>>>> +     [BQ27545]   = BQ27545,
>>>>> +     [BQ27421]   = BQ27421,
>>>>> +     [BQ27425]   = BQ27421,
>>>>> +     [BQ27441]   = BQ27421,
>>>>> +     [BQ27621]   = BQ27421,
>>>>> +};
>>>>> +
>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>
>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>> +};
>>>>> +
>>>>>
>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>               .drv_data = di,
>>>>>       };
>>>>>
>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>> +
>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>
>>>> NACK, this is a mess, you should not be using a table to change what
>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>> it should have the same one correct ID, use a different index or array
>>>> if you would like, but this is very hacky.
>>>
>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>> its normal u32. What do you think of that?
>>>
>>
>> Why would it need multiple IDs for one device? Just pass in the one
>> correct device ID that it is. No reason to make this so complicated.
>> From the one ID you can then use tables to lookup any other info about
>> that device, you don't have to have ever ID populated in every table,
>> you can even have a table of tables if you would like:
>>
>> static const struct chip_lookup lookup_table = {
>>         [bq27425] = {
>>                 .regs = &bq27xxx_regs[bq27425],
>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>         },
>>         [bq27343] = {
>> ...
>>
>> Not sure if that is valid C but I think you can get the idea.
> 
> I considered a master lookup_table, but its ref to
> bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
> The latter is still necessary as di->chip is used to select
> conditional behaviors; we can't change its value in this patch.
> 

Sure we can, in fact you could add the conditional behavior flags to the
master lookup_table and cleanup all the current switch-case checks we do
now.

> So instead lets amend the I2C table to carry both its original value
> (fake-ID) and the real-ID. That looks like:
> 
> static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>     /* dest.    di->real_chip       di->chip      */
>     { "bq27200",   (BQ27000   << 16) |  BQ27000   },
>     { "bq27210",   (BQ27010   << 16) |  BQ27010   },
>     ...
>     { "bq27441",   (BQ27441   << 16) |  BQ27421   },
>     { "bq27621",   (BQ27621   << 16) |  BQ27421   },
>     {},
> };
> 
> In probe, retrieve two values:
> 
>     di->real_chip = id->driver_data >> 16;
>     di->chip = (u16) id->driver_data;
> 

NAK, I'm sure you can see how hacky this is right? This doesn't give us
anything that the old table didn't. And it fails to correct the issue
with the tables: We have two chip IDs still!

Try the lookup_table again, send be what you have and I can help get
everything else working if you have any problems with the conditional
behavior cleanups.

> In setup, use two values:
> 
>     di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>     di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>     di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src
> 
> That yields the most compact implementation of this patch, and you get
> di->real_chip in the bargain :-)
> 
> 
>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>> are clones, so we can be done with this series already. I'm sure you
>>>> want that more than you want this "clever" work-around, right?
>>>>
>>>> Also fix all the checkpatch --strict warnings.
>>>>
>>>>> +
>>>>> +     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 11e1168..543c10e 100644
>>>>> --- a/include/linux/power/bq27xxx_battery.h
>>>>> +++ b/include/linux/power/bq27xxx_battery.h
>>>>> @@ -2,6 +2,8 @@
>>>>>  #define __LINUX_BQ27X00_BATTERY_H__
>>>>>
>>>>>  enum bq27xxx_chip {
>>>>> +     /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
>>>>> +     /* and map to themselves in bq27xxx_chips[]             */
>>>>>       BQ27000 = 1, /* bq27000, bq27200 */
>>>>>       BQ27010, /* bq27010, bq27210 */
>>>>>       BQ2750X, /* bq27500 deprecated alias */
>>>>> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>>>>>       BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>>>>>       BQ27545, /* bq27545 */
>>>>>       BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
>>>>> +     BQ27MAX,
>>>>> +
>>>>> +     /* these map to above in bq27xxx_chips[] */
>>>>> +     BQ2752X, /* deprecated alias */
>>>>> +     BQ27531,
>>>>> +     BQ27542,
>>>>> +     BQ27546,
>>>>> +     BQ27742,
>>>>> +     BQ27425,
>>>>> +     BQ27441,
>>>>> +     BQ27621,
>>>>>  };
>>>>>
>>>>>  /**
>>>>>

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

On Mon, May 8, 2017 at 8:00 AM, Andrew F. Davis <[e-mail]> wrote:
> On 05/08/2017 01:40 AM, Liam Breck wrote:
>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>> From: Liam Breck <[e-mail]>
>>>>>>
>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>> ---
>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>> index 06f15da..0aecd41 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"
>>>>>>
>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>
>>>>>>  /* Register mappings */
>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>       [BQ27000] = {
>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>  static struct {
>>>>>>       enum power_supply_property *props;
>>>>>>       size_t size;
>>>>>> -} bq27xxx_battery_props[] = {
>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>  };
>>>>>>
>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>> +     [BQ27000]   = BQ27000,
>>>>>> +     [BQ27010]   = BQ27010,
>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>> +     [BQ27500]   = BQ27500,
>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>> +     [BQ27530]   = BQ27530,
>>>>>> +     [BQ27531]   = BQ27530,
>>>>>> +     [BQ27541]   = BQ27541,
>>>>>> +     [BQ27542]   = BQ27541,
>>>>>> +     [BQ27546]   = BQ27541,
>>>>>> +     [BQ27742]   = BQ27541,
>>>>>> +     [BQ27545]   = BQ27545,
>>>>>> +     [BQ27421]   = BQ27421,
>>>>>> +     [BQ27425]   = BQ27421,
>>>>>> +     [BQ27441]   = BQ27421,
>>>>>> +     [BQ27621]   = BQ27421,
>>>>>> +};
>>>>>> +
>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>
>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>> +};
>>>>>> +
>>>>>>
>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>               .drv_data = di,
>>>>>>       };
>>>>>>
>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>> +
>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>
>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>> it should have the same one correct ID, use a different index or array
>>>>> if you would like, but this is very hacky.
>>>>
>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>> its normal u32. What do you think of that?
>>>>
>>>
>>> Why would it need multiple IDs for one device? Just pass in the one
>>> correct device ID that it is. No reason to make this so complicated.
>>> From the one ID you can then use tables to lookup any other info about
>>> that device, you don't have to have ever ID populated in every table,
>>> you can even have a table of tables if you would like:
>>>
>>> static const struct chip_lookup lookup_table = {
>>>         [bq27425] = {
>>>                 .regs = &bq27xxx_regs[bq27425],
>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>         },
>>>         [bq27343] = {
>>> ...
>>>
>>> Not sure if that is valid C but I think you can get the idea.
>>
>> I considered a master lookup_table, but its ref to
>> bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
>> The latter is still necessary as di->chip is used to select
>> conditional behaviors; we can't change its value in this patch.
>>
>
> Sure we can, in fact you could add the conditional behavior flags to the
> master lookup_table and cleanup all the current switch-case checks we do
> now.

You are proposing a significant change to the ID handling, which is
fine, but I don't wish to code that. Accept this minimal patch to
enable DM update, and then alter the ID logic when you have time in a
future patch.


>> So instead lets amend the I2C table to carry both its original value
>> (fake-ID) and the real-ID. That looks like:
>>
>> static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>     /* dest.    di->real_chip       di->chip      */
>>     { "bq27200",   (BQ27000   << 16) |  BQ27000   },
>>     { "bq27210",   (BQ27010   << 16) |  BQ27010   },
>>     ...
>>     { "bq27441",   (BQ27441   << 16) |  BQ27421   },
>>     { "bq27621",   (BQ27621   << 16) |  BQ27421   },
>>     {},
>> };
>>
>> In probe, retrieve two values:
>>
>>     di->real_chip = id->driver_data >> 16;
>>     di->chip = (u16) id->driver_data;
>>
>
> NAK, I'm sure you can see how hacky this is right? This doesn't give us
> anything that the old table didn't. And it fails to correct the issue
> with the tables: We have two chip IDs still!

I'm sorry you don't like the extra ID, but you're the party
responsible for (if not the author of) the fake-ID scheme :-)


> Try the lookup_table again, send be what you have and I can help get
> everything else working if you have any problems with the conditional
> behavior cleanups.
>
>> In setup, use two values:
>>
>>     di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>>     di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>>     di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src
>>
>> That yields the most compact implementation of this patch, and you get
>> di->real_chip in the bargain :-)
>>
>>
>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>> want that more than you want this "clever" work-around, right?
>>>>>
>>>>> Also fix all the checkpatch --strict warnings.
>>>>>

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

On 05/08/2017 02:02 PM, Liam Breck wrote:
> On Mon, May 8, 2017 at 8:00 AM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/08/2017 01:40 AM, Liam Breck wrote:
>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>
>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>> ---
>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>
>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>
>>>>>>>  /* Register mappings */
>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>       [BQ27000] = {
>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>  static struct {
>>>>>>>       enum power_supply_property *props;
>>>>>>>       size_t size;
>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>  };
>>>>>>>
>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>> +};
>>>>>>> +
>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>
>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>> +};
>>>>>>> +
>>>>>>>
>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>               .drv_data = di,
>>>>>>>       };
>>>>>>>
>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>> +
>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>
>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>> it should have the same one correct ID, use a different index or array
>>>>>> if you would like, but this is very hacky.
>>>>>
>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>> its normal u32. What do you think of that?
>>>>>
>>>>
>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>> correct device ID that it is. No reason to make this so complicated.
>>>> From the one ID you can then use tables to lookup any other info about
>>>> that device, you don't have to have ever ID populated in every table,
>>>> you can even have a table of tables if you would like:
>>>>
>>>> static const struct chip_lookup lookup_table = {
>>>>         [bq27425] = {
>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>         },
>>>>         [bq27343] = {
>>>> ...
>>>>
>>>> Not sure if that is valid C but I think you can get the idea.
>>>
>>> I considered a master lookup_table, but its ref to
>>> bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
>>> The latter is still necessary as di->chip is used to select
>>> conditional behaviors; we can't change its value in this patch.
>>>
>>
>> Sure we can, in fact you could add the conditional behavior flags to the
>> master lookup_table and cleanup all the current switch-case checks we do
>> now.
> 
> You are proposing a significant change to the ID handling, which is
> fine, but I don't wish to code that. Accept this minimal patch to
> enable DM update, and then alter the ID logic when you have time in a
> future patch.
> 

Doesn't work like that, you can't just hack something together and hope
someone else will fix it later. If you want to change something you
should make it right the first time. We don't need DM update, but if you
would like it then you need to fix the problems it will create.

> 
>>> So instead lets amend the I2C table to carry both its original value
>>> (fake-ID) and the real-ID. That looks like:
>>>
>>> static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>     /* dest.    di->real_chip       di->chip      */
>>>     { "bq27200",   (BQ27000   << 16) |  BQ27000   },
>>>     { "bq27210",   (BQ27010   << 16) |  BQ27010   },
>>>     ...
>>>     { "bq27441",   (BQ27441   << 16) |  BQ27421   },
>>>     { "bq27621",   (BQ27621   << 16) |  BQ27421   },
>>>     {},
>>> };
>>>
>>> In probe, retrieve two values:
>>>
>>>     di->real_chip = id->driver_data >> 16;
>>>     di->chip = (u16) id->driver_data;
>>>
>>
>> NAK, I'm sure you can see how hacky this is right? This doesn't give us
>> anything that the old table didn't. And it fails to correct the issue
>> with the tables: We have two chip IDs still!
> 
> I'm sorry you don't like the extra ID, but you're the party
> responsible for (if not the author of) the fake-ID scheme :-)
> 

It has worked fine until now, your additions require a different scheme,
hacking around the old one is not a workable solution.

> 
>> Try the lookup_table again, send be what you have and I can help get
>> everything else working if you have any problems with the conditional
>> behavior cleanups.
>>
>>> In setup, use two values:
>>>
>>>     di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>>>     di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>>>     di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src
>>>
>>> That yields the most compact implementation of this patch, and you get
>>> di->real_chip in the bargain :-)
>>>
>>>
>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>
>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>

Liam Breck May 8, 2017, 8:01 p.m. UTC | #16

On Mon, May 8, 2017 at 12:09 PM, Andrew F. Davis <[e-mail]> wrote:
> On 05/08/2017 02:02 PM, Liam Breck wrote:
>> On Mon, May 8, 2017 at 8:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>> On 05/08/2017 01:40 AM, Liam Breck wrote:
>>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>
>>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>>
>>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>>
>>>>>>>>  /* Register mappings */
>>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>>       [BQ27000] = {
>>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>>  static struct {
>>>>>>>>       enum power_supply_property *props;
>>>>>>>>       size_t size;
>>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>>  };
>>>>>>>>
>>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>>
>>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>>> +};
>>>>>>>> +
>>>>>>>>
>>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>>               .drv_data = di,
>>>>>>>>       };
>>>>>>>>
>>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>>> +
>>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>>
>>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>>> it should have the same one correct ID, use a different index or array
>>>>>>> if you would like, but this is very hacky.
>>>>>>
>>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>>> its normal u32. What do you think of that?
>>>>>>
>>>>>
>>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>>> correct device ID that it is. No reason to make this so complicated.
>>>>> From the one ID you can then use tables to lookup any other info about
>>>>> that device, you don't have to have ever ID populated in every table,
>>>>> you can even have a table of tables if you would like:
>>>>>
>>>>> static const struct chip_lookup lookup_table = {
>>>>>         [bq27425] = {
>>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>>         },
>>>>>         [bq27343] = {
>>>>> ...
>>>>>
>>>>> Not sure if that is valid C but I think you can get the idea.
>>>>
>>>> I considered a master lookup_table, but its ref to
>>>> bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
>>>> The latter is still necessary as di->chip is used to select
>>>> conditional behaviors; we can't change its value in this patch.
>>>>
>>>
>>> Sure we can, in fact you could add the conditional behavior flags to the
>>> master lookup_table and cleanup all the current switch-case checks we do
>>> now.
>>
>> You are proposing a significant change to the ID handling, which is
>> fine, but I don't wish to code that. Accept this minimal patch to
>> enable DM update, and then alter the ID logic when you have time in a
>> future patch.
>>
>
> Doesn't work like that, you can't just hack something together and hope
> someone else will fix it later. If you want to change something you
> should make it right the first time. We don't need DM update, but if you
> would like it then you need to fix the problems it will create.

You do indeed need DM update for the RAM-only chips; they expect to be
configured on boot, as I documented previously.

Adding real-ID to the I2C table and using it in *exactly two lines* of
setup() is a minimal workaround for a pre-existing design flaw. You
have a concept for fixing it, so you should code it. I gracefully
decline.

I have labored on this patchset for some 19 revisions. Sebastian seems
to think that's enough, as he queued most of the series last week even
tho you hadn't acked it.



>>>> So instead lets amend the I2C table to carry both its original value
>>>> (fake-ID) and the real-ID. That looks like:
>>>>
>>>> static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>     /* dest.    di->real_chip       di->chip      */
>>>>     { "bq27200",   (BQ27000   << 16) |  BQ27000   },
>>>>     { "bq27210",   (BQ27010   << 16) |  BQ27010   },
>>>>     ...
>>>>     { "bq27441",   (BQ27441   << 16) |  BQ27421   },
>>>>     { "bq27621",   (BQ27621   << 16) |  BQ27421   },
>>>>     {},
>>>> };
>>>>
>>>> In probe, retrieve two values:
>>>>
>>>>     di->real_chip = id->driver_data >> 16;
>>>>     di->chip = (u16) id->driver_data;
>>>>
>>>
>>> NAK, I'm sure you can see how hacky this is right? This doesn't give us
>>> anything that the old table didn't. And it fails to correct the issue
>>> with the tables: We have two chip IDs still!
>>
>> I'm sorry you don't like the extra ID, but you're the party
>> responsible for (if not the author of) the fake-ID scheme :-)
>>
>
> It has worked fine until now, your additions require a different scheme,
> hacking around the old one is not a workable solution.
>
>>
>>> Try the lookup_table again, send be what you have and I can help get
>>> everything else working if you have any problems with the conditional
>>> behavior cleanups.
>>>
>>>> In setup, use two values:
>>>>
>>>>     di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>>>>     di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>>>>     di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src
>>>>
>>>> That yields the most compact implementation of this patch, and you get
>>>> di->real_chip in the bargain :-)
>>>>
>>>>
>>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>>
>>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>>

Andrew Davis May 9, 2017, 4:20 p.m. UTC | #17

On 05/08/2017 03:01 PM, Liam Breck wrote:
> On Mon, May 8, 2017 at 12:09 PM, Andrew F. Davis <[e-mail]> wrote:
>> On 05/08/2017 02:02 PM, Liam Breck wrote:
>>> On Mon, May 8, 2017 at 8:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>> On 05/08/2017 01:40 AM, Liam Breck wrote:
>>>>> On Fri, May 5, 2017 at 12:45 PM, Andrew F. Davis <[e-mail]> wrote:
>>>>>> On 05/05/2017 02:31 PM, Liam Breck wrote:
>>>>>>> On Thu, May 4, 2017 at 10:00 AM, Andrew F. Davis <[e-mail]> wrote:
>>>>>>>> On 05/04/2017 01:18 AM, Liam Breck wrote:
>>>>>>>>> From: Liam Breck <[e-mail]>
>>>>>>>>>
>>>>>>>>> Support data memory update on BQ27500, 545, 425, 421, 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>>>>>>>>>  3 files changed, 107 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>>> index 06f15da..0aecd41 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"
>>>>>>>>>
>>>>>>>>> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>>>>>>>>>       [BQ27XXX_DM_CKSUM] = 0x60
>>>>>>>>>
>>>>>>>>>  /* Register mappings */
>>>>>>>>> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
>>>>>>>>> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>>>>>>>>>       [BQ27000] = {
>>>>>>>>>               [BQ27XXX_REG_CTRL] = 0x00,
>>>>>>>>>               [BQ27XXX_REG_TEMP] = 0x06,
>>>>>>>>> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>>>>>>>>>  static struct {
>>>>>>>>>       enum power_supply_property *props;
>>>>>>>>>       size_t size;
>>>>>>>>> -} bq27xxx_battery_props[] = {
>>>>>>>>> +} bq27xxx_battery_props[BQ27MAX] = {
>>>>>>>>>       BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>>>>>>>>>       BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>>>>>>>>>       BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
>>>>>>>>> @@ -798,6 +798,33 @@ static struct {
>>>>>>>>>       BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>>>>>>>>>  };
>>>>>>>>>
>>>>>>>>> +static enum bq27xxx_chip bq27xxx_chips[] = {
>>>>>>>>> +     [BQ27000]   = BQ27000,
>>>>>>>>> +     [BQ27010]   = BQ27010,
>>>>>>>>> +     [BQ2750X]   = BQ2750X,
>>>>>>>>> +     [BQ2751X]   = BQ2751X,
>>>>>>>>> +     [BQ2752X]   = BQ2751X,
>>>>>>>>> +     [BQ27500]   = BQ27500,
>>>>>>>>> +     [BQ27510G1] = BQ27510G1,
>>>>>>>>> +     [BQ27510G2] = BQ27510G2,
>>>>>>>>> +     [BQ27510G3] = BQ27510G3,
>>>>>>>>> +     [BQ27520G1] = BQ27520G1,
>>>>>>>>> +     [BQ27520G2] = BQ27520G2,
>>>>>>>>> +     [BQ27520G3] = BQ27520G3,
>>>>>>>>> +     [BQ27520G4] = BQ27520G4,
>>>>>>>>> +     [BQ27530]   = BQ27530,
>>>>>>>>> +     [BQ27531]   = BQ27530,
>>>>>>>>> +     [BQ27541]   = BQ27541,
>>>>>>>>> +     [BQ27542]   = BQ27541,
>>>>>>>>> +     [BQ27546]   = BQ27541,
>>>>>>>>> +     [BQ27742]   = BQ27541,
>>>>>>>>> +     [BQ27545]   = BQ27545,
>>>>>>>>> +     [BQ27421]   = BQ27421,
>>>>>>>>> +     [BQ27425]   = BQ27421,
>>>>>>>>> +     [BQ27441]   = BQ27421,
>>>>>>>>> +     [BQ27621]   = BQ27421,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>  static DEFINE_MUTEX(bq27xxx_list_lock);
>>>>>>>>>  static LIST_HEAD(bq27xxx_battery_devices);
>>>>>>>>>
>>>>>>>>> @@ -856,6 +883,54 @@ static const char * const 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,
>>>>>>>>> +};
>>>>>>>>> +
>>>>>>>>>
>>>>>>>>>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>>>>>>>>>  static bool bq27xxx_dt_to_nvm = true;
>>>>>>>>> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>>>>>>>>>               .drv_data = di,
>>>>>>>>>       };
>>>>>>>>>
>>>>>>>>> +     di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
>>>>>>>>> +
>>>>>>>>> +     di->unseal_key = bq27xxx_unseal_keys[di->chip];
>>>>>>>>> +     di->dm_regs = bq27xxx_dm_regs[di->chip];
>>>>>>>>> +     di->chip = bq27xxx_chips[di->chip];
>>>>>>>>
>>>>>>>> NACK, this is a mess, you should not be using a table to change what
>>>>>>>> chip was passed in, it may be needed later. The chip is still the same,
>>>>>>>> it should have the same one correct ID, use a different index or array
>>>>>>>> if you would like, but this is very hacky.
>>>>>>>
>>>>>>> The only way I see to make the I2C subsystem deliver multiple IDs for
>>>>>>> a device is to treat i2c_device_id.driver_data as a u16[2] instead of
>>>>>>> its normal u32. What do you think of that?
>>>>>>>
>>>>>>
>>>>>> Why would it need multiple IDs for one device? Just pass in the one
>>>>>> correct device ID that it is. No reason to make this so complicated.
>>>>>> From the one ID you can then use tables to lookup any other info about
>>>>>> that device, you don't have to have ever ID populated in every table,
>>>>>> you can even have a table of tables if you would like:
>>>>>>
>>>>>> static const struct chip_lookup lookup_table = {
>>>>>>         [bq27425] = {
>>>>>>                 .regs = &bq27xxx_regs[bq27425],
>>>>>>                 .dm_regs = &bq27xxx_dm_regs[bq274xx],
>>>>>>                 .unseal_key = INVALID_FOR_THIS_DEVICE,
>>>>>>         },
>>>>>>         [bq27343] = {
>>>>>> ...
>>>>>>
>>>>>> Not sure if that is valid C but I think you can get the idea.
>>>>>
>>>>> I considered a master lookup_table, but its ref to
>>>>> bq27xxx_regs[fake-ID] duplicates the mapping of real-ID to di->chip.
>>>>> The latter is still necessary as di->chip is used to select
>>>>> conditional behaviors; we can't change its value in this patch.
>>>>>
>>>>
>>>> Sure we can, in fact you could add the conditional behavior flags to the
>>>> master lookup_table and cleanup all the current switch-case checks we do
>>>> now.
>>>
>>> You are proposing a significant change to the ID handling, which is
>>> fine, but I don't wish to code that. Accept this minimal patch to
>>> enable DM update, and then alter the ID logic when you have time in a
>>> future patch.
>>>
>>
>> Doesn't work like that, you can't just hack something together and hope
>> someone else will fix it later. If you want to change something you
>> should make it right the first time. We don't need DM update, but if you
>> would like it then you need to fix the problems it will create.
> 
> You do indeed need DM update for the RAM-only chips; they expect to be
> configured on boot, as I documented previously.
> 
> Adding real-ID to the I2C table and using it in *exactly two lines* of
> setup() is a minimal workaround for a pre-existing design flaw. You
> have a concept for fixing it, so you should code it. I gracefully
> decline.
> 

Just because it was done wrong before doesn't mean it should continue to
be done wrong.

> I have labored on this patchset for some 19 revisions. Sebastian seems
> to think that's enough, as he queued most of the series last week even
> tho you hadn't acked it.
> 

I know you said v1 came out in Jan, but I've only been looking at this
for one cycle, also you wouldn't be at v19 if you didn't push 2 series a
week, let each version sit on the list for a while to build up multiple
comments. Patch series version number is not a metric of quality or
"readiness".

+ queuing patches is not approval, it just sets it up for testing in
-next, nothing is final until Linus takes the pull request, and even
then there is always revert patches.

I've offered to work with you on this, if you would stop fighting every
minor suggestion I give we could get all this in this cycle.

I empathize with you, this is a very difficult driver with a lot of
legacy attached and more by-name supported devices than almost any other
driver I've seen, we have to get this right or it will negatively effect
a lot of people down the road.

> 
> 
>>>>> So instead lets amend the I2C table to carry both its original value
>>>>> (fake-ID) and the real-ID. That looks like:
>>>>>
>>>>> static const struct i2c_device_id bq27xxx_i2c_id_table[] = {
>>>>>     /* dest.    di->real_chip       di->chip      */
>>>>>     { "bq27200",   (BQ27000   << 16) |  BQ27000   },
>>>>>     { "bq27210",   (BQ27010   << 16) |  BQ27010   },
>>>>>     ...
>>>>>     { "bq27441",   (BQ27441   << 16) |  BQ27421   },
>>>>>     { "bq27621",   (BQ27621   << 16) |  BQ27421   },
>>>>>     {},
>>>>> };
>>>>>
>>>>> In probe, retrieve two values:
>>>>>
>>>>>     di->real_chip = id->driver_data >> 16;
>>>>>     di->chip = (u16) id->driver_data;
>>>>>
>>>>
>>>> NAK, I'm sure you can see how hacky this is right? This doesn't give us
>>>> anything that the old table didn't. And it fails to correct the issue
>>>> with the tables: We have two chip IDs still!
>>>
>>> I'm sorry you don't like the extra ID, but you're the party
>>> responsible for (if not the author of) the fake-ID scheme :-)
>>>
>>
>> It has worked fine until now, your additions require a different scheme,
>> hacking around the old one is not a workable solution.
>>
>>>
>>>> Try the lookup_table again, send be what you have and I can help get
>>>> everything else working if you have any problems with the conditional
>>>> behavior cleanups.
>>>>
>>>>> In setup, use two values:
>>>>>
>>>>>     di->unseal_key = bq27xxx_unseal_keys[di->real_chip];
>>>>>     di->dm_regs = bq27xxx_dm_regs[di->real_chip];
>>>>>     di->regs = bq27xxx_regs[di->chip]; // unchanged from orig src
>>>>>
>>>>> That yields the most compact implementation of this patch, and you get
>>>>> di->real_chip in the bargain :-)
>>>>>
>>>>>
>>>>>>>> Just stop trying to hack around it, add the extra tables, even if they
>>>>>>>> are clones, so we can be done with this series already. I'm sure you
>>>>>>>> want that more than you want this "clever" work-around, right?
>>>>>>>>
>>>>>>>> Also fix all the checkpatch --strict warnings.
>>>>>>>>

Liam Breck May 10, 2017, 9:16 a.m. UTC | #18

Hi Andrew,

On Wed, May 3, 2017 at 11:18 PM, Liam Breck <[e-mail]> wrote:
> From: Liam Breck <[e-mail]>
>
> Support data memory update on BQ27500, 545, 425, 421, 441, 621.

Re testing, I'll comment out code in this patch for chips I can't
test. We can let future users test chips they use and re-enable/insert
them. No need to make you test everything.

> Create IDs for for previously unID'd chips, to index arrays for unseal keys
> and data memory register tables.
>
> Signed-off-by: Liam Breck <[e-mail]>
> ---
>  drivers/power/supply/bq27xxx_battery.c     | 90 ++++++++++++++++++++++++++++--
>  drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++---
>  include/linux/power/bq27xxx_battery.h      | 13 +++++
>  3 files changed, 107 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 06f15da..0aecd41 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"
>
> @@ -132,7 +132,7 @@ enum bq27xxx_reg_index {
>         [BQ27XXX_DM_CKSUM] = 0x60
>
>  /* Register mappings */
> -static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
> +static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
>         [BQ27000] = {
>                 [BQ27XXX_REG_CTRL] = 0x00,
>                 [BQ27XXX_REG_TEMP] = 0x06,
> @@ -779,7 +779,7 @@ static enum power_supply_property bq27421_battery_props[] = {
>  static struct {
>         enum power_supply_property *props;
>         size_t size;
> -} bq27xxx_battery_props[] = {
> +} bq27xxx_battery_props[BQ27MAX] = {
>         BQ27XXX_PROP(BQ27000, bq27000_battery_props),
>         BQ27XXX_PROP(BQ27010, bq27010_battery_props),
>         BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
> @@ -798,6 +798,33 @@ static struct {
>         BQ27XXX_PROP(BQ27421, bq27421_battery_props),
>  };
>
> +static enum bq27xxx_chip bq27xxx_chips[] = {
> +       [BQ27000]   = BQ27000,
> +       [BQ27010]   = BQ27010,
> +       [BQ2750X]   = BQ2750X,
> +       [BQ2751X]   = BQ2751X,
> +       [BQ2752X]   = BQ2751X,
> +       [BQ27500]   = BQ27500,
> +       [BQ27510G1] = BQ27510G1,
> +       [BQ27510G2] = BQ27510G2,
> +       [BQ27510G3] = BQ27510G3,
> +       [BQ27520G1] = BQ27520G1,
> +       [BQ27520G2] = BQ27520G2,
> +       [BQ27520G3] = BQ27520G3,
> +       [BQ27520G4] = BQ27520G4,
> +       [BQ27530]   = BQ27530,
> +       [BQ27531]   = BQ27530,
> +       [BQ27541]   = BQ27541,
> +       [BQ27542]   = BQ27541,
> +       [BQ27546]   = BQ27541,
> +       [BQ27742]   = BQ27541,
> +       [BQ27545]   = BQ27545,
> +       [BQ27421]   = BQ27421,
> +       [BQ27425]   = BQ27421,
> +       [BQ27441]   = BQ27421,
> +       [BQ27621]   = BQ27421,
> +};
> +
>  static DEFINE_MUTEX(bq27xxx_list_lock);
>  static LIST_HEAD(bq27xxx_battery_devices);
>
> @@ -856,6 +883,54 @@ static const char * const bq27xxx_dm_reg_name[] = {
>         [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage",
>  };

Specifically...

#ifdef DEBUG here

> +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 },
> +};

#endif here, moving one table outside the ifdef.

> +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,
> +};
> +

And the same inside the above table.

> +static u32 bq27xxx_unseal_keys[] = {
> +       [BQ27500] = 0x04143672,
> +       [BQ27545] = 0x04143672,
> +       [BQ27421] = 0x80008000,
> +       [BQ27425] = 0x04143672,
> +       [BQ27441] = 0x80008000,
> +       [BQ27621] = 0x80008000,
> +};
> +
>
>  #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
>  static bool bq27xxx_dt_to_nvm = true;
> @@ -1865,9 +1940,16 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
>                 .drv_data = di,
>         };
>
> +       di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
> +
> +       di->unseal_key = bq27xxx_unseal_keys[di->chip];
> +       di->dm_regs = bq27xxx_dm_regs[di->chip];
> +       di->chip = bq27xxx_chips[di->chip];
> +
> +       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 11e1168..543c10e 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -2,6 +2,8 @@
>  #define __LINUX_BQ27X00_BATTERY_H__
>
>  enum bq27xxx_chip {
> +       /* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
> +       /* and map to themselves in bq27xxx_chips[]             */
>         BQ27000 = 1, /* bq27000, bq27200 */
>         BQ27010, /* bq27010, bq27210 */
>         BQ2750X, /* bq27500 deprecated alias */
> @@ -18,6 +20,17 @@ enum bq27xxx_chip {
>         BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
>         BQ27545, /* bq27545 */
>         BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
> +       BQ27MAX,
> +
> +       /* these map to above in bq27xxx_chips[] */
> +       BQ2752X, /* deprecated alias */
> +       BQ27531,
> +       BQ27542,
> +       BQ27546,
> +       BQ27742,
> +       BQ27425,
> +       BQ27441,
> +       BQ27621,
>  };
>
>  /**
> --
> 2.9.3
>

Patch

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 06f15da..0aecd41 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"
 
@@ -132,7 +132,7 @@  enum bq27xxx_reg_index {
 	[BQ27XXX_DM_CKSUM] = 0x60
 
 /* Register mappings */
-static u8 bq27xxx_regs[][BQ27XXX_REG_MAX] = {
+static u8 bq27xxx_regs[BQ27MAX][BQ27XXX_REG_MAX] = {
 	[BQ27000] = {
 		[BQ27XXX_REG_CTRL] = 0x00,
 		[BQ27XXX_REG_TEMP] = 0x06,
@@ -779,7 +779,7 @@  static enum power_supply_property bq27421_battery_props[] = {
 static struct {
 	enum power_supply_property *props;
 	size_t size;
-} bq27xxx_battery_props[] = {
+} bq27xxx_battery_props[BQ27MAX] = {
 	BQ27XXX_PROP(BQ27000, bq27000_battery_props),
 	BQ27XXX_PROP(BQ27010, bq27010_battery_props),
 	BQ27XXX_PROP(BQ2750X, bq2750x_battery_props),
@@ -798,6 +798,33 @@  static struct {
 	BQ27XXX_PROP(BQ27421, bq27421_battery_props),
 };
 
+static enum bq27xxx_chip bq27xxx_chips[] = {
+	[BQ27000]   = BQ27000,
+	[BQ27010]   = BQ27010,
+	[BQ2750X]   = BQ2750X,
+	[BQ2751X]   = BQ2751X,
+	[BQ2752X]   = BQ2751X,
+	[BQ27500]   = BQ27500,
+	[BQ27510G1] = BQ27510G1,
+	[BQ27510G2] = BQ27510G2,
+	[BQ27510G3] = BQ27510G3,
+	[BQ27520G1] = BQ27520G1,
+	[BQ27520G2] = BQ27520G2,
+	[BQ27520G3] = BQ27520G3,
+	[BQ27520G4] = BQ27520G4,
+	[BQ27530]   = BQ27530,
+	[BQ27531]   = BQ27530,
+	[BQ27541]   = BQ27541,
+	[BQ27542]   = BQ27541,
+	[BQ27546]   = BQ27541,
+	[BQ27742]   = BQ27541,
+	[BQ27545]   = BQ27545,
+	[BQ27421]   = BQ27421,
+	[BQ27425]   = BQ27421,
+	[BQ27441]   = BQ27421,
+	[BQ27621]   = BQ27421,
+};
+
 static DEFINE_MUTEX(bq27xxx_list_lock);
 static LIST_HEAD(bq27xxx_battery_devices);
 
@@ -856,6 +883,54 @@  static const char * const 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,
+};
+
 
 #ifdef CONFIG_BATTERY_BQ27XXX_DT_UPDATES_NVM
 static bool bq27xxx_dt_to_nvm = true;
@@ -1865,9 +1940,16 @@  int bq27xxx_battery_setup(struct bq27xxx_device_info *di)
 		.drv_data = di,
 	};
 
+	di->ram_chip = di->chip == BQ27421 || di->chip == BQ27441 || di->chip == BQ27621;
+
+	di->unseal_key = bq27xxx_unseal_keys[di->chip];
+	di->dm_regs = bq27xxx_dm_regs[di->chip];
+	di->chip = bq27xxx_chips[di->chip];
+
+	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 11e1168..543c10e 100644
--- a/include/linux/power/bq27xxx_battery.h
+++ b/include/linux/power/bq27xxx_battery.h
@@ -2,6 +2,8 @@ 
 #define __LINUX_BQ27X00_BATTERY_H__
 
 enum bq27xxx_chip {
+	/* these index bq27xxx_regs[] & bq27xxx_battery_props[] */
+	/* and map to themselves in bq27xxx_chips[]             */
 	BQ27000 = 1, /* bq27000, bq27200 */
 	BQ27010, /* bq27010, bq27210 */
 	BQ2750X, /* bq27500 deprecated alias */
@@ -18,6 +20,17 @@  enum bq27xxx_chip {
 	BQ27541, /* bq27541, bq27542, bq27546, bq27742 */
 	BQ27545, /* bq27545 */
 	BQ27421, /* bq27421, bq27425, bq27441, bq27621 */
+	BQ27MAX,
+
+	/* these map to above in bq27xxx_chips[] */
+	BQ2752X, /* deprecated alias */
+	BQ27531,
+	BQ27542,
+	BQ27546,
+	BQ27742,
+	BQ27425,
+	BQ27441,
+	BQ27621,
 };
 
 /**