Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge

[Ryan Mallon <ryan@bluewatersys.com>]

Mike Rapoport wrote:
> Hi Ryan,
> 
> Ryan Mallon wrote:
>> Mike Rapoport wrote:
>>> From: Yulia Vilensky <vilensky@compulab.co.il>
>>
>> Thanks for this, some comments below.
>>
>>> Signed-off-by: Yulia Vilensky <vilensky@compulab.co.il>
>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>> ---
>>>  drivers/power/Kconfig          |    4 +-
>>>  drivers/power/ds2782_battery.c |  217
>>> +++++++++++++++++++++++++++++++--------
>>>  include/linux/ds2782_battery.h |    8 ++
>>>  3 files changed, 182 insertions(+), 47 deletions(-)
>>>  create mode 100644 include/linux/ds2782_battery.h
>>>
>>> diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig
>>> index faaa9b4..d8b8a19 100644
>>> --- a/drivers/power/Kconfig
>>> +++ b/drivers/power/Kconfig
>>> @@ -65,10 +65,10 @@ config BATTERY_DS2760
>>>        Say Y here to enable support for batteries with ds2760 chip.
>>>  
>>>  config BATTERY_DS2782
>>> -    tristate "DS2782 standalone gas-gauge"
>>> +    tristate "DS2782/DS2786 standalone gas-gauge"
>>>      depends on I2C
>>>      help
>>> -      Say Y here to enable support for the DS2782 standalone battery
>>> +      Say Y here to enable support for the DS2782/DS2786 standalone
>>> battery
>>
>> I have only used the DS2782 chip. Can we just change this to DS278x? May
>> as well change to CONFIG_BATTERY_DS278x while we are here.
>>
> 
> Shall we move ds2782_battery.c to ds278x_battery.c at the same time?
> Changing Kconfig invites the .c file move as well :)

Yes. Are there other gas-gauges in the same family?

>>>        gas-gauge.
>>>  
>>>  config BATTERY_PMU
>>> diff --git a/drivers/power/ds2782_battery.c
>>> b/drivers/power/ds2782_battery.c
>>> index 99c8997..0df49b4 100644
>>> --- a/drivers/power/ds2782_battery.c
>>> +++ b/drivers/power/ds2782_battery.c
>>> @@ -5,6 +5,8 @@
>>>   *
>>>   * Author: Ryan Mallon <ryan@bluewatersys.com>
>>>   *
>>> + * DS278 added by Yulia Vilensky <vilensky@compulab.co.il>
>>> + *
>>>   * This program is free software; you can redistribute it and/or modify
>>>   * it under the terms of the GNU General Public License version 2 as
>>>   * published by the Free Software Foundation.
>>> @@ -20,6 +22,7 @@
>>>  #include <linux/idr.h>
>>>  #include <linux/power_supply.h>
>>>  #include <linux/slab.h>
>>> +#include <linux/ds2782_battery.h>
>>>  
>>>  #define DS2782_REG_RARC        0x06    /* Remaining active relative
>>> capacity */
>>>  
>>> @@ -33,18 +36,39 @@
>>>  /* Current unit measurement in uA for a 1 milli-ohm sense resistor */
>>>  #define DS2782_CURRENT_UNITS    1563
>>>  
>>> -#define to_ds2782_info(x) container_of(x, struct ds2782_info, battery)
>>> +#define DS2786_REG_RARC        0x02    /* Remaining active relative
>>> capacity */
>>> +
>>> +#define DS2786_REG_VOLT_MSB    0x0c
>>> +#define DS2786_REG_TEMP_MSB    0x0a
>>> +#define DS2786_REG_CURRENT_MSB    0x0e
>>
>> #define DS278x_REG_CURRENT_MSB        0x0e
>>
>> Its the same register on both chips, no need for two #defines. Same of
>> the other registers, except for RARC.
> 
> Ok.
> 
>>> +
>>> +#define DS2786_CURRENT_UNITS    25
>>
>> If we make this a member of the info structure, we can easily use it in
>> calculations for both chips, ie
>>
>>      current_uA = raw * (info->current_units / sense_res);
> 
> not quite, see comments below.
>     
>>> +#define DS278X_SIGN_BIT_MASK16  0x8000
>>> +
>>> +struct ds278x_info;
>>>  
>>> -struct ds2782_info {
>>> +struct ds278x_battery_ops {
>>> +    int    (*get_temp)(struct ds278x_info *info, int *temp);
>>> +    int    (*get_current)(struct ds278x_info *info, int *current_uA);
>>> +    int    (*get_voltage)(struct ds278x_info *info, int *voltage_uA);
>>> +    int    (*get_capacity)(struct ds278x_info *info, int *capacity_uA);
>>> +};
>>> +
> 
> [ snip ]
> 
>>>  
>>> -static int ds2782_get_status(struct ds2782_info *info, int *status)
>>> +static int ds2786_get_temp(struct ds278x_info *info, int *temp)
>>> +{
>>> +    s16 raw;
>>> +    int err;
>>> +
>>> +    /*
>>> +     * Temperature is measured in units of 0.125 degrees celcius, the
>>> +     * power_supply class measures temperature in tenths of degrees
>>> +     * celsius. The temperature value is stored as a 10 bit number,
>>> plus
>>> +     * sign in the upper bits of a 16 bit register.
>>> +     */
>>> +    err = ds278x_read_reg16(info, DS2786_REG_TEMP_MSB, &raw);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    if (raw &  DS278X_SIGN_BIT_MASK16)
>>> +        *temp = -(((~raw >> 5)+1) * 125)/100;
>>> +    else
>>> +        *temp = ((raw >> 5) * 125)/100;
>>> +
>>> +    return 0;
>>> +}
>>
>> This is basically the same as the ds2782 version. See Jean's comments on
>> my original patch about the sign math:
>> http://www.mail-archive.com/linux-i2c@vger.kernel.org/msg01220.html
> 
> Ok, we'll fix the math
> 
>>> +static int ds2786_get_current(struct ds278x_info *info, int
>>> *current_uA)
>>> +{
>>> +    int err;
>>> +    s16 raw;
>>> +
>>> +    err = ds278x_read_reg16(info, DS2786_REG_CURRENT_MSB, &raw);
>>> +    if (err)
>>> +        return err;
>>> +
>>> +    if (raw &  DS278X_SIGN_BIT_MASK16)
>>> +        *current_uA = -(((~raw >> 4)+1) *
>>> +                   (DS2786_CURRENT_UNITS / info->rsns));
>>> +    else
>>> +        *current_uA = (raw >> 4) *
>>> +                  (DS2786_CURRENT_UNITS / info->rsns);
>>> +    return 0;
>>
>> Can we combine the implementations of the get_current function? Both
>> have get rsns (though in different ways) and eventually divide the
>> current register by the rsns value? Possibly move the get_rsns into
>> separate battery ops and attempt to coalesce these?
> 
> It's possible to have get_rsns and coalesce _get_current for both chips.
> However, ds2872 and ds2786 need slightly different formula for current
> calculations. Part of these calculations can be wrapped into
> ds2786_get_rsns, but it makes the later really not clear to follow.

Fair enough. I just didn't want to have two versions of each function,
that were doing almost the same thing. If you think it is
cleaner/clearer to have separate functions, then stick with that.

~Ryan

-- 
Bluewater Systems Ltd - ARM Technology Solution Centre

Ryan Mallon         		5 Amuri Park, 404 Barbadoes St
ryan@bluewatersys.com         	PO Box 13 889, Christchurch 8013
http://www.bluewatersys.com	New Zealand
Phone: +64 3 3779127		Freecall: Australia 1800 148 751
Fax:   +64 3 3779135			  USA 1800 261 2934