* [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
@ 2010-04-22 6:52 Mike Rapoport
2010-04-22 7:14 ` Anton Vorontsov
0 siblings, 1 reply; 7+ messages in thread
From: Mike Rapoport @ 2010-04-22 6:52 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: Ryan Mallon, linux-kernel, Yulia Vilensky, Mike Rapoport
From: Yulia Vilensky <vilensky@compulab.co.il>
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 | 216 +++++++++++++++++++++++++++++++--------
2 files changed, 173 insertions(+), 47 deletions(-)
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
gas-gauge.
config BATTERY_PMU
diff --git a/drivers/power/ds2782_battery.c b/drivers/power/ds2782_battery.c
index 99c8997..63d47d5 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.
@@ -33,18 +35,40 @@
/* 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 DS2786_CURRENT_UNITS 25
+
+#define DS278X_SIGN_BIT_MASK16 0x8000
+
+struct ds278x_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);
+
+};
+
+#define to_ds278x_info(x) container_of(x, struct ds278x_info, battery)
-struct ds2782_info {
+struct ds278x_info {
struct i2c_client *client;
struct power_supply battery;
+ struct ds278x_battery_ops *ops;
int id;
+ int rsns;
};
static DEFINE_IDR(battery_id);
static DEFINE_MUTEX(battery_lock);
-static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
+static inline int ds278x_read_reg(struct ds278x_info *info, int reg, u8 *val)
{
int ret;
@@ -58,7 +82,7 @@ static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
return 0;
}
-static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
+static inline int ds278x_read_reg16(struct ds278x_info *info, int reg_msb,
s16 *val)
{
int ret;
@@ -73,7 +97,7 @@ static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
return 0;
}
-static int ds2782_get_temp(struct ds2782_info *info, int *temp)
+static int ds2782_get_temp(struct ds278x_info *info, int *temp)
{
s16 raw;
int err;
@@ -84,14 +108,14 @@ static int ds2782_get_temp(struct ds2782_info *info, int *temp)
* celsius. The temperature value is stored as a 10 bit number, plus
* sign in the upper bits of a 16 bit register.
*/
- err = ds2782_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
if (err)
return err;
*temp = ((raw / 32) * 125) / 100;
return 0;
}
-static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
+static int ds2782_get_current(struct ds278x_info *info, int *current_uA)
{
int sense_res;
int err;
@@ -102,7 +126,7 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
* The units of measurement for current are dependent on the value of
* the sense resistor.
*/
- err = ds2782_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
+ err = ds278x_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
if (err)
return err;
if (sense_res_raw == 0) {
@@ -113,14 +137,14 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
dev_dbg(&info->client->dev, "sense resistor = %d milli-ohms\n",
sense_res);
- err = ds2782_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
if (err)
return err;
*current_uA = raw * (DS2782_CURRENT_UNITS / sense_res);
return 0;
}
-static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
+static int ds2782_get_voltage(struct ds278x_info *info, int *voltage_uA)
{
s16 raw;
int err;
@@ -129,36 +153,110 @@ static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
* Voltage is measured in units of 4.88mV. The voltage is stored as
* a 10-bit number plus sign, in the upper bits of a 16-bit register
*/
- err = ds2782_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
if (err)
return err;
*voltage_uA = (raw / 32) * 4800;
return 0;
}
-static int ds2782_get_capacity(struct ds2782_info *info, int *capacity)
+static int ds2782_get_capacity(struct ds278x_info *info, int *capacity)
{
int err;
u8 raw;
- err = ds2782_read_reg(info, DS2782_REG_RARC, &raw);
+ err = ds278x_read_reg(info, DS2782_REG_RARC, &raw);
if (err)
return err;
*capacity = raw;
return raw;
}
-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;
+}
+
+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;
+}
+
+static int ds2786_get_voltage(struct ds278x_info *info, int *voltage_uA)
+{
+ s16 raw;
+ int err;
+
+ /*
+ * Voltage is measured in units of 1.22mV. The voltage 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_VOLT_MSB, &raw);
+ if (err)
+ return err;
+
+ if (raw & DS278X_SIGN_BIT_MASK16)
+ *voltage_uA = -(((~raw >> 3)+1) * 1220);
+ else
+ *voltage_uA = (raw >> 3) * 1220;
+ return 0;
+}
+
+static int ds2786_get_capacity(struct ds278x_info *info, int *capacity)
+{
+ int err;
+ u8 raw;
+
+ err = ds278x_read_reg(info, DS2786_REG_RARC, &raw);
+ if (err)
+ return err;
+ /* Relative capacity is displayed with resolution 0.5 % */
+ *capacity = raw/2 ;
+ return 0;
+}
+
+static int ds278x_get_status(struct ds278x_info *info, int *status)
{
int err;
int current_uA;
int capacity;
- err = ds2782_get_current(info, ¤t_uA);
+ err = info->ops->get_current(info, ¤t_uA);
if (err)
return err;
- err = ds2782_get_capacity(info, &capacity);
+ err = info->ops->get_capacity(info, &capacity);
if (err)
return err;
@@ -174,32 +272,32 @@ static int ds2782_get_status(struct ds2782_info *info, int *status)
return 0;
}
-static int ds2782_battery_get_property(struct power_supply *psy,
+static int ds278x_battery_get_property(struct power_supply *psy,
enum power_supply_property prop,
union power_supply_propval *val)
{
- struct ds2782_info *info = to_ds2782_info(psy);
+ struct ds278x_info *info = to_ds278x_info(psy);
int ret;
switch (prop) {
case POWER_SUPPLY_PROP_STATUS:
- ret = ds2782_get_status(info, &val->intval);
+ ret = ds278x_get_status(info, &val->intval);
break;
case POWER_SUPPLY_PROP_CAPACITY:
- ret = ds2782_get_capacity(info, &val->intval);
+ ret = info->ops->get_capacity(info, &val->intval);
break;
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
- ret = ds2782_get_voltage(info, &val->intval);
+ ret = info->ops->get_voltage(info, &val->intval);
break;
case POWER_SUPPLY_PROP_CURRENT_NOW:
- ret = ds2782_get_current(info, &val->intval);
+ ret = info->ops->get_current(info, &val->intval);
break;
case POWER_SUPPLY_PROP_TEMP:
- ret = ds2782_get_temp(info, &val->intval);
+ ret = info->ops->get_temp(info, &val->intval);
break;
default:
@@ -209,7 +307,7 @@ static int ds2782_battery_get_property(struct power_supply *psy,
return ret;
}
-static enum power_supply_property ds2782_battery_props[] = {
+static enum power_supply_property ds278x_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_CAPACITY,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -217,18 +315,18 @@ static enum power_supply_property ds2782_battery_props[] = {
POWER_SUPPLY_PROP_TEMP,
};
-static void ds2782_power_supply_init(struct power_supply *battery)
+static void ds278x_power_supply_init(struct power_supply *battery)
{
battery->type = POWER_SUPPLY_TYPE_BATTERY;
- battery->properties = ds2782_battery_props;
- battery->num_properties = ARRAY_SIZE(ds2782_battery_props);
- battery->get_property = ds2782_battery_get_property;
+ battery->properties = ds278x_battery_props;
+ battery->num_properties = ARRAY_SIZE(ds278x_battery_props);
+ battery->get_property = ds278x_battery_get_property;
battery->external_power_changed = NULL;
}
-static int ds2782_battery_remove(struct i2c_client *client)
+static int ds278x_battery_remove(struct i2c_client *client)
{
- struct ds2782_info *info = i2c_get_clientdata(client);
+ struct ds278x_info *info = i2c_get_clientdata(client);
power_supply_unregister(&info->battery);
kfree(info->battery.name);
@@ -243,13 +341,37 @@ static int ds2782_battery_remove(struct i2c_client *client)
return 0;
}
-static int ds2782_battery_probe(struct i2c_client *client,
+static struct ds278x_battery_ops ds278x_ops[] = {
+ [0] = {
+ .get_temp = ds2782_get_temp,
+ .get_current = ds2782_get_current,
+ .get_voltage = ds2782_get_voltage,
+ .get_capacity = ds2782_get_capacity,
+ },
+ [1] = {
+ .get_temp = ds2786_get_temp,
+ .get_current = ds2786_get_current,
+ .get_voltage = ds2786_get_voltage,
+ .get_capacity = ds2786_get_capacity,
+ }
+};
+
+static int ds278x_battery_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- struct ds2782_info *info;
+ struct ds278x_info *info;
int ret;
int num;
+ /*
+ * ds2786 should have the sense resistor value set
+ * in the platform data .
+ */
+ if (id->driver_data == 1 && client->dev.platform_data == 0) {
+ dev_err(&client->dev, "missing platform data for ds2786\n");
+ return -EINVAL;
+ }
+
/* Get an ID for this battery */
ret = idr_pre_get(&battery_id, GFP_KERNEL);
if (ret == 0) {
@@ -269,7 +391,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
goto fail_info;
}
- info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num);
+ info->battery.name = kasprintf(GFP_KERNEL, "ds278x-%d", num);
if (!info->battery.name) {
ret = -ENOMEM;
goto fail_name;
@@ -277,7 +399,10 @@ static int ds2782_battery_probe(struct i2c_client *client,
i2c_set_clientdata(client, info);
info->client = client;
- ds2782_power_supply_init(&info->battery);
+ info->id = num;
+ info->ops = &ds278x_ops[id->driver_data];
+ info->rsns = *((int *)info->client->dev.platform_data);
+ ds278x_power_supply_init(&info->battery);
ret = power_supply_register(&client->dev, &info->battery);
if (ret) {
@@ -300,31 +425,32 @@ fail_id:
return ret;
}
-static const struct i2c_device_id ds2782_id[] = {
+static const struct i2c_device_id ds278x_id[] = {
{"ds2782", 0},
+ {"ds2786", 1},
{},
};
-static struct i2c_driver ds2782_battery_driver = {
+static struct i2c_driver ds278x_battery_driver = {
.driver = {
.name = "ds2782-battery",
},
- .probe = ds2782_battery_probe,
- .remove = ds2782_battery_remove,
- .id_table = ds2782_id,
+ .probe = ds278x_battery_probe,
+ .remove = ds278x_battery_remove,
+ .id_table = ds278x_id,
};
-static int __init ds2782_init(void)
+static int __init ds278x_init(void)
{
- return i2c_add_driver(&ds2782_battery_driver);
+ return i2c_add_driver(&ds278x_battery_driver);
}
-module_init(ds2782_init);
+module_init(ds278x_init);
-static void __exit ds2782_exit(void)
+static void __exit ds278x_exit(void)
{
- i2c_del_driver(&ds2782_battery_driver);
+ i2c_del_driver(&ds278x_battery_driver);
}
-module_exit(ds2782_exit);
+module_exit(ds278x_exit);
MODULE_AUTHOR("Ryan Mallon <ryan@bluewatersys.com>");
MODULE_DESCRIPTION("Maxim/Dallas DS2782 Stand-Alone Fuel Gauage IC driver");
--
1.6.6.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-22 6:52 [PATCH] ds2782_battery: add support for ds2786 battery gas gauge Mike Rapoport
@ 2010-04-22 7:14 ` Anton Vorontsov
2010-04-22 7:35 ` Mike Rapoport
0 siblings, 1 reply; 7+ messages in thread
From: Anton Vorontsov @ 2010-04-22 7:14 UTC (permalink / raw)
To: Mike Rapoport; +Cc: Ryan Mallon, linux-kernel, Yulia Vilensky
Hi,
On Thu, Apr 22, 2010 at 09:52:52AM +0300, Mike Rapoport wrote:
> From: Yulia Vilensky <vilensky@compulab.co.il>
>
> Signed-off-by: Yulia Vilensky <vilensky@compulab.co.il>
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> ---
Thanks for the patch, looks good overall. Few comments below.
[...]
> +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);
> +
Unneeded empty line.
[...]
>
> - info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num);
> + info->battery.name = kasprintf(GFP_KERNEL, "ds278x-%d", num);
I'm a bit worried about this, as this will change sysfs stuff.
I tend to think that this won't cause any real issues, but just
to be on a safe side, can we change that to
kasprintf(GFP_KERNEL, "%s-%d", client->name, num); ?
> if (!info->battery.name) {
> ret = -ENOMEM;
> goto fail_name;
> @@ -277,7 +399,10 @@ static int ds2782_battery_probe(struct i2c_client *client,
>
> i2c_set_clientdata(client, info);
> info->client = client;
> - ds2782_power_supply_init(&info->battery);
> + info->id = num;
> + info->ops = &ds278x_ops[id->driver_data];
> + info->rsns = *((int *)info->client->dev.platform_data);
Please introduce 'struct ds278x_platform_data {int rsns;};'
for this. Place it into include/linux/ds2782_battery.h.
Thanks!
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-22 7:14 ` Anton Vorontsov
@ 2010-04-22 7:35 ` Mike Rapoport
2010-04-22 21:24 ` Ryan Mallon
0 siblings, 1 reply; 7+ messages in thread
From: Mike Rapoport @ 2010-04-22 7:35 UTC (permalink / raw)
To: Anton Vorontsov; +Cc: Ryan Mallon, linux-kernel, Yulia Vilensky, Mike Rapoport
From: Yulia Vilensky <vilensky@compulab.co.il>
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
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 DS2786_CURRENT_UNITS 25
+
+#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);
+};
+
+#define to_ds278x_info(x) container_of(x, struct ds278x_info, battery)
+
+struct ds278x_info {
struct i2c_client *client;
struct power_supply battery;
+ struct ds278x_battery_ops *ops;
int id;
+ int rsns;
};
static DEFINE_IDR(battery_id);
static DEFINE_MUTEX(battery_lock);
-static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
+static inline int ds278x_read_reg(struct ds278x_info *info, int reg, u8 *val)
{
int ret;
@@ -58,7 +82,7 @@ static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
return 0;
}
-static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
+static inline int ds278x_read_reg16(struct ds278x_info *info, int reg_msb,
s16 *val)
{
int ret;
@@ -73,7 +97,7 @@ static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
return 0;
}
-static int ds2782_get_temp(struct ds2782_info *info, int *temp)
+static int ds2782_get_temp(struct ds278x_info *info, int *temp)
{
s16 raw;
int err;
@@ -84,14 +108,14 @@ static int ds2782_get_temp(struct ds2782_info *info, int *temp)
* celsius. The temperature value is stored as a 10 bit number, plus
* sign in the upper bits of a 16 bit register.
*/
- err = ds2782_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
if (err)
return err;
*temp = ((raw / 32) * 125) / 100;
return 0;
}
-static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
+static int ds2782_get_current(struct ds278x_info *info, int *current_uA)
{
int sense_res;
int err;
@@ -102,7 +126,7 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
* The units of measurement for current are dependent on the value of
* the sense resistor.
*/
- err = ds2782_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
+ err = ds278x_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
if (err)
return err;
if (sense_res_raw == 0) {
@@ -113,14 +137,14 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
dev_dbg(&info->client->dev, "sense resistor = %d milli-ohms\n",
sense_res);
- err = ds2782_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
if (err)
return err;
*current_uA = raw * (DS2782_CURRENT_UNITS / sense_res);
return 0;
}
-static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
+static int ds2782_get_voltage(struct ds278x_info *info, int *voltage_uA)
{
s16 raw;
int err;
@@ -129,36 +153,110 @@ static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
* Voltage is measured in units of 4.88mV. The voltage is stored as
* a 10-bit number plus sign, in the upper bits of a 16-bit register
*/
- err = ds2782_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
+ err = ds278x_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
if (err)
return err;
*voltage_uA = (raw / 32) * 4800;
return 0;
}
-static int ds2782_get_capacity(struct ds2782_info *info, int *capacity)
+static int ds2782_get_capacity(struct ds278x_info *info, int *capacity)
{
int err;
u8 raw;
- err = ds2782_read_reg(info, DS2782_REG_RARC, &raw);
+ err = ds278x_read_reg(info, DS2782_REG_RARC, &raw);
if (err)
return err;
*capacity = raw;
return raw;
}
-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;
+}
+
+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;
+}
+
+static int ds2786_get_voltage(struct ds278x_info *info, int *voltage_uA)
+{
+ s16 raw;
+ int err;
+
+ /*
+ * Voltage is measured in units of 1.22mV. The voltage 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_VOLT_MSB, &raw);
+ if (err)
+ return err;
+
+ if (raw & DS278X_SIGN_BIT_MASK16)
+ *voltage_uA = -(((~raw >> 3)+1) * 1220);
+ else
+ *voltage_uA = (raw >> 3) * 1220;
+ return 0;
+}
+
+static int ds2786_get_capacity(struct ds278x_info *info, int *capacity)
+{
+ int err;
+ u8 raw;
+
+ err = ds278x_read_reg(info, DS2786_REG_RARC, &raw);
+ if (err)
+ return err;
+ /* Relative capacity is displayed with resolution 0.5 % */
+ *capacity = raw/2 ;
+ return 0;
+}
+
+static int ds278x_get_status(struct ds278x_info *info, int *status)
{
int err;
int current_uA;
int capacity;
- err = ds2782_get_current(info, ¤t_uA);
+ err = info->ops->get_current(info, ¤t_uA);
if (err)
return err;
- err = ds2782_get_capacity(info, &capacity);
+ err = info->ops->get_capacity(info, &capacity);
if (err)
return err;
@@ -174,32 +272,32 @@ static int ds2782_get_status(struct ds2782_info *info, int *status)
return 0;
}
-static int ds2782_battery_get_property(struct power_supply *psy,
+static int ds278x_battery_get_property(struct power_supply *psy,
enum power_supply_property prop,
union power_supply_propval *val)
{
- struct ds2782_info *info = to_ds2782_info(psy);
+ struct ds278x_info *info = to_ds278x_info(psy);
int ret;
switch (prop) {
case POWER_SUPPLY_PROP_STATUS:
- ret = ds2782_get_status(info, &val->intval);
+ ret = ds278x_get_status(info, &val->intval);
break;
case POWER_SUPPLY_PROP_CAPACITY:
- ret = ds2782_get_capacity(info, &val->intval);
+ ret = info->ops->get_capacity(info, &val->intval);
break;
case POWER_SUPPLY_PROP_VOLTAGE_NOW:
- ret = ds2782_get_voltage(info, &val->intval);
+ ret = info->ops->get_voltage(info, &val->intval);
break;
case POWER_SUPPLY_PROP_CURRENT_NOW:
- ret = ds2782_get_current(info, &val->intval);
+ ret = info->ops->get_current(info, &val->intval);
break;
case POWER_SUPPLY_PROP_TEMP:
- ret = ds2782_get_temp(info, &val->intval);
+ ret = info->ops->get_temp(info, &val->intval);
break;
default:
@@ -209,7 +307,7 @@ static int ds2782_battery_get_property(struct power_supply *psy,
return ret;
}
-static enum power_supply_property ds2782_battery_props[] = {
+static enum power_supply_property ds278x_battery_props[] = {
POWER_SUPPLY_PROP_STATUS,
POWER_SUPPLY_PROP_CAPACITY,
POWER_SUPPLY_PROP_VOLTAGE_NOW,
@@ -217,18 +315,18 @@ static enum power_supply_property ds2782_battery_props[] = {
POWER_SUPPLY_PROP_TEMP,
};
-static void ds2782_power_supply_init(struct power_supply *battery)
+static void ds278x_power_supply_init(struct power_supply *battery)
{
battery->type = POWER_SUPPLY_TYPE_BATTERY;
- battery->properties = ds2782_battery_props;
- battery->num_properties = ARRAY_SIZE(ds2782_battery_props);
- battery->get_property = ds2782_battery_get_property;
+ battery->properties = ds278x_battery_props;
+ battery->num_properties = ARRAY_SIZE(ds278x_battery_props);
+ battery->get_property = ds278x_battery_get_property;
battery->external_power_changed = NULL;
}
-static int ds2782_battery_remove(struct i2c_client *client)
+static int ds278x_battery_remove(struct i2c_client *client)
{
- struct ds2782_info *info = i2c_get_clientdata(client);
+ struct ds278x_info *info = i2c_get_clientdata(client);
power_supply_unregister(&info->battery);
kfree(info->battery.name);
@@ -243,13 +341,38 @@ static int ds2782_battery_remove(struct i2c_client *client)
return 0;
}
-static int ds2782_battery_probe(struct i2c_client *client,
+static struct ds278x_battery_ops ds278x_ops[] = {
+ [0] = {
+ .get_temp = ds2782_get_temp,
+ .get_current = ds2782_get_current,
+ .get_voltage = ds2782_get_voltage,
+ .get_capacity = ds2782_get_capacity,
+ },
+ [1] = {
+ .get_temp = ds2786_get_temp,
+ .get_current = ds2786_get_current,
+ .get_voltage = ds2786_get_voltage,
+ .get_capacity = ds2786_get_capacity,
+ }
+};
+
+static int ds278x_battery_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
- struct ds2782_info *info;
+ struct ds278x_platform_data *pdata = client->dev.platform_data;
+ struct ds278x_info *info;
int ret;
int num;
+ /*
+ * ds2786 should have the sense resistor value set
+ * in the platform data .
+ */
+ if (id->driver_data == 1 && pdata == 0) {
+ dev_err(&client->dev, "missing platform data for ds2786\n");
+ return -EINVAL;
+ }
+
/* Get an ID for this battery */
ret = idr_pre_get(&battery_id, GFP_KERNEL);
if (ret == 0) {
@@ -269,7 +392,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
goto fail_info;
}
- info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num);
+ info->battery.name = kasprintf(GFP_KERNEL, "%s-%d", client->name, num);
if (!info->battery.name) {
ret = -ENOMEM;
goto fail_name;
@@ -277,7 +400,10 @@ static int ds2782_battery_probe(struct i2c_client *client,
i2c_set_clientdata(client, info);
info->client = client;
- ds2782_power_supply_init(&info->battery);
+ info->id = num;
+ info->ops = &ds278x_ops[id->driver_data];
+ info->rsns = pdata->rsns;
+ ds278x_power_supply_init(&info->battery);
ret = power_supply_register(&client->dev, &info->battery);
if (ret) {
@@ -300,31 +426,32 @@ fail_id:
return ret;
}
-static const struct i2c_device_id ds2782_id[] = {
+static const struct i2c_device_id ds278x_id[] = {
{"ds2782", 0},
+ {"ds2786", 1},
{},
};
-static struct i2c_driver ds2782_battery_driver = {
+static struct i2c_driver ds278x_battery_driver = {
.driver = {
.name = "ds2782-battery",
},
- .probe = ds2782_battery_probe,
- .remove = ds2782_battery_remove,
- .id_table = ds2782_id,
+ .probe = ds278x_battery_probe,
+ .remove = ds278x_battery_remove,
+ .id_table = ds278x_id,
};
-static int __init ds2782_init(void)
+static int __init ds278x_init(void)
{
- return i2c_add_driver(&ds2782_battery_driver);
+ return i2c_add_driver(&ds278x_battery_driver);
}
-module_init(ds2782_init);
+module_init(ds278x_init);
-static void __exit ds2782_exit(void)
+static void __exit ds278x_exit(void)
{
- i2c_del_driver(&ds2782_battery_driver);
+ i2c_del_driver(&ds278x_battery_driver);
}
-module_exit(ds2782_exit);
+module_exit(ds278x_exit);
MODULE_AUTHOR("Ryan Mallon <ryan@bluewatersys.com>");
MODULE_DESCRIPTION("Maxim/Dallas DS2782 Stand-Alone Fuel Gauage IC driver");
diff --git a/include/linux/ds2782_battery.h b/include/linux/ds2782_battery.h
new file mode 100644
index 0000000..b4e281f
--- /dev/null
+++ b/include/linux/ds2782_battery.h
@@ -0,0 +1,8 @@
+#ifndef __LINUX_DS2782_BATTERY_H
+#define __LINUX_DS2782_BATTERY_H
+
+struct ds278x_platform_data {
+ int rsns;
+};
+
+#endif
--
1.6.6.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-22 7:35 ` Mike Rapoport
@ 2010-04-22 21:24 ` Ryan Mallon
2010-04-25 15:33 ` Mike Rapoport
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Mallon @ 2010-04-22 21:24 UTC (permalink / raw)
To: Mike Rapoport; +Cc: Anton Vorontsov, linux-kernel, Yulia Vilensky
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.
> 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.
> +
> +#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);
> +#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);
> +};
> +
> +#define to_ds278x_info(x) container_of(x, struct ds278x_info, battery)
> +
> +struct ds278x_info {
> struct i2c_client *client;
> struct power_supply battery;
> + struct ds278x_battery_ops *ops;
> int id;
> + int rsns;
> };
>
> static DEFINE_IDR(battery_id);
> static DEFINE_MUTEX(battery_lock);
>
> -static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
> +static inline int ds278x_read_reg(struct ds278x_info *info, int reg, u8 *val)
> {
> int ret;
>
> @@ -58,7 +82,7 @@ static inline int ds2782_read_reg(struct ds2782_info *info, int reg, u8 *val)
> return 0;
> }
>
> -static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
> +static inline int ds278x_read_reg16(struct ds278x_info *info, int reg_msb,
> s16 *val)
> {
> int ret;
> @@ -73,7 +97,7 @@ static inline int ds2782_read_reg16(struct ds2782_info *info, int reg_msb,
> return 0;
> }
>
> -static int ds2782_get_temp(struct ds2782_info *info, int *temp)
> +static int ds2782_get_temp(struct ds278x_info *info, int *temp)
> {
> s16 raw;
> int err;
> @@ -84,14 +108,14 @@ static int ds2782_get_temp(struct ds2782_info *info, int *temp)
> * celsius. The temperature value is stored as a 10 bit number, plus
> * sign in the upper bits of a 16 bit register.
> */
> - err = ds2782_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
> + err = ds278x_read_reg16(info, DS2782_REG_TEMP_MSB, &raw);
> if (err)
> return err;
> *temp = ((raw / 32) * 125) / 100;
> return 0;
> }
>
> -static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
> +static int ds2782_get_current(struct ds278x_info *info, int *current_uA)
> {
> int sense_res;
> int err;
> @@ -102,7 +126,7 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
> * The units of measurement for current are dependent on the value of
> * the sense resistor.
> */
> - err = ds2782_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
> + err = ds278x_read_reg(info, DS2782_REG_RSNSP, &sense_res_raw);
> if (err)
> return err;
> if (sense_res_raw == 0) {
> @@ -113,14 +137,14 @@ static int ds2782_get_current(struct ds2782_info *info, int *current_uA)
>
> dev_dbg(&info->client->dev, "sense resistor = %d milli-ohms\n",
> sense_res);
> - err = ds2782_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
> + err = ds278x_read_reg16(info, DS2782_REG_CURRENT_MSB, &raw);
> if (err)
> return err;
> *current_uA = raw * (DS2782_CURRENT_UNITS / sense_res);
> return 0;
> }
>
> -static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
> +static int ds2782_get_voltage(struct ds278x_info *info, int *voltage_uA)
> {
> s16 raw;
> int err;
> @@ -129,36 +153,110 @@ static int ds2782_get_voltage(struct ds2782_info *info, int *voltage_uA)
> * Voltage is measured in units of 4.88mV. The voltage is stored as
> * a 10-bit number plus sign, in the upper bits of a 16-bit register
> */
> - err = ds2782_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
> + err = ds278x_read_reg16(info, DS2782_REG_VOLT_MSB, &raw);
> if (err)
> return err;
> *voltage_uA = (raw / 32) * 4800;
> return 0;
> }
>
> -static int ds2782_get_capacity(struct ds2782_info *info, int *capacity)
> +static int ds2782_get_capacity(struct ds278x_info *info, int *capacity)
> {
> int err;
> u8 raw;
>
> - err = ds2782_read_reg(info, DS2782_REG_RARC, &raw);
> + err = ds278x_read_reg(info, DS2782_REG_RARC, &raw);
> if (err)
> return err;
> *capacity = raw;
> return raw;
> }
>
> -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
> +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?
> +}
> +
> +static int ds2786_get_voltage(struct ds278x_info *info, int *voltage_uA)
> +{
> + s16 raw;
> + int err;
> +
> + /*
> + * Voltage is measured in units of 1.22mV. The voltage 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_VOLT_MSB, &raw);
> + if (err)
> + return err;
> +
> + if (raw & DS278X_SIGN_BIT_MASK16)
> + *voltage_uA = -(((~raw >> 3)+1) * 1220);
> + else
> + *voltage_uA = (raw >> 3) * 1220;
> + return 0;
> +}
Again, if we move the multiplier value (1220 for ds2786 and 4800 for
ds2782) to the info structure then we can use the same code to get the
voltage for both chips.
> +static int ds2786_get_capacity(struct ds278x_info *info, int *capacity)
> +{
> + int err;
> + u8 raw;
> +
> + err = ds278x_read_reg(info, DS2786_REG_RARC, &raw);
> + if (err)
> + return err;
> + /* Relative capacity is displayed with resolution 0.5 % */
> + *capacity = raw/2 ;
> + return 0;
> +}
Same here, move the divider to the info structure (will be 1 for ds2782)
and combine the functions.
> +
> +static int ds278x_get_status(struct ds278x_info *info, int *status)
> {
> int err;
> int current_uA;
> int capacity;
>
> - err = ds2782_get_current(info, ¤t_uA);
> + err = info->ops->get_current(info, ¤t_uA);
> if (err)
> return err;
>
> - err = ds2782_get_capacity(info, &capacity);
> + err = info->ops->get_capacity(info, &capacity);
> if (err)
> return err;
>
> @@ -174,32 +272,32 @@ static int ds2782_get_status(struct ds2782_info *info, int *status)
> return 0;
> }
>
> -static int ds2782_battery_get_property(struct power_supply *psy,
> +static int ds278x_battery_get_property(struct power_supply *psy,
> enum power_supply_property prop,
> union power_supply_propval *val)
> {
> - struct ds2782_info *info = to_ds2782_info(psy);
> + struct ds278x_info *info = to_ds278x_info(psy);
> int ret;
>
> switch (prop) {
> case POWER_SUPPLY_PROP_STATUS:
> - ret = ds2782_get_status(info, &val->intval);
> + ret = ds278x_get_status(info, &val->intval);
> break;
>
> case POWER_SUPPLY_PROP_CAPACITY:
> - ret = ds2782_get_capacity(info, &val->intval);
> + ret = info->ops->get_capacity(info, &val->intval);
> break;
>
> case POWER_SUPPLY_PROP_VOLTAGE_NOW:
> - ret = ds2782_get_voltage(info, &val->intval);
> + ret = info->ops->get_voltage(info, &val->intval);
> break;
>
> case POWER_SUPPLY_PROP_CURRENT_NOW:
> - ret = ds2782_get_current(info, &val->intval);
> + ret = info->ops->get_current(info, &val->intval);
> break;
>
> case POWER_SUPPLY_PROP_TEMP:
> - ret = ds2782_get_temp(info, &val->intval);
> + ret = info->ops->get_temp(info, &val->intval);
> break;
>
> default:
> @@ -209,7 +307,7 @@ static int ds2782_battery_get_property(struct power_supply *psy,
> return ret;
> }
>
> -static enum power_supply_property ds2782_battery_props[] = {
> +static enum power_supply_property ds278x_battery_props[] = {
> POWER_SUPPLY_PROP_STATUS,
> POWER_SUPPLY_PROP_CAPACITY,
> POWER_SUPPLY_PROP_VOLTAGE_NOW,
> @@ -217,18 +315,18 @@ static enum power_supply_property ds2782_battery_props[] = {
> POWER_SUPPLY_PROP_TEMP,
> };
>
> -static void ds2782_power_supply_init(struct power_supply *battery)
> +static void ds278x_power_supply_init(struct power_supply *battery)
> {
> battery->type = POWER_SUPPLY_TYPE_BATTERY;
> - battery->properties = ds2782_battery_props;
> - battery->num_properties = ARRAY_SIZE(ds2782_battery_props);
> - battery->get_property = ds2782_battery_get_property;
> + battery->properties = ds278x_battery_props;
> + battery->num_properties = ARRAY_SIZE(ds278x_battery_props);
> + battery->get_property = ds278x_battery_get_property;
> battery->external_power_changed = NULL;
> }
>
> -static int ds2782_battery_remove(struct i2c_client *client)
> +static int ds278x_battery_remove(struct i2c_client *client)
> {
> - struct ds2782_info *info = i2c_get_clientdata(client);
> + struct ds278x_info *info = i2c_get_clientdata(client);
>
> power_supply_unregister(&info->battery);
> kfree(info->battery.name);
> @@ -243,13 +341,38 @@ static int ds2782_battery_remove(struct i2c_client *client)
> return 0;
> }
>
> -static int ds2782_battery_probe(struct i2c_client *client,
> +static struct ds278x_battery_ops ds278x_ops[] = {
> + [0] = {
> + .get_temp = ds2782_get_temp,
> + .get_current = ds2782_get_current,
> + .get_voltage = ds2782_get_voltage,
> + .get_capacity = ds2782_get_capacity,
> + },
> + [1] = {
> + .get_temp = ds2786_get_temp,
> + .get_current = ds2786_get_current,
> + .get_voltage = ds2786_get_voltage,
> + .get_capacity = ds2786_get_capacity,
> + }
> +};
> +
> +static int ds278x_battery_probe(struct i2c_client *client,
> const struct i2c_device_id *id)
> {
> - struct ds2782_info *info;
> + struct ds278x_platform_data *pdata = client->dev.platform_data;
> + struct ds278x_info *info;
> int ret;
> int num;
>
> + /*
> + * ds2786 should have the sense resistor value set
> + * in the platform data .
> + */
> + if (id->driver_data == 1 && pdata == 0) {
> + dev_err(&client->dev, "missing platform data for ds2786\n");
> + return -EINVAL;
> + }
> +
> /* Get an ID for this battery */
> ret = idr_pre_get(&battery_id, GFP_KERNEL);
> if (ret == 0) {
> @@ -269,7 +392,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
> goto fail_info;
> }
>
> - info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num);
> + info->battery.name = kasprintf(GFP_KERNEL, "%s-%d", client->name, num);
> if (!info->battery.name) {
> ret = -ENOMEM;
> goto fail_name;
> @@ -277,7 +400,10 @@ static int ds2782_battery_probe(struct i2c_client *client,
>
> i2c_set_clientdata(client, info);
> info->client = client;
> - ds2782_power_supply_init(&info->battery);
> + info->id = num;
> + info->ops = &ds278x_ops[id->driver_data];
> + info->rsns = pdata->rsns;
> + ds278x_power_supply_init(&info->battery);
>
> ret = power_supply_register(&client->dev, &info->battery);
> if (ret) {
> @@ -300,31 +426,32 @@ fail_id:
> return ret;
> }
>
> -static const struct i2c_device_id ds2782_id[] = {
> +static const struct i2c_device_id ds278x_id[] = {
> {"ds2782", 0},
> + {"ds2786", 1},
> {},
> };
>
> -static struct i2c_driver ds2782_battery_driver = {
> +static struct i2c_driver ds278x_battery_driver = {
> .driver = {
> .name = "ds2782-battery",
> },
> - .probe = ds2782_battery_probe,
> - .remove = ds2782_battery_remove,
> - .id_table = ds2782_id,
> + .probe = ds278x_battery_probe,
> + .remove = ds278x_battery_remove,
> + .id_table = ds278x_id,
> };
>
> -static int __init ds2782_init(void)
> +static int __init ds278x_init(void)
> {
> - return i2c_add_driver(&ds2782_battery_driver);
> + return i2c_add_driver(&ds278x_battery_driver);
> }
> -module_init(ds2782_init);
> +module_init(ds278x_init);
>
> -static void __exit ds2782_exit(void)
> +static void __exit ds278x_exit(void)
> {
> - i2c_del_driver(&ds2782_battery_driver);
> + i2c_del_driver(&ds278x_battery_driver);
> }
> -module_exit(ds2782_exit);
> +module_exit(ds278x_exit);
>
> MODULE_AUTHOR("Ryan Mallon <ryan@bluewatersys.com>");
> MODULE_DESCRIPTION("Maxim/Dallas DS2782 Stand-Alone Fuel Gauage IC driver");
> diff --git a/include/linux/ds2782_battery.h b/include/linux/ds2782_battery.h
> new file mode 100644
> index 0000000..b4e281f
> --- /dev/null
> +++ b/include/linux/ds2782_battery.h
> @@ -0,0 +1,8 @@
> +#ifndef __LINUX_DS2782_BATTERY_H
> +#define __LINUX_DS2782_BATTERY_H
> +
> +struct ds278x_platform_data {
> + int rsns;
> +};
> +
> +#endif
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-22 21:24 ` Ryan Mallon
@ 2010-04-25 15:33 ` Mike Rapoport
2010-04-25 20:44 ` Ryan Mallon
0 siblings, 1 reply; 7+ messages in thread
From: Mike Rapoport @ 2010-04-25 15:33 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Anton Vorontsov, linux-kernel, Yulia Vilensky, Mike Rapoport
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 :)
>> 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.
>> +}
>> +
>> +static int ds2786_get_voltage(struct ds278x_info *info, int *voltage_uA)
>> +{
>> + s16 raw;
>> + int err;
>> +
>> + /*
>> + * Voltage is measured in units of 1.22mV. The voltage 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_VOLT_MSB, &raw);
>> + if (err)
>> + return err;
>> +
>> + if (raw & DS278X_SIGN_BIT_MASK16)
>> + *voltage_uA = -(((~raw >> 3)+1) * 1220);
>> + else
>> + *voltage_uA = (raw >> 3) * 1220;
>> + return 0;
>> +}
>
> Again, if we move the multiplier value (1220 for ds2786 and 4800 for
> ds2782) to the info structure then we can use the same code to get the
> voltage for both chips.
We need to the divider to the info structure as well.
>> +static int ds2786_get_capacity(struct ds278x_info *info, int *capacity)
>> +{
>> + int err;
>> + u8 raw;
>> +
>> + err = ds278x_read_reg(info, DS2786_REG_RARC, &raw);
>> + if (err)
>> + return err;
>> + /* Relative capacity is displayed with resolution 0.5 % */
>> + *capacity = raw/2 ;
>> + return 0;
>> +}
>
> Same here, move the divider to the info structure (will be 1 for ds2782)
> and combine the functions.
Here again we need to add the divider and RARC register to the info
structure.
>> +
>> +static int ds278x_get_status(struct ds278x_info *info, int *status)
>> {
[ snip ]
>>
>> -static int ds2782_battery_probe(struct i2c_client *client,
>> +static struct ds278x_battery_ops ds278x_ops[] = {
>> + [0] = {
>> + .get_temp = ds2782_get_temp,
>> + .get_current = ds2782_get_current,
>> + .get_voltage = ds2782_get_voltage,
>> + .get_capacity = ds2782_get_capacity,
>> + },
>> + [1] = {
>> + .get_temp = ds2786_get_temp,
>> + .get_current = ds2786_get_current,
>> + .get_voltage = ds2786_get_voltage,
>> + .get_capacity = ds2786_get_capacity,
>> + }
>> +};
>> +
>> +static int ds278x_battery_probe(struct i2c_client *client,
>> const struct i2c_device_id *id)
>> {
>> - struct ds2782_info *info;
>> + struct ds278x_platform_data *pdata = client->dev.platform_data;
>> + struct ds278x_info *info;
>> int ret;
>> int num;
>>
>> + /*
>> + * ds2786 should have the sense resistor value set
>> + * in the platform data .
>> + */
>> + if (id->driver_data == 1 && pdata == 0) {
>> + dev_err(&client->dev, "missing platform data for ds2786\n");
>> + return -EINVAL;
>> + }
>> +
>> /* Get an ID for this battery */
>> ret = idr_pre_get(&battery_id, GFP_KERNEL);
>> if (ret == 0) {
>> @@ -269,7 +392,7 @@ static int ds2782_battery_probe(struct i2c_client *client,
>> goto fail_info;
>> }
>>
>> - info->battery.name = kasprintf(GFP_KERNEL, "ds2782-%d", num);
>> + info->battery.name = kasprintf(GFP_KERNEL, "%s-%d", client->name, num);
>> if (!info->battery.name) {
>> ret = -ENOMEM;
>> goto fail_name;
>> @@ -277,7 +400,10 @@ static int ds2782_battery_probe(struct i2c_client *client,
>>
>> i2c_set_clientdata(client, info);
>> info->client = client;
>> - ds2782_power_supply_init(&info->battery);
>> + info->id = num;
>> + info->ops = &ds278x_ops[id->driver_data];
Summarizing all the parameters that should be added to the info
structure we'd get here switch with lots of 'param = value' for each
case which seems to me less elegant and efficient than simple info->ops
assignment. I think keeping different functions for ds2782 and ds2786
keeps the code more readable and easier to follow.
>> + info->rsns = pdata->rsns;
>> + ds278x_power_supply_init(&info->battery);
>>
>> ret = power_supply_register(&client->dev, &info->battery);
>> if (ret) {
>> @@ -300,31 +426,32 @@ fail_id:
>> return ret;
>> }
>>
>> -static const struct i2c_device_id ds2782_id[] = {
>> +static const struct i2c_device_id ds278x_id[] = {
>> {"ds2782", 0},
>> + {"ds2786", 1},
>> {},
>> };
>>
>> -static struct i2c_driver ds2782_battery_driver = {
>> +static struct i2c_driver ds278x_battery_driver = {
>> .driver = {
>> .name = "ds2782-battery",
>> },
>> - .probe = ds2782_battery_probe,
>> - .remove = ds2782_battery_remove,
>> - .id_table = ds2782_id,
>> + .probe = ds278x_battery_probe,
>> + .remove = ds278x_battery_remove,
>> + .id_table = ds278x_id,
>> };
>>
>> -static int __init ds2782_init(void)
>> +static int __init ds278x_init(void)
>> {
>> - return i2c_add_driver(&ds2782_battery_driver);
>> + return i2c_add_driver(&ds278x_battery_driver);
>> }
>> -module_init(ds2782_init);
>> +module_init(ds278x_init);
>>
>> -static void __exit ds2782_exit(void)
>> +static void __exit ds278x_exit(void)
>> {
>> - i2c_del_driver(&ds2782_battery_driver);
>> + i2c_del_driver(&ds278x_battery_driver);
>> }
>> -module_exit(ds2782_exit);
>> +module_exit(ds278x_exit);
>>
>> MODULE_AUTHOR("Ryan Mallon <ryan@bluewatersys.com>");
>> MODULE_DESCRIPTION("Maxim/Dallas DS2782 Stand-Alone Fuel Gauage IC driver");
>> diff --git a/include/linux/ds2782_battery.h b/include/linux/ds2782_battery.h
>> new file mode 100644
>> index 0000000..b4e281f
>> --- /dev/null
>> +++ b/include/linux/ds2782_battery.h
>> @@ -0,0 +1,8 @@
>> +#ifndef __LINUX_DS2782_BATTERY_H
>> +#define __LINUX_DS2782_BATTERY_H
>> +
>> +struct ds278x_platform_data {
>> + int rsns;
>> +};
>> +
>> +#endif
>
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-25 15:33 ` Mike Rapoport
@ 2010-04-25 20:44 ` Ryan Mallon
2010-04-26 10:09 ` Anton Vorontsov
0 siblings, 1 reply; 7+ messages in thread
From: Ryan Mallon @ 2010-04-25 20:44 UTC (permalink / raw)
To: Mike Rapoport; +Cc: Anton Vorontsov, linux-kernel, Yulia Vilensky
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
2010-04-25 20:44 ` Ryan Mallon
@ 2010-04-26 10:09 ` Anton Vorontsov
0 siblings, 0 replies; 7+ messages in thread
From: Anton Vorontsov @ 2010-04-26 10:09 UTC (permalink / raw)
To: Ryan Mallon; +Cc: Mike Rapoport, linux-kernel, Yulia Vilensky
On Mon, Apr 26, 2010 at 08:44:36AM +1200, Ryan Mallon wrote:
[...]
> >>> 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?
Please don't, this is an unneeded churn. I'd won't bother with
renaming Kconfig symbol as well (and btw, that renaming will
break oldconfig, so I strongly suggest not to do it).
So, your current approch of changing the description and help
text is OK.
Thanks,
--
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-04-26 10:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-04-22 6:52 [PATCH] ds2782_battery: add support for ds2786 battery gas gauge Mike Rapoport
2010-04-22 7:14 ` Anton Vorontsov
2010-04-22 7:35 ` Mike Rapoport
2010-04-22 21:24 ` Ryan Mallon
2010-04-25 15:33 ` Mike Rapoport
2010-04-25 20:44 ` Ryan Mallon
2010-04-26 10:09 ` Anton Vorontsov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox