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 09:44:42 -0500 Message-ID: <3c2259be-5f63-1f07-8e4d-1bb059a9447f@ti.com> References: <20170315192653.26799-1-liam@networkimprov.net> <20170315192653.26799-7-liam@networkimprov.net> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: Received: from lelnx193.ext.ti.com ([198.47.27.77]:36503 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753470AbdCPOov (ORCPT ); Thu, 16 Mar 2017 10:44:51 -0400 In-Reply-To: <20170315192653.26799-7-liam@networkimprov.net> Sender: linux-pm-owner@vger.kernel.org List-Id: linux-pm@vger.kernel.org To: Liam Breck , Sebastian Reichel Cc: linux-pm@vger.kernel.org, Matt Ranostay , Liam Breck 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. > 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? > + 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. > + } > + > 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. > }; > > /** >