| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 12 May 2011 09:21:20 +1200
From: Ryan Mallon <ryan@...ewatersys.com>
To: Clifton Barnes <cabarnes@...esign-llc.com>
CC: akpm@...ux-foundation.org, haojian.zhuang@...vell.com,
johnpol@....mipt.ru, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] w1: Add Maxim/Dallas DS2780 Stand-Alone Fuel Gauge
IC support.
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 <cabarnes@...esign-llc.com>
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 <cabarnes@...esign-llc.com>
> + *
> + * 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 <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/param.h>
> +#include <linux/pm.h>
> +#include <linux/platform_device.h>
> +#include <linux/power_supply.h>
> +#include <linux/idr.h>
> +
> +#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 <cabarnes@...esign-llc.com>");
> +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 <cabarnes@...esign-llc.com>
> + *
> + * 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 <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <linux/mutex.h>
> +#include <linux/idr.h>
> +
> +#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 <cabarnes@...esign-llc.com>");
> +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 <cabarnes@...esign-llc.com>
> + *
> + * 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@...ewatersys.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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists