Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756624Ab1EKVVV (ORCPT ); Wed, 11 May 2011 17:21:21 -0400 Received: from mail.bluewatersys.com ([202.124.120.130]:42852 "EHLO hayes.bluewaternz.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1755889Ab1EKVVT (ORCPT ); Wed, 11 May 2011 17:21:19 -0400 Message-ID: <4DCAFDD0.9060707@bluewatersys.com> Date: Thu, 12 May 2011 09:21:20 +1200 From: Ryan Mallon User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.13) Gecko/20101208 Lightning/1.0b2 Thunderbird/3.1.7 MIME-Version: 1.0 To: Clifton Barnes CC: akpm@linux-foundation.org, haojian.zhuang@marvell.com, johnpol@2ka.mipt.ru, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] w1: Add Maxim/Dallas DS2780 Stand-Alone Fuel Gauge IC support. References: <4DCACAA1.902@indesign-llc.com> In-Reply-To: <4DCACAA1.902@indesign-llc.com> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 44396 Lines: 1623 On 05/12/2011 05:42 AM, Clifton Barnes wrote: > Add support for the Maxim/Dallas DS2780 Stand-Alone Fuel Gauge IC. > > It was suggested to combine this functionality with the current ds2782 > driver. Unfortunately, I'm unable to commit the time to refactoring this > driver to that extent and I don't have a platform with the ds2782 part > to validate that there are no regression issues by adding this functionality. > > Changes for v2: > - Change simple_strto* functions to kstrto* > - Remove warning due to bin_attribute function prototype changing > - Remove errors/warnings found by checkpatch > - Changed email client to properly send patch with tabs > > Signed-off-by: Clifton Barnes Hi Clifton, Quick review below. Thanks, ~Ryan > --- > drivers/power/Kconfig | 7 + > drivers/power/Makefile | 1 + > drivers/power/ds2780_battery.c | 886 ++++++++++++++++++++++++++++++++++++++++ > drivers/w1/slaves/Kconfig | 13 + > drivers/w1/slaves/Makefile | 1 + > drivers/w1/slaves/w1_ds2780.c | 240 +++++++++++ > drivers/w1/slaves/w1_ds2780.h | 132 ++++++ > drivers/w1/w1_family.h | 1 + > 8 files changed, 1281 insertions(+), 0 deletions(-) > create mode 100644 drivers/power/ds2780_battery.c > create mode 100644 drivers/w1/slaves/w1_ds2780.c > create mode 100644 drivers/w1/slaves/w1_ds2780.h > > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig > index 52a462f..dc8c531 100644 > --- a/drivers/power/Kconfig > +++ b/drivers/power/Kconfig > @@ -68,6 +68,13 @@ config BATTERY_DS2760 > help > Say Y here to enable support for batteries with ds2760 chip. > > +config BATTERY_DS2780 > + tristate "DS2780 battery driver" > + select W1 > + select W1_SLAVE_DS2780 > + help > + Say Y here to enable support for batteries with ds2780 chip. > + > config BATTERY_DS2782 > tristate "DS2782/DS2786 standalone gas-gauge" > depends on I2C > diff --git a/drivers/power/Makefile b/drivers/power/Makefile > index 8385bfa..8224990 100644 > --- a/drivers/power/Makefile > +++ b/drivers/power/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_WM8350_POWER) += wm8350_power.o > obj-$(CONFIG_TEST_POWER) += test_power.o > > obj-$(CONFIG_BATTERY_DS2760) += ds2760_battery.o > +obj-$(CONFIG_BATTERY_DS2780) += ds2780_battery.o > obj-$(CONFIG_BATTERY_DS2782) += ds2782_battery.o > obj-$(CONFIG_BATTERY_PMU) += pmu_battery.o > obj-$(CONFIG_BATTERY_OLPC) += olpc_battery.o > diff --git a/drivers/power/ds2780_battery.c b/drivers/power/ds2780_battery.c > new file mode 100644 > index 0000000..a4da870 > --- /dev/null > +++ b/drivers/power/ds2780_battery.c > @@ -0,0 +1,886 @@ > +/* > + * 1-wire client/driver for the Maxim/Dallas DS2780 Stand-Alone Fuel Gauge IC > + * > + * Copyright (C) 2010 Indesign, LLC > + * > + * Author: Clifton Barnes > + * > + * Based on ds2760_battery and ds2782_battery drivers > + * > + * 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. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "../w1/w1.h" > +#include "../w1/slaves/w1_ds2780.h" > + > +#define to_ds2780_device_info(x) container_of(x, struct ds2780_device_info, bat) > +#define to_power_supply(x) (struct power_supply *) dev_get_drvdata(x) Static inline functions are better since they have type safety, etc: static inline struct ds2780_device_info * to_ds2780_device_info(struct power_supply *psy) { return container_of(psy, struct ds2780_device_info, bat); } static inline struct power_supply *to_power_supply(struct device *dev) { return dev_get_drvdata(dev); } > + > +/* Current unit measurement in uA for a 1 milli-ohm sense resistor */ > +#define DS2780_CURRENT_UNITS 1563 > +/* Charge unit measurement in uAh for a 1 milli-ohm sense resistor */ > +#define DS2780_CHARGE_UNITS 6250 > +/* Number of bytes in user EEPROM space */ > +#define DS2780_USER_EEPROM_SIZE (DS2780_EEPROM_BLOCK0_END - \ > + DS2780_EEPROM_BLOCK0_START + 1) > +/* Number of bytes in parameter EEPROM space */ > +#define DS2780_PARAM_EEPROM_SIZE (DS2780_EEPROM_BLOCK1_END - \ > + DS2780_EEPROM_BLOCK1_START + 1) > + > +struct ds2780_device_info { > + struct device *dev; > + struct power_supply bat; > + struct device *w1_dev; > +}; > + > +enum current_types { > + CURRENT_NOW, > + CURRENT_AVG, > +}; > + > +static const char model[] = "DS2780"; > +static const char manufacturer[] = "Maxim/Dallas"; > + > +/* Set sense resistor value in mhos */ > +static int ds2780_set_sense_register(struct ds2780_device_info *dev_info, > + u8 conductance) > +{ > + int ret; > + > + ret = w1_ds2780_write(dev_info->w1_dev, &conductance, > + DS2780_RSNSP_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_store_eeprom(dev_info->w1_dev, DS2780_RSNSP_REG); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_recall_eeprom(dev_info->w1_dev, DS2780_RSNSP_REG); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +/* Get RSGAIN value from 0 to 1.999 in steps of 0.001 */ > +static int ds2780_get_rsgain_register(struct ds2780_device_info *dev_info, > + u16 *rsgain) > +{ > + int ret; > + char bytes[2]; > + > + ret = w1_ds2780_read(dev_info->w1_dev, bytes, > + DS2780_RSGAIN_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, bytes + 1, > + DS2780_RSGAIN_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; This is a common idiom in this driver. You might want to add w1_ds2780_read/write16 functions. > + > + *rsgain = (bytes[0] << 8) | bytes[1]; > + return 0; > +} > + > +/* Set RSGAIN value from 0 to 1.999 in steps of 0.001 */ > +static int ds2780_set_rsgain_register(struct ds2780_device_info *dev_info, > + u16 rsgain) > +{ > + int ret; > + char bytes[] = {rsgain >> 8, rsgain & 0xFF}; > + > + ret = w1_ds2780_write(dev_info->w1_dev, bytes, > + DS2780_RSGAIN_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_write(dev_info->w1_dev, bytes + 1, > + DS2780_RSGAIN_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_store_eeprom(dev_info->w1_dev, DS2780_RSGAIN_MSB_REG); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_recall_eeprom(dev_info->w1_dev, DS2780_RSGAIN_MSB_REG); > + if (ret < 0) > + return ret; > + > + return 0; > +} > + > +static int ds2780_get_voltage(struct ds2780_device_info *dev_info, > + int *voltage_uV) > +{ > + char raw[2]; > + s16 voltage_raw; > + int ret; > + > + /* The voltage value is located in 10 bits across the voltage MSB > + and LSB registers in two's compliment form > + Sign bit of the voltage value is in bit 7 of the voltage MSB register > + Bits 9 - 3 of the voltage value are in bits 6 - 0 of the > + voltage MSB register > + Bits 2 - 0 of the voltage value are in bits 7 - 5 of the > + voltage LSB register > + */ /* * Multi-line comments should look like this. * Several places in this file need fixing. */ > + ret = w1_ds2780_read(dev_info->w1_dev, raw, > + DS2780_VOLT_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, raw + 1, > + DS2780_VOLT_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + voltage_raw = (raw[0] << 3) | (raw[1] >> 5); > + /* DS2780 reports voltage in units of 4.88mV, but the battery class > + * reports in units of uV, so convert by multiplying by 4880. */ > + *voltage_uV = voltage_raw * 4880; Should be a blank line before the comment. Also, your comment style makes the last line look like part of the comment :-/. Use the multi-line comment style mentioned above. > + return 0; > +} > + > +static int ds2780_get_temperature(struct ds2780_device_info *dev_info, > + int *temperature) > +{ > + char raw[2]; > + s16 temperature_raw; > + int ret; > + > + /* The temperature value is located in 10 bits across the temperature > + MSB and LSB registers in two's compliment form > + Sign bit of the temperature value is in bit 7 of the temperature > + MSB register > + Bits 9 - 3 of the temperature value are in bits 6 - 0 of the > + temperature MSB register > + Bits 2 - 0 of the temperature value are in bits 7 - 5 of the > + temperature LSB register > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, raw, > + DS2780_TEMP_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, raw + 1, > + DS2780_TEMP_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + temperature_raw = (((signed char)raw[0] << 3)) | (raw[1] >> 5); Blank between this line and the comment below. > + /* DS2780 reports temperature in signed units of 0.125 C, but the > + * battery class reports in units of 1/10 C, so we convert by > + * multiplying by .125 * 10 = 1.25. */ > + *temperature = temperature_raw + (temperature_raw / 4); Is too early for math, but this doesn't look right. If you add a 16 bit register read function it should be the same as the ds2782 driver right? e.g. u16 raw; /* * 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. */ ret = w1_ds2780_read16(dev_info->w1_dev, &raw, DS2780_TEMP_MSB_REG); if (ret) return ret; *temperature = ((raw / 32) * 125) / 100; > + return 0; > +} > + > +static int ds2780_get_current(struct ds2780_device_info *dev_info, > + enum current_types type, int *current_uA) > +{ > + int ret; > + char raw[2]; > + s16 current_raw; > + int sense_res; > + u8 sense_res_raw; > + u8 reg_msb; > + u8 reg_lsb; Put common types on the same line, i.e: u8 sense_res_raw, reg_msb, reg_lsb; int ret, sense_res; > + > + /* > + * The units of measurement for current are dependent on the value of > + * the sense resistor. > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, &sense_res_raw, > + DS2780_RSNSP_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + if (sense_res_raw == 0) { > + dev_err(dev_info->dev, "sense resistor value is 0\n"); > + return -ENXIO; > + } > + sense_res = 1000 / sense_res_raw; > + > + if (type == CURRENT_NOW) { > + reg_msb = DS2780_CURRENT_MSB_REG; > + reg_lsb = DS2780_CURRENT_LSB_REG; > + } else if (type == CURRENT_AVG) { > + reg_msb = DS2780_IAVG_MSB_REG; > + reg_lsb = DS2780_IAVG_LSB_REG; > + } else { > + return -EINVAL; > + } > + > + /* The current value is located in 16 bits across the current MSB > + and LSB registers in two's compliment form > + Sign bit of the current value is in bit 7 of the current MSB register > + Bits 14 - 8 of the current value are in bits 6 - 0 of the current > + MSB register > + Bits 7 - 0 of the current value are in bits 7 - 0 of the current > + LSB register > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, raw, reg_msb, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, raw + 1, reg_lsb, sizeof(char)); > + if (ret < 0) > + return ret; > + > + current_raw = ((signed char)raw[0] << 8) | raw[1]; This cast looks suspect. char may be signed or unsigned by default depending on the architecture. Probably raw should be changed to u8 or s8. > + *current_uA = current_raw * (DS2780_CURRENT_UNITS / sense_res); > + return 0; > +} > + > +static int ds2780_get_accumulated_current(struct ds2780_device_info *dev_info, > + int *accumulated_current) > +{ > + char raw[2]; > + s16 current_raw; > + int sense_res; > + u8 sense_res_raw; > + int ret; > + > + /* > + The units of measurement for accumulated current are dependent on > + the value of the sense resistor. > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, &sense_res_raw, > + DS2780_RSNSP_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + if (sense_res_raw == 0) { > + dev_err(dev_info->dev, "sense resistor value is 0\n"); > + return -ENXIO; > + } > + sense_res = 1000 / sense_res_raw; > + > + /* The ACR value is located in 16 bits across the ACR MSB and > + LSB registers > + Bits 15 - 8 of the ACR value are in bits 7 - 0 of the ACR > + MSB register > + Bits 7 - 0 of the ACR value are in bits 7 - 0 of the ACR > + LSB register > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, raw, > + DS2780_ACR_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, raw + 1, > + DS2780_ACR_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + current_raw = (raw[0] << 8) | raw[1]; No cast this time? Maybe it is not needed above? I would still fix the types. > + *accumulated_current = current_raw * (DS2780_CHARGE_UNITS / sense_res); > + return 0; > +} > + > +static int ds2780_get_capacity(struct ds2780_device_info *dev_info, > + int *capacity) > +{ > + int ret; > + u8 raw; > + > + ret = w1_ds2780_read(dev_info->w1_dev, &raw, > + DS2780_RARC_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + *capacity = raw; > + return raw; Should return 0 on success I think. Same with other functions. > +} > + > +static int ds2780_get_status(struct ds2780_device_info *dev_info, int *status) > +{ > + int ret; > + int current_uA; > + int capacity; int ret, current_uA, capacity; > + > + ret = ds2780_get_current(dev_info, CURRENT_NOW, ¤t_uA); > + if (ret < 0) > + return ret; > + > + ret = ds2780_get_capacity(dev_info, &capacity); > + if (ret < 0) > + return ret; > + > + if (capacity == 100) > + *status = POWER_SUPPLY_STATUS_FULL; > + else if (current_uA == 0) > + *status = POWER_SUPPLY_STATUS_NOT_CHARGING; > + else if (current_uA < 0) > + *status = POWER_SUPPLY_STATUS_DISCHARGING; > + else > + *status = POWER_SUPPLY_STATUS_CHARGING; > + > + return 0; > +} > + > +static int ds2780_get_charge_now(struct ds2780_device_info *dev_info, > + int *charge_now) > +{ > + char raw[2]; > + u16 charge_raw; > + int ret; > + > + /* The RAAC value is located in 16 bits across the RAAC MSB and > + LSB registers > + Bits 15 - 8 of the RAAC value are in bits 7 - 0 of the RAAC > + MSB register > + Bits 7 - 0 of the RAAC value are in bits 7 - 0 of the RAAC > + LSB register > + */ > + ret = w1_ds2780_read(dev_info->w1_dev, raw, > + DS2780_RAAC_MSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, raw + 1, > + DS2780_RAAC_LSB_REG, sizeof(char)); > + if (ret < 0) > + return ret; > + > + charge_raw = (raw[0] << 8) | raw[1]; > + *charge_now = charge_raw * 1600; > + > + return 0; > + Remove the blank line between the return and the closing brace. > +} > + > +static int ds2780_get_control_register(struct ds2780_device_info *dev_info, > + u8 *control_reg) > +{ > + u8 reg; > + int ret; > + > + ret = w1_ds2780_read(dev_info->w1_dev, ®, > + DS2780_CONTROL_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + *control_reg = reg; You could just do: ret = w1_ds2780_read(dev_info->w1_dev, control_reg, DS2780_CONTROL_REG, sizeof(u8)); and remove the local variable reg. If the read fails then the caller should not be checking the value of control_reg. > + return 0; > +} > + > +static int ds2780_set_control_register(struct ds2780_device_info *dev_info, > + u8 control_reg) > +{ > + int ret; > + > + ret = w1_ds2780_write(dev_info->w1_dev, &control_reg, > + DS2780_CONTROL_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_store_eeprom(dev_info->w1_dev, DS2780_CONTROL_REG); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_recall_eeprom(dev_info->w1_dev, DS2780_CONTROL_REG); > + if (ret < 0) > + return ret; store_eeprom/recall_eeprom is another common idiom. Perhaps wrap it up in a helper function. > + return 0; > +} > + > +static int ds2780_battery_get_property(struct power_supply *psy, > + enum power_supply_property psp, > + union power_supply_propval *val) > +{ > + int ret = 0; > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + switch (psp) { > + case POWER_SUPPLY_PROP_VOLTAGE_NOW: > + ret = ds2780_get_voltage(dev_info, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_TEMP: > + ret = ds2780_get_temperature(dev_info, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_MODEL_NAME: > + val->strval = model; > + break; > + > + case POWER_SUPPLY_PROP_MANUFACTURER: > + val->strval = manufacturer; > + break; > + > + case POWER_SUPPLY_PROP_CURRENT_NOW: > + ret = ds2780_get_current(dev_info, CURRENT_NOW, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_CURRENT_AVG: > + ret = ds2780_get_current(dev_info, CURRENT_AVG, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_STATUS: > + ret = ds2780_get_status(dev_info, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_CAPACITY: > + ret = ds2780_get_capacity(dev_info, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_COUNTER: > + ret = ds2780_get_accumulated_current(dev_info, &val->intval); > + break; > + > + case POWER_SUPPLY_PROP_CHARGE_NOW: > + ret = ds2780_get_charge_now(dev_info, &val->intval); > + break; > + > + default: > + ret = -EINVAL; > + } > + > + return ret; > +} > + > +static enum power_supply_property ds2780_battery_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_TEMP, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_CURRENT_AVG, > + POWER_SUPPLY_PROP_CAPACITY, > + POWER_SUPPLY_PROP_CHARGE_COUNTER, > + POWER_SUPPLY_PROP_CHARGE_NOW, > +}; > + > +static ssize_t ds2780_get_pmod_enabled(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + u8 control_reg; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + /* Get power mode */ > + ret = ds2780_get_control_register(dev_info, &control_reg); > + if (ret < 0) > + return ret; > + > + if (control_reg & DS2780_CONTROL_REG_PMOD) > + ret = sprintf(buf, "1\n"); > + else > + ret = sprintf(buf, "0\n"); > + > + return ret; return sprintf(buf, "%d\n", !!(control_reg & DS2780_CONTROL_REG_PMOD)); > +} > + > +static ssize_t ds2780_set_pmod_enabled(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + u8 control_reg; > + u8 new_setting; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + /* Set power mode */ > + ret = ds2780_get_control_register(dev_info, &control_reg); > + if (ret < 0) > + return ret; > + > + ret = kstrtou8(buf, 10, &new_setting); > + if (ret < 0) > + return ret; > + > + if ((new_setting != 0) && (new_setting != 1)) { Don't need the inner parens. > + dev_err(dev_info->dev, "Invalid pmod setting (0 or 1)\n"); > + return -EINVAL; > + } > + > + if (new_setting) > + control_reg |= DS2780_CONTROL_REG_PMOD; > + else > + control_reg &= ~DS2780_CONTROL_REG_PMOD; > + > + ret = ds2780_set_control_register(dev_info, control_reg); > + if (ret < 0) > + return ret; > + > + return count; > + > +} > + > +static ssize_t ds2780_get_sense_resistor_value(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + u8 sense_resistor; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = w1_ds2780_read(dev_info->w1_dev, &sense_resistor, > + DS2780_RSNSP_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + ret = sprintf(buf, "%i\n", sense_resistor); %d is more common. > + return ret; > +} > + > +static ssize_t ds2780_set_sense_resistor_value(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + u8 new_setting; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = kstrtou8(buf, 10, &new_setting); Might be worth allowing people to write register values in hex also. > + if (ret < 0) > + return ret; > + > + ret = ds2780_set_sense_register(dev_info, new_setting); > + if (ret < 0) > + return ret; You will need some form of locking, probably just a mutex, inside functions such as ds2780_set_sense_register since they are callable via both sysfs and from the power core and are therefore potentially racy. > + > + return count; > +} > + > +static ssize_t ds2780_get_rsgain_setting(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + u16 rsgain; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = ds2780_get_rsgain_register(dev_info, &rsgain); > + if (ret < 0) > + return ret; > + > + ret = sprintf(buf, "%d\n", rsgain); > + return ret; return sprintf(buf, "%d\n", rsgain); > +} > + > +static ssize_t ds2780_set_rsgain_setting(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + u16 new_setting; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = kstrtou16(buf, 10, &new_setting); > + if (ret < 0) > + return ret; > + > + /* Gain can only be from 0 to 1.999 in steps of .001 */ > + if (new_setting > 1999) { > + dev_err(dev_info->dev, "Invalid rsgain setting (0 - 1999)\n"); > + return -EINVAL; > + } > + > + ret = ds2780_set_rsgain_register(dev_info, new_setting); > + if (ret < 0) > + return ret; > + > + return count; > +} > + > +static ssize_t ds2780_get_user_eeprom(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = w1_ds2780_read(dev_info->w1_dev, buf, DS2780_EEPROM_BLOCK0_START, > + DS2780_USER_EEPROM_SIZE); > + if (ret < 0) > + return ret; Not sure that this is really obeying the rules of sysfs which state that files should only contain a single value. There is the firmware subsystem, but I'm not sure that is really what you want either. Perhaps somebody else can suggest an alternative. > + > + return DS2780_USER_EEPROM_SIZE; > +} > + > +static ssize_t ds2780_set_user_eeprom(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + /* Only write as much as there is user EEPROM to not overwrite > + parameter EEPROM > + */ > + if (count > DS2780_USER_EEPROM_SIZE) > + count = DS2780_USER_EEPROM_SIZE; count = min(count, DS2780_USER_EEPROM_SIZE); > + > + ret = w1_ds2780_write(dev_info->w1_dev, (char *)buf, > + DS2780_EEPROM_BLOCK0_START, count); No, bad cast, bad cast. > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_store_eeprom(dev_info->w1_dev, > + DS2780_EEPROM_BLOCK0_START); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_recall_eeprom(dev_info->w1_dev, > + DS2780_EEPROM_BLOCK0_START); > + if (ret < 0) > + return ret; > + > + return count; > +} > + > +static ssize_t ds2780_get_param_eeprom(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = w1_ds2780_read(dev_info->w1_dev, buf, DS2780_EEPROM_BLOCK1_START, > + DS2780_PARAM_EEPROM_SIZE); > + if (ret < 0) > + return ret; Again, I think this violates the sysfs rules a bit. > + > + return DS2780_PARAM_EEPROM_SIZE; > +} > + > +static ssize_t ds2780_set_param_eeprom(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + /* Only write as much as there is parameter EEPROM */ > + if (count > DS2780_PARAM_EEPROM_SIZE) > + count = DS2780_PARAM_EEPROM_SIZE; count = min(count, DS2780_PARAM_EEPROM_SIZE); > + > + ret = w1_ds2780_write(dev_info->w1_dev, (char *)buf, > + DS2780_EEPROM_BLOCK1_START, count); Dodgy casting again. > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_store_eeprom(dev_info->w1_dev, > + DS2780_EEPROM_BLOCK1_START); > + if (ret < 0) > + return ret; > + > + ret = w1_ds2780_recall_eeprom(dev_info->w1_dev, > + DS2780_EEPROM_BLOCK1_START); > + if (ret < 0) > + return ret; > + > + return count; > +} > + > +static ssize_t ds2780_get_pio_pin(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + int ret; > + u8 sfr; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = w1_ds2780_read(dev_info->w1_dev, &sfr, > + DS2780_SFR_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + ret = sprintf(buf, "%d\n", sfr & DS2780_SFR_REG_PIOSC); > + return ret; > +} > + > +static ssize_t ds2780_set_pio_pin(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t count) > +{ > + int ret; > + u8 new_setting; > + struct power_supply *psy = to_power_supply(dev); > + struct ds2780_device_info *dev_info = to_ds2780_device_info(psy); > + > + ret = kstrtou8(buf, 10, &new_setting); > + if (ret < 0) > + return ret; > + > + if ((new_setting != 0) && (new_setting != 1)) { Inner parens are not needed. > + dev_err(dev_info->dev, "Invalid pio_pin setting (0 or 1)\n"); > + return -EINVAL; > + } > + > + ret = w1_ds2780_write(dev_info->w1_dev, &new_setting, > + DS2780_SFR_REG, sizeof(u8)); > + if (ret < 0) > + return ret; > + > + return count; > +} > + > + > +static DEVICE_ATTR(pmod_enabled, 0644, ds2780_get_pmod_enabled, > + ds2780_set_pmod_enabled); You should use the macros in include/linux/stat.h for the file mode. S_IRUGO | S_IWUSR is typical. > +static DEVICE_ATTR(sense_resistor_value, 0644, ds2780_get_sense_resistor_value, > + ds2780_set_sense_resistor_value); > +static DEVICE_ATTR(rsgain_setting, 0644, ds2780_get_rsgain_setting, > + ds2780_set_rsgain_setting); > +static DEVICE_ATTR(user_eeprom, 0644, ds2780_get_user_eeprom, > + ds2780_set_user_eeprom); > +static DEVICE_ATTR(param_eeprom, 0644, ds2780_get_param_eeprom, > + ds2780_set_param_eeprom); > +static DEVICE_ATTR(pio_pin, 0644, ds2780_get_pio_pin, ds2780_set_pio_pin); > + > + > +static struct attribute *ds2780_attributes[] = { > + &dev_attr_pmod_enabled.attr, > + &dev_attr_sense_resistor_value.attr, > + &dev_attr_rsgain_setting.attr, > + &dev_attr_user_eeprom.attr, > + &dev_attr_param_eeprom.attr, > + &dev_attr_pio_pin.attr, > + NULL > +}; > + > +static const struct attribute_group ds2780_attr_group = { > + .attrs = ds2780_attributes, > +}; > + > +static int ds2780_battery_probe(struct platform_device *pdev) > +{ Probably should be __devinit > + int ret = 0; > + struct ds2780_device_info *dev_info; > + > + dev_info = kzalloc(sizeof(*dev_info), GFP_KERNEL); > + if (!dev_info) { > + ret = -ENOMEM; > + goto dev_info_alloc_failed; > + } > + > + platform_set_drvdata(pdev, dev_info); > + > + dev_info->dev = &pdev->dev; > + dev_info->w1_dev = pdev->dev.parent; > + dev_info->bat.name = dev_name(&pdev->dev); > + dev_info->bat.type = POWER_SUPPLY_TYPE_BATTERY; > + dev_info->bat.properties = ds2780_battery_props; > + dev_info->bat.num_properties = ARRAY_SIZE(ds2780_battery_props); > + dev_info->bat.get_property = ds2780_battery_get_property; > + > + ret = power_supply_register(&pdev->dev, &dev_info->bat); > + if (ret) { > + dev_err(dev_info->dev, "failed to register battery\n"); > + goto batt_failed; > + } > + > + ret = sysfs_create_group(&dev_info->bat.dev->kobj, &ds2780_attr_group); > + if (ret) { > + printk(KERN_INFO "ds2780: failed to create sysfs group\n"); dev_err > + goto fail_sysfs; > + } > + > + return 0; > + > +fail_sysfs: > + power_supply_unregister(&dev_info->bat); > +batt_failed: > + kfree(dev_info); > +dev_info_alloc_failed: > + return ret; Exit labels should follow a common pattern, e.g. fail_unregister: power_supply_unregister(&dev_info->bat); fail_free_info: kfree(dev_info); fail: return ret; > +} > + > +static int ds2780_battery_remove(struct platform_device *pdev) > +{ Probably should be __devexit. > + struct ds2780_device_info *dev_info = platform_get_drvdata(pdev); > + > + /* remove attributes */ > + sysfs_remove_group(&dev_info->bat.dev->kobj, &ds2780_attr_group); > + > + power_supply_unregister(&dev_info->bat); > + > + kfree(dev_info); > + return 0; > +} > + > +#ifdef CONFIG_PM > + > +static int ds2780_battery_suspend(struct platform_device *pdev, > + pm_message_t state) > +{ > + return 0; > +} > + > +static int ds2780_battery_resume(struct platform_device *pdev) > +{ > + return 0; > +} > + > +#else > + > +#define ds2780_battery_suspend NULL > +#define ds2780_battery_resume NULL Just remove all of the suspend/resume stuff since it doesn't do anything. Also, I think you are meant to use the dev_pm_ops structure now for suspend/resume calls rather than doing ifdef CONFIG_PM. > +#endif /* CONFIG_PM */ > + > +MODULE_ALIAS("platform:ds2780-battery"); > + > +static struct platform_driver ds2780_battery_driver = { > + .driver = { .owner = THIS_MODULE, ? > + .name = "ds2780-battery", > + }, > + .probe = ds2780_battery_probe, > + .remove = ds2780_battery_remove, .remove = __devexit_p(ds2780_battery_remove), > + .suspend = ds2780_battery_suspend, > + .resume = ds2780_battery_resume, Remove the suspend/resume callbacks. > +}; > + > +static int __init ds2780_battery_init(void) > +{ > + return platform_driver_register(&ds2780_battery_driver); > +} > + > +static void __exit ds2780_battery_exit(void) > +{ > + platform_driver_unregister(&ds2780_battery_driver); > +} > + > +module_init(ds2780_battery_init); > +module_exit(ds2780_battery_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Clifton Barnes "); > +MODULE_DESCRIPTION("Maxim/Dallas DS2780 Stand-Alone Fuel Gauage IC driver"); > diff --git a/drivers/w1/slaves/Kconfig b/drivers/w1/slaves/Kconfig > index f0c9096..d9fa263 100644 > --- a/drivers/w1/slaves/Kconfig > +++ b/drivers/w1/slaves/Kconfig > @@ -61,6 +61,19 @@ config W1_SLAVE_DS2760 > > If you are unsure, say N. > > +config W1_SLAVE_DS2780 > + tristate "Dallas 2780 battery monitor chip" > + depends on W1 > + help > + If you enable this you will have the DS2780 battery monitor > + chip support. > + > + The battery monitor chip is used in many batteries/devices > + as the one who is responsible for charging/discharging/monitoring > + Li+ batteries. > + > + If you are unsure, say N. > + This should just be: config W1_SLAVE_DS2780: tristate since CONFIG_BATTERY_DS2780 selects this there is never any need for it to be a visible config option. > config W1_SLAVE_BQ27000 > tristate "BQ27000 slave support" > depends on W1 > diff --git a/drivers/w1/slaves/Makefile b/drivers/w1/slaves/Makefile > index 3c76350..00c9134 100644 > --- a/drivers/w1/slaves/Makefile > +++ b/drivers/w1/slaves/Makefile > @@ -8,4 +8,5 @@ obj-$(CONFIG_W1_SLAVE_DS2423) += w1_ds2423.o > obj-$(CONFIG_W1_SLAVE_DS2431) += w1_ds2431.o > obj-$(CONFIG_W1_SLAVE_DS2433) += w1_ds2433.o > obj-$(CONFIG_W1_SLAVE_DS2760) += w1_ds2760.o > +obj-$(CONFIG_W1_SLAVE_DS2780) += w1_ds2780.o > obj-$(CONFIG_W1_SLAVE_BQ27000) += w1_bq27000.o > diff --git a/drivers/w1/slaves/w1_ds2780.c b/drivers/w1/slaves/w1_ds2780.c > new file mode 100644 > index 0000000..5b096a8 > --- /dev/null > +++ b/drivers/w1/slaves/w1_ds2780.c > @@ -0,0 +1,240 @@ > +/* > + * 1-Wire implementation for the ds2780 chip > + * > + * Copyright (C) 2010 Indesign, LLC > + * > + * Author: Clifton Barnes > + * > + * Based on w1-ds2760 driver > + * > + * 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. > + * > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "../w1.h" > +#include "../w1_int.h" > +#include "../w1_family.h" > +#include "w1_ds2780.h" > + > +static int w1_ds2780_io(struct device *dev, char *buf, int addr, size_t count, > + int io) > +{ > + struct w1_slave *sl = container_of(dev, struct w1_slave, dev); > + > + if (!dev) > + return -ENODEV; > + > + mutex_lock(&sl->master->mutex); > + > + if (addr > DS2780_DATA_SIZE || addr < 0) { > + count = 0; > + goto out; > + } > + if (addr + count > DS2780_DATA_SIZE) > + count = DS2780_DATA_SIZE - addr; count = min(count, DS2780_DATA_SIZE - addr); > + > + if (!w1_reset_select_slave(sl)) { Should be if (w1_reset_select_slave(sl) == 0) To be consistent with the usage below? > + if (!io) { Doing if (io) and reversing the two clauses is more clear I think. > + w1_write_8(sl->master, W1_DS2780_READ_DATA); > + w1_write_8(sl->master, addr); > + count = w1_read_block(sl->master, buf, count); > + } else { > + w1_write_8(sl->master, W1_DS2780_WRITE_DATA); > + w1_write_8(sl->master, addr); > + w1_write_block(sl->master, buf, count); > + /* XXX w1_write_block returns void, not n_written */ > + } > + } > + > +out: > + mutex_unlock(&sl->master->mutex); > + > + return count; > +} > + > +int w1_ds2780_read(struct device *dev, char *buf, int addr, size_t count) > +{ > + return w1_ds2780_io(dev, buf, addr, count, 0); > +} > +EXPORT_SYMBOL(w1_ds2780_read); > + > +int w1_ds2780_write(struct device *dev, char *buf, int addr, size_t count) > +{ > + return w1_ds2780_io(dev, buf, addr, count, 1); > +} > +EXPORT_SYMBOL(w1_ds2780_write); You could move the read/write functions into the battery driver proper so that you only need to export w1_ds2780_io. > + > +static int w1_ds2780_eeprom_cmd(struct device *dev, int addr, int cmd) > +{ > + struct w1_slave *sl = container_of(dev, struct w1_slave, dev); > + > + if (!dev) > + return -EINVAL; > + > + mutex_lock(&sl->master->mutex); > + > + if (w1_reset_select_slave(sl) == 0) { > + w1_write_8(sl->master, cmd); > + w1_write_8(sl->master, addr); > + } > + > + mutex_unlock(&sl->master->mutex); > + return 0; > +} > + > +int w1_ds2780_store_eeprom(struct device *dev, int addr) > +{ > + return w1_ds2780_eeprom_cmd(dev, addr, W1_DS2780_COPY_DATA); > +} > +EXPORT_SYMBOL(w1_ds2780_store_eeprom); > + > +int w1_ds2780_recall_eeprom(struct device *dev, int addr) > +{ > + return w1_ds2780_eeprom_cmd(dev, addr, W1_DS2780_RECALL_DATA); > +} > +EXPORT_SYMBOL(w1_ds2780_recall_eeprom); Same again, just export w1_ds2780_eeprom_cmd and then implement the other functions in the battery driver. > + > +static ssize_t w1_ds2780_read_bin(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, > + char *buf, loff_t off, size_t count) > +{ > + struct device *dev = container_of(kobj, struct device, kobj); > + return w1_ds2780_read(dev, buf, off, count); > +} What is this for? > + > +static struct bin_attribute w1_ds2780_bin_attr = { > + .attr = { > + .name = "w1_slave", > + .mode = S_IRUGO, > + }, > + .size = DS2780_DATA_SIZE, > + .read = w1_ds2780_read_bin, > +}; > + > +static DEFINE_IDR(bat_idr); > +static DEFINE_MUTEX(bat_idr_lock); > + > +static int new_bat_id(void) > +{ > + int ret; > + > + while (1) { > + int id; > + > + ret = idr_pre_get(&bat_idr, GFP_KERNEL); > + if (ret == 0) > + return -ENOMEM; > + > + mutex_lock(&bat_idr_lock); > + ret = idr_get_new(&bat_idr, NULL, &id); > + mutex_unlock(&bat_idr_lock); > + > + if (ret == 0) { > + ret = id & MAX_ID_MASK; > + break; > + } else if (ret == -EAGAIN) { > + continue; > + } else { > + break; > + } > + } Is it common to do this in a while loop? In my experience if the idr_get_new fails an error should be returned. > + > + return ret; > +} > + > +static void release_bat_id(int id) > +{ > + mutex_lock(&bat_idr_lock); > + idr_remove(&bat_idr, id); > + mutex_unlock(&bat_idr_lock); > +} > + > +static int w1_ds2780_add_slave(struct w1_slave *sl) > +{ > + int ret; > + int id; int ret, id; > + struct platform_device *pdev; > + > + id = new_bat_id(); > + if (id < 0) { > + ret = id; > + goto noid; > + } > + > + pdev = platform_device_alloc("ds2780-battery", id); > + if (!pdev) { > + ret = -ENOMEM; > + goto pdev_alloc_failed; > + } > + pdev->dev.parent = &sl->dev; > + > + ret = platform_device_add(pdev); > + if (ret) > + goto pdev_add_failed; > + > + ret = sysfs_create_bin_file(&sl->dev.kobj, &w1_ds2780_bin_attr); > + if (ret) > + goto bin_attr_failed; > + > + dev_set_drvdata(&sl->dev, pdev); > + > + goto success; return 0; > + > +bin_attr_failed: > +pdev_add_failed: > + platform_device_unregister(pdev); > +pdev_alloc_failed: > + release_bat_id(id); > +noid: > +success: > + return ret; > +} > + > +static void w1_ds2780_remove_slave(struct w1_slave *sl) > +{ > + struct platform_device *pdev = dev_get_drvdata(&sl->dev); > + int id = pdev->id; > + > + platform_device_unregister(pdev); > + release_bat_id(id); > + sysfs_remove_bin_file(&sl->dev.kobj, &w1_ds2780_bin_attr); > +} > + > +static struct w1_family_ops w1_ds2780_fops = { > + .add_slave = w1_ds2780_add_slave, > + .remove_slave = w1_ds2780_remove_slave, > +}; > + > +static struct w1_family w1_ds2780_family = { > + .fid = W1_FAMILY_DS2780, > + .fops = &w1_ds2780_fops, > +}; > + > +static int __init w1_ds2780_init(void) > +{ > + idr_init(&bat_idr); > + return w1_register_family(&w1_ds2780_family); > +} > + > +static void __exit w1_ds2780_exit(void) > +{ > + w1_unregister_family(&w1_ds2780_family); > + idr_destroy(&bat_idr); > +} > + > +module_init(w1_ds2780_init); > +module_exit(w1_ds2780_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Clifton Barnes "); > +MODULE_DESCRIPTION("1-wire Driver for Maxim/Dallas DS2780 Stand-Alone Fuel Gauge IC"); > diff --git a/drivers/w1/slaves/w1_ds2780.h b/drivers/w1/slaves/w1_ds2780.h > new file mode 100644 > index 0000000..671cb6a > --- /dev/null > +++ b/drivers/w1/slaves/w1_ds2780.h > @@ -0,0 +1,132 @@ > +/* > + * 1-Wire implementation for the ds2780 chip > + * > + * Copyright (C) 2010 Indesign, LLC > + * > + * Author: Clifton Barnes > + * > + * Based on w1-ds2760 driver > + * > + * 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. > + * > + */ > + > +#ifndef __w1_ds2780_h__ > +#define __w1_ds2780_h__ Common style is for header guards is: #ifndef _W1_DS2780_H #define _W1_DS2780_H > + > +/* Function commands */ > +#define W1_DS2780_READ_DATA 0x69 > +#define W1_DS2780_WRITE_DATA 0x6C > +#define W1_DS2780_COPY_DATA 0x48 > +#define W1_DS2780_RECALL_DATA 0xB8 > +#define W1_DS2780_LOCK 0x6A > + > +/* Register map */ > +/* Register 0x00 Reserved */ > +#define DS2780_STATUS_REG 0x01 > +#define DS2780_RAAC_MSB_REG 0x02 > +#define DS2780_RAAC_LSB_REG 0x03 > +#define DS2780_RSAC_MSB_REG 0x04 > +#define DS2780_RSAC_LSB_REG 0x05 > +#define DS2780_RARC_REG 0x06 > +#define DS2780_RSRC_REG 0x07 > +#define DS2780_IAVG_MSB_REG 0x08 > +#define DS2780_IAVG_LSB_REG 0x09 > +#define DS2780_TEMP_MSB_REG 0x0A > +#define DS2780_TEMP_LSB_REG 0x0B > +#define DS2780_VOLT_MSB_REG 0x0C > +#define DS2780_VOLT_LSB_REG 0x0D > +#define DS2780_CURRENT_MSB_REG 0x0E > +#define DS2780_CURRENT_LSB_REG 0x0F > +#define DS2780_ACR_MSB_REG 0x10 > +#define DS2780_ACR_LSB_REG 0x11 > +#define DS2780_ACRL_MSB_REG 0x12 > +#define DS2780_ACRL_LSB_REG 0x13 > +#define DS2780_AS_REG 0x14 > +#define DS2780_SFR_REG 0x15 > +#define DS2780_FULL_MSB_REG 0x16 > +#define DS2780_FULL_LSB_REG 0x17 > +#define DS2780_AE_MSB_REG 0x18 > +#define DS2780_AE_LSB_REG 0x19 > +#define DS2780_SE_MSB_REG 0x1A > +#define DS2780_SE_LSB_REG 0x1B > +/* Register 0x1C - 0x1E Reserved */ > +#define DS2780_EEPROM_REG 0x1F > +#define DS2780_EEPROM_BLOCK0_START 0x20 > +/* Register 0x20 - 0x2F User EEPROM */ > +#define DS2780_EEPROM_BLOCK0_END 0x2F > +/* Register 0x30 - 0x5F Reserved */ > +#define DS2780_EEPROM_BLOCK1_START 0x60 > +#define DS2780_CONTROL_REG 0x60 > +#define DS2780_AB_REG 0x61 > +#define DS2780_AC_MSB_REG 0x62 > +#define DS2780_AC_LSB_REG 0x63 > +#define DS2780_VCHG_REG 0x64 > +#define DS2780_IMIN_REG 0x65 > +#define DS2780_VAE_REG 0x66 > +#define DS2780_IAE_REG 0x67 > +#define DS2780_AE_40_REG 0x68 > +#define DS2780_RSNSP_REG 0x69 > +#define DS2780_FULL_40_MSB_REG 0x6A > +#define DS2780_FULL_40_LSB_REG 0x6B > +#define DS2780_FULL_3040_SLOPE_REG 0x6C > +#define DS2780_FULL_2030_SLOPE_REG 0x6D > +#define DS2780_FULL_1020_SLOPE_REG 0x6E > +#define DS2780_FULL_0010_SLOPE_REG 0x6F > +#define DS2780_AE_3040_SLOPE_REG 0x70 > +#define DS2780_AE_2030_SLOPE_REG 0x71 > +#define DS2780_AE_1020_SLOPE_REG 0x72 > +#define DS2780_AE_0010_SLOPE_REG 0x73 > +#define DS2780_SE_3040_SLOPE_REG 0x74 > +#define DS2780_SE_2030_SLOPE_REG 0x75 > +#define DS2780_SE_1020_SLOPE_REG 0x76 > +#define DS2780_SE_0010_SLOPE_REG 0x77 > +#define DS2780_RSGAIN_MSB_REG 0x78 > +#define DS2780_RSGAIN_LSB_REG 0x79 > +#define DS2780_RSTC_REG 0x7A > +#define DS2780_FRSGAIN_MSB_REG 0x7B > +#define DS2780_FRSGAIN_LSB_REG 0x7C > +#define DS2780_EEPROM_BLOCK1_END 0x7C > +/* Register 0x7D - 0xFF Reserved */ > + > +/* Number of valid register addresses */ > +#define DS2780_DATA_SIZE 0x80 > + > +/* Status register bits */ > +#define DS2780_STATUS_REG_CHGTF (1 << 7) > +#define DS2780_STATUS_REG_AEF (1 << 6) > +#define DS2780_STATUS_REG_SEF (1 << 5) > +#define DS2780_STATUS_REG_LEARNF (1 << 4) > +/* Bit 3 Reserved */ > +#define DS2780_STATUS_REG_UVF (1 << 2) > +#define DS2780_STATUS_REG_PORF (1 << 1) > +/* Bit 0 Reserved */ > + > +/* Control register bits */ > +/* Bit 7 Reserved */ > +#define DS2780_CONTROL_REG_UVEN (1 << 6) > +#define DS2780_CONTROL_REG_PMOD (1 << 5) > +#define DS2780_CONTROL_REG_RNAOP (1 << 4) > +/* Bit 0 - 3 Reserved */ > + > +/* Special feature register bits */ > +/* Bit 1 - 7 Reserved */ > +#define DS2780_SFR_REG_PIOSC (1 << 0) > + > +/* EEPROM register bits */ > +#define DS2780_EEPROM_REG_EEC (1 << 7) > +#define DS2780_EEPROM_REG_LOCK (1 << 6) > +/* Bit 2 - 6 Reserved */ > +#define DS2780_EEPROM_REG_BL1 (1 << 1) > +#define DS2780_EEPROM_REG_BL0 (1 << 0) > + > +extern int w1_ds2780_read(struct device *dev, char *buf, int addr, > + size_t count); > +extern int w1_ds2780_write(struct device *dev, char *buf, int addr, > + size_t count); > +extern int w1_ds2780_store_eeprom(struct device *dev, int addr); > +extern int w1_ds2780_recall_eeprom(struct device *dev, int addr); > + > +#endif /* !__w1_ds2780_h__ */ > diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h > index f3b636d..e76125c 100644 > --- a/drivers/w1/w1_family.h > +++ b/drivers/w1/w1_family.h > @@ -36,6 +36,7 @@ > #define W1_THERM_DS18B20 0x28 > #define W1_EEPROM_DS2431 0x2D > #define W1_FAMILY_DS2760 0x30 > +#define W1_FAMILY_DS2780 0x32 > > #define MAXNAMELEN 32 > -- 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 -- 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/