lkml.org 
[lkml]   [2010]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] ds2782_battery: add support for ds2786 battery gas gauge
    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, &current_uA);
    > + err = info->ops->get_current(info, &current_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


    \
     
     \ /
      Last update: 2010-04-22 23:27    [W:0.059 / U:34.236 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site