| Message ID | 20170315192653.26799-7-liam@networkimprov.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 03/15/2017 02:26 PM, Liam Breck wrote: > From: Liam Breck <kernel@networkimprov.net> > > 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 <kernel@networkimprov.net> > --- 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. > }; > > /** >
On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: > On 03/15/2017 02:26 PM, Liam Breck wrote: >> From: Liam Breck <kernel@networkimprov.net> >> >> 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 <kernel@networkimprov.net> >> --- > > 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 >> 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. 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.
On 03/16/2017 03:12 PM, Liam Breck wrote: > On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >> On 03/15/2017 02:26 PM, Liam Breck wrote: >>> From: Liam Breck <kernel@networkimprov.net> >>> >>> 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 <kernel@networkimprov.net> >>> --- >> >> 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. >
On Thu, Mar 16, 2017 at 1:50 PM, Andrew F. Davis <afd@ti.com> wrote: > On 03/16/2017 03:12 PM, Liam Breck wrote: >> On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >>> On 03/15/2017 02:26 PM, Liam Breck wrote: >>>> From: Liam Breck <kernel@networkimprov.net> >>>> >>>> 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 <kernel@networkimprov.net> >>>> --- >>> >>> 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? You, if I may impose... >>>> 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. Don't 1-wire users have a platform-data config? You plan to force them to replace it with DT? I thought that wasn't kosher. >> 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. >>
On 03/16/2017 04:26 PM, Liam Breck wrote: > On Thu, Mar 16, 2017 at 1:50 PM, Andrew F. Davis <afd@ti.com> wrote: >> On 03/16/2017 03:12 PM, Liam Breck wrote: >>> On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >>>> On 03/15/2017 02:26 PM, Liam Breck wrote: >>>>> From: Liam Breck <kernel@networkimprov.net> >>>>> >>>>> 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 <kernel@networkimprov.net> >>>>> --- >>>> >>>> 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? > > You, if I may impose... > Just rebase on v4.11-rc1... >>>>> 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. > > Don't 1-wire users have a platform-data config? You plan to force them > to replace it with DT? I thought that wasn't kosher. > The 1-wire driver generates this platform-data, 1-wire is a discoverable bus, they never needed to define DT or platform data and still will not. >>> 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. >>>
On Thu, Mar 16, 2017 at 2:30 PM, Andrew F. Davis <afd@ti.com> wrote: > On 03/16/2017 04:26 PM, Liam Breck wrote: >> On Thu, Mar 16, 2017 at 1:50 PM, Andrew F. Davis <afd@ti.com> wrote: >>> On 03/16/2017 03:12 PM, Liam Breck wrote: >>>> On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >>>>> On 03/15/2017 02:26 PM, Liam Breck wrote: >>>>>> From: Liam Breck <kernel@networkimprov.net> >>>>>> >>>>>> 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 <kernel@networkimprov.net> >>>>>> --- >>>>> >>>>> 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? >> >> You, if I may impose... >> > > Just rebase on v4.11-rc1... > >>>>>> 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. >> >> Don't 1-wire users have a platform-data config? You plan to force them >> to replace it with DT? I thought that wasn't kosher. >> > > The 1-wire driver generates this platform-data, 1-wire is a discoverable > bus, they never needed to define DT or platform data and still will not. How does it map a discovered chip type to a bq27xxx_chip enum? >>>> 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. >>>>
On 03/16/2017 04:47 PM, Liam Breck wrote: > On Thu, Mar 16, 2017 at 2:30 PM, Andrew F. Davis <afd@ti.com> wrote: >> On 03/16/2017 04:26 PM, Liam Breck wrote: >>> On Thu, Mar 16, 2017 at 1:50 PM, Andrew F. Davis <afd@ti.com> wrote: >>>> On 03/16/2017 03:12 PM, Liam Breck wrote: >>>>> On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >>>>>> On 03/15/2017 02:26 PM, Liam Breck wrote: >>>>>>> From: Liam Breck <kernel@networkimprov.net> >>>>>>> >>>>>>> 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 <kernel@networkimprov.net> >>>>>>> --- >>>>>> >>>>>> 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? >>> >>> You, if I may impose... >>> >> >> Just rebase on v4.11-rc1... >> >>>>>>> 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. >>> >>> Don't 1-wire users have a platform-data config? You plan to force them >>> to replace it with DT? I thought that wasn't kosher. >>> >> >> The 1-wire driver generates this platform-data, 1-wire is a discoverable >> bus, they never needed to define DT or platform data and still will not. > > How does it map a discovered chip type to a bq27xxx_chip enum? > Right now it just assumes the chip is a BQ27000, see line 45 of drivers/w1/slaves/w1_bq27000.c >>>>> 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. >>>>>
On Thu, Mar 16, 2017 at 2:53 PM, Andrew F. Davis <afd@ti.com> wrote: > On 03/16/2017 04:47 PM, Liam Breck wrote: >> On Thu, Mar 16, 2017 at 2:30 PM, Andrew F. Davis <afd@ti.com> wrote: >>> On 03/16/2017 04:26 PM, Liam Breck wrote: >>>> On Thu, Mar 16, 2017 at 1:50 PM, Andrew F. Davis <afd@ti.com> wrote: >>>>> On 03/16/2017 03:12 PM, Liam Breck wrote: >>>>>> On Thu, Mar 16, 2017 at 7:44 AM, Andrew F. Davis <afd@ti.com> wrote: >>>>>>> On 03/15/2017 02:26 PM, Liam Breck wrote: >>>>>>>> From: Liam Breck <kernel@networkimprov.net> >>>>>>>> >>>>>>>> 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 <kernel@networkimprov.net> >>>>>>>> --- >>>>>>> >>>>>>> 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? >>>> >>>> You, if I may impose... >>>> >>> >>> Just rebase on v4.11-rc1... >>> >>>>>>>> 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. >>>> >>>> Don't 1-wire users have a platform-data config? You plan to force them >>>> to replace it with DT? I thought that wasn't kosher. >>>> >>> >>> The 1-wire driver generates this platform-data, 1-wire is a discoverable >>> bus, they never needed to define DT or platform data and still will not. >> >> How does it map a discovered chip type to a bq27xxx_chip enum? >> > > Right now it just assumes the chip is a BQ27000, see line 45 of > drivers/w1/slaves/w1_bq27000.c OK then we don't need explicit enum values. We still need the category members after the categories, since the latter index bq27xxx_regs[]. >>>>>> 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. >>>>>>
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; + 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; + } + 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, }; /**