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

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

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.

> 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.