Mike Rapoport wrote:
quoted text > From: Yulia Vilensky <vilensky@compulab.co.il>
Thanks for this, some comments below.
quoted text > 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.
quoted text > 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.
quoted text > +
> +#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);
quoted text > +#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
quoted text > +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?
quoted text > +}
> +
> +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.
quoted text > +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.
quoted text > +
> +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
--
unsubscribe notice To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
majordomo@vger.kernel.org
More majordomo info at
http://vger.kernel.org/majordomo-info.html
Please read the FAQ at
http://www.tux.org/lkml/ Messages in current thread:
Re: [PATCH] ds2782_battery: add support for ds2786 battery ... , Ryan Mallon , (Thu Apr 22, 2:24 pm)
