From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andrew F. Davis" Subject: Re: [PATCH v10 6/8] power: bq27xxx_battery: Keep track of specific chip id Date: Thu, 16 Mar 2017 15:50:05 -0500 Message-ID: <840adbba-6251-b565-5f85-611dd840cc39@ti.com> References: <20170315192653.26799-1-liam@networkimprov.net> <20170315192653.26799-7-liam@networkimprov.net> <3c2259be-5f63-1f07-8e4d-1bb059a9447f@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Return-path: Received: from fllnx210.ext.ti.com ([198.47.19.17]:12936 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752686AbdCPVNZ (ORCPT ); Thu, 16 Mar 2017 17:13:25 -0400 In-Reply-To: Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck Cc: Sebastian Reichel , linux-pm@vger.kernel.org, Matt Ranostay , Liam Breck On 03/16/2017 03:12 PM, Liam Breck wrote: > On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis wrote: >> On 03/15/2017 02:26 PM, Liam Breck wrote: >>> From: Liam Breck >>> >>> Pass actual chip ID into _setup(), which translates it to a group ID, >>> to allow support for all chips by the power_supply_battery_info code. >>> There are no functional changes to the driver. >>> >>> Signed-off-by: Liam Breck >>> --- >> >> What is this patch based on, it doesn't apply to v4.11-rc1 for me. > > Sorry, I don't have all of Chris' patches here. It wasn't a factor > until we attacked the register arrays. Could you send me his patchset > with: > git format-patch --stdout HEAD~x..HEAD~y > bq27xxx-lapa.mbox > Who are you asking? >>> drivers/power/supply/bq27xxx_battery.c | 19 +++++++++++++++++++ >>> drivers/power/supply/bq27xxx_battery_i2c.c | 16 ++++++++-------- >>> include/linux/power/bq27xxx_battery.h | 25 ++++++++++++++++++------- >>> 3 files changed, 45 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c >>> index 7272d1e..d613d3d 100644 >>> --- a/drivers/power/supply/bq27xxx_battery.c >>> +++ b/drivers/power/supply/bq27xxx_battery.c >>> @@ -1020,6 +1020,25 @@ int bq27xxx_battery_setup(struct bq27xxx_device_info *di) >>> struct power_supply_desc *psy_desc; >>> struct power_supply_config psy_cfg = { .drv_data = di, }; >>> >>> + switch(di->chip) { >>> + case BQ27000: >>> + case BQ27010: >>> + case BQ27500: >>> + case BQ27510: >>> + case BQ27530: >>> + case BQ27541: >>> + case BQ27545: >>> + case BQ27421: break; >> >> Why even have these cases then? > > You'll get a compiler warning if any are missing. Helps when adding new chips. > >>> + case BQ27520: di->chip = BQ27510; break; >>> + case BQ27531: di->chip = BQ27530; break; >>> + case BQ27542: di->chip = BQ27541; break; >>> + case BQ27546: di->chip = BQ27541; break; >>> + case BQ27742: di->chip = BQ27541; break; >>> + case BQ27425: di->chip = BQ27421; break; >>> + case BQ27441: di->chip = BQ27421; break; >>> + case BQ27621: di->chip = BQ27421; break; >> >> Nope, don't like this at all, make a different variable, ->registers or >> something to map to the register table. Plus I think changing the >> variable you are switching on can cause undefined behavior. > > We had a different variable, .dmid, but you rejected a second ID > field. I think this is better, as we eliminated the static arrays > .dmid indexed. I didn't rename .chip to .category as that would cause > trivial changes all over the file. I could do that in a final patch > tho. > > You can modify a variable after switching on it, I promise. > >>> + } >>> + >>> INIT_DELAYED_WORK(&di->work, bq27xxx_battery_poll); >>> mutex_init(&di->lock); >>> di->regs = bq27xxx_regs[di->chip]; >>> diff --git a/drivers/power/supply/bq27xxx_battery_i2c.c b/drivers/power/supply/bq27xxx_battery_i2c.c >>> index 5c5c3a6..13def59 100644 >>> --- a/drivers/power/supply/bq27xxx_battery_i2c.c >>> +++ b/drivers/power/supply/bq27xxx_battery_i2c.c >>> @@ -150,18 +150,18 @@ static const struct i2c_device_id bq27xxx_i2c_id_table[] = { >>> { "bq27210", BQ27010 }, >>> { "bq27500", BQ27500 }, >>> { "bq27510", BQ27510 }, >>> - { "bq27520", BQ27510 }, >>> + { "bq27520", BQ27520 }, >>> { "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 92df553..90db1cf 100644 >>> --- a/include/linux/power/bq27xxx_battery.h >>> +++ b/include/linux/power/bq27xxx_battery.h >>> @@ -2,14 +2,25 @@ >>> #define __LINUX_BQ27X00_BATTERY_H__ >>> >>> enum bq27xxx_chip { >>> + /* categories; index for bq27xxx_regs[] */ >>> BQ27000 = 1, /* bq27000, bq27200 */ >>> - BQ27010, /* bq27010, bq27210 */ >>> - BQ27500, /* bq27500 */ >>> - BQ27510, /* bq27510, bq27520 */ >>> - BQ27530, /* bq27530, bq27531 */ >>> - BQ27541, /* bq27541, bq27542, bq27546, bq27742 */ >>> - BQ27545, /* bq27545 */ >>> - BQ27421, /* bq27421, bq27425, bq27441, bq27621 */ >>> + BQ27010 = 2, /* bq27010, bq27210 */ >>> + BQ27500 = 3, /* bq27500 */ >>> + BQ27510 = 4, /* bq27510, bq27520 */ >>> + BQ27530 = 5, /* bq27530, bq27531 */ >>> + BQ27541 = 6, /* bq27541, bq27542, bq27546, bq27742 */ >>> + BQ27545 = 7, /* bq27545 */ >>> + BQ27421 = 8, /* bq27421, bq27425, bq27441, bq27621 */ >>> + >>> + /* members of categories; translate these to category in _setup() */ >>> + BQ27520 = 101, >>> + BQ27531 = 102, >>> + BQ27542 = 103, >>> + BQ27546 = 104, >>> + BQ27742 = 105, >>> + BQ27425 = 106, >>> + BQ27441 = 107, >>> + BQ27621 = 108, >> >> Get rid of this, just add new chip enum names if you need support for >> new chips. > > How does a non-DT config obtain chip ID? We want explicit enum values > if they appear in external platform-data objects. I'll mention that in > the next patch description. > platform-data is going away, I'm not sure what you are saying here. > The category (original) IDs index bq27xxx_regs[], and you'll probably > extend that in time. So I placed the member (new) IDs well above the > categories to allow permanent explicit values. >