* [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips @ 2017-04-05 5:45 Liam Breck 2017-04-05 8:07 ` Liam Breck 0 siblings, 1 reply; 5+ messages in thread From: Liam Breck @ 2017-04-05 5:45 UTC (permalink / raw) To: Andrew F. Davis, linux-pm; +Cc: Liam Breck From: Liam Breck <kernel@networkimprov.net> Support data memory update of BQ27500, 545, 421, 425, 441, 621. Create IDs for for previously unID'd chips, to index arrays for unseal keys and data memory register tables. Signed-off-by: Liam Breck <kernel@networkimprov.net> --- drivers/power/supply/bq27xxx_battery.c | 88 ++++++++++++++++++++++++++++-- drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++--- include/linux/power/bq27xxx_battery.h | 12 ++++ 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c index a2a5926..0dbd7e4 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_prototypes[] = { + [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* bq27xxx_dm_reg_name[] = { [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", }; +static struct bq27xxx_dm_reg bq27500_dm_regs[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 10, 2, 0, 65535 }, + [BQ27XXX_DM_DESIGN_ENERGY] = { }, /* missing on chip */ + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 }, +}; + +static struct bq27xxx_dm_reg bq27545_dm_regs[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 23, 2, 0, 32767 }, + [BQ27XXX_DM_DESIGN_ENERGY] = { 48, 25, 2, 0, 32767 }, + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800, 3700 }, +}; + +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, +}; + +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, +}; + +static struct bq27xxx_dm_reg bq27621_dm_regs[] = { + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, +}; + +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { + [BQ27500] = bq27500_dm_regs, + [BQ27545] = bq27545_dm_regs, + [BQ27421] = bq27421_dm_regs, + [BQ27425] = bq27425_dm_regs, + [BQ27441] = bq27421_dm_regs, + [BQ27621] = bq27621_dm_regs, +}; + +static u32 bq27xxx_unseal_keys[] = { + [BQ27500] = 0x04143672, + [BQ27545] = 0x04143672, + [BQ27421] = 0x80008000, + [BQ27425] = 0x04143672, + [BQ27441] = 0x80008000, + [BQ27621] = 0x80008000, +}; + static int poll_interval_param_set(const char *val, const struct kernel_param *kp) { @@ -1831,9 +1906,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) .drv_data = di, }; + di->unseal_key = bq27xxx_unseal_keys[di->chip]; + di->dm_regs = bq27xxx_dm_regs[di->chip]; + di->chip = bq27xxx_prototypes[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 227eb08..fc9b08a 100644 --- a/include/linux/power/bq27xxx_battery.h +++ b/include/linux/power/bq27xxx_battery.h @@ -2,6 +2,7 @@ #define __LINUX_BQ27X00_BATTERY_H__ enum bq27xxx_chip { + /* prototypes; index for bq27xxx_regs[] & bq27xxx_battery_props[] */ BQ27000 = 1, /* bq27000, bq27200 */ BQ27010, /* bq27010, bq27210 */ BQ2750X, /* bq27500 deprecated alias */ @@ -18,6 +19,17 @@ enum bq27xxx_chip { BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ BQ27545, /* bq27545 */ BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ + BQ27MAX, + + /* others, mapped to prototypes in bq27xxx_prototypes[] */ + BQ2752X, /* deprecated alias */ + BQ27531, + BQ27542, + BQ27546, + BQ27742, + BQ27425, + BQ27441, + BQ27621, }; /** -- 2.9.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips 2017-04-05 5:45 [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips Liam Breck @ 2017-04-05 8:07 ` Liam Breck 2017-04-05 14:54 ` Andrew F. Davis 0 siblings, 1 reply; 5+ messages in thread From: Liam Breck @ 2017-04-05 8:07 UTC (permalink / raw) To: Andrew F. Davis, linux-pm; +Cc: Liam Breck Hi Andrew, On Tue, Apr 4, 2017 at 10:45 PM, Liam Breck <liam@networkimprov.net> wrote: > From: Liam Breck <kernel@networkimprov.net> > > Support data memory update of BQ27500, 545, 421, 425, 441, 621. > > Create IDs for for previously unID'd chips, to index arrays for unseal keys > and data memory register tables. > > Signed-off-by: Liam Breck <kernel@networkimprov.net> > --- > drivers/power/supply/bq27xxx_battery.c | 88 ++++++++++++++++++++++++++++-- > drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++--- > include/linux/power/bq27xxx_battery.h | 12 ++++ > 3 files changed, 104 insertions(+), 12 deletions(-) > > diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c > index a2a5926..0dbd7e4 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_prototypes[] = { > + [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, > +}; The above is essentially the old I2C ID table. > static DEFINE_MUTEX(bq27xxx_list_lock); > static LIST_HEAD(bq27xxx_battery_devices); > > @@ -856,6 +883,54 @@ static const char* bq27xxx_dm_reg_name[] = { > [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", > }; > > +static struct bq27xxx_dm_reg bq27500_dm_regs[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 10, 2, 0, 65535 }, > + [BQ27XXX_DM_DESIGN_ENERGY] = { }, /* missing on chip */ > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 }, > +}; > + > +static struct bq27xxx_dm_reg bq27545_dm_regs[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 23, 2, 0, 32767 }, > + [BQ27XXX_DM_DESIGN_ENERGY] = { 48, 25, 2, 0, 32767 }, > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800, 3700 }, > +}; > + > +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, > + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, > +}; > + > +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, > + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, > +}; > + > +static struct bq27xxx_dm_reg bq27621_dm_regs[] = { > + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, > + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, > + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, > +}; > + > +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { > + [BQ27500] = bq27500_dm_regs, > + [BQ27545] = bq27545_dm_regs, > + [BQ27421] = bq27421_dm_regs, > + [BQ27425] = bq27425_dm_regs, > + [BQ27441] = bq27421_dm_regs, > + [BQ27621] = bq27621_dm_regs, > +}; > + > +static u32 bq27xxx_unseal_keys[] = { > + [BQ27500] = 0x04143672, > + [BQ27545] = 0x04143672, > + [BQ27421] = 0x80008000, > + [BQ27425] = 0x04143672, > + [BQ27441] = 0x80008000, > + [BQ27621] = 0x80008000, > +}; > + > > static int poll_interval_param_set(const char *val, const struct kernel_param *kp) > { > @@ -1831,9 +1906,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) > .drv_data = di, > }; > > + di->unseal_key = bq27xxx_unseal_keys[di->chip]; > + di->dm_regs = bq27xxx_dm_regs[di->chip]; > + di->chip = bq27xxx_prototypes[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 227eb08..fc9b08a 100644 > --- a/include/linux/power/bq27xxx_battery.h > +++ b/include/linux/power/bq27xxx_battery.h > @@ -2,6 +2,7 @@ > #define __LINUX_BQ27X00_BATTERY_H__ > > enum bq27xxx_chip { > + /* prototypes; index for bq27xxx_regs[] & bq27xxx_battery_props[] */ > BQ27000 = 1, /* bq27000, bq27200 */ > BQ27010, /* bq27010, bq27210 */ > BQ2750X, /* bq27500 deprecated alias */ > @@ -18,6 +19,17 @@ enum bq27xxx_chip { > BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ > BQ27545, /* bq27545 */ > BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ > + BQ27MAX, > + > + /* others, mapped to prototypes in bq27xxx_prototypes[] */ > + BQ2752X, /* deprecated alias */ > + BQ27531, > + BQ27542, > + BQ27546, > + BQ27742, > + BQ27425, > + BQ27441, > + BQ27621, > }; The "prototypes" subset prevents holes in bq27xxx_regs[] & _battery_props[]. Holes aren't a problem if the bq27xxx_prototypes map is correct. But holes have 0x00 vs. 0xff, so mapping to a hole isn't caught by testing for invalid_reg_addr. Also leaving holes may look like a bug, albeit without ill effect. Re a second di->id to index bq27xxx_regs without holes, that entails a second enum replicating the prototypes subset. To add a chip with a unique regmap, you'd add to both enums. How is that better than one enum with subsets? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips 2017-04-05 8:07 ` Liam Breck @ 2017-04-05 14:54 ` Andrew F. Davis 2017-04-05 17:56 ` Liam Breck 0 siblings, 1 reply; 5+ messages in thread From: Andrew F. Davis @ 2017-04-05 14:54 UTC (permalink / raw) To: Liam Breck, linux-pm; +Cc: Liam Breck On 04/05/2017 03:07 AM, Liam Breck wrote: > Hi Andrew, > > On Tue, Apr 4, 2017 at 10:45 PM, Liam Breck <liam@networkimprov.net> wrote: >> From: Liam Breck <kernel@networkimprov.net> >> >> Support data memory update of BQ27500, 545, 421, 425, 441, 621. >> >> Create IDs for for previously unID'd chips, to index arrays for unseal keys >> and data memory register tables. >> >> Signed-off-by: Liam Breck <kernel@networkimprov.net> >> --- >> drivers/power/supply/bq27xxx_battery.c | 88 ++++++++++++++++++++++++++++-- >> drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++--- >> include/linux/power/bq27xxx_battery.h | 12 ++++ >> 3 files changed, 104 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c >> index a2a5926..0dbd7e4 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_prototypes[] = { >> + [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, >> +}; > > The above is essentially the old I2C ID table. > > >> static DEFINE_MUTEX(bq27xxx_list_lock); >> static LIST_HEAD(bq27xxx_battery_devices); >> >> @@ -856,6 +883,54 @@ static const char* bq27xxx_dm_reg_name[] = { >> [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", >> }; >> >> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = { >> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 10, 2, 0, 65535 }, >> + [BQ27XXX_DM_DESIGN_ENERGY] = { }, /* missing on chip */ >> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 }, >> +}; >> + >> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = { >> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 23, 2, 0, 32767 }, >> + [BQ27XXX_DM_DESIGN_ENERGY] = { 48, 25, 2, 0, 32767 }, >> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800, 3700 }, >> +}; >> + >> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { >> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, >> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, >> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, >> +}; >> + >> +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { >> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, >> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, >> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, >> +}; >> + >> +static struct bq27xxx_dm_reg bq27621_dm_regs[] = { >> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, >> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, >> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, >> +}; >> + >> +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { >> + [BQ27500] = bq27500_dm_regs, >> + [BQ27545] = bq27545_dm_regs, >> + [BQ27421] = bq27421_dm_regs, >> + [BQ27425] = bq27425_dm_regs, >> + [BQ27441] = bq27421_dm_regs, >> + [BQ27621] = bq27621_dm_regs, >> +}; >> + >> +static u32 bq27xxx_unseal_keys[] = { >> + [BQ27500] = 0x04143672, >> + [BQ27545] = 0x04143672, >> + [BQ27421] = 0x80008000, >> + [BQ27425] = 0x04143672, >> + [BQ27441] = 0x80008000, >> + [BQ27621] = 0x80008000, >> +}; >> + >> >> static int poll_interval_param_set(const char *val, const struct kernel_param *kp) >> { >> @@ -1831,9 +1906,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) >> .drv_data = di, >> }; >> >> + di->unseal_key = bq27xxx_unseal_keys[di->chip]; >> + di->dm_regs = bq27xxx_dm_regs[di->chip]; >> + di->chip = bq27xxx_prototypes[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 227eb08..fc9b08a 100644 >> --- a/include/linux/power/bq27xxx_battery.h >> +++ b/include/linux/power/bq27xxx_battery.h >> @@ -2,6 +2,7 @@ >> #define __LINUX_BQ27X00_BATTERY_H__ >> >> enum bq27xxx_chip { >> + /* prototypes; index for bq27xxx_regs[] & bq27xxx_battery_props[] */ >> BQ27000 = 1, /* bq27000, bq27200 */ >> BQ27010, /* bq27010, bq27210 */ >> BQ2750X, /* bq27500 deprecated alias */ >> @@ -18,6 +19,17 @@ enum bq27xxx_chip { >> BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ >> BQ27545, /* bq27545 */ >> BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ >> + BQ27MAX, >> + >> + /* others, mapped to prototypes in bq27xxx_prototypes[] */ >> + BQ2752X, /* deprecated alias */ >> + BQ27531, >> + BQ27542, >> + BQ27546, >> + BQ27742, >> + BQ27425, >> + BQ27441, >> + BQ27621, >> }; > > The "prototypes" subset prevents holes in bq27xxx_regs[] & > _battery_props[]. Holes aren't a problem if the bq27xxx_prototypes map > is correct. But holes have 0x00 vs. 0xff, so mapping to a hole isn't > caught by testing for invalid_reg_addr. Also leaving holes may look > like a bug, albeit without ill effect. > > Re a second di->id to index bq27xxx_regs without holes, that entails a > second enum replicating the prototypes subset. To add a chip with a > unique regmap, you'd add to both enums. How is that better than one > enum with subsets? > If you would like to avoid holes you can re-order the enum like you did, but calling them something else is not easy to understand for someone adding new devices, even worse we now use this chip->id to map into multiple tables, how will you always avoid holes? Lets just fill in the holes with table entries and be done with this series already.. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips 2017-04-05 14:54 ` Andrew F. Davis @ 2017-04-05 17:56 ` Liam Breck 2017-04-06 18:29 ` Liam Breck 0 siblings, 1 reply; 5+ messages in thread From: Liam Breck @ 2017-04-05 17:56 UTC (permalink / raw) To: Andrew F. Davis; +Cc: linux-pm, Liam Breck On Wed, Apr 5, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote: > On 04/05/2017 03:07 AM, Liam Breck wrote: >> Hi Andrew, >> >> On Tue, Apr 4, 2017 at 10:45 PM, Liam Breck <liam@networkimprov.net> wrote: >>> From: Liam Breck <kernel@networkimprov.net> >>> >>> Support data memory update of BQ27500, 545, 421, 425, 441, 621. >>> >>> Create IDs for for previously unID'd chips, to index arrays for unseal keys >>> and data memory register tables. >>> >>> Signed-off-by: Liam Breck <kernel@networkimprov.net> >>> --- >>> drivers/power/supply/bq27xxx_battery.c | 88 ++++++++++++++++++++++++++++-- >>> drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++--- >>> include/linux/power/bq27xxx_battery.h | 12 ++++ >>> 3 files changed, 104 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c >>> index a2a5926..0dbd7e4 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_prototypes[] = { >>> + [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, >>> +}; >> >> The above is essentially the old I2C ID table. >> >> >>> static DEFINE_MUTEX(bq27xxx_list_lock); >>> static LIST_HEAD(bq27xxx_battery_devices); >>> >>> @@ -856,6 +883,54 @@ static const char* bq27xxx_dm_reg_name[] = { >>> [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", >>> }; >>> >>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = { >>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 10, 2, 0, 65535 }, >>> + [BQ27XXX_DM_DESIGN_ENERGY] = { }, /* missing on chip */ >>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 }, >>> +}; >>> + >>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = { >>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 23, 2, 0, 32767 }, >>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 48, 25, 2, 0, 32767 }, >>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800, 3700 }, >>> +}; >>> + >>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { >>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, >>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, >>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, >>> +}; >>> + >>> +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { >>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, >>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, >>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, >>> +}; >>> + >>> +static struct bq27xxx_dm_reg bq27621_dm_regs[] = { >>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, >>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, >>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, >>> +}; >>> + >>> +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { >>> + [BQ27500] = bq27500_dm_regs, >>> + [BQ27545] = bq27545_dm_regs, >>> + [BQ27421] = bq27421_dm_regs, >>> + [BQ27425] = bq27425_dm_regs, >>> + [BQ27441] = bq27421_dm_regs, >>> + [BQ27621] = bq27621_dm_regs, >>> +}; >>> + >>> +static u32 bq27xxx_unseal_keys[] = { >>> + [BQ27500] = 0x04143672, >>> + [BQ27545] = 0x04143672, >>> + [BQ27421] = 0x80008000, >>> + [BQ27425] = 0x04143672, >>> + [BQ27441] = 0x80008000, >>> + [BQ27621] = 0x80008000, >>> +}; >>> + >>> >>> static int poll_interval_param_set(const char *val, const struct kernel_param *kp) >>> { >>> @@ -1831,9 +1906,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) >>> .drv_data = di, >>> }; >>> >>> + di->unseal_key = bq27xxx_unseal_keys[di->chip]; >>> + di->dm_regs = bq27xxx_dm_regs[di->chip]; >>> + di->chip = bq27xxx_prototypes[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 227eb08..fc9b08a 100644 >>> --- a/include/linux/power/bq27xxx_battery.h >>> +++ b/include/linux/power/bq27xxx_battery.h >>> @@ -2,6 +2,7 @@ >>> #define __LINUX_BQ27X00_BATTERY_H__ >>> >>> enum bq27xxx_chip { >>> + /* prototypes; index for bq27xxx_regs[] & bq27xxx_battery_props[] */ >>> BQ27000 = 1, /* bq27000, bq27200 */ >>> BQ27010, /* bq27010, bq27210 */ >>> BQ2750X, /* bq27500 deprecated alias */ >>> @@ -18,6 +19,17 @@ enum bq27xxx_chip { >>> BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ >>> BQ27545, /* bq27545 */ >>> BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ >>> + BQ27MAX, >>> + >>> + /* others, mapped to prototypes in bq27xxx_prototypes[] */ >>> + BQ2752X, /* deprecated alias */ >>> + BQ27531, >>> + BQ27542, >>> + BQ27546, >>> + BQ27742, >>> + BQ27425, >>> + BQ27441, >>> + BQ27621, >>> }; >> >> The "prototypes" subset prevents holes in bq27xxx_regs[] & >> _battery_props[]. Holes aren't a problem if the bq27xxx_prototypes map >> is correct. But holes have 0x00 vs. 0xff, so mapping to a hole isn't >> caught by testing for invalid_reg_addr. Also leaving holes may look >> like a bug, albeit without ill effect. >> >> Re a second di->id to index bq27xxx_regs without holes, that entails a >> second enum replicating the prototypes subset. To add a chip with a >> unique regmap, you'd add to both enums. How is that better than one >> enum with subsets? >> > > If you would like to avoid holes you can re-order the enum like you did, > but calling them something else is not easy to understand for someone I can drop "prototypes" and "others" in the comments. > adding new devices, even worse we now use this chip->id to map into > multiple tables, how will you always avoid holes? Lets just I considered one table with 5 fields: *regs, *props, unseal_key, *dm_regs, chip. However that is a large separate patch, which scraps the 2D reg array. And we only use any of that data once, during probe(). BTW, 0x00 holes are expected in dm_regs and unseal_keys, and in battery_props only leave /sys/class...bq27nnn/ empty. > fill in the holes with table entries and be done with this series already.. The codebase has long had prototype devices, and avoided duplicate reg maps. I am convinced that duplicate maps would be a design flaw worse than holes. Indeed, I wrote patches to remove dupes and check for them. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips 2017-04-05 17:56 ` Liam Breck @ 2017-04-06 18:29 ` Liam Breck 0 siblings, 0 replies; 5+ messages in thread From: Liam Breck @ 2017-04-06 18:29 UTC (permalink / raw) To: Andrew F. Davis; +Cc: linux-pm, Liam Breck On Wed, Apr 5, 2017 at 10:56 AM, Liam Breck <liam@networkimprov.net> wrote: > On Wed, Apr 5, 2017 at 7:54 AM, Andrew F. Davis <afd@ti.com> wrote: >> On 04/05/2017 03:07 AM, Liam Breck wrote: >>> Hi Andrew, >>> >>> On Tue, Apr 4, 2017 at 10:45 PM, Liam Breck <liam@networkimprov.net> wrote: >>>> From: Liam Breck <kernel@networkimprov.net> >>>> >>>> Support data memory update of BQ27500, 545, 421, 425, 441, 621. >>>> >>>> Create IDs for for previously unID'd chips, to index arrays for unseal keys >>>> and data memory register tables. >>>> >>>> Signed-off-by: Liam Breck <kernel@networkimprov.net> >>>> --- >>>> drivers/power/supply/bq27xxx_battery.c | 88 ++++++++++++++++++++++++++++-- >>>> drivers/power/supply/bq27xxx_battery_i2c.c | 16 +++--- >>>> include/linux/power/bq27xxx_battery.h | 12 ++++ >>>> 3 files changed, 104 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c >>>> index a2a5926..0dbd7e4 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_prototypes[] = { >>>> + [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, >>>> +}; >>> >>> The above is essentially the old I2C ID table. I've renamed the above array bq27xxx_chips. >>>> static DEFINE_MUTEX(bq27xxx_list_lock); >>>> static LIST_HEAD(bq27xxx_battery_devices); >>>> >>>> @@ -856,6 +883,54 @@ static const char* bq27xxx_dm_reg_name[] = { >>>> [BQ27XXX_DM_TERMINATE_VOLTAGE] = "terminate-voltage", >>>> }; >>>> >>>> +static struct bq27xxx_dm_reg bq27500_dm_regs[] = { >>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 10, 2, 0, 65535 }, >>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { }, /* missing on chip */ >>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 48, 2, 1000, 32767 }, >>>> +}; >>>> + >>>> +static struct bq27xxx_dm_reg bq27545_dm_regs[] = { >>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 48, 23, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 48, 25, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 80, 67, 2, 2800, 3700 }, >>>> +}; >>>> + >>>> +static struct bq27xxx_dm_reg bq27421_dm_regs[] = { >>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 10, 2, 0, 8000 }, >>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 12, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 16, 2, 2500, 3700 }, >>>> +}; >>>> + >>>> +static struct bq27xxx_dm_reg bq27425_dm_regs[] = { >>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 12, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 14, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 18, 2, 2800, 3700 }, >>>> +}; >>>> + >>>> +static struct bq27xxx_dm_reg bq27621_dm_regs[] = { >>>> + [BQ27XXX_DM_DESIGN_CAPACITY] = { 82, 3, 2, 0, 8000 }, >>>> + [BQ27XXX_DM_DESIGN_ENERGY] = { 82, 5, 2, 0, 32767 }, >>>> + [BQ27XXX_DM_TERMINATE_VOLTAGE] = { 82, 9, 2, 2500, 3700 }, >>>> +}; >>>> + >>>> +static struct bq27xxx_dm_reg *bq27xxx_dm_regs[] = { >>>> + [BQ27500] = bq27500_dm_regs, >>>> + [BQ27545] = bq27545_dm_regs, >>>> + [BQ27421] = bq27421_dm_regs, >>>> + [BQ27425] = bq27425_dm_regs, >>>> + [BQ27441] = bq27421_dm_regs, >>>> + [BQ27621] = bq27621_dm_regs, >>>> +}; >>>> + >>>> +static u32 bq27xxx_unseal_keys[] = { >>>> + [BQ27500] = 0x04143672, >>>> + [BQ27545] = 0x04143672, >>>> + [BQ27421] = 0x80008000, >>>> + [BQ27425] = 0x04143672, >>>> + [BQ27441] = 0x80008000, >>>> + [BQ27621] = 0x80008000, >>>> +}; >>>> + >>>> >>>> static int poll_interval_param_set(const char *val, const struct kernel_param *kp) >>>> { >>>> @@ -1831,9 +1906,14 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) >>>> .drv_data = di, >>>> }; >>>> >>>> + di->unseal_key = bq27xxx_unseal_keys[di->chip]; >>>> + di->dm_regs = bq27xxx_dm_regs[di->chip]; >>>> + di->chip = bq27xxx_prototypes[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 227eb08..fc9b08a 100644 >>>> --- a/include/linux/power/bq27xxx_battery.h >>>> +++ b/include/linux/power/bq27xxx_battery.h >>>> @@ -2,6 +2,7 @@ >>>> #define __LINUX_BQ27X00_BATTERY_H__ >>>> >>>> enum bq27xxx_chip { >>>> + /* prototypes; index for bq27xxx_regs[] & bq27xxx_battery_props[] */ New comment is: /* 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 +19,17 @@ enum bq27xxx_chip { >>>> BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ >>>> BQ27545, /* bq27545 */ >>>> BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ >>>> + BQ27MAX, >>>> + >>>> + /* others, mapped to prototypes in bq27xxx_prototypes[] */ New comment is: /* these map to above in bq27xxx_chips[] */ >>>> + BQ2752X, /* deprecated alias */ >>>> + BQ27531, >>>> + BQ27542, >>>> + BQ27546, >>>> + BQ27742, >>>> + BQ27425, >>>> + BQ27441, >>>> + BQ27621, >>>> }; >>> >>> The "prototypes" subset prevents holes in bq27xxx_regs[] & >>> _battery_props[]. Holes aren't a problem if the bq27xxx_prototypes map >>> is correct. But holes have 0x00 vs. 0xff, so mapping to a hole isn't >>> caught by testing for invalid_reg_addr. Also leaving holes may look >>> like a bug, albeit without ill effect. >>> >>> Re a second di->id to index bq27xxx_regs without holes, that entails a >>> second enum replicating the prototypes subset. To add a chip with a >>> unique regmap, you'd add to both enums. How is that better than one >>> enum with subsets? >>> >> >> If you would like to avoid holes you can re-order the enum like you did, >> but calling them something else is not easy to understand for someone > > I can drop "prototypes" and "others" in the comments. > >> adding new devices, even worse we now use this chip->id to map into >> multiple tables, how will you always avoid holes? Lets just > > I considered one table with 5 fields: *regs, *props, unseal_key, > *dm_regs, chip. However that is a large separate patch, which scraps > the 2D reg array. And we only use any of that data once, during > probe(). > > BTW, 0x00 holes are expected in dm_regs and unseal_keys, and in > battery_props only leave /sys/class...bq27nnn/ empty. > >> fill in the holes with table entries and be done with this series already.. > > The codebase has long had prototype devices, and avoided duplicate reg > maps. I am convinced that duplicate maps would be a design flaw worse > than holes. Indeed, I wrote patches to remove dupes and check for > them. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-04-06 18:29 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-05 5:45 [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips Liam Breck 2017-04-05 8:07 ` Liam Breck 2017-04-05 14:54 ` Andrew F. Davis 2017-04-05 17:56 ` Liam Breck 2017-04-06 18:29 ` Liam Breck
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox