Re: [PATCH] power: bq27xxx_battery: Unique chip IDs

["Andrew F. Davis" <afd@ti.com>]

On 03/08/2017 05:49 PM, Liam Breck wrote:
> On Wed, Mar 8, 2017 at 1:22 PM, Liam Breck <liam@networkimprov.net> wrote:
>> On Wed, Mar 8, 2017 at 10:23 AM, Andrew F. Davis <afd@ti.com> wrote:
>>> On 03/08/2017 12:16 PM, Liam Breck wrote:
>>>> On Wed, Mar 8, 2017 at 9:58 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>> On 03/08/2017 11:45 AM, Liam Breck wrote:
>>>>>> On Wed, Mar 8, 2017 at 9:23 AM, Andrew F. Davis <afd@ti.com> wrote:
>>>>>>> On 03/07/2017 05:05 PM, Liam Breck wrote:
>>>>>>>> From: Liam Breck <kernel@networkimprov.net>
>>>>>>>>
>>>>>>>> Assign every chip a unique ID, to enable
>>>>>>>> power_supply_battery_info config for all supported chips.
>>>>>>>> There are no functional changes to the driver.
>>>>>>>>
>>>>>>>> Depends-on: power: bq27xxx_battery: Add power_supply_battery_info support
>>>>>>>> Signed-off-by: Liam Breck <kernel@networkimprov.net>
>>>>>>>>
>>>>>>>> ---
>>>>>>>> Andrew, let me know if you'd like me to include this in the other patchset.
>>>>>>>
>>>>>>> Why do we need this at all? It may be just bit too early for me, but I'm
>>>>>>> not seeing what this gets us, that couldn't be done before.
>>>>>>
>>>>>> Sadly, the chips mostly have different dm_reg tables, so we need the
>>>>>> real chip ID to give each a table.
>>>>>>
>>>>>> My last rev only supported IDs associated with a single chip: 500,
>>>>>> 545, 421. In the 421 case, we had to split off that ID.
>>>>>>
>>>>>
>>>>> Okay, then if we need a new ID lets add it. Going back to having a table
>>>>> of pointers is a step back, we used to do it this way, but every-time a
>>>>> new chip appeared or we found a delta in an existing chip(s) (like you
>>>>> are finding now) we would have to re-organize the pointer table and
>>>>> other associated tables, it is much cleaner and easier to review (more
>>>>> important than code size by orders of magnitude IMHO) to just add a new
>>>>> entry into the big master table of registers.
>>>>
>>>> There were 8 mostly unique reg tables, and you'd need 8 dups to make
>>>> all the IDs explicit.
>>>>
>>>
>>> Why do all IDs need to be explicit, most have the same register set.
>>
>> The DM registers are different. See:
>>   bq27500_dm_regs[]
>>   bq27545_dm_regs[]
>>   bq27421_dm_regs[]
>>   bq27425_dm_regs[]
>>   bq27621_dm_regs[]
> 
> Another way to handle this is using a separate field for the dm_regs[]
> index, e.g. di->dmid.
> 
> That's a smaller patch, roughly +20 LoC.
> 

This might work better

>>>> You're already using a table of pointers for the property lists.
>>>>
>>>> I don't see how adding an entry to a pointer table is worse than
>>>> adding an entire reg table. Sure, now and then you'd need to add both.
>>>>
>>>>>>>>  drivers/power/supply/bq27xxx_battery.c     | 94 +++++++++++++++++-------------
>>>>>>>>  drivers/power/supply/bq27xxx_battery_i2c.c | 14 ++---
>>>>>>>>  include/linux/power/bq27xxx_battery.h      | 15 +++--
>>>>>>>>  3 files changed, 71 insertions(+), 52 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
>>>>>>>> index ed976a1..30af0e4 100644
>>>>>>>> --- a/drivers/power/supply/bq27xxx_battery.c
>>>>>>>> +++ b/drivers/power/supply/bq27xxx_battery.c
>>>>>>
>>>>>>>> +static u8* bq27xxx_regs[] = {
>>>>>>>> +     [BQ27000] = bq27000_regs, /* really bq27200 */
>>>>>>>> +     [BQ27010] = bq27010_regs, /* really bq27210 */
>>>>>>>
>>>>>>> Not strictly true, the bq270x0 and bq272x0 are different chips, one has
>>>>>>> an HDQ bus and the other I2C, later chips have both ports on each chip.
>>>>>>
>>>>>> Oh, I saw only the 200 & 210 in _i2c.c, and forgot that the driver
>>>>>> supports another interface. I'll remove those comments.
>>>>>>
>>>>>> I won't separate these IDs as none of them support DM update.
>>>>>>
>>>>>> Think you could do some testing soon?
>>>>>>
>>>>>
>>>>> Yeah, I could probably fit some testing into my schedule, I'll see what
>>>>> v9 looks like before I unpack the boards.