[PATCH] Add driver for Dallas DS1682 elapsed time recorder

[PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---

Here is the 3rd iteration with the following changes:
- Modified to be an i2c "new style" driver based on David Brownell's work
- uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
  reads instead of a single byte at a time.

 drivers/i2c/chips/ds1682.c |  270 ++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 270 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
new file mode 100644
index 0000000..0a4a89e
--- /dev/null
+++ b/drivers/i2c/chips/ds1682.c
@@ -0,0 +1,270 @@
+/*
+ * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
+ *
+ * Written by: Grant Likely <grant.likely@secretlab.ca>
+ *
+ * Copyright (C) 2007 Secret Lab Technologies Ltd.
+ * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
+ * Copyright (C) 2000 Russell King
+ *
+ * Derived from ds1337 real time clock device 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.
+ */
+
+/*
+ * The DS1682 elapsed timer recorder is a simple device that implements
+ * one elapsed time counter, one event counter, an alarm signal and 10
+ * bytes of general purpose EEPROM.
+ *
+ * This driver provides access to the DS1682 counters and user data via
+ * the sysfs.  The following attributes are added to the device node:
+ *     elapsed_time (u32): Total elapsed event time in ms resolution
+ *     alarm_time (u32): When elapsed time exceeds the value in alarm_time,
+ *                       then the alarm pin is asserted.
+ *     event_count (u16): number of times the event pin has gone low.
+ *     eeprom (u8[10]): general purpose EEPROM
+ *
+ * Counter registers and user data are both read/write unless the device
+ * has been write protected.  This driver does not support turning off write
+ * protection.  Once write protection is turned on, it is impossible to
+ * turn it off again, so I have left the feature out of this driver to avoid
+ * accidental enabling, but it is trivial to add write protect support.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/string.h>
+#include <linux/list.h>
+#include <linux/sysfs.h>
+#include <linux/ctype.h>
+#include <linux/hwmon-sysfs.h>
+
+/* Device registers */
+#define DS1682_REG_CONFIG		0x00
+#define DS1682_REG_ALARM		0x01
+#define DS1682_REG_ELAPSED		0x05
+#define DS1682_REG_EVT_CNTR		0x09
+#define DS1682_REG_EEPROM		0x0b
+#define DS1682_REG_RESET		0x1d
+#define DS1682_REG_WRITE_DISABLE	0x1e
+#define DS1682_REG_WRITE_MEM_DISABLE	0x1f
+
+#define DS1682_EEPROM_SIZE		10
+
+/*
+ * Generic counter attributes
+ */
+static ssize_t ds1682_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	__le32 val = 0;
+	int rc;
+
+	dev_dbg(dev, "ds1682_show() called on %s\n", attr->attr.name);
+
+	/* Read the register */
+	rc = i2c_smbus_read_i2c_block_data(client, sattr->index, sattr->nr,
+					   (u8 *) & val);
+	if (rc < 0)
+		return -EIO;
+
+	/* Special case: the 32 bit regs are time values with 1/4s
+	 * resolution, scale them up to milliseconds */
+	if (sattr->nr == 4)
+		return sprintf(buf, "%lli\n", ((u64) le32_to_cpu(val)) * 250);
+
+	/* Format the output string and return # of bytes */
+	return sprintf(buf, "%li\n", (long)le32_to_cpu(val));
+}
+
+static ssize_t ds1682_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	char *endp;
+	u64 val;
+	__le32 val_le;
+	int rc;
+
+	dev_dbg(dev, "ds1682_store() called on %s\n", attr->attr.name);
+
+	/* Decode input */
+	val = simple_strtoull(buf, &endp, 0);
+	if (buf == endp) {
+		dev_dbg(dev, "input string not a number\n");
+		return -EIO;
+	}
+
+	/* Special case: the 32 bit regs are time values with 1/4s
+	 * resolution, scale input down to quarter-seconds */
+	if (sattr->nr == 4)
+		do_div(val, 250);
+
+	/* write out the value */
+	val_le = cpu_to_le32(val);
+	rc = i2c_smbus_write_i2c_block_data(client, sattr->index, sattr->nr,
+					    (u8 *) & val);
+	if (rc < 0) {
+		dev_err(dev, "register write failed; reg=0x%x, size=%i\n",
+			sattr->index, sattr->nr);
+		return -EIO;
+	}
+
+	return count;
+}
+
+/*
+ * Simple register attributes
+ */
+SENSOR_DEVICE_ATTR_2(config, S_IRUGO, ds1682_show, NULL, DS1682_REG_CONFIG, 1);
+SENSOR_DEVICE_ATTR_2(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     4, DS1682_REG_ELAPSED);
+SENSOR_DEVICE_ATTR_2(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     4, DS1682_REG_ALARM);
+SENSOR_DEVICE_ATTR_2(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     2, DS1682_REG_EVT_CNTR);
+
+static const struct attribute_group ds1682_group = {
+	.attrs = (struct attribute *[]) {
+		&sensor_dev_attr_config.dev_attr.attr,
+		&sensor_dev_attr_elapsed_time.dev_attr.attr,
+		&sensor_dev_attr_alarm_time.dev_attr.attr,
+		&sensor_dev_attr_event_count.dev_attr.attr,
+		NULL,
+	},
+};
+
+/*
+ * User data attribute
+ */
+static ssize_t ds1682_eeprom_read(struct kobject *kobj, char *buf, loff_t off,
+				  size_t count)
+{
+	struct i2c_client *client = kobj_to_i2c_client(kobj);
+	int rc;
+
+	dev_dbg(&client->dev, "ds1682_eeprom_read(p=%p, off=%lli, c=%zi)\n",
+		buf, off, count);
+
+	if (off > DS1682_EEPROM_SIZE)
+		return 0;
+
+	if (off + count > DS1682_EEPROM_SIZE)
+		count = DS1682_EEPROM_SIZE - off;
+
+	rc = i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + off,
+					   count, buf);
+	if (rc < 0)
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t ds1682_eeprom_write(struct kobject *kobj, char *buf, loff_t off,
+				   size_t count)
+{
+	struct i2c_client *client = kobj_to_i2c_client(kobj);
+
+	dev_dbg(&client->dev, "ds1682_eeprom_write(p=%p, off=%lli, c=%zi)\n",
+		buf, off, count);
+
+	if (off >= DS1682_EEPROM_SIZE)
+		return -ENOSPC;
+
+	if (off + count > DS1682_EEPROM_SIZE)
+		count = DS1682_EEPROM_SIZE - off;
+
+	/* Write out to the device */
+	if (i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + off,
+					   count, buf) < 0)
+		return -EIO;
+
+	return count;
+}
+
+static struct bin_attribute ds1682_eeprom_attr = {
+	.attr = {
+		.name = "eeprom",
+		.mode = S_IRUGO | S_IWUSR,
+		.owner = THIS_MODULE,
+	},
+	.size = DS1682_EEPROM_SIZE,
+	.read = ds1682_eeprom_read,
+	.write = ds1682_eeprom_write,
+};
+
+/*
+ * Called when a device is found at the ds1682 address
+ */
+static int ds1682_probe(struct i2c_client *client)
+{
+	int err = 0;
+
+	if (client->addr != 0x6b) {
+		dev_err(&client->dev, "%x is not a valid address\n",
+			client->addr);
+		return -ENODEV;
+	}
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+		dev_err(&client->dev, "i2c bus does not support the ds1682\n");
+		goto exit;
+	}
+
+	if (sysfs_create_group(&client->dev.kobj, &ds1682_group))
+		goto exit;
+
+	if (sysfs_create_bin_file(&client->dev.kobj, &ds1682_eeprom_attr))
+		goto exit_bin_attr;
+
+	return 0;
+
+      exit_bin_attr:
+	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
+      exit:
+	return err;
+}
+
+static int ds1682_remove(struct i2c_client *client)
+{
+	sysfs_remove_bin_file(&client->dev.kobj, &ds1682_eeprom_attr);
+	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
+
+	return 0;
+}
+
+static struct i2c_driver ds1682_driver = {
+	.driver = {
+		.name = "ds1682",
+	},
+	.probe = ds1682_probe,
+	.remove = ds1682_remove,
+};
+
+static int __init ds1682_init(void)
+{
+	return i2c_add_driver(&ds1682_driver);
+}
+
+static void __exit ds1682_exit(void)
+{
+	i2c_del_driver(&ds1682_driver);
+}
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("DS1682 Elapsed Time Indicator driver");
+MODULE_LICENSE("GPL");
+
+module_init(ds1682_init);
+module_exit(ds1682_exit);

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Jean Delvare]

Hi Grant,

On Sun, 13 May 2007 01:43:42 -0600, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
> Here is the 3rd iteration with the following changes:
> - Modified to be an i2c "new style" driver based on David Brownell's work

Can we see the corresponding architecture patch (device declaration)?

> - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
>   reads instead of a single byte at a time.
> 
>  drivers/i2c/chips/ds1682.c |  270 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 270 insertions(+), 0 deletions(-)

Where have the changes to Kconfig and Makefile gone? Without them, this
patch is a no-op.

BTW, the config option shouldn't be named SENSORS_DS1682 as your
original patch had - this device isn't a sensor. It should be just
"DS1682".

> 
> diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
> new file mode 100644
> index 0000000..0a4a89e
> --- /dev/null
> +++ b/drivers/i2c/chips/ds1682.c
> @@ -0,0 +1,270 @@
> +/*
> + * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
> + *
> + * Written by: Grant Likely <grant.likely@secretlab.ca>
> + *
> + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> + * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
> + * Copyright (C) 2000 Russell King
> + *
> + * Derived from ds1337 real time clock device driver

Technically, this is the ds1337 driver on which James and Russell have
a copyright, not yours. I don't think there is anything left from the
code you originally copied from, BTW.

> + *
> + * 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.
> + */
> +
> +/*
> + * The DS1682 elapsed timer recorder is a simple device that implements
> + * one elapsed time counter, one event counter, an alarm signal and 10
> + * bytes of general purpose EEPROM.
> + *
> + * This driver provides access to the DS1682 counters and user data via
> + * the sysfs.  The following attributes are added to the device node:
> + *     elapsed_time (u32): Total elapsed event time in ms resolution
> + *     alarm_time (u32): When elapsed time exceeds the value in alarm_time,
> + *                       then the alarm pin is asserted.
> + *     event_count (u16): number of times the event pin has gone low.
> + *     eeprom (u8[10]): general purpose EEPROM
> + *
> + * Counter registers and user data are both read/write unless the device
> + * has been write protected.  This driver does not support turning off write
> + * protection.  Once write protection is turned on, it is impossible to
> + * turn it off again, so I have left the feature out of this driver to avoid
> + * accidental enabling, but it is trivial to add write protect support.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/string.h>
> +#include <linux/list.h>
> +#include <linux/sysfs.h>
> +#include <linux/ctype.h>
> +#include <linux/hwmon-sysfs.h>
> +
> +/* Device registers */
> +#define DS1682_REG_CONFIG		0x00
> +#define DS1682_REG_ALARM		0x01
> +#define DS1682_REG_ELAPSED		0x05
> +#define DS1682_REG_EVT_CNTR		0x09
> +#define DS1682_REG_EEPROM		0x0b
> +#define DS1682_REG_RESET		0x1d
> +#define DS1682_REG_WRITE_DISABLE	0x1e
> +#define DS1682_REG_WRITE_MEM_DISABLE	0x1f
> +
> +#define DS1682_EEPROM_SIZE		10
> +
> +/*
> + * Generic counter attributes
> + */
> +static ssize_t ds1682_show(struct device *dev, struct device_attribute *attr,
> +			   char *buf)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	__le32 val = 0;
> +	int rc;
> +
> +	dev_dbg(dev, "ds1682_show() called on %s\n", attr->attr.name);
> +
> +	/* Read the register */
> +	rc = i2c_smbus_read_i2c_block_data(client, sattr->index, sattr->nr,
> +					   (u8 *) & val);
> +	if (rc < 0)
> +		return -EIO;
> +
> +	/* Special case: the 32 bit regs are time values with 1/4s
> +	 * resolution, scale them up to milliseconds */
> +	if (sattr->nr == 4)
> +		return sprintf(buf, "%lli\n", ((u64) le32_to_cpu(val)) * 250);

%llu

> +
> +	/* Format the output string and return # of bytes */
> +	return sprintf(buf, "%li\n", (long)le32_to_cpu(val));
> +}
> +
> +static ssize_t ds1682_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> +	struct i2c_client *client = to_i2c_client(dev);
> +	char *endp;
> +	u64 val;
> +	__le32 val_le;
> +	int rc;
> +
> +	dev_dbg(dev, "ds1682_store() called on %s\n", attr->attr.name);
> +
> +	/* Decode input */
> +	val = simple_strtoull(buf, &endp, 0);
> +	if (buf == endp) {
> +		dev_dbg(dev, "input string not a number\n");
> +		return -EIO;

Rather -EINVAL, this isn't an I/O error.

> +	}
> +
> +	/* Special case: the 32 bit regs are time values with 1/4s
> +	 * resolution, scale input down to quarter-seconds */
> +	if (sattr->nr == 4)
> +		do_div(val, 250);
> +
> +	/* write out the value */
> +	val_le = cpu_to_le32(val);
> +	rc = i2c_smbus_write_i2c_block_data(client, sattr->index, sattr->nr,
> +					    (u8 *) & val);

Shouldn't this be (u8 *)&val_le?

> +	if (rc < 0) {
> +		dev_err(dev, "register write failed; reg=0x%x, size=%i\n",
> +			sattr->index, sattr->nr);
> +		return -EIO;
> +	}
> +
> +	return count;
> +}
> +
> +/*
> + * Simple register attributes
> + */
> +SENSOR_DEVICE_ATTR_2(config, S_IRUGO, ds1682_show, NULL, DS1682_REG_CONFIG, 1);

Bug! The last two arguments are swapped.

Also, I don't see the point of exporting this value to user-space, as
the format is proprietary.

> +SENSOR_DEVICE_ATTR_2(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> +		     4, DS1682_REG_ELAPSED);
> +SENSOR_DEVICE_ATTR_2(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> +		     4, DS1682_REG_ALARM);
> +SENSOR_DEVICE_ATTR_2(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> +		     2, DS1682_REG_EVT_CNTR);
> +
> +static const struct attribute_group ds1682_group = {
> +	.attrs = (struct attribute *[]) {
> +		&sensor_dev_attr_config.dev_attr.attr,
> +		&sensor_dev_attr_elapsed_time.dev_attr.attr,
> +		&sensor_dev_attr_alarm_time.dev_attr.attr,
> +		&sensor_dev_attr_event_count.dev_attr.attr,
> +		NULL,
> +	},
> +};

I'm not sure if all compilers support this.

> +
> +/*
> + * User data attribute
> + */
> +static ssize_t ds1682_eeprom_read(struct kobject *kobj, char *buf, loff_t off,
> +				  size_t count)
> +{
> +	struct i2c_client *client = kobj_to_i2c_client(kobj);
> +	int rc;
> +
> +	dev_dbg(&client->dev, "ds1682_eeprom_read(p=%p, off=%lli, c=%zi)\n",
> +		buf, off, count);
> +
> +	if (off > DS1682_EEPROM_SIZE)

Should be ">=", otherwise you might end up doing a 0-length I2C block
read, which is bad.

> +		return 0;
> +
> +	if (off + count > DS1682_EEPROM_SIZE)
> +		count = DS1682_EEPROM_SIZE - off;
> +
> +	rc = i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + off,
> +					   count, buf);
> +	if (rc < 0)
> +		return -EIO;
> +
> +	return count;
> +}
> +
> +static ssize_t ds1682_eeprom_write(struct kobject *kobj, char *buf, loff_t off,
> +				   size_t count)
> +{
> +	struct i2c_client *client = kobj_to_i2c_client(kobj);
> +
> +	dev_dbg(&client->dev, "ds1682_eeprom_write(p=%p, off=%lli, c=%zi)\n",
> +		buf, off, count);
> +
> +	if (off >= DS1682_EEPROM_SIZE)
> +		return -ENOSPC;
> +
> +	if (off + count > DS1682_EEPROM_SIZE)
> +		count = DS1682_EEPROM_SIZE - off;
> +
> +	/* Write out to the device */
> +	if (i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + off,
> +					   count, buf) < 0)
> +		return -EIO;
> +
> +	return count;
> +}
> +
> +static struct bin_attribute ds1682_eeprom_attr = {
> +	.attr = {
> +		.name = "eeprom",
> +		.mode = S_IRUGO | S_IWUSR,
> +		.owner = THIS_MODULE,
> +	},
> +	.size = DS1682_EEPROM_SIZE,
> +	.read = ds1682_eeprom_read,
> +	.write = ds1682_eeprom_write,
> +};
> +
> +/*
> + * Called when a device is found at the ds1682 address
> + */

This comment is no longer true.

> +static int ds1682_probe(struct i2c_client *client)
> +{
> +	int err = 0;
> +
> +	if (client->addr != 0x6b) {
> +		dev_err(&client->dev, "%x is not a valid address\n",
> +			client->addr);
> +		return -ENODEV;
> +	}

Why check this? You should trust the person who wrote the device
definition in the platform code. The only reason why the older i2c chip
drivers did address checks is because that was one preliminary
device identification step. With the new model, you don't need to
identify the device, you already know what's there as per
architecture-level device declaration.

Not checking the address even has a benefit: if a compatible device
exists (today or in the future) with a different address, we don't need
to patch the driver to support it.

> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
> +				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {

This no longer reflects what the driver is using.

> +		dev_err(&client->dev, "i2c bus does not support the ds1682\n");
> +		goto exit;
> +	}
> +
> +	if (sysfs_create_group(&client->dev.kobj, &ds1682_group))

Missing "err = ".

> +		goto exit;
> +
> +	if (sysfs_create_bin_file(&client->dev.kobj, &ds1682_eeprom_attr))

Missing "err = ".

> +		goto exit_bin_attr;
> +
> +	return 0;
> +
> +      exit_bin_attr:
> +	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
> +      exit:
> +	return err;
> +}
> +
> +static int ds1682_remove(struct i2c_client *client)
> +{
> +	sysfs_remove_bin_file(&client->dev.kobj, &ds1682_eeprom_attr);
> +	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver ds1682_driver = {
> +	.driver = {
> +		.name = "ds1682",
> +	},
> +	.probe = ds1682_probe,
> +	.remove = ds1682_remove,
> +};
> +
> +static int __init ds1682_init(void)
> +{
> +	return i2c_add_driver(&ds1682_driver);
> +}
> +
> +static void __exit ds1682_exit(void)
> +{
> +	i2c_del_driver(&ds1682_driver);
> +}
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("DS1682 Elapsed Time Indicator driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(ds1682_init);
> +module_exit(ds1682_exit);


-- 
Jean Delvare

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Grant Likely]

Ugh.  Well that was a rather sloppy patch on my part.  I'll fix up and resubmit.

On 5/14/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Grant,
>
> On Sun, 13 May 2007 01:43:42 -0600, Grant Likely wrote:
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > Here is the 3rd iteration with the following changes:
> > - Modified to be an i2c "new style" driver based on David Brownell's work
>
> Can we see the corresponding architecture patch (device declaration)?

Yes, I'll include it in the commit comment.  It is for a board that
isn't in mainline (yet) so a seperate patch doesn't make much sense.

>
> > - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
> >   reads instead of a single byte at a time.
> >
> >  drivers/i2c/chips/ds1682.c |  270 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 files changed, 270 insertions(+), 0 deletions(-)
>
> Where have the changes to Kconfig and Makefile gone? Without them, this
> patch is a no-op.

Oops, eaten by stgit (or rather; eaten by my inexperience with stgit.

>
> BTW, the config option shouldn't be named SENSORS_DS1682 as your
> original patch had - this device isn't a sensor. It should be just
> "DS1682".

Done.

BTW, I've just left the driver in drivers/i2c/chips.  It seems
sufficiently different from an rtc (and the interface is totally
difference) that putting it in drivers/rtc seems wrong.  If you
disagree, I can change this.

>
> >
> > diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
> > new file mode 100644
> > index 0000000..0a4a89e
> > --- /dev/null
> > +++ b/drivers/i2c/chips/ds1682.c
> > @@ -0,0 +1,270 @@
> > +/*
> > + * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
> > + *
> > + * Written by: Grant Likely <grant.likely@secretlab.ca>
> > + *
> > + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> > + * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
> > + * Copyright (C) 2000 Russell King
> > + *
> > + * Derived from ds1337 real time clock device driver
j>
> Technically, this is the ds1337 driver on which James and Russell have
> a copyright, not yours. I don't think there is anything left from the
> code you originally copied from, BTW.

ok, fixed.  I like to err on the side of caution when it comes to
copyright notices.

> > + * Simple register attributes
> > + */
> > +SENSOR_DEVICE_ATTR_2(config, S_IRUGO, ds1682_show, NULL, DS1682_REG_CONFIG, 1);
>
> Bug! The last two arguments are swapped.
>
> Also, I don't see the point of exporting this value to user-space, as
> the format is proprietary.

You're right.  Removed.

> > +SENSOR_DEVICE_ATTR_2(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> > +                  4, DS1682_REG_ELAPSED);
> > +SENSOR_DEVICE_ATTR_2(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> > +                  4, DS1682_REG_ALARM);
> > +SENSOR_DEVICE_ATTR_2(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
> > +                  2, DS1682_REG_EVT_CNTR);
> > +
> > +static const struct attribute_group ds1682_group = {
> > +     .attrs = (struct attribute *[]) {
> > +             &sensor_dev_attr_config.dev_attr.attr,
> > +             &sensor_dev_attr_elapsed_time.dev_attr.attr,
> > +             &sensor_dev_attr_alarm_time.dev_attr.attr,
> > +             &sensor_dev_attr_event_count.dev_attr.attr,
> > +             NULL,
> > +     },
> > +};
>
> I'm not sure if all compilers support this.

I think it should be okay.  It is used abundantly in the
arch/ppc/syslib/*_devices.c code, and that code is compiled with a lot
of different compiler versions.

> > +static ssize_t ds1682_eeprom_read(struct kobject *kobj, char *buf, loff_t off,
> > +                               size_t count)
> > +{
> > +     struct i2c_client *client = kobj_to_i2c_client(kobj);
> > +     int rc;
> > +
> > +     dev_dbg(&client->dev, "ds1682_eeprom_read(p=%p, off=%lli, c=%zi)\n",
> > +             buf, off, count);
> > +
> > +     if (off > DS1682_EEPROM_SIZE)
>
> Should be ">=", otherwise you might end up doing a 0-length I2C block
> read, which is bad.

Right, good catch

> > +
> > +/*
> > + * Called when a device is found at the ds1682 address
> > + */
>
> This comment is no longer true.
>
> > +static int ds1682_probe(struct i2c_client *client)
> > +{
> > +     int err = 0;
> > +
> > +     if (client->addr != 0x6b) {
> > +             dev_err(&client->dev, "%x is not a valid address\n",
> > +                     client->addr);
> > +             return -ENODEV;
> > +     }
>
> Why check this? You should trust the person who wrote the device
> definition in the platform code. The only reason why the older i2c chip
> drivers did address checks is because that was one preliminary
> device identification step. With the new model, you don't need to
> identify the device, you already know what's there as per
> architecture-level device declaration.

Me being paranoid a guess.  Removed.

>
> Not checking the address even has a benefit: if a compatible device
> exists (today or in the future) with a different address, we don't need
> to patch the driver to support it.
>
> > +
> > +     if (!i2c_check_functionality(client->adapter,
> > +                                  I2C_FUNC_SMBUS_READ_BYTE_DATA |
> > +                                  I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
>
> This no longer reflects what the driver is using.
>
> > +             dev_err(&client->dev, "i2c bus does not support the ds1682\n");
> > +             goto exit;
> > +     }
> > +
> > +     if (sysfs_create_group(&client->dev.kobj, &ds1682_group))
>
> Missing "err = ".

Gah!

Thanks for the comments,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Jean Delvare]

On Mon, 14 May 2007 11:29:04 -0600, Grant Likely wrote:
> BTW, I've just left the driver in drivers/i2c/chips.  It seems
> sufficiently different from an rtc (and the interface is totally
> difference) that putting it in drivers/rtc seems wrong.  If you
> disagree, I can change this.

That's fine with me, I have no strong opinion on this.

> Thanks for the comments,
> g.

You're welcome.

-- 
Jean Delvare

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Andrew Morton]

On Sun, 13 May 2007 01:43:42 -0600
Grant Likely <grant.likely@secretlab.ca> wrote:

> 
> Here is the 3rd iteration with the following changes:
> - Modified to be an i2c "new style" driver based on David Brownell's work
> - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
>   reads instead of a single byte at a time.
> 
>  drivers/i2c/chips/ds1682.c |  270 ++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 270 insertions(+), 0 deletions(-)

This version of the patch omitted the changes to drivers/i2c/chips/Kconfig
and drivers/i2c/chips/Makefile, which I assume was accidental.

As usual, I generated the delta so we can see what was done:


From: Grant Likely <grant.likely@secretlab.ca>

- Modified to be an i2c "new style" driver based on David Brownell's work
- uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
  reads instead of a single byte at a time.

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
Cc: Jean Delvare <khali@linux-fr.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 drivers/i2c/chips/ds1682.c |  333 ++++++++++++-----------------------
 1 files changed, 121 insertions(+), 212 deletions(-)

diff -puN drivers/i2c/chips/ds1682.c~i2c-add-driver-for-dallas-ds1682-elapsed-time-recorder-update drivers/i2c/chips/ds1682.c
--- a/drivers/i2c/chips/ds1682.c~i2c-add-driver-for-dallas-ds1682-elapsed-time-recorder-update
+++ a/drivers/i2c/chips/ds1682.c
@@ -16,132 +16,106 @@
 
 /*
  * The DS1682 elapsed timer recorder is a simple device that implements
- * one elapsed time counters, one event counter, an alarm signal and 10
+ * one elapsed time counter, one event counter, an alarm signal and 10
  * bytes of general purpose EEPROM.
  *
  * This driver provides access to the DS1682 counters and user data via
  * the sysfs.  The following attributes are added to the device node:
- *     elapsed_time (u32): Total elapsed event time in 1/4s resolution
+ *     elapsed_time (u32): Total elapsed event time in ms resolution
  *     alarm_time (u32): When elapsed time exceeds the value in alarm_time,
  *                       then the alarm pin is asserted.
  *     event_count (u16): number of times the event pin has gone low.
- *     user_data (u8[10]): general purpose EEPROM
+ *     eeprom (u8[10]): general purpose EEPROM
  *
  * Counter registers and user data are both read/write unless the device
- * has been write protected.  This driver does not support turning on write
+ * has been write protected.  This driver does not support turning off write
  * protection.  Once write protection is turned on, it is impossible to
- * turn off so I have left the feature out of this driver to avoid
+ * turn it off again, so I have left the feature out of this driver to avoid
  * accidental enabling, but it is trivial to add write protect support.
  *
  */
 
-#include <linux/device.h>
 #include <linux/module.h>
 #include <linux/init.h>
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/string.h>
-#include <linux/bcd.h>
 #include <linux/list.h>
 #include <linux/sysfs.h>
 #include <linux/ctype.h>
+#include <linux/hwmon-sysfs.h>
 
 /* Device registers */
-#define DS1682_ADDR			0x6B
-
 #define DS1682_REG_CONFIG		0x00
 #define DS1682_REG_ALARM		0x01
 #define DS1682_REG_ELAPSED		0x05
 #define DS1682_REG_EVT_CNTR		0x09
-#define DS1682_REG_USER_DATA		0x0b
+#define DS1682_REG_EEPROM		0x0b
 #define DS1682_REG_RESET		0x1d
 #define DS1682_REG_WRITE_DISABLE	0x1e
 #define DS1682_REG_WRITE_MEM_DISABLE	0x1f
 
-/*
- * Probing class
- */
-static unsigned short normal_i2c[] = { DS1682_ADDR, I2C_CLIENT_END };
-
-I2C_CLIENT_INSMOD;
-
-/*
- * Low level chip access functions
- */
-static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
-{
-	u8 *bytes = buf;
-	int val;
-
-	while (len--) {
-		val = i2c_smbus_read_byte_data(client, reg++);
-		if (val < 0)
-			return -EIO;
-		*bytes++ = val;
-	}
-	return 0;
-}
+#define DS1682_EEPROM_SIZE		10
 
 /*
  * Generic counter attributes
  */
-static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize);
-
-static ssize_t ds1682_show(struct device *dev,
-			   struct device_attribute *attr, char *buf)
+static ssize_t ds1682_show(struct device *dev, struct device_attribute *attr,
+			   char *buf)
 {
-	u32 val = 0;
-	int reg;
-	int regsize;
-
-	dev_dbg(dev, "ds1682_show() called for %s\n", attr->attr.name);
-
-	/* Get the register address */
-	reg = ds1682_attr_to_reg(attr, &regsize);
-	if (reg < 0)
-		return -EIO;
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
+	struct i2c_client *client = to_i2c_client(dev);
+	__le32 val = 0;
+	int rc;
+
+	dev_dbg(dev, "ds1682_show() called on %s\n", attr->attr.name);
 
 	/* Read the register */
-	if (ds1682_read(to_i2c_client(dev), reg, &val, regsize))
+	rc = i2c_smbus_read_i2c_block_data(client, sattr->index, sattr->nr,
+					   (u8 *) & val);
+	if (rc < 0)
 		return -EIO;
 
+	/* Special case: the 32 bit regs are time values with 1/4s
+	 * resolution, scale them up to milliseconds */
+	if (sattr->nr == 4)
+		return sprintf(buf, "%lli\n", ((u64) le32_to_cpu(val)) * 250);
+
 	/* Format the output string and return # of bytes */
-	return sprintf(buf, "0x%.2x\n", le32_to_cpu(val));
+	return sprintf(buf, "%li\n", (long)le32_to_cpu(val));
 }
 
-static ssize_t ds1682_store(struct device *dev,
-			    struct device_attribute *attr, const char *buf,
-			    size_t count)
+static ssize_t ds1682_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
 {
+	struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
 	struct i2c_client *client = to_i2c_client(dev);
-	int reg;
-	int regsize;
 	char *endp;
-	u32 val;
+	u64 val;
+	__le32 val_le;
 	int rc;
 
-	/* Sanitize the input */
-	reg = ds1682_attr_to_reg(attr, &regsize);
-
-	if ((reg < 0) || (count >= 16) || (buf[count] != '\0')) {
-		dev_dbg(dev, "insane input; reg=0x%i count=%zd%s\n",
-			reg, count, buf[count] == '\0' ? " unterminated" : "");
-		return -EIO;
-	}
+	dev_dbg(dev, "ds1682_store() called on %s\n", attr->attr.name);
 
 	/* Decode input */
-	val = simple_strtoul(buf, &endp, 0);
+	val = simple_strtoull(buf, &endp, 0);
 	if (buf == endp) {
 		dev_dbg(dev, "input string not a number\n");
 		return -EIO;
 	}
 
+	/* Special case: the 32 bit regs are time values with 1/4s
+	 * resolution, scale input down to quarter-seconds */
+	if (sattr->nr == 4)
+		do_div(val, 250);
+
 	/* write out the value */
-	val = cpu_to_le32(val);
-	rc = i2c_smbus_write_i2c_block_data(client, reg, regsize, (void *)&val);
+	val_le = cpu_to_le32(val);
+	rc = i2c_smbus_write_i2c_block_data(client, sattr->index, sattr->nr,
+					    (u8 *) & val);
 	if (rc < 0) {
 		dev_err(dev, "register write failed; reg=0x%x, size=%i\n",
-			reg, regsize);
+			sattr->index, sattr->nr);
 		return -EIO;
 	}
 
@@ -151,196 +125,131 @@ static ssize_t ds1682_store(struct devic
 /*
  * Simple register attributes
  */
-DEVICE_ATTR(config, S_IRUGO, ds1682_show, NULL);
-DEVICE_ATTR(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
-DEVICE_ATTR(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
-DEVICE_ATTR(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
-
-/* Get register size and address from device_attribute structure.  This
- * function must come after the DEVICE_ATTR() lines as it depends on the
- * device_attribute lines being declared */
-static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize)
-{
-	if (attr == &dev_attr_elapsed_time) {
-		*regsize = 4;
-		return DS1682_REG_ELAPSED;
-	}
-
-	if (attr == &dev_attr_alarm_time) {
-		*regsize = 4;
-		return DS1682_REG_ALARM;
-	}
-
-	if (attr == &dev_attr_event_count) {
-		*regsize = 2;
-		return DS1682_REG_EVT_CNTR;
-	}
-
-	if (attr == &dev_attr_config) {
-		*regsize = 1;
-		return DS1682_REG_CONFIG;
-	}
-
-	pr_debug("Cannot find registers for device_attribute %p\n", attr);
-	return -ENODEV;
-}
+SENSOR_DEVICE_ATTR_2(config, S_IRUGO, ds1682_show, NULL, DS1682_REG_CONFIG, 1);
+SENSOR_DEVICE_ATTR_2(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     4, DS1682_REG_ELAPSED);
+SENSOR_DEVICE_ATTR_2(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     4, DS1682_REG_ALARM);
+SENSOR_DEVICE_ATTR_2(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store,
+		     2, DS1682_REG_EVT_CNTR);
+
+static const struct attribute_group ds1682_group = {
+	.attrs = (struct attribute *[]) {
+		&sensor_dev_attr_config.dev_attr.attr,
+		&sensor_dev_attr_elapsed_time.dev_attr.attr,
+		&sensor_dev_attr_alarm_time.dev_attr.attr,
+		&sensor_dev_attr_event_count.dev_attr.attr,
+		NULL,
+	},
+};
 
 /*
  * User data attribute
  */
-static ssize_t ds1682_user_data_show(struct device *dev,
-				     struct device_attribute *attr, char *buf)
+static ssize_t ds1682_eeprom_read(struct kobject *kobj, char *buf, loff_t off,
+				  size_t count)
 {
-	char *end = buf;
-	u8 val[10];
-	int i;
+	struct i2c_client *client = kobj_to_i2c_client(kobj);
+	int rc;
 
-	if (ds1682_read(to_i2c_client(dev), DS1682_REG_USER_DATA, &val, 10))
-		return -EIO;
+	dev_dbg(&client->dev, "ds1682_eeprom_read(p=%p, off=%lli, c=%zi)\n",
+		buf, off, count);
 
-	for (i = 0; i < 10; i++)
-		end += sprintf(end, "%.2x ", val[i]);
+	if (off > DS1682_EEPROM_SIZE)
+		return 0;
 
-	*(end - 1) = '\n';	/* Eliminate trailing space */
-	return end - buf;
+	if (off + count > DS1682_EEPROM_SIZE)
+		count = DS1682_EEPROM_SIZE - off;
+
+	rc = i2c_smbus_read_i2c_block_data(client, DS1682_REG_EEPROM + off,
+					   count, buf);
+	if (rc < 0)
+		return -EIO;
+
+	return count;
 }
 
-static ssize_t ds1682_user_data_store(struct device *dev,
-				      struct device_attribute *attr,
-				      const char *buf, size_t count)
+static ssize_t ds1682_eeprom_write(struct kobject *kobj, char *buf, loff_t off,
+				   size_t count)
 {
-	u8 data[10];
-	char *endp;
-	int bytecount = 0;
-
-	/* Check input for sanity */
-	if ((count >= 80) || (buf[count] != '\0')) {
-		dev_dbg(dev, "insane input; count=%zd%s\n",
-			count, buf[count] == '\0' ? " unterminated" : "");
-		return -EIO;
-	}
+	struct i2c_client *client = kobj_to_i2c_client(kobj);
 
-	/* Parse the data */
-	while (bytecount < 10) {
-		while (isspace(*buf))
-			buf++;
-
-		data[bytecount] = simple_strtoul(buf, &endp, 16);
-		if (endp == buf)	/* make sure it's a real data value */
-			break;
+	dev_dbg(&client->dev, "ds1682_eeprom_write(p=%p, off=%lli, c=%zi)\n",
+		buf, off, count);
 
-		buf = endp;
-		bytecount++;
-	}
-	while (isspace(*endp))
-		endp++;
+	if (off >= DS1682_EEPROM_SIZE)
+		return -ENOSPC;
 
-	/* Abort on invalid data */
-	if ((bytecount == 0) || (*endp != '\0')) {
-		dev_dbg(dev, "invalid input *buf=\"%s\" *endp=\"%s\"\n",
-			buf, endp);
-		return -EIO;
-	}
+	if (off + count > DS1682_EEPROM_SIZE)
+		count = DS1682_EEPROM_SIZE - off;
 
 	/* Write out to the device */
-	if (i2c_smbus_write_i2c_block_data(to_i2c_client(dev),
-					   DS1682_REG_USER_DATA, bytecount,
-					   data) < 0)
+	if (i2c_smbus_write_i2c_block_data(client, DS1682_REG_EEPROM + off,
+					   count, buf) < 0)
 		return -EIO;
+
 	return count;
 }
 
-DEVICE_ATTR(user_data, S_IRUGO | S_IWUSR, ds1682_user_data_show,
-	    ds1682_user_data_store);
+static struct bin_attribute ds1682_eeprom_attr = {
+	.attr = {
+		.name = "eeprom",
+		.mode = S_IRUGO | S_IWUSR,
+		.owner = THIS_MODULE,
+	},
+	.size = DS1682_EEPROM_SIZE,
+	.read = ds1682_eeprom_read,
+	.write = ds1682_eeprom_write,
+};
 
 /*
  * Called when a device is found at the ds1682 address
  */
-static struct i2c_driver ds1682_driver;
-static int ds1682_detect(struct i2c_adapter *adapter, int address, int kind)
+static int ds1682_probe(struct i2c_client *client)
 {
-	struct i2c_client *new_client;
 	int err = 0;
 
-	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
-				     I2C_FUNC_I2C))
-		goto exit;
+	if (client->addr != 0x6b) {
+		dev_err(&client->dev, "%x is not a valid address\n",
+			client->addr);
+		return -ENODEV;
+	}
 
-	if (!(new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
-		err = -ENOMEM;
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_BYTE_DATA |
+				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+		dev_err(&client->dev, "i2c bus does not support the ds1682\n");
 		goto exit;
 	}
 
-	new_client->addr = address;
-	new_client->adapter = adapter;
-	new_client->driver = &ds1682_driver;
-	new_client->flags = 0;
-
-	/* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */
-
-	/* We can fill in the remaining client fields */
-	strlcpy(new_client->name, "ds1682", I2C_NAME_SIZE);
-
-	/* Tell the I2C layer a new client has arrived */
-	if ((err = i2c_attach_client(new_client)))
-		goto exit_free;
-
-	if (device_create_file(&new_client->dev, &dev_attr_config))
-		goto exit_attr_config;
-	if (device_create_file(&new_client->dev, &dev_attr_elapsed_time))
-		goto exit_attr_elapsed;
-	if (device_create_file(&new_client->dev, &dev_attr_alarm_time))
-		goto exit_attr_alarm;
-	if (device_create_file(&new_client->dev, &dev_attr_event_count))
-		goto exit_attr_event;
-	if (device_create_file(&new_client->dev, &dev_attr_user_data))
-		goto exit_attr_userdata;
+	if (sysfs_create_group(&client->dev.kobj, &ds1682_group))
+		goto exit;
+
+	if (sysfs_create_bin_file(&client->dev.kobj, &ds1682_eeprom_attr))
+		goto exit_bin_attr;
 
 	return 0;
 
-      exit_attr_userdata:
-	device_remove_file(&new_client->dev, &dev_attr_event_count);
-      exit_attr_event:
-	device_remove_file(&new_client->dev, &dev_attr_alarm_time);
-      exit_attr_alarm:
-	device_remove_file(&new_client->dev, &dev_attr_elapsed_time);
-      exit_attr_elapsed:
-	device_remove_file(&new_client->dev, &dev_attr_config);
-      exit_attr_config:
-	i2c_detach_client(new_client);
-      exit_free:
-	kfree(new_client);
+      exit_bin_attr:
+	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
       exit:
 	return err;
 }
 
-static int ds1682_attach_adapter(struct i2c_adapter *adapter)
+static int ds1682_remove(struct i2c_client *client)
 {
-	return i2c_probe(adapter, &addr_data, ds1682_detect);
-}
-
-static int ds1682_detach_client(struct i2c_client *client)
-{
-	int err;
-	device_remove_file(&client->dev, &dev_attr_user_data);
-	device_remove_file(&client->dev, &dev_attr_event_count);
-	device_remove_file(&client->dev, &dev_attr_alarm_time);
-	device_remove_file(&client->dev, &dev_attr_elapsed_time);
-	device_remove_file(&client->dev, &dev_attr_config);
-
-	if ((err = i2c_detach_client(client)))
-		return err;
+	sysfs_remove_bin_file(&client->dev.kobj, &ds1682_eeprom_attr);
+	sysfs_remove_group(&client->dev.kobj, &ds1682_group);
 
-	kfree(client);
 	return 0;
 }
 
 static struct i2c_driver ds1682_driver = {
 	.driver = {
-		   .name = "ds1682",
-		   },
-	.attach_adapter = ds1682_attach_adapter,
-	.detach_client = ds1682_detach_client,
+		.name = "ds1682",
+	},
+	.probe = ds1682_probe,
+	.remove = ds1682_remove,
 };
 
 static int __init ds1682_init(void)
_

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Andrew Morton]

On Sun, 13 May 2007 01:43:42 -0600 Grant Likely <grant.likely@secretlab.ca> wrote:

> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> 
> Here is the 3rd iteration with the following changes:
> - Modified to be an i2c "new style" driver based on David Brownell's work
> - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
>   reads instead of a single byte at a time.\

What new prototype for i2c_smbus_read_i2c_block_data()?

drivers/i2c/chips/ds1682.c: In function 'ds1682_show':
drivers/i2c/chips/ds1682.c:75: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
drivers/i2c/chips/ds1682.c:75: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'
drivers/i2c/chips/ds1682.c: In function 'ds1682_eeprom_read':                                                                                                                             drivers/i2c/chips/ds1682.c:165: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
drivers/i2c/chips/ds1682.c:165: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Grant Likely]

On 5/15/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Sun, 13 May 2007 01:43:42 -0600 Grant Likely <grant.likely@secretlab.ca> wrote:
>
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >
> > Here is the 3rd iteration with the following changes:
> > - Modified to be an i2c "new style" driver based on David Brownell's work
> > - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
> >   reads instead of a single byte at a time.\
>
> What new prototype for i2c_smbus_read_i2c_block_data()?
>
> drivers/i2c/chips/ds1682.c: In function 'ds1682_show':
> drivers/i2c/chips/ds1682.c:75: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
> drivers/i2c/chips/ds1682.c:75: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'
> drivers/i2c/chips/ds1682.c: In function 'ds1682_eeprom_read':                                                                                                                             drivers/i2c/chips/ds1682.c:165: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
> drivers/i2c/chips/ds1682.c:165: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'

Sorry.  I forgot to drop linux-kernel when sending this patch.  It
depends on another patch that hasn't been merged yet.

Cheers,
g.

-- 
Grant Likely, B.Sc. P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Jean Delvare]

Hi Andrew,

On Mon, 14 May 2007 23:34:01 -0700, Andrew Morton wrote:
> On Sun, 13 May 2007 01:43:42 -0600 Grant Likely <grant.likely@secretlab.ca> wrote:
> 
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> > 
> > Here is the 3rd iteration with the following changes:
> > - Modified to be an i2c "new style" driver based on David Brownell's work
> > - uses the new prototype for i2c_smbus_read_i2c_block_data() for block data
> >   reads instead of a single byte at a time.\
> 
> What new prototype for i2c_smbus_read_i2c_block_data()?
> 
> drivers/i2c/chips/ds1682.c: In function 'ds1682_show':
> drivers/i2c/chips/ds1682.c:75: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
> drivers/i2c/chips/ds1682.c:75: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'
> drivers/i2c/chips/ds1682.c: In function 'ds1682_eeprom_read':
> drivers/i2c/chips/ds1682.c:165: warning: passing argument 3 of 'i2c_smbus_read_i2c_block_data' makes pointer from integer without a cast
> drivers/i2c/chips/ds1682.c:165: error: too many arguments to function 'i2c_smbus_read_i2c_block_data'

Please drop your version of the DS1682 patch, and refresh my
jdelvare-i2c quilt tree. You'll get the i2c_smbus_read_i2c_block_data
prototype update as well as the final version of Grant's DS1682 patch.

You would save yourself quite some work if you were waiting for i2c
patches to come to you through my tree. I am paying attention to the
dependencies, exactly in order to prevent problems like the one above.
Not to say I am not grateful when you collect important i2c fixes which
have been sent only to LKML and that I would otherwise have missed, but
in the case of the DS1682 patch, it was pretty visible that I was taking
care of it.

Thanks,
-- 
Jean Delvare

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Jean Delvare]

Hi Grant,

On Thu, 10 May 2007 14:37:54 -0600, Grant Likely wrote:
> Thanks for the comments Jean.  I've got some questions/comments below.
> 
> On 5/10/07, Jean Delvare <khali@linux-fr.org> wrote:
> > Hi Grant,
> >
> > On Mon,  7 May 2007 14:45:42 -0600, Grant Likely wrote:
> > > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > > ---
> > >  drivers/i2c/chips/Kconfig  |   10 ++
> > >  drivers/i2c/chips/Makefile |    1 +
> > >  drivers/i2c/chips/ds1682.c |  363 ++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 374 insertions(+), 0 deletions(-)
> > >  create mode 100644 drivers/i2c/chips/ds1682.c
> > >
> >
> > Please fix the following warnings:
> >
> > drivers/i2c/chips/ds1682.c: In function 'ds1682_store':
> > drivers/i2c/chips/ds1682.c:129: warning: format '%i' expects type 'int', but argument 5 has type 'size_t'
> > drivers/i2c/chips/ds1682.c: In function 'ds1682_user_data_store':
> > drivers/i2c/chips/ds1682.c:220: warning: format '%i' expects type 'int', but argument 4 has type 'size_t'
> 
> Hmmm; I didn't see those warning here, but I'm probably running on a
> different arch (ppc32).  I'll make it more robust.

Typical warning that hows on either 32-bit or 64-bit systems but not
both. It's usually easily fixed by replacing %i by %zi.

> > > +     help
> > > +       If you say yes here you get support for Dallas Semiconductor
> > > +       DS1682 Total Elapsed Time Recorder
> > > +
> > > +       This driver can also be built as a module.  If so, the module
> > > +       will be called ds1682.
> > > +
> >
> > I know this isn't exactly a RTC chip, but this is still related with
> > timekeeping, so wouldn't it be better placed under drivers/rtc?
> > Alessandro, what do you think?
> 
> I was following the lead of the ds1337 and ds1374 chips, but I will
> happily move it if so desired.

Said drivers are in the process of being moved to drivers/rtc - which
is why I was asking.

> > > +/*
> > > + * Low level chip access functions
> > > + */
> > > +static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
> > > +{
> > > +     u8 *bytes = buf;
> > > +     int val;
> > > +
> > > +     while (len--) {
> > > +             val = i2c_smbus_read_byte_data(client, reg++);
> > > +             if (val < 0)
> > > +                     return -EIO;
> > > +             *bytes++ = val;
> > > +     }
> > > +     return 0;
> > > +}
> >
> > Did you check if the device supports I2C block reads? This would be
> > much faster, and might also solve consistency issues. Are you certain
> > that the above brings you back bytes which all belong to the same
> > "time measurement"? Does the device latch the register values on the
> > first read?
> >
> > I find it strange that you use an I2C block write when writing values
> > to the chip, but individual byte reads in the other direction.
> 
> Mostly because i2c_smbus_write_i2c_block_data allows me to specify the
> tranfer length, but i2c_smbus_read_i2c_block_data does not.  I
> couldn't figure out how to request an exact transfer size and I was
> using ds1374 as an example which also transfers byte-at-a-time.

Ah. Very good point. I always considered this
(i2c_smbus_read_i2c_block_data not allowing the caller to specify the
transfer length) a bug. Maybe the time has come to finally fix it, so
that drivers which need that kind of transaction can finally use it.
I'll work on it, patch to follow.

> > > +
> > > +     /* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */
> >
> > The "kind" value has nothing to do with the address. It tells you if
> > the user asked (through a module parameter) for the device
> > detection/identification to be skipped. Which brings me to the point
> > that your driver does no device identification at all. Fortunately,
> > address 0x6b isn't very popular, but I would still like to see some
> > detection code. Is there a way to identify the DS1682? I know that such
> > devices usually (unfortunately) don't have an identification register,
> > but maybe there are unused configuration bits which can be checked? Or
> > you can check the number of registers?
> 
> :-(  No, there is no way to id the device from the contents of the
> registers.  How do I test for the number of registers?

Try a byte read of the last register, followed by a byte read of the
next register (which doesn't exist). Some chips will succeed the first
read and fail the second one, allowing you to test the number of
registers. The pca9539 driver does this if you want an example.

But it is also possible that reading a non-existent register will
return something (not an error), such as the value of the last read
register, or the value of an arbitrary register. You really have to
try. The user-space tool "i2cdump" comes in handy for such tests.

If there really is no way to detect the chip, this is one more reason
to switch to the new i2c model.

> > > +
> > > +     return 0;
> > > +
> > > +      exit_attr_userdata:
> >
> > Please use a single space for label indentation.
> 
> This was done by scripts/Lindent.  I'll change it if you really want
> me to.  I've gotten into the habit of running all my code through
> Lindent before committing to avoid as many whitespace complaints as
> possible.
> 
> If this spacing is wrong, does Lindent need to be fixed?
> 
> > > +static struct i2c_driver ds1682_driver = {
> > > +     .driver = {
> > > +                .name = "ds1682",
> > > +                },
> >
> > Indentation.
> 
> Ditto on Lindent.

Thanks for using Lindent, I can only encourage people to do that
especially for new drivers. As a matter of fact, the rest of your
driver was perfect with regards to indentation :) However, if Lindent
indeed did these two mistakes, it needs to be fixed.

-- 
Jean Delvare

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Grant Likely]

Thanks for the comments Jean.  I've got some questions/comments below.

On 5/10/07, Jean Delvare <khali@linux-fr.org> wrote:
> Hi Grant,
>
> On Mon,  7 May 2007 14:45:42 -0600, Grant Likely wrote:
> > Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> > ---
> >  drivers/i2c/chips/Kconfig  |   10 ++
> >  drivers/i2c/chips/Makefile |    1 +
> >  drivers/i2c/chips/ds1682.c |  363 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 374 insertions(+), 0 deletions(-)
> >  create mode 100644 drivers/i2c/chips/ds1682.c
> >
>
> Please fix the following warnings:
>
> drivers/i2c/chips/ds1682.c: In function 'ds1682_store':
> drivers/i2c/chips/ds1682.c:129: warning: format '%i' expects type 'int', but argument 5 has type 'size_t'
> drivers/i2c/chips/ds1682.c: In function 'ds1682_user_data_store':
> drivers/i2c/chips/ds1682.c:220: warning: format '%i' expects type 'int', but argument 4 has type 'size_t'

Hmmm; I didn't see those warning here, but I'm probably running on a
different arch (ppc32).  I'll make it more robust.

> > +     help
> > +       If you say yes here you get support for Dallas Semiconductor
> > +       DS1682 Total Elapsed Time Recorder
> > +
> > +       This driver can also be built as a module.  If so, the module
> > +       will be called ds1682.
> > +
>
> I know this isn't exactly a RTC chip, but this is still related with
> timekeeping, so wouldn't it be better placed under drivers/rtc?
> Alessandro, what do you think?

I was following the lead of the ds1337 and ds1374 chips, but I will
happily move it if so desired.

> > +static unsigned short normal_i2c[] = { DS1682_ADDR, I2C_CLIENT_END };
> > +
> > +I2C_CLIENT_INSMOD;
> > +
> > +/*
> > + * Low level chip access functions
> > + */
> > +static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
> > +{
> > +     u8 *bytes = buf;
> > +     int val;
> > +
> > +     while (len--) {
> > +             val = i2c_smbus_read_byte_data(client, reg++);
> > +             if (val < 0)
> > +                     return -EIO;
> > +             *bytes++ = val;
> > +     }
> > +     return 0;
> > +}
>
> Did you check if the device supports I2C block reads? This would be
> much faster, and might also solve consistency issues. Are you certain
> that the above brings you back bytes which all belong to the same
> "time measurement"? Does the device latch the register values on the
> first read?
>
> I find it strange that you use an I2C block write when writing values
> to the chip, but individual byte reads in the other direction.

Mostly because i2c_smbus_write_i2c_block_data allows me to specify the
tranfer length, but i2c_smbus_read_i2c_block_data does not.  I
couldn't figure out how to request an exact transfer size and I was
using ds1374 as an example which also transfers byte-at-a-time.

> > +static ssize_t ds1682_store(struct device *dev,
> > +                         struct device_attribute *attr, const char *buf,
> > +                         size_t count)
> > +{
> > +     struct i2c_client *client = to_i2c_client(dev);
> > +     int reg;
> > +     int regsize;
> > +     char *endp;
> > +     u32 val;
> > +     int rc;
> > +
> > +     /* Sanitize the input */
> > +     reg = ds1682_attr_to_reg(attr, &regsize);
> > +
> > +     if ((reg < 0) || (count >= 16) || (buf[count] != '\0')) {
>
> How could buf[count] not be '\0'? As far as I know this is guaranteed
> by the underlying sysfs code. I also don't see the point of checking
> the value of count.

Both of these checks are from an ignorance of sysfs.  I didn't know
what was guaranteed and based on what was writting in LDDv3, I thought
that I could end up receiving a page of *anything* from userspace.
I've just looked through the code, and fill_write_buffer() does do
what you said.  I'll remove these checks.

> > +
> > +/*
> > + * Called when a device is found at the ds1682 address
> > + */
> > +static struct i2c_driver ds1682_driver;
> > +static int ds1682_detect(struct i2c_adapter *adapter, int address, int kind)
> > +{
> > +     struct i2c_client *new_client;
>
> Please rename this to just "client". This "new_client" name is a
> disease :(

Heh; I'll submit a patch for Documentation/i2c/writing-clients too then.

> > +     if (!(new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> > +             err = -ENOMEM;
> > +             goto exit;
> > +     }
> > +
> > +     new_client->addr = address;
> > +     new_client->adapter = adapter;
> > +     new_client->driver = &ds1682_driver;
> > +     new_client->flags = 0;
>
> Not needed, the kzalloc above did it for you.

This is also from Documentation/i2c/writing-clients.  I'll fix this.

>
> > +
> > +     /* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */
>
> The "kind" value has nothing to do with the address. It tells you if
> the user asked (through a module parameter) for the device
> detection/identification to be skipped. Which brings me to the point
> that your driver does no device identification at all. Fortunately,
> address 0x6b isn't very popular, but I would still like to see some
> detection code. Is there a way to identify the DS1682? I know that such
> devices usually (unfortunately) don't have an identification register,
> but maybe there are unused configuration bits which can be checked? Or
> you can check the number of registers?

:-(  No, there is no way to id the device from the contents of the
registers.  How do I test for the number of registers?

> > +
> > +     return 0;
> > +
> > +      exit_attr_userdata:
>
> Please use a single space for label indentation.

This was done by scripts/Lindent.  I'll change it if you really want
me to.  I've gotten into the habit of running all my code through
Lindent before committing to avoid as many whitespace complaints as
possible.

If this spacing is wrong, does Lindent need to be fixed?

> > +static struct i2c_driver ds1682_driver = {
> > +     .driver = {
> > +                .name = "ds1682",
> > +                },
>
> Indentation.

Ditto on Lindent.

>
> The code is quite nice otherwise, good work!

Thanks, I'll fix it up and resubmit.

g.

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Alessandro Zummo]

On Thu, 10 May 2007 13:48:50 +0200
Jean Delvare <khali@linux-fr.org> wrote:

> I know this isn't exactly a RTC chip, but this is still related with
> timekeeping, so wouldn't it be better placed under drivers/rtc?
> Alessandro, what do you think?

 Hi,

   I don't know if the rtc subsystem can offer help/framework
 for such a kind of driver. maybe it could be extended to 
 incorporate it. probably the alarms could be handled
 in some way.

-- 

 Best regards,

 Alessandro Zummo,
  Tower Technologies - Torino, Italy

  http://www.towertech.it

Re: [PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Jean Delvare]

Hi Grant,

On Mon,  7 May 2007 14:45:42 -0600, Grant Likely wrote:
> Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/i2c/chips/Kconfig  |   10 ++
>  drivers/i2c/chips/Makefile |    1 +
>  drivers/i2c/chips/ds1682.c |  363 ++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 374 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/chips/ds1682.c
> 

Please fix the following warnings:

drivers/i2c/chips/ds1682.c: In function ‘ds1682_store’:
drivers/i2c/chips/ds1682.c:129: warning: format ‘%i’ expects type ‘int’, but argument 5 has type ‘size_t’
drivers/i2c/chips/ds1682.c: In function ‘ds1682_user_data_store’:
drivers/i2c/chips/ds1682.c:220: warning: format ‘%i’ expects type ‘int’, but argument 4 has type ‘size_t’


> diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> index 87ee3ce..21d73e6 100644
> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig
> @@ -25,6 +25,16 @@ config SENSORS_DS1374
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called ds1374.
>  
> +config SENSORS_DS1682
> +	tristate "Maxim/Dallas DS1682 Total Elapsed Time Recorder with Alarm"
> +	depends on I2C && EXPERIMENTAL

Dependency on I2C is no longer needed in this directory, this is done
at menu level.

> +	help
> +	  If you say yes here you get support for Dallas Semiconductor
> +	  DS1682 Total Elapsed Time Recorder
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called ds1682.
> +

I know this isn't exactly a RTC chip, but this is still related with
timekeeping, so wouldn't it be better placed under drivers/rtc?
Alessandro, what do you think?

>  config SENSORS_EEPROM
>  	tristate "EEPROM reader"
>  	depends on I2C && EXPERIMENTAL
> diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> index 779868e..5f76bda 100644
> --- a/drivers/i2c/chips/Makefile
> +++ b/drivers/i2c/chips/Makefile
> @@ -4,6 +4,7 @@
>  
>  obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
>  obj-$(CONFIG_SENSORS_DS1374)	+= ds1374.o
> +obj-$(CONFIG_SENSORS_DS1682)	+= ds1682.o
>  obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
>  obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
>  obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
> diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
> new file mode 100644
> index 0000000..181175a
> --- /dev/null
> +++ b/drivers/i2c/chips/ds1682.c
> @@ -0,0 +1,363 @@
> +/*
> + * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
> + *
> + * Written by: Grant Likely <grant.likely@secretlab.ca>
> + *
> + * Copyright (C) 2007 Secret Lab Technologies Ltd.
> + * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
> + * Copyright (C) 2000 Russell King
> + *
> + * Derived from ds1337 real time clock device 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.
> + */
> +
> +/*
> + * The DS1682 elapsed timer recorder is a simple device that implements
> + * one elapsed time counters, one event counter, an alarm signal and 10

"time counter", no s.

> + * bytes of general purpose EEPROM.
> + *
> + * This driver provides access to the DS1682 counters and user data via
> + * the sysfs.  The following attributes are added to the device node:
> + *     elapsed_time (u32): Total elapsed event time in 1/4s resolution

That's a bad idea. Please use a standard unit, so that drivers for
devices with a similar feature but a different implementation can be
used the same way. Milliseconds would be fine, I guess.

> + *     alarm_time (u32): When elapsed time exceeds the value in alarm_time,
> + *                       then the alarm pin is asserted.
> + *     event_count (u16): number of times the event pin has gone low.
> + *     user_data (u8[10]): general purpose EEPROM

The file is called "eeprom" in other drivers (eeprom, max6875), it
would be nice to use the same here for consistency.

> + *
> + * Counter registers and user data are both read/write unless the device
> + * has been write protected.  This driver does not support turning on write
> + * protection.  Once write protection is turned on, it is impossible to
> + * turn off so I have left the feature out of this driver to avoid
> + * accidental enabling, but it is trivial to add write protect support.
> + *
> + */
> +
> +#define DEBUG

No. DEBUG in i2c chip drivers is set based on CONFIG_I2C_DEBUG_CHIP.

> +
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/string.h>
> +#include <linux/bcd.h>

You don't appear to use this one?

> +#include <linux/list.h>
> +#include <linux/sysfs.h>
> +#include <linux/ctype.h>
> +
> +/* Device registers */
> +#define DS1682_ADDR			0x6B

Given that you use this only once a few lines below, I'm not sure
there's much value in a define.

> +
> +#define DS1682_REG_CONFIG		0x00
> +#define DS1682_REG_ALARM		0x01
> +#define DS1682_REG_ELAPSED		0x05
> +#define DS1682_REG_EVT_CNTR		0x09
> +#define DS1682_REG_USER_DATA		0x0b
> +#define DS1682_REG_RESET		0x1d
> +#define DS1682_REG_WRITE_DISABLE	0x1e
> +#define DS1682_REG_WRITE_MEM_DISABLE	0x1f
> +
> +/*
> + * Probing class
> + */

This is an address, not a class.

> +static unsigned short normal_i2c[] = { DS1682_ADDR, I2C_CLIENT_END };
> +
> +I2C_CLIENT_INSMOD;
> +
> +/*
> + * Low level chip access functions
> + */
> +static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
> +{
> +	u8 *bytes = buf;
> +	int val;
> +
> +	while (len--) {
> +		val = i2c_smbus_read_byte_data(client, reg++);
> +		if (val < 0)
> +			return -EIO;
> +		*bytes++ = val;
> +	}
> +	return 0;
> +}

Did you check if the device supports I2C block reads? This would be
much faster, and might also solve consistency issues. Are you certain
that the above brings you back bytes which all belong to the same
"time measurement"? Does the device latch the register values on the
first read?

I find it strange that you use an I2C block write when writing values
to the chip, but individual byte reads in the other direction.

> +
> +/*
> + * Generic counter attributes
> + */
> +static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize);
> +
> +static ssize_t ds1682_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	u32 val = 0;

Shouldn't this be declared __le32 instead? Same for the other places
where you explicitly handle little-endian values.

> +	int reg;
> +	int regsize;
> +
> +	dev_dbg(dev, "ds1682_show() called for %s\n", attr->attr.name);
> +
> +	/* Get the register address */
> +	reg = ds1682_attr_to_reg(attr, &regsize);
> +	if (reg < 0)
> +		return -EIO;
> +
> +	/* Read the register */
> +	if (ds1682_read(to_i2c_client(dev), reg, &val, regsize))
> +		return -EIO;
> +
> +	/* Format the output string and return # of bytes */
> +	return sprintf(buf, "0x%.2x\n", le32_to_cpu(val));
> +}
> +
> +static ssize_t ds1682_store(struct device *dev,
> +			    struct device_attribute *attr, const char *buf,
> +			    size_t count)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	int reg;
> +	int regsize;
> +	char *endp;
> +	u32 val;
> +	int rc;
> +
> +	/* Sanitize the input */
> +	reg = ds1682_attr_to_reg(attr, &regsize);
> +
> +	if ((reg < 0) || (count >= 16) || (buf[count] != '\0')) {

How could buf[count] not be '\0'? As far as I know this is guaranteed
by the underlying sysfs code. I also don't see the point of checking
the value of count.

> +		dev_dbg(dev, "insane input; reg=0x%i count=%i%s\n",
> +			reg, count, buf[count] == '\0' ? " unterminated" : "");
> +		return -EIO;
> +	}
> +
> +	/* Decode input */
> +	val = simple_strtoul(buf, &endp, 0);
> +	if (buf == endp) {
> +		dev_dbg(dev, "input string not a number\n");
> +		return -EIO;
> +	}
> +
> +	/* write out the value */
> +	val = cpu_to_le32(val);
> +	rc = i2c_smbus_write_i2c_block_data(client, reg, regsize, (void *)&val);

You should cast to u8*, that's what i2c_smbus_write_i2c_block_data
takes.

> +	if (rc < 0) {
> +		dev_err(dev, "register write failed; reg=0x%x, size=%i\n",
> +			reg, regsize);
> +		return -EIO;
> +	}
> +
> +	return count;
> +}
> +
> +/*
> + * Simple register attributes
> + */
> +DEVICE_ATTR(config, S_IRUGO, ds1682_show, NULL);
> +DEVICE_ATTR(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
> +DEVICE_ATTR(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
> +DEVICE_ATTR(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
> +
> +/* Get register size and address from device_attribute structure.  This
> + * function must come after the DEVICE_ATTR() lines as it depends on the
> + * device_attribute lines being declared */
> +static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize)
> +{
> +	if (attr == &dev_attr_elapsed_time) {
> +		*regsize = 4;
> +		return DS1682_REG_ELAPSED;
> +	}
> +
> +	if (attr == &dev_attr_alarm_time) {
> +		*regsize = 4;
> +		return DS1682_REG_ALARM;
> +	}
> +
> +	if (attr == &dev_attr_event_count) {
> +		*regsize = 2;
> +		return DS1682_REG_EVT_CNTR;
> +	}
> +
> +	if (attr == &dev_attr_config) {
> +		*regsize = 1;
> +		return DS1682_REG_CONFIG;
> +	}
> +
> +	pr_debug("Cannot find registers for device_attribute %p\n", attr);
> +	return -ENODEV;
> +}

You can do much better than that, using additional attribute
properties. This is done in many hardware monitoring drivers. Look at
<linux/hwmon-sysfs.h>, then see the w83792d driver for an example of
use. Using a similar technique you could easily attach the register and
size information directly to each attribute, so you don't need this
expensive look-up.

> +
> +/*
> + * User data attribute
> + */
> +static ssize_t ds1682_user_data_show(struct device *dev,
> +				     struct device_attribute *attr, char *buf)
> +{
> +	char *end = buf;
> +	u8 val[10];
> +	int i;
> +
> +	if (ds1682_read(to_i2c_client(dev), DS1682_REG_USER_DATA, &val, 10))
> +		return -EIO;
> +
> +	for (i = 0; i < 10; i++)
> +		end += sprintf(end, "%.2x ", val[i]);

I'd suggest a define instead of the hard-coded "10".

> +
> +	*(end - 1) = '\n';	/* Eliminate trailing space */
> +	return end - buf;
> +}

The other drivers export the EEPROM data as a binary file, please do
the same for consistency. See the max6875 driver for an example. This
will also save you the parsing code below.

> +
> +static ssize_t ds1682_user_data_store(struct device *dev,
> +				      struct device_attribute *attr,
> +				      const char *buf, size_t count)
> +{
> +	u8 data[10];
> +	char *endp;
> +	int bytecount = 0;
> +
> +	/* Check input for sanity */
> +	if ((count >= 80) || (buf[count] != '\0')) {
> +		dev_dbg(dev, "insane input; count=%i%s\n",
> +			count, buf[count] == '\0' ? " unterminated" : "");
> +		return -EIO;
> +	}
> +
> +	/* Parse the data */
> +	while (bytecount < 10) {
> +		while (isspace(*buf))
> +			buf++;
> +
> +		data[bytecount] = simple_strtoul(buf, &endp, 16);
> +		if (endp == buf)	/* make sure it's a real data value */
> +			break;
> +
> +		buf = endp;
> +		bytecount++;
> +	}
> +	while (isspace(*endp))
> +		endp++;
> +
> +	/* Abort on invalid data */
> +	if ((bytecount == 0) || (*endp != '\0')) {
> +		dev_dbg(dev, "invalid input *buf=\"%s\" *endp=\"%s\"\n",
> +			buf, endp);
> +		return -EIO;
> +	}
> +
> +	/* Write out to the device */
> +	if (i2c_smbus_write_i2c_block_data(to_i2c_client(dev),
> +					   DS1682_REG_USER_DATA, bytecount,
> +					   data) < 0)
> +		return -EIO;
> +	return count;
> +}
> +
> +DEVICE_ATTR(user_data, S_IRUGO | S_IWUSR, ds1682_user_data_show,
> +	    ds1682_user_data_store);
> +
> +/*
> + * Called when a device is found at the ds1682 address
> + */
> +static struct i2c_driver ds1682_driver;
> +static int ds1682_detect(struct i2c_adapter *adapter, int address, int kind)
> +{
> +	struct i2c_client *new_client;

Please rename this to just "client". This "new_client" name is a
disease :(

> +	int err = 0;
> +
> +	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
> +				     I2C_FUNC_I2C))

This check doesn't match the functions you are using in the driver. You
are using i2c_smbus_read_byte_data() and
i2c_smbus_write_i2c_block_data(), so you should check for
I2C_FUNC_SMBUS_READ_BYTE_DATA | I2C_FUNC_SMBUS_WRITE_I2C_BLOCK.

> +		goto exit;
> +
> +	if (!(new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	new_client->addr = address;
> +	new_client->adapter = adapter;
> +	new_client->driver = &ds1682_driver;
> +	new_client->flags = 0;

Not needed, the kzalloc above did it for you.

> +
> +	/* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */

The "kind" value has nothing to do with the address. It tells you if
the user asked (through a module parameter) for the device
detection/identification to be skipped. Which brings me to the point
that your driver does no device identification at all. Fortunately,
address 0x6b isn't very popular, but I would still like to see some
detection code. Is there a way to identify the DS1682? I know that such
devices usually (unfortunately) don't have an identification register,
but maybe there are unused configuration bits which can be checked? Or
you can check the number of registers?

Alternatively, what system are you targeting with this driver? We just
merged David Brownell's new I2C infrastructure which lets I2C devices
be declared as platform data. This is particularly suitable for
embedded architectures.

> +
> +	/* We can fill in the remaining client fields */
> +	strlcpy(new_client->name, "ds1682", I2C_NAME_SIZE);
> +
> +	/* Tell the I2C layer a new client has arrived */
> +	if ((err = i2c_attach_client(new_client)))
> +		goto exit_free;
> +
> +	if (device_create_file(&new_client->dev, &dev_attr_config))
> +		goto exit_attr_config;
> +	if (device_create_file(&new_client->dev, &dev_attr_elapsed_time))
> +		goto exit_attr_elapsed;
> +	if (device_create_file(&new_client->dev, &dev_attr_alarm_time))
> +		goto exit_attr_alarm;
> +	if (device_create_file(&new_client->dev, &dev_attr_event_count))
> +		goto exit_attr_event;
> +	if (device_create_file(&new_client->dev, &dev_attr_user_data))
> +		goto exit_attr_userdata;

Please use a file group and sysfs_create_group(), it'll make the code
much cleaner.

> +
> +	return 0;
> +
> +      exit_attr_userdata:

Please use a single space for label indentation.

> +	device_remove_file(&new_client->dev, &dev_attr_event_count);
> +      exit_attr_event:
> +	device_remove_file(&new_client->dev, &dev_attr_alarm_time);
> +      exit_attr_alarm:
> +	device_remove_file(&new_client->dev, &dev_attr_elapsed_time);
> +      exit_attr_elapsed:
> +	device_remove_file(&new_client->dev, &dev_attr_config);
> +      exit_attr_config:
> +	i2c_detach_client(new_client);
> +      exit_free:
> +	kfree(new_client);
> +      exit:
> +	return err;
> +}
> +
> +static int ds1682_attach_adapter(struct i2c_adapter *adapter)
> +{
> +	return i2c_probe(adapter, &addr_data, ds1682_detect);
> +}
> +
> +static int ds1682_detach_client(struct i2c_client *client)
> +{
> +	int err;
> +	device_remove_file(&client->dev, &dev_attr_user_data);
> +	device_remove_file(&client->dev, &dev_attr_event_count);
> +	device_remove_file(&client->dev, &dev_attr_alarm_time);
> +	device_remove_file(&client->dev, &dev_attr_elapsed_time);
> +	device_remove_file(&client->dev, &dev_attr_config);
> +
> +	if ((err = i2c_detach_client(client)))
> +		return err;
> +
> +	kfree(client);
> +	return 0;
> +}
> +
> +static struct i2c_driver ds1682_driver = {
> +	.driver = {
> +		   .name = "ds1682",
> +		   },

Indentation.

> +	.attach_adapter = ds1682_attach_adapter,
> +	.detach_client = ds1682_detach_client,
> +};
> +
> +static int __init ds1682_init(void)
> +{
> +	return i2c_add_driver(&ds1682_driver);
> +}
> +
> +static void __exit ds1682_exit(void)
> +{
> +	i2c_del_driver(&ds1682_driver);
> +}
> +
> +MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
> +MODULE_DESCRIPTION("DS1682 Elapsed Time Indicator driver");
> +MODULE_LICENSE("GPL");
> +
> +module_init(ds1682_init);
> +module_exit(ds1682_exit);

The code is quite nice otherwise, good work!

-- 
Jean Delvare

[PATCH] Add driver for Dallas DS1682 elapsed time recorder

[Grant Likely]

Signed-off-by: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/i2c/chips/Kconfig  |   10 ++
 drivers/i2c/chips/Makefile |    1 +
 drivers/i2c/chips/ds1682.c |  363 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 374 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/ds1682.c

diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
index 87ee3ce..21d73e6 100644
--- a/drivers/i2c/chips/Kconfig
+++ b/drivers/i2c/chips/Kconfig
@@ -25,6 +25,16 @@ config SENSORS_DS1374
 	  This driver can also be built as a module.  If so, the module
 	  will be called ds1374.
 
+config SENSORS_DS1682
+	tristate "Maxim/Dallas DS1682 Total Elapsed Time Recorder with Alarm"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for Dallas Semiconductor
+	  DS1682 Total Elapsed Time Recorder
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called ds1682.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on I2C && EXPERIMENTAL
diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
index 779868e..5f76bda 100644
--- a/drivers/i2c/chips/Makefile
+++ b/drivers/i2c/chips/Makefile
@@ -4,6 +4,7 @@
 
 obj-$(CONFIG_SENSORS_DS1337)	+= ds1337.o
 obj-$(CONFIG_SENSORS_DS1374)	+= ds1374.o
+obj-$(CONFIG_SENSORS_DS1682)	+= ds1682.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_M41T00)	+= m41t00.o
diff --git a/drivers/i2c/chips/ds1682.c b/drivers/i2c/chips/ds1682.c
new file mode 100644
index 0000000..181175a
--- /dev/null
+++ b/drivers/i2c/chips/ds1682.c
@@ -0,0 +1,363 @@
+/*
+ * Dallas Semiconductor DS1682 Elapsed Time Recorder device driver
+ *
+ * Written by: Grant Likely <grant.likely@secretlab.ca>
+ *
+ * Copyright (C) 2007 Secret Lab Technologies Ltd.
+ * Copyright (C) 2005 James Chapman <jchapman@katalix.com>
+ * Copyright (C) 2000 Russell King
+ *
+ * Derived from ds1337 real time clock device 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.
+ */
+
+/*
+ * The DS1682 elapsed timer recorder is a simple device that implements
+ * one elapsed time counters, one event counter, an alarm signal and 10
+ * bytes of general purpose EEPROM.
+ *
+ * This driver provides access to the DS1682 counters and user data via
+ * the sysfs.  The following attributes are added to the device node:
+ *     elapsed_time (u32): Total elapsed event time in 1/4s resolution
+ *     alarm_time (u32): When elapsed time exceeds the value in alarm_time,
+ *                       then the alarm pin is asserted.
+ *     event_count (u16): number of times the event pin has gone low.
+ *     user_data (u8[10]): general purpose EEPROM
+ *
+ * Counter registers and user data are both read/write unless the device
+ * has been write protected.  This driver does not support turning on write
+ * protection.  Once write protection is turned on, it is impossible to
+ * turn off so I have left the feature out of this driver to avoid
+ * accidental enabling, but it is trivial to add write protect support.
+ *
+ */
+
+#define DEBUG
+
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/i2c.h>
+#include <linux/string.h>
+#include <linux/bcd.h>
+#include <linux/list.h>
+#include <linux/sysfs.h>
+#include <linux/ctype.h>
+
+/* Device registers */
+#define DS1682_ADDR			0x6B
+
+#define DS1682_REG_CONFIG		0x00
+#define DS1682_REG_ALARM		0x01
+#define DS1682_REG_ELAPSED		0x05
+#define DS1682_REG_EVT_CNTR		0x09
+#define DS1682_REG_USER_DATA		0x0b
+#define DS1682_REG_RESET		0x1d
+#define DS1682_REG_WRITE_DISABLE	0x1e
+#define DS1682_REG_WRITE_MEM_DISABLE	0x1f
+
+/*
+ * Probing class
+ */
+static unsigned short normal_i2c[] = { DS1682_ADDR, I2C_CLIENT_END };
+
+I2C_CLIENT_INSMOD;
+
+/*
+ * Low level chip access functions
+ */
+static int ds1682_read(struct i2c_client *client, int reg, void *buf, int len)
+{
+	u8 *bytes = buf;
+	int val;
+
+	while (len--) {
+		val = i2c_smbus_read_byte_data(client, reg++);
+		if (val < 0)
+			return -EIO;
+		*bytes++ = val;
+	}
+	return 0;
+}
+
+/*
+ * Generic counter attributes
+ */
+static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize);
+
+static ssize_t ds1682_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	u32 val = 0;
+	int reg;
+	int regsize;
+
+	dev_dbg(dev, "ds1682_show() called for %s\n", attr->attr.name);
+
+	/* Get the register address */
+	reg = ds1682_attr_to_reg(attr, &regsize);
+	if (reg < 0)
+		return -EIO;
+
+	/* Read the register */
+	if (ds1682_read(to_i2c_client(dev), reg, &val, regsize))
+		return -EIO;
+
+	/* Format the output string and return # of bytes */
+	return sprintf(buf, "0x%.2x\n", le32_to_cpu(val));
+}
+
+static ssize_t ds1682_store(struct device *dev,
+			    struct device_attribute *attr, const char *buf,
+			    size_t count)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	int reg;
+	int regsize;
+	char *endp;
+	u32 val;
+	int rc;
+
+	/* Sanitize the input */
+	reg = ds1682_attr_to_reg(attr, &regsize);
+
+	if ((reg < 0) || (count >= 16) || (buf[count] != '\0')) {
+		dev_dbg(dev, "insane input; reg=0x%i count=%i%s\n",
+			reg, count, buf[count] == '\0' ? " unterminated" : "");
+		return -EIO;
+	}
+
+	/* Decode input */
+	val = simple_strtoul(buf, &endp, 0);
+	if (buf == endp) {
+		dev_dbg(dev, "input string not a number\n");
+		return -EIO;
+	}
+
+	/* write out the value */
+	val = cpu_to_le32(val);
+	rc = i2c_smbus_write_i2c_block_data(client, reg, regsize, (void *)&val);
+	if (rc < 0) {
+		dev_err(dev, "register write failed; reg=0x%x, size=%i\n",
+			reg, regsize);
+		return -EIO;
+	}
+
+	return count;
+}
+
+/*
+ * Simple register attributes
+ */
+DEVICE_ATTR(config, S_IRUGO, ds1682_show, NULL);
+DEVICE_ATTR(elapsed_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
+DEVICE_ATTR(alarm_time, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
+DEVICE_ATTR(event_count, S_IRUGO | S_IWUSR, ds1682_show, ds1682_store);
+
+/* Get register size and address from device_attribute structure.  This
+ * function must come after the DEVICE_ATTR() lines as it depends on the
+ * device_attribute lines being declared */
+static int ds1682_attr_to_reg(struct device_attribute *attr, int *regsize)
+{
+	if (attr == &dev_attr_elapsed_time) {
+		*regsize = 4;
+		return DS1682_REG_ELAPSED;
+	}
+
+	if (attr == &dev_attr_alarm_time) {
+		*regsize = 4;
+		return DS1682_REG_ALARM;
+	}
+
+	if (attr == &dev_attr_event_count) {
+		*regsize = 2;
+		return DS1682_REG_EVT_CNTR;
+	}
+
+	if (attr == &dev_attr_config) {
+		*regsize = 1;
+		return DS1682_REG_CONFIG;
+	}
+
+	pr_debug("Cannot find registers for device_attribute %p\n", attr);
+	return -ENODEV;
+}
+
+/*
+ * User data attribute
+ */
+static ssize_t ds1682_user_data_show(struct device *dev,
+				     struct device_attribute *attr, char *buf)
+{
+	char *end = buf;
+	u8 val[10];
+	int i;
+
+	if (ds1682_read(to_i2c_client(dev), DS1682_REG_USER_DATA, &val, 10))
+		return -EIO;
+
+	for (i = 0; i < 10; i++)
+		end += sprintf(end, "%.2x ", val[i]);
+
+	*(end - 1) = '\n';	/* Eliminate trailing space */
+	return end - buf;
+}
+
+static ssize_t ds1682_user_data_store(struct device *dev,
+				      struct device_attribute *attr,
+				      const char *buf, size_t count)
+{
+	u8 data[10];
+	char *endp;
+	int bytecount = 0;
+
+	/* Check input for sanity */
+	if ((count >= 80) || (buf[count] != '\0')) {
+		dev_dbg(dev, "insane input; count=%i%s\n",
+			count, buf[count] == '\0' ? " unterminated" : "");
+		return -EIO;
+	}
+
+	/* Parse the data */
+	while (bytecount < 10) {
+		while (isspace(*buf))
+			buf++;
+
+		data[bytecount] = simple_strtoul(buf, &endp, 16);
+		if (endp == buf)	/* make sure it's a real data value */
+			break;
+
+		buf = endp;
+		bytecount++;
+	}
+	while (isspace(*endp))
+		endp++;
+
+	/* Abort on invalid data */
+	if ((bytecount == 0) || (*endp != '\0')) {
+		dev_dbg(dev, "invalid input *buf=\"%s\" *endp=\"%s\"\n",
+			buf, endp);
+		return -EIO;
+	}
+
+	/* Write out to the device */
+	if (i2c_smbus_write_i2c_block_data(to_i2c_client(dev),
+					   DS1682_REG_USER_DATA, bytecount,
+					   data) < 0)
+		return -EIO;
+	return count;
+}
+
+DEVICE_ATTR(user_data, S_IRUGO | S_IWUSR, ds1682_user_data_show,
+	    ds1682_user_data_store);
+
+/*
+ * Called when a device is found at the ds1682 address
+ */
+static struct i2c_driver ds1682_driver;
+static int ds1682_detect(struct i2c_adapter *adapter, int address, int kind)
+{
+	struct i2c_client *new_client;
+	int err = 0;
+
+	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_BYTE_DATA |
+				     I2C_FUNC_I2C))
+		goto exit;
+
+	if (!(new_client = kzalloc(sizeof(struct i2c_client), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	new_client->addr = address;
+	new_client->adapter = adapter;
+	new_client->driver = &ds1682_driver;
+	new_client->flags = 0;
+
+	/* Note: Ignoring 'kind' value as the ds1682 uses a fixed address */
+
+	/* We can fill in the remaining client fields */
+	strlcpy(new_client->name, "ds1682", I2C_NAME_SIZE);
+
+	/* Tell the I2C layer a new client has arrived */
+	if ((err = i2c_attach_client(new_client)))
+		goto exit_free;
+
+	if (device_create_file(&new_client->dev, &dev_attr_config))
+		goto exit_attr_config;
+	if (device_create_file(&new_client->dev, &dev_attr_elapsed_time))
+		goto exit_attr_elapsed;
+	if (device_create_file(&new_client->dev, &dev_attr_alarm_time))
+		goto exit_attr_alarm;
+	if (device_create_file(&new_client->dev, &dev_attr_event_count))
+		goto exit_attr_event;
+	if (device_create_file(&new_client->dev, &dev_attr_user_data))
+		goto exit_attr_userdata;
+
+	return 0;
+
+      exit_attr_userdata:
+	device_remove_file(&new_client->dev, &dev_attr_event_count);
+      exit_attr_event:
+	device_remove_file(&new_client->dev, &dev_attr_alarm_time);
+      exit_attr_alarm:
+	device_remove_file(&new_client->dev, &dev_attr_elapsed_time);
+      exit_attr_elapsed:
+	device_remove_file(&new_client->dev, &dev_attr_config);
+      exit_attr_config:
+	i2c_detach_client(new_client);
+      exit_free:
+	kfree(new_client);
+      exit:
+	return err;
+}
+
+static int ds1682_attach_adapter(struct i2c_adapter *adapter)
+{
+	return i2c_probe(adapter, &addr_data, ds1682_detect);
+}
+
+static int ds1682_detach_client(struct i2c_client *client)
+{
+	int err;
+	device_remove_file(&client->dev, &dev_attr_user_data);
+	device_remove_file(&client->dev, &dev_attr_event_count);
+	device_remove_file(&client->dev, &dev_attr_alarm_time);
+	device_remove_file(&client->dev, &dev_attr_elapsed_time);
+	device_remove_file(&client->dev, &dev_attr_config);
+
+	if ((err = i2c_detach_client(client)))
+		return err;
+
+	kfree(client);
+	return 0;
+}
+
+static struct i2c_driver ds1682_driver = {
+	.driver = {
+		   .name = "ds1682",
+		   },
+	.attach_adapter = ds1682_attach_adapter,
+	.detach_client = ds1682_detach_client,
+};
+
+static int __init ds1682_init(void)
+{
+	return i2c_add_driver(&ds1682_driver);
+}
+
+static void __exit ds1682_exit(void)
+{
+	i2c_del_driver(&ds1682_driver);
+}
+
+MODULE_AUTHOR("Grant Likely <grant.likely@secretlab.ca>");
+MODULE_DESCRIPTION("DS1682 Elapsed Time Indicator driver");
+MODULE_LICENSE("GPL");
+
+module_init(ds1682_init);
+module_exit(ds1682_exit);
-- 
1.5.1