| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4BD4A9B4.1010200@bluewatersys.com>
Date: Mon, 26 Apr 2010 08:44:36 +1200
From: Ryan Mallon <ryan@...ewatersys.com>
To: Mike Rapoport <mike@...pulab.co.il>
CC: Anton Vorontsov <cbouatmailru@...il.com>,
linux-kernel@...r.kernel.org,
Yulia Vilensky <vilensky@...pulab.co.il>
Subject: Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
Mike Rapoport wrote:
> Hi Ryan,
>
> Ryan Mallon wrote:
>> Mike Rapoport wrote:
>>> From: Yulia Vilensky <vilensky@...pulab.co.il>
>>
>> Thanks for this, some comments below.
>>
>>> Signed-off-by: Yulia Vilensky <vilensky@...pulab.co.il>
>>> Signed-off-by: Mike Rapoport <mike@...pulab.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@...ewatersys.com>
>>> *
>>> + * DS278 added by Yulia Vilensky <vilensky@...pulab.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@...ewatersys.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists