From: "Andrew F. Davis" <afd@ti.com>
To: Liam Breck <liam@networkimprov.net>, linux-pm@vger.kernel.org
Cc: Liam Breck <kernel@networkimprov.net>
Subject: Re: [PATCH v12.5 7/10] power: bq27xxx_battery: Enable chip data memory update for certain chips
Date: Wed, 5 Apr 2017 09:54:08 -0500 [thread overview]
Message-ID: <7fa6ddd8-c13d-ca70-feb1-b0862f43d36e@ti.com> (raw)
In-Reply-To: <CAKvHMgQ-DMPj4Tm5nBZiUyAec=i4rXQcPFc3XQuCRa43n3kOmg@mail.gmail.com>
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..
next prev parent reply other threads:[~2017-04-05 14:54 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2017-04-05 17:56 ` Liam Breck
2017-04-06 18:29 ` Liam Breck
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7fa6ddd8-c13d-ca70-feb1-b0862f43d36e@ti.com \
--to=afd@ti.com \
--cc=kernel@networkimprov.net \
--cc=liam@networkimprov.net \
--cc=linux-pm@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox