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

From: Mike Rapoport
Date: Sun Apr 25 2010 - 11:35:01 EST

Next message: Miles Lane: "Re: [PATCH] RCU: don't turn off lockdep when find suspicious rcu_dereference_check() usage"
Previous message: Dan Magenheimer: "RE: Frontswap [PATCH 0/4] (was Transcendent Memory): overview"
Next in thread: Ryan Mallon: "Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ryan,

Ryan Mallon wrote:

Mike Rapoport wrote:
From: Yulia Vilensky <vilensky@xxxxxxxxxxxxxx>

Thanks for this, some comments below.

Signed-off-by: Yulia Vilensky <vilensky@xxxxxxxxxxxxxx>
Signed-off-by: Mike Rapoport <mike@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxx>
*
+ * DS278 added by Yulia Vilensky <vilensky@xxxxxxxxxxxxxx>
+ *
* 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@xxxxxxxxxxxxxxx/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@xxxxxxxxxxxxxxxx>");
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Miles Lane: "Re: [PATCH] RCU: don't turn off lockdep when find suspicious rcu_dereference_check() usage"
Previous message: Dan Magenheimer: "RE: Frontswap [PATCH 0/4] (was Transcendent Memory): overview"
Next in thread: Ryan Mallon: "Re: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]