[PATCH] Add a new-style driver for most I2C EEPROMs

[PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
access to their data. Tested with various chips and clock rates.

Updates in this version:

 - move chip data out of the driver into a seperate .h-file
 - prefix defined constants with AT24_
 - make bin file readonly if requested by flags
 - introduce AT24_MAX_CLIENTS
 - bugfix: check correct retval in at24_ee_write

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/i2c/chips/Kconfig  |   26 ++
 drivers/i2c/chips/Makefile |    1 +
 drivers/i2c/chips/at24.c   |  553 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/at24.h   |   96 ++++++++
 4 files changed, 676 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/at24.c
 create mode 100644 include/linux/i2c/at24.h

diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
index b21593f..46a3d1e 100644
--- a/drivers/i2c/chips/Kconfig
+++ b/drivers/i2c/chips/Kconfig
@@ -14,6 +14,32 @@ config DS1682
 	  This driver can also be built as a module.  If so, the module
 	  will be called ds1682.
 
+config I2C_AT24
+	tristate "EEPROMs from most vendors"
+	depends on I2C && SYSFS && EXPERIMENTAL
+	help
+	  Enable this driver to get read/write support to most I2C EEPROMs,
+	  after you configure the driver to know about each eeprom on on
+	  your target board.  Use these generic chip names, instead of
+	  vendor-specific ones like at24c64 or 24lc02:
+
+	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
+	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
+
+	  Unless you like data loss puzzles, always be sure that any chip
+	  you configure as a 24c32 (32 Kbits) or larger is NOT really a
+	  24c16 (16 Kbits) or smaller, and vice versa.  (Marking the chip
+	  as readonly won't help recover from this.)  Also, if your chip
+	  has any software write-protect mechanism you may want to make
+	  sure this driver won't turn it on by accident.
+
+	  If you use this with an SMBus adapter instead of an I2C adapter,
+	  full functionality is not availble.  Only smaller devices are
+	  supported (24c16 and below, max 4 KBytes).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called at24.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on EXPERIMENTAL
diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
index e47aca0..aa1a88c 100644
--- a/drivers/i2c/chips/Makefile
+++ b/drivers/i2c/chips/Makefile
@@ -10,6 +10,7 @@
 #
 
 obj-$(CONFIG_DS1682)		+= ds1682.o
+obj-$(CONFIG_I2C_AT24)		+= at24.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
diff --git a/drivers/i2c/chips/at24.c b/drivers/i2c/chips/at24.c
new file mode 100644
index 0000000..137f250
--- /dev/null
+++ b/drivers/i2c/chips/at24.c
@@ -0,0 +1,553 @@
+/*
+ * at24.c - handle most I2C EEPROMs
+ *
+ * Copyright (C) 2005-2007 David Brownell
+ * Copyright (C) 2008 Wolfram Sang, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/i2c/at24.h>
+
+/*
+ * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
+ * Differences between different vendor product lines (like Atmel AT24C or
+ * MicroChip 24LC, etc) won't much matter for typical read/write access.
+ * There are also I2C RAM chips, likewise interchangeable.  One example
+ * would be the PCF8570, which acts like a 24c02 EEPROM (256 bytes).
+ *
+ * However, misconfiguration can lose data. "Set 16-bit memory address"
+ * to a part with 8-bit addressing will overwrite data.  Writing with too
+ * big a page size also loses data.  And it's not safe to assume that the
+ * conventional addresses 0x50..0x57 only hold eeproms ... a pcf8563 RTC
+ * uses 0x51, for just one example.
+ *
+ * Accordingly, explicit board-specific configuration data should be used
+ * in almost all cases.  (One partial exception is an SMBus used to access
+ * "SPD" data for DRAM sticks.  Those only use 24c02 EEPROMs.)
+ *
+ * So this driver uses "new style" I2C driver binding, expecting to be
+ * told what devices exist.  That may be in arch/X/mach-Y/board-Z.c or
+ * similar kernel-resident tables; or, configuration data coming from
+ * a bootloader.  Be sure to set the board_info "type" to identify the
+ * eeprom type.
+ *
+ * Other than binding model, current differences from "eeprom" driver are
+ * that this one handles write access and isn't restricted to 24c02 devices.
+ * It also handles larger devices (32 kbit and up) with two-byte addresess,
+ * which won't work on pure SMBus systems.
+ */
+
+/* Specs often allow 5 msec for a page write, sometimes 20 msec;
+ * it's important to recover from write timeouts.
+ */
+#define	AT24_EE_TIMEOUT	25
+
+/* One chip may consume up to this numbe of clients */
+#define AT24_MAX_CLIENTS 8
+
+struct at24_data {
+	struct at24_platform_data	chip;
+	bool				use_smbus;
+
+	/* Lock protects against activities from other Linux tasks,
+	 * but not from changes by other I2C masters.
+	 */
+	struct mutex		lock;
+	struct bin_attribute	bin;
+
+	/* Some chips tie up multiple I2C addresses; dummy devices reserve
+	 * them for ourselves, and we'll use them with SMBus calls.
+	 */
+	struct i2c_client	*client[AT24_MAX_CLIENTS];
+
+	u8			*writebuf;
+	unsigned		write_max;
+};
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * This parameter is to help this driver avoid blocking other drivers out
+ * of I2C for potentially troublesome amounts of time.  With a 100 KHz I2C
+ * clock, one 256 byte read takes about 1/43 second which is excessive;
+ * but the 1/170 second it takes at 400 KHz may be quite reasonable; and
+ * at 1 MHz (Fm+) a 1/430 second delay could easily be invisible.
+ *
+ * This value is forced to be a power of two so that writes align on pages.
+ */
+static unsigned io_limit = 128;
+module_param(io_limit, uint, 0);
+MODULE_PARM_DESC(io_limit, "maximum bytes per i/o (default 128)");
+
+
+/*
+ * This routine supports chips which consume multiple I2C addresess.  It
+ * computes the addressing information to be used for a given r/w request.
+ */
+static struct i2c_client *at24_ee_address(
+	struct at24_data	*at24,
+	u16			*addr,
+	unsigned		*offset
+)
+{
+	unsigned			per_address = 256;
+	struct at24_platform_data	*chip = &at24->chip;
+	unsigned			i;
+
+	if (*offset >= chip->byte_len)
+		return NULL;
+
+	if (chip->flags & AT24_EE_ADDR2)
+		per_address = 64 * 1024;
+	*addr = at24->client[0]->addr;
+	for (i = 0; *offset >= per_address; i++) {
+		(*addr)++;
+		*offset -= per_address;
+	}
+
+	return at24->client[i];
+}
+
+/*-------------------------------------------------------------------------*/
+
+static ssize_t
+at24_ee_read(
+	struct at24_data	*at24,
+	char			*buf,
+	unsigned		offset,
+	size_t			count
+)
+{
+	struct i2c_msg		msg[2];
+	u8			addr[2];
+	struct i2c_client	*client;
+	int			status;
+
+	memset(msg, 0, sizeof msg);
+
+	/* REVISIT some multi-address chips don't rollover page reads to
+	 * the next slave address, so we may need to truncate the count.
+	 * Those chips might need another quirk flag...
+	 *
+	 * If the real hardware used four adjacent 24c02 chips and that
+	 * were misconfiged as one 24c08, that would be a similar effect:
+	 * one "eeprom" file not four, but larger reads would fail when
+	 * they crossed certain pages.
+	 */
+	if (count > io_limit)
+		count = io_limit;
+
+	/* Slave address and byte offset derive from the offset.  Always
+	 * set the byte address; on a multi-master board, another master
+	 * may have changed the chip's "current" address pointer.
+	 */
+	client = at24_ee_address(at24, &msg[0].addr, &offset);
+	if (!client)
+		return -EINVAL;
+
+	/* Smaller eproms can work given some SMBus extension calls */
+	if (at24->use_smbus) {
+		if (count > 32)
+			count = 32;
+		status = i2c_smbus_read_i2c_block_data(client, offset,
+				count, buf);
+		dev_dbg(&client->dev, "smbus read %zd@%d --> %d\n",
+				count, offset, status);
+		return (status < 0) ? -EIO : status;
+	}
+
+	/* When we have a better choice than SMBus calls, use a combined
+	 * I2C message.  Write address; then read up to io_limit data bytes.
+	 * Note that read page rollover helps us here (unlike writes).
+	 */
+	msg[0].buf = addr;
+	addr[1] = (u8) offset;
+	addr[0] = (u8) (offset >> 8);
+
+	if (at24->chip.flags & AT24_EE_ADDR2)
+		msg[0].len = 2;
+	else {
+		msg[0].len = 1;
+		msg[0].buf++;
+	}
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].buf = buf;
+	msg[1].len = count;
+
+	status = i2c_transfer(client->adapter, msg, 2);
+	dev_dbg(&client->dev, "i2c read %zd@%d --> %d\n",
+			count, offset, status);
+
+	if (status == 2)
+		return count;
+	else if (status >= 0)
+		return -EIO;
+	else
+		return status;
+}
+
+static ssize_t
+at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct i2c_client	*client;
+	struct at24_data	*at24;
+	ssize_t			retval = 0;
+
+	client = to_i2c_client(container_of(kobj, struct device, kobj));
+	at24 = i2c_get_clientdata(client);
+
+	if (unlikely(off >= at24->bin.size))
+		return 0;
+	if ((off + count) > at24->bin.size)
+		count = at24->bin.size - off;
+	if (unlikely(!count))
+		return count;
+
+	/* Read data from chip, protecting against concurrent updates
+	 * from this host ... but not from other i2c masters.
+	 */
+	mutex_lock(&at24->lock);
+	do {
+		ssize_t		status;
+
+		status = at24_ee_read(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+
+	} while (count);
+	mutex_unlock(&at24->lock);
+
+	return retval;
+}
+
+
+/* REVISIT:  export at24_bin{read,write}() to let other kernel code use
+ * eeprom data.  For example, it might hold a board's Ethernet address, or
+ * board-specific calibration data generated on the manufacturing floor.
+ */
+
+
+/* Note that if the hardware write-protect pin is pulled high, the whole
+ * chip is normally write protected.  But there are plenty of product
+ * variants here, including OTP fuses and partial chip protect.
+ *
+ * We only use page mode writes; the alternative is sloooow.  This routine
+ * writes at most one page.
+ */
+static ssize_t
+at24_ee_write(struct at24_data *at24, char *buf, loff_t off, size_t count)
+{
+	struct i2c_client	*client;
+	struct i2c_msg		msg;
+	unsigned		offset = (unsigned) off;
+	ssize_t			status = 0;
+	unsigned long		timeout, retries;
+	unsigned		c0, c1;
+
+	/* Maybe adjust i2c address and offset */
+	client = at24_ee_address(at24, &msg.addr, &offset);
+	if (!client)
+		return -EINVAL;
+
+	/* write_max is at most a page */
+	if (count > at24->write_max)
+		count = at24->write_max;
+
+	/* Never roll over backwards, to the start of this page */
+	c0 = offset + count;
+	c1 = roundup(offset + 1, at24->chip.page_size);
+	if (c0 > c1)
+		count -= c1 - c0;
+
+	/* If we'll use i2c calls for i/o, set up the message */
+	if (!at24->use_smbus) {
+		msg.flags = 0;
+		msg.len = count + 1;
+		msg.buf = at24->writebuf;
+		if (at24->chip.flags & AT24_EE_ADDR2) {
+			msg.len++;
+			msg.buf[1] = (u8) offset;
+			msg.buf[0] = (u8) (offset >> 8);
+			memcpy(&msg.buf[2], buf, count);
+		} else {
+			msg.buf[0] = offset;
+			memcpy(&msg.buf[1], buf, count);
+		}
+	}
+
+	/* Writes fail if the previous one didn't complete yet.  We'll
+	 * loop a few times until this one succeeds, waiting at least
+	 * long enough for one entire page write to work.
+	 */
+	timeout = jiffies + msecs_to_jiffies(AT24_EE_TIMEOUT);
+	for (retries = 0; retries < 3; retries++) {
+
+		if (at24->use_smbus) {
+			status = i2c_smbus_write_i2c_block_data(client,
+					offset, count, buf);
+			if (status == 0)
+				status = count;
+		} else {
+			status = i2c_transfer(client->adapter, &msg, 1);
+			if (status == 1)
+				status = count;
+		}
+		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
+				count, offset, status, jiffies);
+
+		if (status == count)
+			return count;
+
+		if (retries < 3 || time_after(timeout, jiffies)) {
+			/* REVISIT:  at HZ=100, this is sloooow */
+			msleep(1);
+			continue;
+		}
+	}
+
+	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
+			count, offset, jiffies - (timeout - AT24_EE_TIMEOUT));
+	return -ETIMEDOUT;
+}
+
+static ssize_t
+at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct i2c_client	*client;
+	struct at24_data	*at24;
+	ssize_t			retval = 0;
+
+	client = to_i2c_client(container_of(kobj, struct device, kobj));
+	at24 = i2c_get_clientdata(client);
+
+	if (unlikely(off >= at24->bin.size))
+		return -EFBIG;
+	if ((off + count) > at24->bin.size)
+		count = at24->bin.size - off;
+	if (unlikely(!count))
+		return count;
+
+	/* Write data from chip, protecting against concurrent updates
+	 * from this host ... but not from other i2c masters.
+	 */
+	mutex_lock(&at24->lock);
+
+	/* buffer big enough to stick the address at the beginning */
+	at24->writebuf = kmalloc(at24->write_max + 2, GFP_KERNEL);
+	if (!at24->writebuf) {
+		retval = -ENOMEM;
+		count = 0;
+	}
+
+	while (count) {
+		ssize_t		status;
+
+		status = at24_ee_write(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+	}
+
+	kfree(at24->writebuf);
+	at24->writebuf = NULL;
+	mutex_unlock(&at24->lock);
+
+	return retval;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int at24_probe(struct i2c_client *client)
+{
+	struct at24_platform_data	*chip;
+	bool				writable;
+	bool				use_smbus = false;
+	struct at24_data		*at24;
+	int				err;
+
+	chip = client->dev.platform_data;
+	if (!chip) {
+		err = -ENODEV;
+		goto fail;
+	}
+
+	/* Use I2C operations unless we're stuck with SMBus extensions. */
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		if (chip->flags & AT24_EE_ADDR2) {
+			err = -ECOMM;
+			goto fail;
+		}
+		if (!i2c_check_functionality(client->adapter,
+				I2C_FUNC_SMBUS_I2C_BLOCK)) {
+			err = -EBADMSG;
+			goto fail;
+		}
+		use_smbus = true;
+	}
+
+	if (!(at24 = kzalloc(sizeof(struct at24_data), GFP_KERNEL))) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	mutex_init(&at24->lock);
+	at24->chip = *chip;
+	at24->use_smbus = use_smbus;
+
+	/* Export the EEPROM bytes through sysfs, since that's convenient.
+	 * By default, only root should see the data (maybe passwords etc)
+	 */
+	at24->bin.attr.name = "eeprom";
+	at24->bin.attr.mode = S_IRUSR;
+	at24->bin.attr.owner = THIS_MODULE;
+	at24->bin.read = at24_bin_read;
+
+	at24->bin.size = at24->chip.byte_len;
+
+	writable = ((chip->flags & AT24_EE_READONLY) == 0);
+	if (writable) {
+		unsigned write_max = at24->chip.page_size;
+
+		at24->bin.write = at24_bin_write;
+		at24->bin.attr.mode |= S_IWUSR;
+
+		if (write_max > io_limit)
+			write_max = io_limit;
+		if (use_smbus && write_max > 32)
+			write_max = 32;
+		at24->write_max = write_max;
+	}
+
+	/* REVISIT read a byte, to make sure the chip is actually
+	 * present (vs misconfiguration, or not-populated-here)
+	 */
+
+	at24->client[0] = client;
+	i2c_set_clientdata(client, at24);
+
+	/* use dummy devices for multiple-address chips */
+	if (chip->i2c_addr_mask) {
+		unsigned		i;
+		struct i2c_client	*c;
+
+		for (i = 1; i <= chip->i2c_addr_mask; i++) {
+			c = i2c_new_dummy(client->adapter, client->addr + i,
+					client->name);
+			if (!c) {
+				dev_dbg(&client->dev, "addr %d unavail\n",
+						client->addr + i);
+				err = -ENOCSI;
+				goto cleanup;
+			}
+			at24->client[i] = c;
+		}
+	}
+
+	err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
+	if (err != 0)
+		goto cleanup;
+
+	dev_info(&client->dev, "%Zd byte %s EEPROM%s\n",
+		at24->bin.size, client->name,
+		writable ? " (writable)" : "");
+	dev_dbg(&client->dev,
+		"page_size %d, i2c_addr_mask %d, write_max %d%s\n",
+		at24->chip.page_size, at24->chip.i2c_addr_mask,
+		at24->write_max,
+		use_smbus ? ", use_smbus" : "");
+	return 0;
+
+cleanup:
+	if (at24 && chip->i2c_addr_mask) {
+		unsigned		i;
+		struct i2c_client	*c;
+
+		for (i = 1; i < ARRAY_SIZE(at24->client); i++) {
+			c = at24->client[i];
+			if (!c)
+				break;
+			i2c_unregister_device(c);
+		}
+	}
+
+	kfree(at24);
+fail:
+	dev_dbg(&client->dev, "probe err %d\n", err);
+	return err;
+}
+
+static int __devexit at24_remove(struct i2c_client *client)
+{
+	struct at24_data	*at24;
+	int			i;
+
+	at24 = i2c_get_clientdata(client);
+	sysfs_remove_bin_file(&client->dev.kobj, &at24->bin);
+
+	/* up to AT24_MAX_CLIENTS clients per chip */
+	for (i = 1; i < ARRAY_SIZE(at24->client); i++) {
+		client = at24->client[i];
+		if (!client)
+			break;
+		i2c_unregister_device(client);
+	}
+
+	kfree(at24);
+	return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static struct i2c_driver at24_driver = {
+	.driver = {
+		.name		= "at24",
+		.owner		= THIS_MODULE,
+	},
+	.probe		= at24_probe,
+	.remove		= __devexit_p(at24_remove),
+};
+
+static int __init at24_init(void)
+{
+	io_limit = rounddown_pow_of_two(io_limit);
+
+	return i2c_add_driver(&at24_driver);
+}
+module_init(at24_init);
+
+static void __exit at24_exit(void)
+{
+	i2c_del_driver(&at24_driver);
+}
+module_exit(at24_exit);
+
+MODULE_DESCRIPTION("Driver for most I2C EEPROMs");
+MODULE_AUTHOR("David Brownell");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
new file mode 100644
index 0000000..aac29bb
--- /dev/null
+++ b/include/linux/i2c/at24.h
@@ -0,0 +1,96 @@
+
+#ifndef _LINUX_AT24_H
+#define _LINUX_AT24_H
+
+#include <linux/types.h>
+
+/* As seen through Linux I2C, differences between the most common types
+ * of I2C memory include:
+ *	- How many I2C addresses the chip consumes: 1, 2, 4, or 8?
+ *	- Memory address space for one I2C address:  256 bytes, or 64 KB?
+ *	- How full that memory space is:  16 bytes, 256, 32Kb, etc?
+ *	- What write page size does it support?
+ */
+
+struct at24_platform_data {
+	u32		byte_len;		/* total (of 1..8 i2c addrs) */
+	u16		page_size;		/* for writes */
+	u8		i2c_addr_mask;		/* for multi-addr chips */
+	u8		flags;			/* quirks, etc */
+#define	AT24_EE_ADDR2		0x0080		/* 16 bit addrs; <= 64 KB */
+#define	AT24_EE_READONLY	0x0040
+#define	AT24_EE_24RF08		0x0001		/* SMBUS_QUICK problem */
+};
+
+#define AT24_PLATFORM_DATA(_name, _len, _page, _mask, _flags) \
+struct at24_platform_data at24_platform_data_##_name = {	\
+	.byte_len	= _len,		\
+	.page_size	= _page,	\
+	.i2c_addr_mask	= _mask,	\
+	.flags		= _flags,	\
+};
+
+/* 128 bit chip, I2C A0-A2 ignored */
+#define AT24_PLATFORM_DATA_24C00 \
+	AT24_PLATFORM_DATA(24c00, 128 / 8, 1, 0x07, 0)
+
+/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
+#define AT24_PLATFORM_DATA_24C01 \
+	AT24_PLATFORM_DATA(24c01, 1024 / 8, 8, 0, 0)
+
+/* 2 Kbit chip, some have 16 byte pages: */
+#define AT24_PLATFORM_DATA_24C02 \
+	AT24_PLATFORM_DATA(24c02, 2048 / 8, 8, 0, 0)
+
+/* 2 Kbit chip, 24c02 in memory DIMMs, some have 16 byte pages: */
+#define AT24_PLATFORM_DATA_SPD \
+	AT24_PLATFORM_DATA(spd, 2048 / 8, 8, 0, AT24_EE_READONLY)
+
+/* 2 Kbit chip, SRAM, not EEPROM!, no page size issues, write it all at once */
+#define AT24_PLATFORM_DATA_PCF8570 \
+	AT24_PLATFORM_DATA(pcf8570, 2048 / 8, 2048 / 8, 0, 0)
+
+/* 4 Kbit chip, I2C A0 is MEM A8 */
+#define AT24_PLATFORM_DATA_24C04 \
+	AT24_PLATFORM_DATA(24c04, 4096 / 8, 16, 0x01, 0)
+
+/* 8 Kbit chip, I2C A1-A0 is MEM A9-A8 */
+#define AT24_PLATFORM_DATA_24C08 \
+	AT24_PLATFORM_DATA(24c08, 8192 / 8, 16, 0x03, 0)
+
+/* 8 Kbit chip, I2C A1-A0 is MEM A9-A8 */
+#define AT24_PLATFORM_DATA_24RF08 \
+	AT24_PLATFORM_DATA(24rf08, 8192 / 8, 16, 0x03, AT24_EE_24RF08)
+
+/* 16 Kbit chip, I2C A2-A0 is MEM A10-A8 */
+#define AT24_PLATFORM_DATA_24C16 \
+	AT24_PLATFORM_DATA(24c16, 16384 / 8, 16, 0x07, 0)
+
+
+/* this second block of EEPROMS uses 16 bit memory addressing */
+
+/* 32 Kbits */
+#define AT24_PLATFORM_DATA_24C32 \
+	AT24_PLATFORM_DATA(24c32, 32768 / 8, 32, 0, AT24_EE_ADDR2)
+
+/* 64 Kbits */
+#define AT24_PLATFORM_DATA_24C64 \
+	AT24_PLATFORM_DATA(24c64, 65536 / 8, 32, 0, AT24_EE_ADDR2)
+
+/* 128 Kbits */
+#define AT24_PLATFORM_DATA_24C128 \
+	AT24_PLATFORM_DATA(24c128, 131072 / 8, 64, 0, AT24_EE_ADDR2)
+
+/* 256 Kbits */
+#define AT24_PLATFORM_DATA_24C256 \
+	AT24_PLATFORM_DATA(24c256, 262144 / 8, 64, 0, AT24_EE_ADDR2)
+
+/* 512 Kbits */
+#define AT24_PLATFORM_DATA_24C512 \
+	AT24_PLATFORM_DATA(24c512, 524288 / 8, 128, 0, AT24_EE_ADDR2)
+
+/* 1 Mbits, I2C A0 is MEM A16 */
+#define AT24_PLATFORM_DATA_24C1024 \
+	AT24_PLATFORM_DATA(24c1024, 1048576 / 8, 256, 0x01, AT24_EE_ADDR2)
+
+#endif /* _LINUX_AT24_H */
-- 
1.5.4.4


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

Hello,

I'll just review myself to ask for some comments where I think some more 
opinions would be good. I also tried to contact David Brownell as the 
original author directly last week, but all my mails got blocked; I hope 
we can negotiate through this list.

I tested this version of the driver on a blackfin based board with a 
24c02. In the next days, a powerpc based board with a X24645 (strange 
one) will follow. On x86 and x86-64, it builds without warnings.

> + * at24.c - handle most I2C EEPROMs
Maybe rename the driver? at24 is vendor-specific. 24xx? 24cxx? 
eeprom-ng? I'd go for 24xx.

> +/* One chip may consume up to this numbe of clients */
> +#define AT24_MAX_CLIENTS 8
> +
> +struct at24_data {
> +	struct at24_platform_data	chip;
> +	bool				use_smbus;
> +
> +	/* Lock protects against activities from other Linux tasks,
> +	 * but not from changes by other I2C masters.
> +	 */
> +	struct mutex		lock;
> +	struct bin_attribute	bin;
> +
> +	/* Some chips tie up multiple I2C addresses; dummy devices reserve
> +	 * them for ourselves, and we'll use them with SMBus calls.
> +	 */
> +	struct i2c_client	*client[AT24_MAX_CLIENTS];
> +
> +	u8			*writebuf;
> +	unsigned		write_max;
> +};
To support the X24645, it would be necessary to raise AT24_MAX_CLIENTS 
to 32 (what a beast!). Then again, most eeproms will just need one 
client, so this would cause quite some overhead in most use-cases. Maybe 
it pays off to hande this dynamically?

> +/*
> + * This routine supports chips which consume multiple I2C addresess.  It
> + * computes the addressing information to be used for a given r/w request.
> + */
> +static struct i2c_client *at24_ee_address(
> +	struct at24_data	*at24,
> +	u16			*addr,
> +	unsigned		*offset
> +)
> +{
> +	unsigned			per_address = 256;
> +	struct at24_platform_data	*chip = &at24->chip;
> +	unsigned			i;
> +
> +	if (*offset >= chip->byte_len)
> +		return NULL;
> +
> +	if (chip->flags & AT24_EE_ADDR2)
> +		per_address = 64 * 1024;
> +	*addr = at24->client[0]->addr;
> +	for (i = 0; *offset >= per_address; i++) {
> +		(*addr)++;
> +		*offset -= per_address;
> +	}
> +
> +	return at24->client[i];
> +}
I would suggest shortening it like this, unless I fail to see a drawback:

---

static struct i2c_client *at24_ee_address(
	struct at24_data	*at24,
	u16			*addr,
	unsigned		*offset
)
{
	const struct chip_desc	*chip = &at24->chip;
	unsigned		i;

	if (*offset >= chip->byte_len)
		return NULL;

	if (chip->flags & EE_ADDR2) {
		i = *offset >> 16;
		*offset &= 0xffff;
	} else {
		i = *offset >> 8;
		*offset &= 0x00ff;
	}

	*addr = at24->client[i]->addr;
	return at24->client[i];
}

---

This will save the for-loop and a variable.

> +	/* Export the EEPROM bytes through sysfs, since that's convenient.
> +	 * By default, only root should see the data (maybe passwords etc)
> +	 */
> +	at24->bin.attr.name = "eeprom";
> +	at24->bin.attr.mode = S_IRUSR;
> +	at24->bin.attr.owner = THIS_MODULE;
> +	at24->bin.read = at24_bin_read;
> +
> +	at24->bin.size = at24->chip.byte_len;
Use C99 initialization in struct at24_data above?

I guess, there is more to come during the discussion. But hopefully, 
this is a start to get it into 2.6.26... That would be great!

Kind regards,

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Robert Schwebel]

On Fri, Apr 11, 2008 at 02:24:55PM +0200, Wolfram Sang wrote:
> I'll just review myself to ask for some comments where I think some
> more opinions would be good. I also tried to contact David Brownell as
> the original author directly last week, but all my mails got blocked;
> I hope we can negotiate through this list.

Hmm, usually, this should work.

> > + * at24.c - handle most I2C EEPROMs
> Maybe rename the driver? at24 is vendor-specific. 24xx? 24cxx?
> eeprom-ng? I'd go for 24xx.

Don't use to many "xx" characters. Last time someone did that, he was
blocked by rmk's spam filter :-) Maybe "i2c-eeprom"?

> > +	/* Lock protects against activities from other Linux tasks,
> > +	 * but not from changes by other I2C masters.
> > +	 */

Multi-line comments are usually written like

/*
 * foo
 */

> To support the X24645, it would be necessary to raise AT24_MAX_CLIENTS
> to 32 (what a beast!). Then again, most eeproms will just need one
> client, so this would cause quite some overhead in most use-cases.
> Maybe it pays off to hande this dynamically?

As eeproms are normally slow things anyway, would it be a big
performance impact?

> > +static struct i2c_client *at24_ee_address(
> > +	struct at24_data	*at24,
> > +	u16			*addr,
> > +	unsigned		*offset

Minor nit: the kernel usually doesn't aling the variables vertically; if
you add more and longer data types, it might be necessary to move around
all other lines, which clutters patches. So I'd write this as

	struct at24_data *at24,
	u16 *addr,
	unsigned *offset

Robert
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi Robert, hi Wolfram,

On Mon, 14 Apr 2008 09:22:27 +0200, Robert Schwebel wrote:
> On Fri, Apr 11, 2008 at 02:24:55PM +0200, Wolfram Sang wrote:
> > > + * at24.c - handle most I2C EEPROMs
> > Maybe rename the driver? at24 is vendor-specific. 24xx? 24cxx?
> > eeprom-ng? I'd go for 24xx.
> 
> Don't use to many "xx" characters. Last time someone did that, he was
> blocked by rmk's spam filter :-) Maybe "i2c-eeprom"?

Definitely not. We use i2c-* for I2C _bus_ drivers, not I2C chip
drivers, so "i2c-eeprom" would be confusing. "at24" is just fine and I
see no reason to change the name. The Linux driver tree is full of
vendor-specific names, and this has never been a problem.

<snip>

> > To support the X24645, it would be necessary to raise AT24_MAX_CLIENTS
> > to 32 (what a beast!). Then again, most eeproms will just need one
> > client, so this would cause quite some overhead in most use-cases.
> > Maybe it pays off to hande this dynamically?
> 
> As eeproms are normally slow things anyway, would it be a big
> performance impact?

I agree that going dynamic makes sense. Not that it has anything to do
with the speed of EEPROMs though - the memory allocation will be done
at device initialization time, and after that dynamic or not should
perform just the same.

> > > +static struct i2c_client *at24_ee_address(
> > > +	struct at24_data	*at24,
> > > +	u16			*addr,
> > > +	unsigned		*offset
> 
> Minor nit: the kernel usually doesn't aling the variables vertically; if
> you add more and longer data types, it might be necessary to move around
> all other lines, which clutters patches. So I'd write this as
> 
> 	struct at24_data *at24,
> 	u16 *addr,
> 	unsigned *offset

Or even better, everything on the same line until you hit the 80 column
limit, etc.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Monday 14 April 2008, Jean Delvare wrote:
> > > To support the X24645, it would be necessary to raise AT24_MAX_CLIENTS
> > > to 32 (what a beast!). Then again, most eeproms will just need one
> > > client, so this would cause quite some overhead in most use-cases.
> > > Maybe it pays off to hande this dynamically?
> > 
> > As eeproms are normally slow things anyway, would it be a big
> > performance impact?
> 
> I agree that going dynamic makes sense. Not that it has anything to do
> with the speed of EEPROMs though - the memory allocation will be done
> at device initialization time, and after that dynamic or not should
> perform just the same.

Right.  X24645 looks a bit more bizarre than any EEPROMs
I had come across; it'll need some changes in chip config
data.  You'd think they'd just use two byte addressing!
I guess doing it that way lets them use SMBus-only hosts.


Wolfram's at24_ee_address() cleanup looked fine.

The header looks OK, but I'd add a comment *encouraging*
folk to double check those params for their chips, in
particular to check the page size.  It's OK if their chip
has a bigger page size than those "defaults" although they
could get faster write performance by listing the right
value ... but it's NOT OK if their chip has a smaller write
size than what's listed there, they'll lose data.


I'll be glad to see this go upstream.  :)

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

> drivers, so "i2c-eeprom" would be confusing. "at24" is just fine and I
> see no reason to change the name.  The Linux driver tree is full of
> vendor-specific names, and this has never been a problem.
I just wondered why we should pick one vendor's name altough the driver 
is designed to work for all vendors. Not the biggest question at the 
moment, though ;)

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

Greetings,

(this goes mostly to David, I assume)

In at24_ee_write:
> +	/* Writes fail if the previous one didn't complete yet.  We'll
> +	 * loop a few times until this one succeeds, waiting at least
> +	 * long enough for one entire page write to work.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(AT24_EE_TIMEOUT);
> +	for (retries = 0; retries < 3; retries++) {
> +
> +		if (at24->use_smbus) {
> +			status = i2c_smbus_write_i2c_block_data(client,
> +					offset, count, buf);
> +			if (status == 0)
> +				status = count;
> +		} else {
> +			status = i2c_transfer(client->adapter, &msg, 1);
> +			if (status == 1)
> +				status = count;
> +		}
> +		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
> +				count, offset, status, jiffies);
> +
> +		if (status == count)
> +			return count;
> +
> +		if (retries < 3 || time_after(timeout, jiffies)) {
> +			/* REVISIT:  at HZ=100, this is sloooow */
> +			msleep(1);
> +			continue;
> +		}
> +	}
I assume 'retries < 3' (always true) and 'continue' (nothing follows) 
are left-overs from earlier revisions and can be thrown out? My main 
questions is 'msleep(1)' though: As I understand it, this means the 
for-loop will wait roughly 3ms on failures until it reports a timeout 
(assuming good precision of msleep). Comparing this to AT24_EE_TIMEOUT 
(25 ms), it looks to me that the msleep value could be increased a 
little (maybe to AT24_EE_TIMEOUT / 3)? Or is i2c_transfer slow enough 
and can we count on that behaviour?

All the best,

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

On Thu, 17 Apr 2008 18:05:32 +0200, Wolfram Sang wrote:
> Greetings,
> 
> (this goes mostly to David, I assume)
> 
> In at24_ee_write:
> > +	/* Writes fail if the previous one didn't complete yet.  We'll
> > +	 * loop a few times until this one succeeds, waiting at least
> > +	 * long enough for one entire page write to work.
> > +	 */
> > +	timeout = jiffies + msecs_to_jiffies(AT24_EE_TIMEOUT);
> > +	for (retries = 0; retries < 3; retries++) {
> > +
> > +		if (at24->use_smbus) {
> > +			status = i2c_smbus_write_i2c_block_data(client,
> > +					offset, count, buf);
> > +			if (status == 0)
> > +				status = count;
> > +		} else {
> > +			status = i2c_transfer(client->adapter, &msg, 1);
> > +			if (status == 1)
> > +				status = count;
> > +		}
> > +		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
> > +				count, offset, status, jiffies);
> > +
> > +		if (status == count)
> > +			return count;
> > +
> > +		if (retries < 3 || time_after(timeout, jiffies)) {
> > +			/* REVISIT:  at HZ=100, this is sloooow */
> > +			msleep(1);
> > +			continue;
> > +		}
> > +	}
> I assume 'retries < 3' (always true) and 'continue' (nothing follows) 
> are left-overs from earlier revisions and can be thrown out? My main 
> questions is 'msleep(1)' though: As I understand it, this means the 
> for-loop will wait roughly 3ms on failures until it reports a timeout 
> (assuming good precision of msleep). Comparing this to AT24_EE_TIMEOUT 
> (25 ms), it looks to me that the msleep value could be increased a 
> little (maybe to AT24_EE_TIMEOUT / 3)? Or is i2c_transfer slow enough 
> and can we count on that behaviour?

msleep() can't sleep less than one jiffie, so msleep(1) is the same as
msleep(10) at HZ=100. At HZ=1000, msleep(1) is almost possible,
although it might sleep for up to 2 ms in practice (finish the current
jiffie + the next jiffie.)

The bottom line is that you never really know how much msleep(N)
will sleep for small values of N.

I realize that it doesn't really answer your question, but that's still
worth keeping in mind.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

> In at24_ee_write:
> > +	/* Writes fail if the previous one didn't complete yet.  We'll
> > +	 * loop a few times until this one succeeds, waiting at least
> > +	 * long enough for one entire page write to work.
> > +	 */
> > +	timeout = jiffies + msecs_to_jiffies(AT24_EE_TIMEOUT);
> > +	for (retries = 0; retries < 3; retries++) {
> > +
> > +		if (at24->use_smbus) {
> > +			status = i2c_smbus_write_i2c_block_data(client,
> > +					offset, count, buf);
> > +			if (status == 0)
> > +				status = count;
> > +		} else {
> > +			status = i2c_transfer(client->adapter, &msg, 1);
> > +			if (status == 1)
> > +				status = count;
> > +		}
> > +		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
> > +				count, offset, status, jiffies);
> > +
> > +		if (status == count)
> > +			return count;
> > +
> > +		if (retries < 3 || time_after(timeout, jiffies)) {
> > +			/* REVISIT:  at HZ=100, this is sloooow */
> > +			msleep(1);
> > +			continue;
> > +		}
> > +	}
>
> I assume 'retries < 3' (always true)

You mean, in that "if" at the end.


> and 'continue' (nothing follows) 

That particular "continue" is not necessary.


> are left-overs from earlier revisions and can be thrown out?

Both of those tests should be moved into the for() test,
so the tail end of the loop always does an msleep.  It'd
be clearer as:

	timeout = ... ;
	retries = 0;
	while (retries++ < 3 || time_before(jiffies, timeout)) {
		... try write, quit loop on success ...
		msleep(1);
	}

Yes, that loop has lots of leftovers.  :(


> My main 
> questions is 'msleep(1)' though: As I understand it, this means the
> for-loop will wait roughly 3ms on failures until it reports a timeout
> (assuming good precision of msleep).

That was obviously tested only at HZ=100 or with quick-ish
EEPROMs!  I seem to recall just doing enough work on it to
make sure it didn't break ... that wasn't a top priority.

That loop should (a) run at least a few times, maybe three;
and (b) not exit before the timeout passes.


> Comparing this to AT24_EE_TIMEOUT 
> (25 ms), it looks to me that the msleep value could be increased a
> little (maybe to AT24_EE_TIMEOUT / 3)? Or is i2c_transfer slow enough
> and can we count on that behaviour?

An msleep(1) -- when the platform can deliver it -- would
be desirable, since most EEPROMs don't actually need those
long delays.  Their writes finish in just a few msecs,
hardly anything needs that huge timeout.  At HZ=100 it's
likely to sleep 10 msec or more; that's OK, but slow.

The point of having an msleep() rather than just relying
on scheduler delay is that most EEPROMs do need some short
delay to finish their writes.  So a busy-wait is wasteful.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Trent Piepho]

On Thu, 17 Apr 2008, David Brownell wrote:
> > are left-overs from earlier revisions and can be thrown out?
>
> Both of those tests should be moved into the for() test,
> so the tail end of the loop always does an msleep.  It'd
> be clearer as:
>
> 	timeout = ... ;
> 	retries = 0;
> 	while (retries++ < 3 || time_before(jiffies, timeout)) {
> 		... try write, quit loop on success ...
> 		msleep(1);
> 	}
>
> Yes, that loop has lots of leftovers.  :(

Certainly did.

Note that your loop has a flaw that can cause it to timeout incorrectly.

After the msleep's delay is used up, the process will enter the run
queue.  But there is no guarantee that the scheduler will select it to
run vs the other processes in the run queue!  It's entirely possible that
msleep(1) won't return for 1000s of ms.  On a 100 HZ system, 30 ms or
more would be quite common.  You get 20 just from the msleep, and 10 more
every time another processes runs.

So the loop tries the write for the first time, fails (which is perfectly
acceptable), then sleeps for more than timeout jiffies.  The timeout
expires and the write is never tried again.

Generally, if you want to wait at least a certain amount time for a
condition to become true, you must be sure to check the condition at
least once AFTER the time limit.  I.e, if you want to give the eeprom at
least 25 ms to respond, you need to try to write after at least 25 ms.
If 25 ms have passed, but the last time you tried to write was before 25
ms, then you didn't really give it 25 ms to respond.

Though it's less likely, on a preemptable kernel, you could be preempted
between setting timeout and the loop check, in which case the write might
never even be attempted at all!

Here's two examples of a good way to do this, depending on how you want
the control flow to go.

	unsigned long timeout = ... ;
	for (;;) {
		unsigned long wtime = jiffies;
		if (write() == success)
			break;
		if (time_after_eq(wtime, timeout)) {
			... failure ...
			return -ESOMETHING;
		}
		msleep(1);
	}
	... success ...


	unsigned long timeout = ... ;
	for (wtime = timeout; time_after(wtime, timeout);) {
		wtime = jiffies;
		if (write() == success) {
			... success ...
			return 0;
		}
		msleep(1);
	}
	... failure ...

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

David Brownell wrote:

> That was obviously tested only at HZ=100 or with quick-ish
> EEPROMs!
Ah, okay. I was aware that msleep(1) waits not less than 10ms with 
HZ=100. I simply wondered, how to reach the timeout value for bigger 
HZ-values.

> That loop should (a) run at least a few times, maybe three;
> and (b) not exit before the timeout passes.
...or (c) return successfully as soon as possible. This is why you used 
msleep(1) and not something like msleep(AT24_EE_TIMEOUT / 2). I got it :)

 > 	timeout = ... ;
 > 	retries = 0;
 > 	while (retries++ < 3 || time_before(jiffies, timeout)) {
 > 		... try write, quit loop on success ...
 > 		msleep(1);
 > 	}
This looks good, will include it!

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]

On Fri, Apr 18, 2008 at 01:07:51AM -0700, Trent Piepho wrote:

> So the loop tries the write for the first time, fails (which is perfectly
> acceptable), then sleeps for more than timeout jiffies.  The timeout
> expires and the write is never tried again.
Ehrm, the conditions for the while loop are OR-ed not AND-ed. So, even if
time_before changes to false, 'retries < 3' will keep the loop running.
I think this minimum of three tries will also cover the other issues you
(correctly) mentioned.

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Friday 18 April 2008, Trent Piepho wrote:
> On Thu, 17 Apr 2008, David Brownell wrote:
> > > are left-overs from earlier revisions and can be thrown out?
> >
> > Both of those tests should be moved into the for() test,
> > so the tail end of the loop always does an msleep.  It'd
> > be clearer as:
> >
> > 	timeout = ... ;
> > 	retries = 0;
> > 	while (retries++ < 3 || time_before(jiffies, timeout)) {
> > 		... try write, quit loop on success ...
> > 		msleep(1);
> > 	}
> >
> > Yes, that loop has lots of leftovers.  :(
> 
> Certainly did.
> 
> Note that your loop has a flaw that can cause it to timeout incorrectly.

Well, a "do ... while (...)" would obviously be better; and that's
not actually a "re"try.

But also remember that the specified "timeout" is already way beyond
what most chips need ... and that it's currently measured starting
at some point *well after* the relevant (previous) write returned.
So there's limited practical significance to problems with that loop.
It can/should be improved, sure; I'll wait to see what Wolfram does.


> So the loop tries the write for the first time, fails (which is perfectly
> acceptable), then sleeps for more than timeout jiffies.  The timeout
> expires and the write is never tried again.

That specific scenario can't happen.  It's forced to "re"try a few times,
even if the vagaries of scheduling make it sleep "too long".


> Generally, if you want to wait at least a certain amount time for a
> condition to become true, you must be sure to check the condition at
> least once AFTER the time limit.

Fair enough.  But if you're going to be concerned about that, you
should also be concerned about the start point for this "timeout".
It should begin closer to when the previous write's STOP got to the
chip.


> Though it's less likely, on a preemptable kernel, you could be preempted
> between setting timeout and the loop check, in which case the write might
> never even be attempted at all!

Again, you overlook the "re"try count, which ensures there
will be at least a few attempts.


> Here's two examples of a good way to do this, depending on how you want
> the control flow to go.
> 
> 	unsigned long timeout = ... ;
> 	for (;;) {
> 		unsigned long wtime = jiffies;

Given preemption and other scheduling delays, "wtime" can be
arbitrarily long before the bytes start to arrive at the chip.
This suffers the same flaw you mentioned above.

> 		if (write() == success)
> 			break;
> 		if (time_after_eq(wtime, timeout)) {
> 			... failure ...
> 			return -ESOMETHING;
> 		}
> 		msleep(1);
> 	}
> 	... success ...
> 
> 
> 	unsigned long timeout = ... ;
> 	for (wtime = timeout; time_after(wtime, timeout);) {
> 		wtime = jiffies;
> 		if (write() == success) {
> 			... success ...
> 			return 0;
> 		}
> 		msleep(1);
> 	}
> 	... failure ...

Neither of those loops ever tries more than once, note.

- Dave




_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Trent Piepho]

On Fri, 18 Apr 2008, Wolfram Sang wrote:
> On Fri, Apr 18, 2008 at 01:07:51AM -0700, Trent Piepho wrote:
>
> > So the loop tries the write for the first time, fails (which is perfectly
> > acceptable), then sleeps for more than timeout jiffies.  The timeout
> > expires and the write is never tried again.
> Ehrm, the conditions for the while loop are OR-ed not AND-ed. So, even if
> time_before changes to false, 'retries < 3' will keep the loop running.
> I think this minimum of three tries will also cover the other issues you
> (correctly) mentioned.

Oh, of course.  I had ignored the retries because it seemed like a bad
idea.  If the timeout is based on time, why does it matter how many tries
there were?

Still, if you want to wait at least 25 ms, on a HZ=1000 system you might
wait only 3 ms.  And on a HZ=100 system, you'll wait at least 60 ms when
the timeout only needed to be 25 ms.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Trent Piepho]

On Fri, 18 Apr 2008, David Brownell wrote:
> > So the loop tries the write for the first time, fails (which is perfectly
> > acceptable), then sleeps for more than timeout jiffies.  The timeout
> > expires and the write is never tried again.
>
> That specific scenario can't happen.  It's forced to "re"try a few times,
> even if the vagaries of scheduling make it sleep "too long".

Yeah, I missed that.  Though it's still possible that the last write
attempt will be only 3 ms after the loop started, even if the timeout is
supposed to be longer.


> > Generally, if you want to wait at least a certain amount time for a
> > condition to become true, you must be sure to check the condition at
> > least once AFTER the time limit.
>
> Fair enough.  But if you're going to be concerned about that, you
> should also be concerned about the start point for this "timeout".
> It should begin closer to when the previous write's STOP got to the
> chip.

As long as the start point is after the last write, and we know it is, then
the timeout is correct.  You want to be sure at least X ms have elapsed
since the previous write operation finished and the last failed attempt at
the current current.  Starting the timeout closer to the previous write's
STOP would let you avoid waiting longer than necessary, but it's not needed
for correctness.

> > Here's two examples of a good way to do this, depending on how you want
> > the control flow to go.
> >
> > 	unsigned long timeout = ... ;
> > 	for (;;) {
> > 		unsigned long wtime = jiffies;
>
> Given preemption and other scheduling delays, "wtime" can be
> arbitrarily long before the bytes start to arrive at the chip.
> This suffers the same flaw you mentioned above.

I'm afraid I don't follow you.  How is possible for this loop to timeout
before the chip has been given the minimum time needed to respond?

>
> > 		if (write() == success)
> > 			break;
> > 		if (time_after_eq(wtime, timeout)) {
> > 			... failure ...
> > 			return -ESOMETHING;
> > 		}
> > 		msleep(1);
> > 	}
> > 	... success ...
> >
> >
> > 	unsigned long timeout = ... ;
> > 	for (wtime = timeout; time_after(wtime, timeout);) {
              this should be !time_after
> > 		wtime = jiffies;
> > 		if (write() == success) {
> > 			... success ...
> > 			return 0;
> > 		}
> > 		msleep(1);
> > 	}
> > 	... failure ...
>
> Neither of those loops ever tries more than once, note.

Sure they do.  They will aways try *at least* once, maybe more times
depending on how many chances they get before enough time has elapsed.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

Trent Piepho wrote:

> Oh, of course.  I had ignored the retries because it seemed like a bad
> idea.  If the timeout is based on time, why does it matter how many tries
> there were?
Because then you have a guaranteed number of tries, even if the timeout 
value was reached due to some reason.

> Still, if you want to wait at least 25 ms, on a HZ=1000 system you might
> wait only 3 ms.  
I'm sorry, I fail to see this. If there are more than three retries, 
then there is still the time_before-condition which keeps the loop 
running until the timeout is reached, no?

> And on a HZ=100 system, you'll wait at least 60 ms when
> the timeout only needed to be 25 ms.
Yes, because there is this policy to retry at least three times. Maybe 
it is an idea to introduce a module parameter which lets the user select 
a suitable retry parameter?

    Wolfram

-- 
   Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
  Pengutronix - Linux Solutions for Science and Industry


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Trent Piepho]

On Mon, 21 Apr 2008, Wolfram Sang wrote:
> Trent Piepho wrote:
> > Oh, of course.  I had ignored the retries because it seemed like a bad
> > idea.  If the timeout is based on time, why does it matter how many tries
> > there were?
> Because then you have a guaranteed number of tries, even if the timeout
> value was reached due to some reason.

Is there a reason to always try at least a certain number of times?

If it hasn't been long enough since the last write, the next write isn't
suppose to work.  That's the expected operation of the device.  But if it
has been long enough, and the write still fails, then it seems to me that
the behavior has changed from normal operation to an error.

> > Still, if you want to wait at least 25 ms, on a HZ=1000 system you might
> > wait only 3 ms.
> I'm sorry, I fail to see this. If there are more than three retries,
> then there is still the time_before-condition which keeps the loop
> running until the timeout is reached, no?

Except for the timing problem I pointed out before.  The timeout is checked
before the write takes place.  So if after the 3rd attempt the msleep(), or
kernel preemption, etc., delays for 22 ms or more, the next write will
never happen.

> > And on a HZ=100 system, you'll wait at least 60 ms when
> > the timeout only needed to be 25 ms.
> Yes, because there is this policy to retry at least three times. Maybe
> it is an idea to introduce a module parameter which lets the user select
> a suitable retry parameter?

I wouldn't have extra retries.  It's part of the chip's specs that there
has to be some delay, exactly how much we don't know, between writes.  But
is there an eeprom that says in its specs that writes are unreliable?  That
they may fail for no reason and should be tried multiple times?

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 1614 bytes --]

On Mon, Apr 21, 2008 at 10:20:17AM -0700, Trent Piepho wrote:

> If it hasn't been long enough since the last write, the next write
> isn't suppose to work.  That's the expected operation of the device.
> But if it has been long enough, and the write still fails, then it
> seems to me that the behavior has changed from normal operation to an
> error.
I think you got a point there (unless someone gained experience that
retries do help for some quirky chips or other cases).

> > > Still, if you want to wait at least 25 ms, on a HZ=1000 system you
> > > might wait only 3 ms.
> > I'm sorry, I fail to see this. If there are more than three retries,
> > then there is still the time_before-condition which keeps the loop
> > running until the timeout is reached, no?
> Except for the timing problem I pointed out before.  The timeout is
> checked before the write takes place.  So if after the 3rd attempt the
> msleep(), or kernel preemption, etc., delays for 22 ms or more, the
> next write will never happen.
Got it now, I misunderstood you before. We do wait 25ms in total, it is
just that the last write-try happened at 3ms. This is indeed bad. The
following code should handle it better. (Skipping retries for now)

---
	timeout = jiffies + msecs_to_jiffies(write_timeout);
	do {
		keep_trying = time_before(jiffies, timeout);

		transfer();

		if (success)
			return count;

		msleep(1);
	} while (keep_trying);

	return -ETIMEDOUT;
---

Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Trent Piepho]

On Thu, 24 Apr 2008, Wolfram Sang wrote:
> On Mon, Apr 21, 2008 at 10:20:17AM -0700, Trent Piepho wrote:
> > > > Still, if you want to wait at least 25 ms, on a HZ=1000 system you
> > > > might wait only 3 ms.
> > > I'm sorry, I fail to see this. If there are more than three retries,
> > > then there is still the time_before-condition which keeps the loop
> > > running until the timeout is reached, no?
> > Except for the timing problem I pointed out before.  The timeout is
> > checked before the write takes place.  So if after the 3rd attempt the
> > msleep(), or kernel preemption, etc., delays for 22 ms or more, the
> > next write will never happen.
> Got it now, I misunderstood you before. We do wait 25ms in total, it is
> just that the last write-try happened at 3ms. This is indeed bad. The
> following code should handle it better. (Skipping retries for now)

I guess I should have said "only give the chip 3ms to respond" instead of
"only wait 3ms", since we do wait the 25ms.

> 	timeout = jiffies + msecs_to_jiffies(write_timeout);
> 	do {
> 		keep_trying = time_before(jiffies, timeout);
>
> 		transfer();
>
> 		if (success)
> 			return count;
>
> 		msleep(1);
> 	} while (keep_trying);
>
> 	return -ETIMEDOUT;

That works.  Or a very minor optimization not to evaluate the time_before()
unless the write failes:

 	timeout = jiffies + msecs_to_jiffies(write_timeout);
 	do {
 		unsigned long wtime = jiffies;

 		transfer();

 		if (success)
 			return count;

 		msleep(1);
 	} while (time_before(wtime, jiffies));

 	return -ETIMEDOUT;

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
access to their data. Tested with various chips and clock rates.

Updates since last version:

 - revisited includes
 - made write-timeout a module parameter
 - array of clients is allocated dynamically
 - removed unnecessary indentation within code
 - formatted comments
 - replaced at24_ee_address with a simpler function
 - at24_ee_write now really waits till timeout
 - added simple checks of provided eeprom chip data in at24_probe
 - added comment in at24.h about double-checking custom data
 - minor fixes

Updates in this version:

 - move chip data out of the driver into a seperate .h-file
 - prefix defined constants with AT24_
 - make bin file readonly if requested by flags
 - introduce AT24_MAX_CLIENTS
 - bugfix: check correct retval in at24_ee_write

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---

As we now support device_ids, probably most of the chip data will come back
into the driver. This is yet to be done. Tested on two platforms by me and
another one by David. Builds also on x86.

 drivers/i2c/chips/Kconfig  |   26 ++
 drivers/i2c/chips/Makefile |    1 +
 drivers/i2c/chips/at24.c   |  571 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/at24.h   |  102 ++++++++
 4 files changed, 700 insertions(+), 0 deletions(-)
 create mode 100644 drivers/i2c/chips/at24.c
 create mode 100644 include/linux/i2c/at24.h

diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
index 2da2edf..ad776e3 100644
--- a/drivers/i2c/chips/Kconfig
+++ b/drivers/i2c/chips/Kconfig
@@ -14,6 +14,32 @@ config DS1682
 	  This driver can also be built as a module.  If so, the module
 	  will be called ds1682.
 
+config I2C_AT24
+	tristate "EEPROMs from most vendors"
+	depends on I2C && SYSFS && EXPERIMENTAL
+	help
+	  Enable this driver to get read/write support to most I2C EEPROMs,
+	  after you configure the driver to know about each eeprom on on
+	  your target board.  Use these generic chip names, instead of
+	  vendor-specific ones like at24c64 or 24lc02:
+
+	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
+	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
+
+	  Unless you like data loss puzzles, always be sure that any chip
+	  you configure as a 24c32 (32 Kbits) or larger is NOT really a
+	  24c16 (16 Kbits) or smaller, and vice versa.  (Marking the chip
+	  as readonly won't help recover from this.)  Also, if your chip
+	  has any software write-protect mechanism you may want to make
+	  sure this driver won't turn it on by accident.
+
+	  If you use this with an SMBus adapter instead of an I2C adapter,
+	  full functionality is not availble.  Only smaller devices are
+	  supported (24c16 and below, max 4 KBytes).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called at24.
+
 config SENSORS_EEPROM
 	tristate "EEPROM reader"
 	depends on EXPERIMENTAL
diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
index e47aca0..aa1a88c 100644
--- a/drivers/i2c/chips/Makefile
+++ b/drivers/i2c/chips/Makefile
@@ -10,6 +10,7 @@
 #
 
 obj-$(CONFIG_DS1682)		+= ds1682.o
+obj-$(CONFIG_I2C_AT24)		+= at24.o
 obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
 obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
 obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
diff --git a/drivers/i2c/chips/at24.c b/drivers/i2c/chips/at24.c
new file mode 100644
index 0000000..35c091c
--- /dev/null
+++ b/drivers/i2c/chips/at24.c
@@ -0,0 +1,571 @@
+/*
+ * at24.c - handle most I2C EEPROMs
+ *
+ * Copyright (C) 2005-2007 David Brownell
+ * Copyright (C) 2008 Wolfram Sang, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
+#include <linux/log2.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/i2c/at24.h>
+
+/*
+ * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
+ * Differences between different vendor product lines (like Atmel AT24C or
+ * MicroChip 24LC, etc) won't much matter for typical read/write access.
+ * There are also I2C RAM chips, likewise interchangeable.  One example
+ * would be the PCF8570, which acts like a 24c02 EEPROM (256 bytes).
+ *
+ * However, misconfiguration can lose data. "Set 16-bit memory address"
+ * to a part with 8-bit addressing will overwrite data.  Writing with too
+ * big a page size also loses data.  And it's not safe to assume that the
+ * conventional addresses 0x50..0x57 only hold eeproms ... a pcf8563 RTC
+ * uses 0x51, for just one example.
+ *
+ * Accordingly, explicit board-specific configuration data should be used
+ * in almost all cases.  (One partial exception is an SMBus used to access
+ * "SPD" data for DRAM sticks.  Those only use 24c02 EEPROMs.)
+ *
+ * So this driver uses "new style" I2C driver binding, expecting to be
+ * told what devices exist.  That may be in arch/X/mach-Y/board-Z.c or
+ * similar kernel-resident tables; or, configuration data coming from
+ * a bootloader.  Be sure to set the board_info "type" to identify the
+ * eeprom type.
+ *
+ * Other than binding model, current differences from "eeprom" driver are
+ * that this one handles write access and isn't restricted to 24c02 devices.
+ * It also handles larger devices (32 kbit and up) with two-byte addresess,
+ * which won't work on pure SMBus systems.
+ */
+
+struct at24_data {
+	struct at24_platform_data chip;
+	bool use_smbus;
+
+	/*
+	 * Lock protects against activities from other Linux tasks,
+	 * but not from changes by other I2C masters.
+	 */
+	struct mutex lock;
+	struct bin_attribute bin;
+
+	u8 *writebuf;
+	unsigned write_max;
+
+	/*
+	 * Some chips tie up multiple I2C addresses; dummy devices reserve
+	 * them for ourselves, and we'll use them with SMBus calls.
+	 */
+	struct i2c_client *client[];
+};
+
+/*-------------------------------------------------------------------------*/
+
+/*
+ * This parameter is to help this driver avoid blocking other drivers out
+ * of I2C for potentially troublesome amounts of time.  With a 100 KHz I2C
+ * clock, one 256 byte read takes about 1/43 second which is excessive;
+ * but the 1/170 second it takes at 400 KHz may be quite reasonable; and
+ * at 1 MHz (Fm+) a 1/430 second delay could easily be invisible.
+ *
+ * This value is forced to be a power of two so that writes align on pages.
+ */
+static unsigned io_limit = 128;
+module_param(io_limit, uint, 0);
+MODULE_PARM_DESC(io_limit, "maximum bytes per i/o (default 128)");
+
+/*
+ * Specs often allow 5 msec for a page write, sometimes 20 msec;
+ * it's important to recover from write timeouts.
+ */
+static unsigned write_timeout = 25;
+module_param(write_timeout, uint, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(write_timeout, "time (in ms) to try writes (default 25)");
+/*
+ * This routine supports chips which consume multiple I2C addresess.  It
+ * computes the addressing information to be used for a given r/w request.
+ */
+static struct i2c_client *at24_ee_address(struct at24_data *at24, u16 *addr,
+		unsigned *offset)
+{
+	struct at24_platform_data *chip = &at24->chip;
+	unsigned i;
+
+	if (*offset >= chip->byte_len)
+		return NULL;
+
+	if (chip->flags & AT24_EE_ADDR2) {
+		i = *offset >> 16;
+		*offset &= 0xffff;
+	} else {
+		i = *offset >> 8;
+		*offset &= 0xff;
+	}
+
+	if (unlikely(i > chip->i2c_addr_mask)) {
+		dev_err(&at24->client[0]->dev,
+			"requested client %u not available!\n", i);
+		return NULL;
+	}
+
+	*addr = at24->client[i]->addr;
+	return at24->client[i];
+}
+
+
+/*-------------------------------------------------------------------------*/
+
+static ssize_t at24_ee_read(struct at24_data *at24, char *buf, unsigned offset,
+		size_t count)
+{
+	struct i2c_msg msg[2];
+	u8 addr[2];
+	struct i2c_client *client;
+	int status;
+
+	memset(msg, 0, sizeof msg);
+
+	/*
+	 * REVISIT some multi-address chips don't rollover page reads to
+	 * the next slave address, so we may need to truncate the count.
+	 * Those chips might need another quirk flag...
+	 *
+	 * If the real hardware used four adjacent 24c02 chips and that
+	 * were misconfiged as one 24c08, that would be a similar effect:
+	 * one "eeprom" file not four, but larger reads would fail when
+	 * they crossed certain pages.
+	 */
+	if (count > io_limit)
+		count = io_limit;
+
+	/*
+	 * Slave address and byte offset derive from the offset.  Always
+	 * set the byte address; on a multi-master board, another master
+	 * may have changed the chip's "current" address pointer.
+	 */
+	client = at24_ee_address(at24, &msg[0].addr, &offset);
+	if (!client)
+		return -EINVAL;
+
+	/* Smaller eproms can work given some SMBus extension calls */
+	if (at24->use_smbus) {
+		if (count > 32)
+			count = 32;
+		status = i2c_smbus_read_i2c_block_data(client, offset,
+				count, buf);
+		dev_dbg(&client->dev, "smbus read %zd@%d --> %d\n",
+				count, offset, status);
+		return (status < 0) ? -EIO : status;
+	}
+
+	/*
+	 * When we have a better choice than SMBus calls, use a combined
+	 * I2C message.  Write address; then read up to io_limit data bytes.
+	 * Note that read page rollover helps us here (unlike writes).
+	 */
+	msg[0].buf = addr;
+	addr[1] = (u8) offset;
+	addr[0] = (u8) (offset >> 8);
+
+	if (at24->chip.flags & AT24_EE_ADDR2)
+		msg[0].len = 2;
+	else {
+		msg[0].len = 1;
+		msg[0].buf++;
+	}
+
+	msg[1].addr = client->addr;
+	msg[1].flags = I2C_M_RD;
+	msg[1].buf = buf;
+	msg[1].len = count;
+
+	status = i2c_transfer(client->adapter, msg, 2);
+	dev_dbg(&client->dev, "i2c read %zd@%d --> %d\n",
+			count, offset, status);
+
+	if (status == 2)
+		return count;
+	else if (status >= 0)
+		return -EIO;
+	else
+		return status;
+}
+
+static ssize_t at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct i2c_client *client;
+	struct at24_data *at24;
+	ssize_t retval = 0;
+
+	client = to_i2c_client(container_of(kobj, struct device, kobj));
+	at24 = i2c_get_clientdata(client);
+
+	if (unlikely(off >= at24->bin.size))
+		return 0;
+	if ((off + count) > at24->bin.size)
+		count = at24->bin.size - off;
+	if (unlikely(!count))
+		return count;
+
+	/*
+	 * Read data from chip, protecting against concurrent updates
+	 * from this host ... but not from other i2c masters.
+	 */
+	mutex_lock(&at24->lock);
+	do {
+		ssize_t	status;
+
+		status = at24_ee_read(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+
+	} while (count);
+	mutex_unlock(&at24->lock);
+
+	return retval;
+}
+
+
+/*
+ * REVISIT:  export at24_bin{read,write}() to let other kernel code use
+ * eeprom data.  For example, it might hold a board's Ethernet address, or
+ * board-specific calibration data generated on the manufacturing floor.
+ */
+
+
+/*
+ * Note that if the hardware write-protect pin is pulled high, the whole
+ * chip is normally write protected.  But there are plenty of product
+ * variants here, including OTP fuses and partial chip protect.
+ *
+ * We only use page mode writes; the alternative is sloooow.  This routine
+ * writes at most one page.
+ */
+static ssize_t at24_ee_write(struct at24_data *at24, char *buf, loff_t off,
+		size_t count)
+{
+	struct i2c_client *client;
+	struct i2c_msg  msg;
+	unsigned offset = (unsigned) off;
+	ssize_t status = 0;
+	unsigned long timeout, write_time;
+	unsigned c0, c1;
+
+	/* Maybe adjust i2c address and offset */
+	client = at24_ee_address(at24, &msg.addr, &offset);
+	if (!client)
+		return -EINVAL;
+
+	/* write_max is at most a page */
+	if (count > at24->write_max)
+		count = at24->write_max;
+
+	/* Never roll over backwards, to the start of this page */
+	c0 = offset + count;
+	c1 = roundup(offset + 1, at24->chip.page_size);
+	if (c0 > c1)
+		count -= c1 - c0;
+
+	/* If we'll use i2c calls for i/o, set up the message */
+	if (!at24->use_smbus) {
+		msg.flags = 0;
+		msg.len = count + 1;
+		msg.buf = at24->writebuf;
+		if (at24->chip.flags & AT24_EE_ADDR2) {
+			msg.len++;
+			msg.buf[1] = (u8) offset;
+			msg.buf[0] = (u8) (offset >> 8);
+			memcpy(&msg.buf[2], buf, count);
+		} else {
+			msg.buf[0] = offset;
+			memcpy(&msg.buf[1], buf, count);
+		}
+	}
+
+	/*
+	 * Writes fail if the previous one didn't complete yet.  We'll
+	 * loop a few times until this one succeeds, waiting at least
+	 * long enough for one entire page write to work.
+	 */
+	timeout = jiffies + msecs_to_jiffies(write_timeout);
+	do {
+		write_time = jiffies;
+		if (at24->use_smbus) {
+			status = i2c_smbus_write_i2c_block_data(client,
+					offset, count, buf);
+			if (status == 0)
+				status = count;
+		} else {
+			status = i2c_transfer(client->adapter, &msg, 1);
+			if (status == 1)
+				status = count;
+		}
+		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
+				count, offset, status, jiffies);
+
+		if (status == count)
+			return count;
+
+		/* REVISIT:  at HZ=100, this is sloooow */
+		msleep(1);
+	} while (time_before(write_time, timeout));
+
+	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
+			count, offset, jiffies
+			- (timeout - msecs_to_jiffies(write_timeout)));
+	return -ETIMEDOUT;
+}
+
+static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct i2c_client *client;
+	struct at24_data *at24;
+	ssize_t retval = 0;
+
+	client = to_i2c_client(container_of(kobj, struct device, kobj));
+	at24 = i2c_get_clientdata(client);
+
+	if (unlikely(off >= at24->bin.size))
+		return -EFBIG;
+	if ((off + count) > at24->bin.size)
+		count = at24->bin.size - off;
+	if (unlikely(!count))
+		return count;
+
+	/*
+	 * Write data from chip, protecting against concurrent updates
+	 * from this host ... but not from other i2c masters.
+	 */
+	mutex_lock(&at24->lock);
+
+	/* buffer big enough to stick the address at the beginning */
+	at24->writebuf = kmalloc(at24->write_max + 2, GFP_KERNEL);
+	if (!at24->writebuf) {
+		retval = -ENOMEM;
+		count = 0;
+	}
+
+	while (count) {
+		ssize_t	status;
+
+		status = at24_ee_write(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+	}
+
+	kfree(at24->writebuf);
+	at24->writebuf = NULL;
+	mutex_unlock(&at24->lock);
+
+	return retval;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct at24_platform_data *chip;
+	bool writable;
+	bool use_smbus = false;
+	struct at24_data *at24;
+	int err;
+	unsigned i, var_size;
+	struct i2c_client *c;
+
+	chip = client->dev.platform_data;
+	if (!chip) {
+		err = -ENODEV;
+		goto fail;
+	}
+	if (!is_power_of_2(chip->byte_len))
+		dev_warn(&client->dev,
+			"byte_len looks suspicious (no power of 2)!\n");
+	if (!is_power_of_2(chip->page_size))
+		dev_warn(&client->dev,
+			"page_size looks suspicious (no power of 2)!\n");
+	if (!is_power_of_2(chip->i2c_addr_mask + 1))
+		dev_warn(&client->dev,
+			"i2c_addr_mask looks suspicious (unusual mask)!\n");
+
+	/* Use I2C operations unless we're stuck with SMBus extensions. */
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		if (chip->flags & AT24_EE_ADDR2) {
+			err = -ECOMM;
+			goto fail;
+		}
+		if (!i2c_check_functionality(client->adapter,
+				I2C_FUNC_SMBUS_I2C_BLOCK)) {
+			err = -EBADMSG;
+			goto fail;
+		}
+		use_smbus = true;
+	}
+
+	var_size = (chip->i2c_addr_mask + 1) * sizeof(struct i2c_client *);
+	at24 = kzalloc(sizeof(struct at24_data) + var_size, GFP_KERNEL);
+	if (!at24) {
+		err = -ENOMEM;
+		goto fail;
+	}
+
+	mutex_init(&at24->lock);
+	at24->chip = *chip;
+	at24->use_smbus = use_smbus;
+
+	/*
+	 * Export the EEPROM bytes through sysfs, since that's convenient.
+	 * By default, only root should see the data (maybe passwords etc)
+	 */
+	at24->bin.attr.name = "eeprom";
+	at24->bin.attr.mode = S_IRUSR;
+	at24->bin.attr.owner = THIS_MODULE;
+	at24->bin.read = at24_bin_read;
+
+	at24->bin.size = at24->chip.byte_len;
+
+	writable = ((chip->flags & AT24_EE_READONLY) == 0);
+	if (writable) {
+		unsigned write_max = at24->chip.page_size;
+
+		at24->bin.write = at24_bin_write;
+		at24->bin.attr.mode |= S_IWUSR;
+
+		if (write_max > io_limit)
+			write_max = io_limit;
+		if (use_smbus && write_max > 32)
+			write_max = 32;
+		at24->write_max = write_max;
+	}
+
+	/*
+	 * REVISIT read a byte, to make sure the chip is actually
+	 * present (vs misconfiguration, or not-populated-here)
+	 */
+
+	at24->client[0] = client;
+	i2c_set_clientdata(client, at24);
+
+	/* use dummy devices for multiple-address chips */
+	if (chip->i2c_addr_mask) {
+		for (i = 1; i <= chip->i2c_addr_mask; i++) {
+			c = i2c_new_dummy(client->adapter, client->addr + i);
+			if (!c) {
+				dev_err(&client->dev, "addr %d unavail\n",
+						client->addr + i);
+				err = -ENOCSI;
+				goto cleanup;
+			}
+			at24->client[i] = c;
+		}
+	}
+
+	err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
+	if (err)
+		goto cleanup;
+
+	dev_info(&client->dev, "%Zd byte %s EEPROM%s\n",
+		at24->bin.size, client->name,
+		writable ? " (writable)" : "");
+	dev_dbg(&client->dev,
+		"page_size %d, i2c_addr_mask %d, write_max %d%s\n",
+		at24->chip.page_size, at24->chip.i2c_addr_mask,
+		at24->write_max,
+		use_smbus ? ", use_smbus" : "");
+	return 0;
+
+cleanup:
+	if (chip->i2c_addr_mask) {
+		for (i = 1; i <= chip->i2c_addr_mask; i++) {
+			c = at24->client[i];
+			if (c)
+				i2c_unregister_device(c);
+		}
+	}
+
+	kfree(at24);
+fail:
+	dev_dbg(&client->dev, "probe err %d\n", err);
+	return err;
+}
+
+static int __devexit at24_remove(struct i2c_client *client)
+{
+	struct at24_data *at24;
+	struct i2c_client *c;
+	int i;
+
+	at24 = i2c_get_clientdata(client);
+	sysfs_remove_bin_file(&client->dev.kobj, &at24->bin);
+
+	if (at24->chip.i2c_addr_mask) {
+		for (i = 1; i <= at24->chip.i2c_addr_mask; i++) {
+			c = at24->client[i];
+			if (c)
+				i2c_unregister_device(c);
+		}
+	}
+
+	kfree(at24);
+	return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct i2c_device_id at24_ids[] = {
+	{ "at24", 0, },
+	{ /* END OF LIST */ }
+};
+
+static struct i2c_driver at24_driver = {
+	.driver = {
+		.name = "at24",
+		.owner = THIS_MODULE,
+	},
+	.probe = at24_probe,
+	.remove = __devexit_p(at24_remove),
+	.id_table = at24_ids,
+};
+
+static int __init at24_init(void)
+{
+	io_limit = rounddown_pow_of_two(io_limit);
+	return i2c_add_driver(&at24_driver);
+}
+module_init(at24_init);
+
+static void __exit at24_exit(void)
+{
+	i2c_del_driver(&at24_driver);
+}
+module_exit(at24_exit);
+
+MODULE_DESCRIPTION("Driver for most I2C EEPROMs");
+MODULE_AUTHOR("David Brownell");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
new file mode 100644
index 0000000..74635ee
--- /dev/null
+++ b/include/linux/i2c/at24.h
@@ -0,0 +1,102 @@
+
+#ifndef _LINUX_AT24_H
+#define _LINUX_AT24_H
+
+#include <linux/types.h>
+
+/*
+ * As seen through Linux I2C, differences between the most common types
+ * of I2C memory include:
+ *	- How many I2C addresses the chip consumes: 1, 2, 4...?
+ *	- Memory address space for one I2C address:  256 byte, or 64 KB?
+ *	- How full that memory space is:  16 bytes, 256, 32Kb, etc?
+ *	- What write page size does it support?
+ */
+
+struct at24_platform_data {
+	u32		byte_len;		/* size (sum of all addr) */
+	u16		page_size;		/* for writes */
+	u8		i2c_addr_mask;		/* for multi-addr chips */
+	u8		flags;			/* quirks, etc */
+#define	AT24_EE_ADDR2		0x0080		/* 16 bit addrs; <= 64 KB */
+#define	AT24_EE_READONLY	0x0040
+};
+
+/*
+ * If you use this macro to set up a custom eeprom type, please make sure you
+ * have double checked the parameters. Especially page_size needs extra care,
+ * as you risk data loss if your value is bigger than what the chip actually
+ * supports!
+ */
+#define AT24_PLATFORM_DATA(_name, _len, _page, _mask, _flags) \
+struct at24_platform_data at24_platform_data_##_name = {	\
+	.byte_len	= _len,		\
+	.page_size	= _page,	\
+	.i2c_addr_mask	= _mask,	\
+	.flags		= _flags,	\
+};
+
+/* 128 bit chip, I2C A0-A2 ignored */
+#define AT24_PLATFORM_DATA_24C00 \
+	AT24_PLATFORM_DATA(24c00, 128 / 8, 1, 0x07, 0)
+
+/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
+#define AT24_PLATFORM_DATA_24C01 \
+	AT24_PLATFORM_DATA(24c01, 1024 / 8, 8, 0, 0)
+
+/* 2 Kbit chip, some have 16 byte pages: */
+#define AT24_PLATFORM_DATA_24C02 \
+	AT24_PLATFORM_DATA(24c02, 2048 / 8, 8, 0, 0)
+
+/* 2 Kbit chip, 24c02 in memory DIMMs, some have 16 byte pages */
+#define AT24_PLATFORM_DATA_SPD \
+	AT24_PLATFORM_DATA(spd, 2048 / 8, 8, 0, AT24_EE_READONLY)
+
+/* 2 Kbit chip, SRAM, not EEPROM!, no page size issues, write it all at once */
+#define AT24_PLATFORM_DATA_PCF8570 \
+	AT24_PLATFORM_DATA(pcf8570, 2048 / 8, 2048 / 8, 0, 0)
+
+/* 4 Kbit chip, I2C A0 is MEM A8 */
+#define AT24_PLATFORM_DATA_24C04 \
+	AT24_PLATFORM_DATA(24c04, 4096 / 8, 16, 0x01, 0)
+
+/*
+ * 8 Kbit chip, I2C A1-A0 is MEM A9-A8, works also with 24RF08
+ * (its quirk is handled at i2c-core-level)
+ */
+#define AT24_PLATFORM_DATA_24C08 \
+	AT24_PLATFORM_DATA(24c08, 8192 / 8, 16, 0x03, 0)
+
+/* 16 Kbit chip, I2C A2-A0 is MEM A10-A8 */
+#define AT24_PLATFORM_DATA_24C16 \
+	AT24_PLATFORM_DATA(24c16, 16384 / 8, 16, 0x07, 0)
+
+
+/* this second block of EEPROMS uses 16 bit memory addressing */
+
+/* 32 Kbits */
+#define AT24_PLATFORM_DATA_24C32 \
+	AT24_PLATFORM_DATA(24c32, 32768 / 8, 32, 0, AT24_EE_ADDR2)
+
+/* 64 Kbits */
+#define AT24_PLATFORM_DATA_24C64 \
+	AT24_PLATFORM_DATA(24c64, 65536 / 8, 32, 0, AT24_EE_ADDR2)
+
+/* 128 Kbits */
+#define AT24_PLATFORM_DATA_24C128 \
+	AT24_PLATFORM_DATA(24c128, 131072 / 8, 64, 0, AT24_EE_ADDR2)
+
+/* 256 Kbits */
+#define AT24_PLATFORM_DATA_24C256 \
+	AT24_PLATFORM_DATA(24c256, 262144 / 8, 64, 0, AT24_EE_ADDR2)
+
+/* 512 Kbits */
+#define AT24_PLATFORM_DATA_24C512 \
+	AT24_PLATFORM_DATA(24c512, 524288 / 8, 128, 0, AT24_EE_ADDR2)
+
+/* 1 Mbits, I2C A0 is MEM A16 */
+#define AT24_PLATFORM_DATA_24C1024 \
+	AT24_PLATFORM_DATA(24c1024, 1048576 / 8, 256, 0x01, AT24_EE_ADDR2)
+
+#endif /* _LINUX_AT24_H */
+
-- 
1.5.5.1


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi Wolfram, hi David,

On Thu, 15 May 2008 22:36:39 +0200, Wolfram Sang wrote:
> Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
> access to their data. Tested with various chips and clock rates.

Preliminary note: I'm curious if sysfs is the right interface for
writable EEPROMs. Isn't /dev/mtd supposed to be used for this purpose?

> 
> Updates since last version:
> 
>  - revisited includes
>  - made write-timeout a module parameter
>  - array of clients is allocated dynamically
>  - removed unnecessary indentation within code
>  - formatted comments
>  - replaced at24_ee_address with a simpler function
>  - at24_ee_write now really waits till timeout
>  - added simple checks of provided eeprom chip data in at24_probe
>  - added comment in at24.h about double-checking custom data
>  - minor fixes
> 
> Updates in this version:
> 
>  - move chip data out of the driver into a seperate .h-file
>  - prefix defined constants with AT24_
>  - make bin file readonly if requested by flags
>  - introduce AT24_MAX_CLIENTS
>  - bugfix: check correct retval in at24_ee_write
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Here comes my long overdue review:

> ---
> 
> As we now support device_ids, probably most of the chip data will come back
> into the driver. This is yet to be done. Tested on two platforms by me and
> another one by David. Builds also on x86.

We have to make a decision about the strategy now, and stick to it.
Changing it after the driver is upstream will cause a lot of confusion.
I think that having predefined names for the most common EEPROM sizes
would be a good idea. You can also keep a generic "at24" type where all
the parameters have to be provided by platform code.

> 
>  drivers/i2c/chips/Kconfig  |   26 ++
>  drivers/i2c/chips/Makefile |    1 +
>  drivers/i2c/chips/at24.c   |  571 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/at24.h   |  102 ++++++++
>  4 files changed, 700 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/i2c/chips/at24.c
>  create mode 100644 include/linux/i2c/at24.h
> 
> diff --git a/drivers/i2c/chips/Kconfig b/drivers/i2c/chips/Kconfig
> index 2da2edf..ad776e3 100644
> --- a/drivers/i2c/chips/Kconfig
> +++ b/drivers/i2c/chips/Kconfig
> @@ -14,6 +14,32 @@ config DS1682
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called ds1682.
>  
> +config I2C_AT24

I'd suggest just "AT24".

> +	tristate "EEPROMs from most vendors"
> +	depends on I2C && SYSFS && EXPERIMENTAL

Dependency upon I2C is handled at the menu level, so no need to mention
it here.

> +	help
> +	  Enable this driver to get read/write support to most I2C EEPROMs,
> +	  after you configure the driver to know about each eeprom on on

s/eeprom/EEPROM/, doubled "on".

> +	  your target board.  Use these generic chip names, instead of
> +	  vendor-specific ones like at24c64 or 24lc02:
> +
> +	     24c00, 24c01, 24c02, spd (readonly 24c02), 24c04, 24c08,
> +	     24c16, 24c32, 24c64, 24c128, 24c256, 24c512, 24c1024
> +
> +	  Unless you like data loss puzzles, always be sure that any chip
> +	  you configure as a 24c32 (32 Kbits) or larger is NOT really a
> +	  24c16 (16 Kbits) or smaller, and vice versa.  (Marking the chip

Lowercase k to "kbit". There are many other occurrences of this mistake
all over the patch, I've not commented on all of them, but please fix
them all.

> +	  as readonly won't help recover from this.)  Also, if your chip

The parentheses don't seem to be needed. "read-only".

> +	  has any software write-protect mechanism you may want to make
> +	  sure this driver won't turn it on by accident.

How?

> +
> +	  If you use this with an SMBus adapter instead of an I2C adapter,
> +	  full functionality is not availble.  Only smaller devices are
> +	  supported (24c16 and below, max 4 KBytes).

Lowercase k.

> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called at24.
> +
>  config SENSORS_EEPROM
>  	tristate "EEPROM reader"
>  	depends on EXPERIMENTAL
> diff --git a/drivers/i2c/chips/Makefile b/drivers/i2c/chips/Makefile
> index e47aca0..aa1a88c 100644
> --- a/drivers/i2c/chips/Makefile
> +++ b/drivers/i2c/chips/Makefile
> @@ -10,6 +10,7 @@
>  #
>  
>  obj-$(CONFIG_DS1682)		+= ds1682.o
> +obj-$(CONFIG_I2C_AT24)		+= at24.o
>  obj-$(CONFIG_SENSORS_EEPROM)	+= eeprom.o
>  obj-$(CONFIG_SENSORS_MAX6875)	+= max6875.o
>  obj-$(CONFIG_SENSORS_PCA9539)	+= pca9539.o
> diff --git a/drivers/i2c/chips/at24.c b/drivers/i2c/chips/at24.c
> new file mode 100644
> index 0000000..35c091c
> --- /dev/null
> +++ b/drivers/i2c/chips/at24.c
> @@ -0,0 +1,571 @@
> +/*
> + * at24.c - handle most I2C EEPROMs
> + *
> + * Copyright (C) 2005-2007 David Brownell
> + * Copyright (C) 2008 Wolfram Sang, Pengutronix
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
> +#include <linux/log2.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/i2c/at24.h>
> +
> +/*
> + * I2C EEPROMs from most vendors are inexpensive and mostly interchangeable.
> + * Differences between different vendor product lines (like Atmel AT24C or
> + * MicroChip 24LC, etc) won't much matter for typical read/write access.
> + * There are also I2C RAM chips, likewise interchangeable.  One example
> + * would be the PCF8570, which acts like a 24c02 EEPROM (256 bytes).
> + *
> + * However, misconfiguration can lose data. "Set 16-bit memory address"
> + * to a part with 8-bit addressing will overwrite data.  Writing with too
> + * big a page size also loses data.  And it's not safe to assume that the
> + * conventional addresses 0x50..0x57 only hold eeproms ... a pcf8563 RTC

PCF8563, uppercase.

> + * uses 0x51, for just one example.
> + *
> + * Accordingly, explicit board-specific configuration data should be used
> + * in almost all cases.  (One partial exception is an SMBus used to access
> + * "SPD" data for DRAM sticks.  Those only use 24c02 EEPROMs.)
> + *
> + * So this driver uses "new style" I2C driver binding, expecting to be
> + * told what devices exist.  That may be in arch/X/mach-Y/board-Z.c or
> + * similar kernel-resident tables; or, configuration data coming from
> + * a bootloader.  Be sure to set the board_info "type" to identify the
> + * eeprom type.

i2c_board_info.driver_name is gone, "type" is what is used now for
device/driver matching, so it makes little sense to tell people to make
sure that they set the type... if they don't, the driver simply won't
bind to their device.

> + *
> + * Other than binding model, current differences from "eeprom" driver are
> + * that this one handles write access and isn't restricted to 24c02 devices.
> + * It also handles larger devices (32 kbit and up) with two-byte addresess,

Typo: addresses.

> + * which won't work on pure SMBus systems.
> + */

Another difference is that the eeprom driver caches the data for 5
minutes, while the at24 driver doesn't.

> +
> +struct at24_data {
> +	struct at24_platform_data chip;
> +	bool use_smbus;
> +
> +	/*
> +	 * Lock protects against activities from other Linux tasks,
> +	 * but not from changes by other I2C masters.
> +	 */
> +	struct mutex lock;

You must include <linux/mutex.h>...

> +	struct bin_attribute bin;

... and <linux/sysfs.h>.

> +
> +	u8 *writebuf;
> +	unsigned write_max;
> +
> +	/*
> +	 * Some chips tie up multiple I2C addresses; dummy devices reserve
> +	 * them for ourselves, and we'll use them with SMBus calls.

for us

> +	 */
> +	struct i2c_client *client[];

Isn't this supposed to be

	struct i2c_client *client[0];

? 

> +};
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * This parameter is to help this driver avoid blocking other drivers out
> + * of I2C for potentially troublesome amounts of time.  With a 100 KHz I2C

kHz

> + * clock, one 256 byte read takes about 1/43 second which is excessive;
> + * but the 1/170 second it takes at 400 KHz may be quite reasonable; and

kHz

> + * at 1 MHz (Fm+) a 1/430 second delay could easily be invisible.
> + *
> + * This value is forced to be a power of two so that writes align on pages.
> + */
> +static unsigned io_limit = 128;
> +module_param(io_limit, uint, 0);
> +MODULE_PARM_DESC(io_limit, "maximum bytes per i/o (default 128)");

Maximum, I/O.

> +
> +/*
> + * Specs often allow 5 msec for a page write, sometimes 20 msec;
> + * it's important to recover from write timeouts.
> + */
> +static unsigned write_timeout = 25;
> +module_param(write_timeout, uint, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(write_timeout, "time (in ms) to try writes (default 25)");

Time.

Please insert a blank line here.

> +/*
> + * This routine supports chips which consume multiple I2C addresess.  It

Typo: addresses.

> + * computes the addressing information to be used for a given r/w request.
> + */
> +static struct i2c_client *at24_ee_address(struct at24_data *at24, u16 *addr,
> +		unsigned *offset)
> +{
> +	struct at24_platform_data *chip = &at24->chip;
> +	unsigned i;
> +
> +	if (*offset >= chip->byte_len)
> +		return NULL;
> +
> +	if (chip->flags & AT24_EE_ADDR2) {
> +		i = *offset >> 16;
> +		*offset &= 0xffff;
> +	} else {
> +		i = *offset >> 8;
> +		*offset &= 0xff;
> +	}
> +
> +	if (unlikely(i > chip->i2c_addr_mask)) {
> +		dev_err(&at24->client[0]->dev,
> +			"requested client %u not available!\n", i);
> +		return NULL;
> +	}

That's not just unlikely... that would be a bug in the driver, right?
This could be protected by #ifdef DEBUG, to not slow down the driver
uselessly.

> +
> +	*addr = at24->client[i]->addr;

Strange idea to return addr in a per-address parameter when the caller
can get it from the returned client easily. This would also allow for
some code reordering on the caller side.

> +	return at24->client[i];
> +}
> +
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static ssize_t at24_ee_read(struct at24_data *at24, char *buf, unsigned offset,
> +		size_t count)
> +{
> +	struct i2c_msg msg[2];
> +	u8 addr[2];
> +	struct i2c_client *client;
> +	int status;
> +
> +	memset(msg, 0, sizeof msg);

sizeof(msg)

> +
> +	/*
> +	 * REVISIT some multi-address chips don't rollover page reads to
> +	 * the next slave address, so we may need to truncate the count.
> +	 * Those chips might need another quirk flag...
> +	 *
> +	 * If the real hardware used four adjacent 24c02 chips and that
> +	 * were misconfiged as one 24c08, that would be a similar effect:
> +	 * one "eeprom" file not four, but larger reads would fail when
> +	 * they crossed certain pages.
> +	 */
> +	if (count > io_limit)
> +		count = io_limit;
> +
> +	/*
> +	 * Slave address and byte offset derive from the offset.  Always
> +	 * set the byte address; on a multi-master board, another master
> +	 * may have changed the chip's "current" address pointer.
> +	 */
> +	client = at24_ee_address(at24, &msg[0].addr, &offset);
> +	if (!client)
> +		return -EINVAL;
> +
> +	/* Smaller eproms can work given some SMBus extension calls */
> +	if (at24->use_smbus) {
> +		if (count > 32)
> +			count = 32;

Please use I2C_SMBUS_BLOCK_MAX instead of hard-coding 32.

> +		status = i2c_smbus_read_i2c_block_data(client, offset,
> +				count, buf);
> +		dev_dbg(&client->dev, "smbus read %zd@%d --> %d\n",
> +				count, offset, status);
> +		return (status < 0) ? -EIO : status;
> +	}
> +
> +	/*
> +	 * When we have a better choice than SMBus calls, use a combined
> +	 * I2C message.  Write address; then read up to io_limit data bytes.
> +	 * Note that read page rollover helps us here (unlike writes).
> +	 */
> +	msg[0].buf = addr;
> +	addr[1] = (u8) offset;

Please use a proper mask instead of a cast, it's clearer what you're
doing.

> +	addr[0] = (u8) (offset >> 8);

Cast is not needed.

> +
> +	if (at24->chip.flags & AT24_EE_ADDR2)
> +		msg[0].len = 2;
> +	else {
> +		msg[0].len = 1;
> +		msg[0].buf++;
> +	}
> +
> +	msg[1].addr = client->addr;
> +	msg[1].flags = I2C_M_RD;
> +	msg[1].buf = buf;
> +	msg[1].len = count;
> +
> +	status = i2c_transfer(client->adapter, msg, 2);
> +	dev_dbg(&client->dev, "i2c read %zd@%d --> %d\n",
> +			count, offset, status);
> +
> +	if (status == 2)
> +		return count;
> +	else if (status >= 0)
> +		return -EIO;
> +	else
> +		return status;
> +}
> +
> +static ssize_t at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
> +		char *buf, loff_t off, size_t count)
> +{
> +	struct i2c_client *client;
> +	struct at24_data *at24;
> +	ssize_t retval = 0;
> +
> +	client = to_i2c_client();
> +	at24 = i2c_get_clientdata(client);

If that's the only thing you need client for, you'd rather do:

	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));

> +
> +	if (unlikely(off >= at24->bin.size))
> +		return 0;
> +	if ((off + count) > at24->bin.size)
> +		count = at24->bin.size - off;
> +	if (unlikely(!count))
> +		return count;
> +
> +	/*
> +	 * Read data from chip, protecting against concurrent updates
> +	 * from this host ... but not from other i2c masters.
> +	 */
> +	mutex_lock(&at24->lock);
> +	do {
> +		ssize_t	status;
> +
> +		status = at24_ee_read(at24, buf, off, count);
> +		if (status <= 0) {
> +			if (retval == 0)
> +				retval = status;
> +			break;
> +		}
> +		buf += status;
> +		off += status;
> +		count -= status;
> +		retval += status;
> +
> +	} while (count);
> +	mutex_unlock(&at24->lock);
> +
> +	return retval;
> +}
> +
> +
> +/*
> + * REVISIT:  export at24_bin{read,write}() to let other kernel code use
> + * eeprom data.  For example, it might hold a board's Ethernet address, or
> + * board-specific calibration data generated on the manufacturing floor.
> + */
> +
> +
> +/*
> + * Note that if the hardware write-protect pin is pulled high, the whole
> + * chip is normally write protected.  But there are plenty of product
> + * variants here, including OTP fuses and partial chip protect.
> + *
> + * We only use page mode writes; the alternative is sloooow.  This routine
> + * writes at most one page.
> + */
> +static ssize_t at24_ee_write(struct at24_data *at24, char *buf, loff_t off,
> +		size_t count)
> +{
> +	struct i2c_client *client;
> +	struct i2c_msg  msg;

Doubled space.

> +	unsigned offset = (unsigned) off;

Why don't you simply make the offset parameter an unsigned int, if
that's what you want?

> +	ssize_t status = 0;
> +	unsigned long timeout, write_time;
> +	unsigned c0, c1;
> +
> +	/* Maybe adjust i2c address and offset */
> +	client = at24_ee_address(at24, &msg.addr, &offset);
> +	if (!client)
> +		return -EINVAL;
> +
> +	/* write_max is at most a page */
> +	if (count > at24->write_max)
> +		count = at24->write_max;
> +
> +	/* Never roll over backwards, to the start of this page */
> +	c0 = offset + count;
> +	c1 = roundup(offset + 1, at24->chip.page_size);
> +	if (c0 > c1)
> +		count -= c1 - c0;

What's going on here? If c0 > c1 then c1 - c0 is negative, and you're
making count bigger. Doesn't look right. Maybe you really mean
count = c1 - offset? 

> +
> +	/* If we'll use i2c calls for i/o, set up the message */
> +	if (!at24->use_smbus) {
> +		msg.flags = 0;
> +		msg.len = count + 1;
> +		msg.buf = at24->writebuf;
> +		if (at24->chip.flags & AT24_EE_ADDR2) {
> +			msg.len++;
> +			msg.buf[1] = (u8) offset;
> +			msg.buf[0] = (u8) (offset >> 8);

Same comments as for the read part.

> +			memcpy(&msg.buf[2], buf, count);
> +		} else {
> +			msg.buf[0] = offset;
> +			memcpy(&msg.buf[1], buf, count);
> +		}
> +	}
> +
> +	/*
> +	 * Writes fail if the previous one didn't complete yet.  We'll
> +	 * loop a few times until this one succeeds, waiting at least
> +	 * long enough for one entire page write to work.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(write_timeout);
> +	do {
> +		write_time = jiffies;
> +		if (at24->use_smbus) {
> +			status = i2c_smbus_write_i2c_block_data(client,
> +					offset, count, buf);
> +			if (status == 0)
> +				status = count;
> +		} else {
> +			status = i2c_transfer(client->adapter, &msg, 1);
> +			if (status == 1)
> +				status = count;
> +		}
> +		dev_dbg(&client->dev, "write %zd@%d --> %zd (%ld)\n",
> +				count, offset, status, jiffies);
> +
> +		if (status == count)
> +			return count;
> +
> +		/* REVISIT:  at HZ=100, this is sloooow */
> +		msleep(1);
> +	} while (time_before(write_time, timeout));
> +
> +	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
> +			count, offset, jiffies
> +			- (timeout - msecs_to_jiffies(write_timeout)));

I'm not sure what value there is to print the number of ticks. We
already know that it'll be the next possible value after write_timeout.

> +	return -ETIMEDOUT;
> +}
> +
> +static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
> +		char *buf, loff_t off, size_t count)
> +{
> +	struct i2c_client *client;
> +	struct at24_data *at24;
> +	ssize_t retval = 0;
> +
> +	client = to_i2c_client(container_of(kobj, struct device, kobj));
> +	at24 = i2c_get_clientdata(client);

Same comment at for read.

> +
> +	if (unlikely(off >= at24->bin.size))
> +		return -EFBIG;

Doesn't seem appropriate. The file isn't too big... If anything it's
too _small_ for the request. I'd return -EINVAL.

> +	if ((off + count) > at24->bin.size)
> +		count = at24->bin.size - off;
> +	if (unlikely(!count))
> +		return count;
> +
> +	/*
> +	 * Write data from chip, protecting against concurrent updates

Write data _to_ chip.

> +	 * from this host ... but not from other i2c masters.
> +	 */
> +	mutex_lock(&at24->lock);
> +
> +	/* buffer big enough to stick the address at the beginning */
> +	at24->writebuf = kmalloc(at24->write_max + 2, GFP_KERNEL);
> +	if (!at24->writebuf) {
> +		retval = -ENOMEM;
> +		count = 0;
> +	}

You could move this to before taking the mutex (using a temporary
pointer), so that you can return with an error immediately if
allocation fails. But more importantly: do you really need to allocate
and free a new buffer for each write? You serialize the writes, and the
size of the buffer doesn't depend on the actual write operation, so you
might as well allocate the buffer as part of at24_probe(), this will
make write operations faster and will avoid useless memory
fragmentation.

If you are really worried about the memory waste in case the user
doesn't actually write to the EEPROM, you could alternatively allocate
the buffer on the first write, and free it in the .remove() method.

> +
> +	while (count) {
> +		ssize_t	status;
> +
> +		status = at24_ee_write(at24, buf, off, count);
> +		if (status <= 0) {
> +			if (retval == 0)
> +				retval = status;
> +			break;
> +		}
> +		buf += status;
> +		off += status;
> +		count -= status;
> +		retval += status;
> +	}

Minor inconsistency: in the read case you use do {} while (), and here
you use while() { }.

> +
> +	kfree(at24->writebuf);
> +	at24->writebuf = NULL;
> +	mutex_unlock(&at24->lock);
> +
> +	return retval;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct at24_platform_data *chip;
> +	bool writable;
> +	bool use_smbus = false;
> +	struct at24_data *at24;
> +	int err;
> +	unsigned i, var_size;
> +	struct i2c_client *c;
> +
> +	chip = client->dev.platform_data;
> +	if (!chip) {
> +		err = -ENODEV;
> +		goto fail;
> +	}
> +	if (!is_power_of_2(chip->byte_len))
> +		dev_warn(&client->dev,
> +			"byte_len looks suspicious (no power of 2)!\n");
> +	if (!is_power_of_2(chip->page_size))
> +		dev_warn(&client->dev,
> +			"page_size looks suspicious (no power of 2)!\n");
> +	if (!is_power_of_2(chip->i2c_addr_mask + 1))
> +		dev_warn(&client->dev,
> +			"i2c_addr_mask looks suspicious (unusual mask)!\n");

"unusual mask" doesn't add any information compared to "looks
suspicious", does it?

> +
> +	/* Use I2C operations unless we're stuck with SMBus extensions. */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		if (chip->flags & AT24_EE_ADDR2) {
> +			err = -ECOMM;
> +			goto fail;
> +		}
> +		if (!i2c_check_functionality(client->adapter,
> +				I2C_FUNC_SMBUS_I2C_BLOCK)) {
> +			err = -EBADMSG;
> +			goto fail;
> +		}

These are definitely not the correct error values. I also see no
reason to return different error values for the two cases as the
problem is exactly the same: underlying adapter doesn't support the
transaction types needed to talk to the EEPROM. What about -EOPNOTSUPP?
Or -ENODEV.

> +		use_smbus = true;
> +	}
> +
> +	var_size = (chip->i2c_addr_mask + 1) * sizeof(struct i2c_client *);
> +	at24 = kzalloc(sizeof(struct at24_data) + var_size, GFP_KERNEL);
> +	if (!at24) {
> +		err = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	mutex_init(&at24->lock);
> +	at24->chip = *chip;
> +	at24->use_smbus = use_smbus;
> +
> +	/*
> +	 * Export the EEPROM bytes through sysfs, since that's convenient.
> +	 * By default, only root should see the data (maybe passwords etc)
> +	 */
> +	at24->bin.attr.name = "eeprom";
> +	at24->bin.attr.mode = S_IRUSR;

This makes the data only readable by root, as the comment says. The
eeprom driver makes (most of) the data world-readable. I think it would
be good to at least have the option to make the data world-readable,
for example for SPD or EDID data. Otherwise we will never be able to
drop the eeprom driver.

> +	at24->bin.attr.owner = THIS_MODULE;
> +	at24->bin.read = at24_bin_read;
> +
> +	at24->bin.size = at24->chip.byte_len;
> +
> +	writable = ((chip->flags & AT24_EE_READONLY) == 0);

More usually written !(var & FLAG).

> +	if (writable) {
> +		unsigned write_max = at24->chip.page_size;
> +
> +		at24->bin.write = at24_bin_write;
> +		at24->bin.attr.mode |= S_IWUSR;
> +
> +		if (write_max > io_limit)
> +			write_max = io_limit;
> +		if (use_smbus && write_max > 32)
> +			write_max = 32;

Here again please use I2C_SMBUS_BLOCK_MAX.

> +		at24->write_max = write_max;
> +	}
> +
> +	/*
> +	 * REVISIT read a byte, to make sure the chip is actually
> +	 * present (vs misconfiguration, or not-populated-here)
> +	 */

As discussed in another thread recently, if there is ever a need to
check whether a device is present or not, that would rather be
i2c-core's job, and that would be done only on explicit request by the
platform code.

> +
> +	at24->client[0] = client;
> +	i2c_set_clientdata(client, at24);
> +
> +	/* use dummy devices for multiple-address chips */
> +	if (chip->i2c_addr_mask) {
> +		for (i = 1; i <= chip->i2c_addr_mask; i++) {
> +			c = i2c_new_dummy(client->adapter, client->addr + i);
> +			if (!c) {
> +				dev_err(&client->dev, "addr %d unavail\n",

Please spell words in error messages completely. Address should be
printed in hexadecimal, that's what developers and users are used to.

> +						client->addr + i);
> +				err = -ENOCSI;

Again a funky error value, completely unrelated with the error that
occurred. -EBUSY?

> +				goto cleanup;
> +			}
> +			at24->client[i] = c;

As a side note, I don't get the point of using a temporary variable "c"
here. It makes the code more complex with no benefit.

> +		}
> +	}
> +
> +	err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
> +	if (err)
> +		goto cleanup;
> +
> +	dev_info(&client->dev, "%Zd byte %s EEPROM%s\n",
> +		at24->bin.size, client->name,
> +		writable ? " (writable)" : "");

What about explicitly saying " (read-only)" for read-only EEPROMs?

> +	dev_dbg(&client->dev,
> +		"page_size %d, i2c_addr_mask %d, write_max %d%s\n",
> +		at24->chip.page_size, at24->chip.i2c_addr_mask,
> +		at24->write_max,
> +		use_smbus ? ", use_smbus" : "");
> +	return 0;
> +
> +cleanup:
> +	if (chip->i2c_addr_mask) {

Note that this test is not needed: the for loop below won't do anything
if i2c_addr_mask == 0.

> +		for (i = 1; i <= chip->i2c_addr_mask; i++) {
> +			c = at24->client[i];
> +			if (c)
> +				i2c_unregister_device(c);

Again, using "c" has little value IMHO.

> +		}
> +	}
> +
> +	kfree(at24);
> +fail:
> +	dev_dbg(&client->dev, "probe err %d\n", err);
> +	return err;
> +}
> +
> +static int __devexit at24_remove(struct i2c_client *client)
> +{
> +	struct at24_data *at24;
> +	struct i2c_client *c;
> +	int i;
> +
> +	at24 = i2c_get_clientdata(client);
> +	sysfs_remove_bin_file(&client->dev.kobj, &at24->bin);
> +
> +	if (at24->chip.i2c_addr_mask) {
> +		for (i = 1; i <= at24->chip.i2c_addr_mask; i++) {
> +			c = at24->client[i];
> +			if (c)
> +				i2c_unregister_device(c);
> +		}
> +	}

The same comments I made to the cleanup part of .probe(), apply here.

> +
> +	kfree(at24);

Please restore the client data to NULL.

> +	return 0;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static const struct i2c_device_id at24_ids[] = {
> +	{ "at24", 0, },

No comma after 0.

> +	{ /* END OF LIST */ }
> +};
> +
> +static struct i2c_driver at24_driver = {
> +	.driver = {
> +		.name = "at24",
> +		.owner = THIS_MODULE,
> +	},
> +	.probe = at24_probe,
> +	.remove = __devexit_p(at24_remove),
> +	.id_table = at24_ids,
> +};
> +
> +static int __init at24_init(void)
> +{
> +	io_limit = rounddown_pow_of_two(io_limit);
> +	return i2c_add_driver(&at24_driver);
> +}
> +module_init(at24_init);
> +
> +static void __exit at24_exit(void)
> +{
> +	i2c_del_driver(&at24_driver);
> +}
> +module_exit(at24_exit);
> +
> +MODULE_DESCRIPTION("Driver for most I2C EEPROMs");
> +MODULE_AUTHOR("David Brownell");
> +MODULE_LICENSE("GPL");
> +

Extra blank line at end of file.

> diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
> new file mode 100644
> index 0000000..74635ee
> --- /dev/null
> +++ b/include/linux/i2c/at24.h
> @@ -0,0 +1,102 @@
> +
> +#ifndef _LINUX_AT24_H
> +#define _LINUX_AT24_H
> +
> +#include <linux/types.h>
> +
> +/*
> + * As seen through Linux I2C, differences between the most common types
> + * of I2C memory include:
> + *	- How many I2C addresses the chip consumes: 1, 2, 4...?
> + *	- Memory address space for one I2C address:  256 byte, or 64 KB?

kB

> + *	- How full that memory space is:  16 bytes, 256, 32Kb, etc?

kb (or do you mean kB?)

> + *	- What write page size does it support?
> + */
> +
> +struct at24_platform_data {
> +	u32		byte_len;		/* size (sum of all addr) */
> +	u16		page_size;		/* for writes */
> +	u8		i2c_addr_mask;		/* for multi-addr chips */

This notion of i2c_addr_mask seems more restrictive and easy to get
wrong than it needs to be. What you really have is a number of slave
I2C addresses, that's more intuitive IMHO, and using this would save a
couple "+ 1" in the code. As a matter of fact, that's what you
described in the comment above.

Oh, BTW, can't you compute this value yourself from byte_len and (flags
& AT24_EE_ADDR2)? I think so...

> +	u8		flags;			/* quirks, etc */
> +#define	AT24_EE_ADDR2		0x0080		/* 16 bit addrs; <= 64 KB */

What does the "<= 64 KB" means?

> +#define	AT24_EE_READONLY	0x0040

The flags field is a u8 but you write the flag values as if they were
meant to go in a 16-bit field. Confusing.

> +};
> +
> +/*
> + * If you use this macro to set up a custom eeprom type, please make sure you
> + * have double checked the parameters. Especially page_size needs extra care,
> + * as you risk data loss if your value is bigger than what the chip actually
> + * supports!
> + */
> +#define AT24_PLATFORM_DATA(_name, _len, _page, _mask, _flags) \
> +struct at24_platform_data at24_platform_data_##_name = {	\
> +	.byte_len	= _len,		\
> +	.page_size	= _page,	\
> +	.i2c_addr_mask	= _mask,	\
> +	.flags		= _flags,	\
> +};
> +
> +/* 128 bit chip, I2C A0-A2 ignored */
> +#define AT24_PLATFORM_DATA_24C00 \
> +	AT24_PLATFORM_DATA(24c00, 128 / 8, 1, 0x07, 0)
> +
> +/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
> +#define AT24_PLATFORM_DATA_24C01 \
> +	AT24_PLATFORM_DATA(24c01, 1024 / 8, 8, 0, 0)
> +
> +/* 2 Kbit chip, some have 16 byte pages: */
> +#define AT24_PLATFORM_DATA_24C02 \
> +	AT24_PLATFORM_DATA(24c02, 2048 / 8, 8, 0, 0)
> +
> +/* 2 Kbit chip, 24c02 in memory DIMMs, some have 16 byte pages */
> +#define AT24_PLATFORM_DATA_SPD \
> +	AT24_PLATFORM_DATA(spd, 2048 / 8, 8, 0, AT24_EE_READONLY)
> +
> +/* 2 Kbit chip, SRAM, not EEPROM!, no page size issues, write it all at once */
> +#define AT24_PLATFORM_DATA_PCF8570 \
> +	AT24_PLATFORM_DATA(pcf8570, 2048 / 8, 2048 / 8, 0, 0)
> +
> +/* 4 Kbit chip, I2C A0 is MEM A8 */
> +#define AT24_PLATFORM_DATA_24C04 \
> +	AT24_PLATFORM_DATA(24c04, 4096 / 8, 16, 0x01, 0)
> +
> +/*
> + * 8 Kbit chip, I2C A1-A0 is MEM A9-A8, works also with 24RF08
> + * (its quirk is handled at i2c-core-level)
> + */
> +#define AT24_PLATFORM_DATA_24C08 \
> +	AT24_PLATFORM_DATA(24c08, 8192 / 8, 16, 0x03, 0)
> +
> +/* 16 Kbit chip, I2C A2-A0 is MEM A10-A8 */
> +#define AT24_PLATFORM_DATA_24C16 \
> +	AT24_PLATFORM_DATA(24c16, 16384 / 8, 16, 0x07, 0)
> +
> +
> +/* this second block of EEPROMS uses 16 bit memory addressing */
> +
> +/* 32 Kbits */
> +#define AT24_PLATFORM_DATA_24C32 \
> +	AT24_PLATFORM_DATA(24c32, 32768 / 8, 32, 0, AT24_EE_ADDR2)
> +
> +/* 64 Kbits */
> +#define AT24_PLATFORM_DATA_24C64 \
> +	AT24_PLATFORM_DATA(24c64, 65536 / 8, 32, 0, AT24_EE_ADDR2)
> +
> +/* 128 Kbits */
> +#define AT24_PLATFORM_DATA_24C128 \
> +	AT24_PLATFORM_DATA(24c128, 131072 / 8, 64, 0, AT24_EE_ADDR2)
> +
> +/* 256 Kbits */
> +#define AT24_PLATFORM_DATA_24C256 \
> +	AT24_PLATFORM_DATA(24c256, 262144 / 8, 64, 0, AT24_EE_ADDR2)
> +
> +/* 512 Kbits */
> +#define AT24_PLATFORM_DATA_24C512 \
> +	AT24_PLATFORM_DATA(24c512, 524288 / 8, 128, 0, AT24_EE_ADDR2)
> +
> +/* 1 Mbits, I2C A0 is MEM A16 */
> +#define AT24_PLATFORM_DATA_24C1024 \
> +	AT24_PLATFORM_DATA(24c1024, 1048576 / 8, 256, 0x01, AT24_EE_ADDR2)

As said earlier: IMHO this is calling for separate I2C device
types/names.

> +
> +#endif /* _LINUX_AT24_H */
> +

Extra line at end of file.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Thursday 22 May 2008, Jean Delvare wrote:
> Hi Wolfram, hi David,
> 
> On Thu, 15 May 2008 22:36:39 +0200, Wolfram Sang wrote:
> > Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
> > access to their data. Tested with various chips and clock rates.
> 
> Preliminary note: I'm curious if sysfs is the right interface for
> writable EEPROMs. Isn't /dev/mtd supposed to be used for this purpose?

It's never been used for that before.  Consider also all the
various /sys/class/rtc/rtc0/device/nvram files, as well as
what the current "eeprom" driver does.  Being writable isn't
a good reason to adopt a new naming convention.  And having
char device nodes is both needless and confusing ... there'd
still need to be a way to map the device back to the hardware,
which comes for free with sysfs.

Plus, it doesn't support the MTD model ... which, as someone
pointed out, is more attuned to filesystems (lots of data,
vs a relative handful of bytes).


> > ---
> > 
> > As we now support device_ids, probably most of the chip data will come back
> > into the driver. This is yet to be done. Tested on two platforms by me and
> > another one by David. Builds also on x86.
> 
> We have to make a decision about the strategy now, and stick to it.
> Changing it after the driver is upstream will cause a lot of confusion.
> I think that having predefined names for the most common EEPROM sizes
> would be a good idea. You can also keep a generic "at24" type where all
> the parameters have to be provided by platform code.

That's what I was leaning towards.  It should include a chip name,
so the "hai i found ur chip" message can be properly informative.


> > +	  				 Also, if your chip
> > +	  has any software write-protect mechanism you may want to make
> > +	  sure this driver won't turn it on by accident.
> 
> How?

Code review.  "... you may want to review this code to make sure ..."


> > + * Other than binding model, current differences from "eeprom" driver are
> > + * that this one handles write access and isn't restricted to 24c02 devices.
> > + * It also handles larger devices (32 kbit and up) with two-byte addresess,
> 
> Typo: addresses.
> 
> > + * which won't work on pure SMBus systems.
> > + */
> 
> Another difference is that the eeprom driver caches the data for 5
> minutes, while the at24 driver doesn't.

Which IMO is kind of a waste of energy ... and I think you didn't
say it right.  It caches the data forever, but refreshes it if it's
older than 5 minutes.  If you're going to talk about performance,
it's probably worth just saying that "at24" will usually be faster
since it uses block I/O primitives.


> > +	struct i2c_client *client[];
> 
> Isn't this supposed to be
> 
> 	struct i2c_client *client[0];
> 
> ?

ISTR client[0] is the GNU-ism, while ANSI C allows the last
element of a struct to be an unsized array.


> > +	if (unlikely(i > chip->i2c_addr_mask)) {
> > +		dev_err(&at24->client[0]->dev,
> > +			"requested client %u not available!\n", i);
> > +		return NULL;
> > +	}
> 
> That's not just unlikely... that would be a bug in the driver, right?
> This could be protected by #ifdef DEBUG, to not slow down the driver
> uselessly.

Right ...


> > +
> > +	*addr = at24->client[i]->addr;
> 
> Strange idea to return addr in a per-address parameter when the caller
> can get it from the returned client easily. This would also allow for
> some code reordering on the caller side.

That's probably a bit of a historical artifact.


> > +	client = to_i2c_client();

Needs a parameter ... ;)


> > +	at24 = i2c_get_clientdata(client);
> 
> If that's the only thing you need client for, you'd rather do:
> 
> 	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));

Better:  at24 = i2c_get_clientdata(to_i2c_client(kobj));

GCC shouldn't care, and it's a lot more clear when that line
can be tied to other (i2c-specific) code elsewhere.

 
> > +static ssize_t at24_ee_write(struct at24_data *at24, char *buf, loff_t off,
> > +		size_t count)
> > +{
> > +	struct i2c_client *client;
> > +	struct i2c_msg  msg;
> 
> Doubled space.
> 
> > +	unsigned offset = (unsigned) off;
> 
> Why don't you simply make the offset parameter an unsigned int, if
> that's what you want?

The sysfs API says it's "loff_t"; can't change that.

> > +	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
> > +			count, offset, jiffies
> > +			- (timeout - msecs_to_jiffies(write_timeout)));
> 
> I'm not sure what value there is to print the number of ticks. We
> already know that it'll be the next possible value after write_timeout.

For debugging it was nice to know just how long it waited.


> > +	/*
> > +	 * Export the EEPROM bytes through sysfs, since that's convenient.
> > +	 * By default, only root should see the data (maybe passwords etc)
> > +	 */
> > +	at24->bin.attr.name = "eeprom";
> > +	at24->bin.attr.mode = S_IRUSR;
> 
> This makes the data only readable by root, as the comment says. The
> eeprom driver makes (most of) the data world-readable. I think it would
> be good to at least have the option to make the data world-readable,
> for example for SPD or EDID data. Otherwise we will never be able to
> drop the eeprom driver.

There are a few flag bits available for that purpose.  ;)

And for the SPD case, it's easy to set that (along with "no write").


> > +				err = -ENOCSI;
> 
> Again a funky error value, completely unrelated with the error that
> occurred. -EBUSY?

For the record, not only are all those error code assignments
ancient (predating recent discussion), but the goal was to have
a unique code for each fault path (as I recall).


> > +	dev_info(&client->dev, "%Zd byte %s EEPROM%s\n",
> > +		at24->bin.size, client->name,
> > +		writable ? " (writable)" : "");
> 
> What about explicitly saying " (read-only)" for read-only EEPROMs?

Instead of "(writable)"?  Probably makes sense; the bias here
is to provide full access, and messages should call out what's
uncommon, not what's common.


> > +#define	AT24_EE_ADDR2		0x0080		/* 16 bit addrs; <= 64 KB */
> 
> What does the "<= 64 KB" means?

Just makes the consequence of 16 bit addressing be obvious:
no more than 64 KibiBytes of data.



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi David,

On Thu, 22 May 2008 14:35:36 -0700, David Brownell wrote:
> On Thursday 22 May 2008, Jean Delvare wrote:
> > Hi Wolfram, hi David,
> > 
> > On Thu, 15 May 2008 22:36:39 +0200, Wolfram Sang wrote:
> > > Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
> > > access to their data. Tested with various chips and clock rates.
> > 
> > Preliminary note: I'm curious if sysfs is the right interface for
> > writable EEPROMs. Isn't /dev/mtd supposed to be used for this purpose?
> 
> It's never been used for that before.  Consider also all the
> various /sys/class/rtc/rtc0/device/nvram files,

Isn't there /dev/nvram too?

> as well as what the current "eeprom" driver does.

Whatever the "eeprom" driver does is not necessarily meaningful. It
uses sysfs because it was considered a "sensor" driver for a long time
and hardware monitoring drivers use sysfs.

>                                         Being writable isn't
> a good reason to adopt a new naming convention.  And having
> char device nodes is both needless and confusing ... there'd
> still need to be a way to map the device back to the hardware,
> which comes for free with sysfs.
> 
> Plus, it doesn't support the MTD model ... which, as someone
> pointed out, is more attuned to filesystems (lots of data,
> vs a relative handful of bytes).

I'm not insisting... I just seemed to remember that binary files
in /sysfs were only tolerated and should be avoided when possible. I
don't remember where I read that though.

> > Another difference is that the eeprom driver caches the data for 5
> > minutes, while the at24 driver doesn't.
> 
> Which IMO is kind of a waste of energy ... and I think you didn't
> say it right.  It caches the data forever, but refreshes it if it's
> older than 5 minutes.

What difference does that make?

>                       If you're going to talk about performance,
> it's probably worth just saying that "at24" will usually be faster
> since it uses block I/O primitives.

The slowness of the eeprom driver is exactly the reason why we made it
cache the data. Your at24 driver is faster, but it will fail to work on
most SMBus adapters (I2C block transaction support is a rare feature.)
If we ever want to get rid of the eeprom driver, SMBus-compatible
access will have to be added to the at24 driver, and with it I suspect
that we will want to get the caching feature, unless the situation has
improved a lot meanwhile.

> > > +	client = to_i2c_client();
> 
> Needs a parameter ... ;)

I accidentally broke the quote while replying to it, my bad.

> > > +	at24 = i2c_get_clientdata(client);
> > 
> > If that's the only thing you need client for, you'd rather do:
> > 
> > 	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> 
> Better:  at24 = i2c_get_clientdata(to_i2c_client(kobj));
> 
> GCC shouldn't care, and it's a lot more clear when that line
> can be tied to other (i2c-specific) code elsewhere.

Err, please, no. That would be a fragile hack.

> > > +static ssize_t at24_ee_write(struct at24_data *at24, char *buf, loff_t off,
> > > +		size_t count)
> > > +{
> > > +	struct i2c_client *client;
> > > +	struct i2c_msg  msg;
> > 
> > Doubled space.
> > 
> > > +	unsigned offset = (unsigned) off;
> > 
> > Why don't you simply make the offset parameter an unsigned int, if
> > that's what you want?
> 
> The sysfs API says it's "loff_t"; can't change that.

at24_ee_write() is a helper function, not a sysfs callback function, so
we are free to use whatever parameter types we want. As a matter of
fact, at24_ee_read() has its offset passed as an unsigned.

> > > +	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
> > > +			count, offset, jiffies
> > > +			- (timeout - msecs_to_jiffies(write_timeout)));
> > 
> > I'm not sure what value there is to print the number of ticks. We
> > already know that it'll be the next possible value after write_timeout.
> 
> For debugging it was nice to know just how long it waited.

So it can go away now? Or be moved to a dev_dbg().

> > > +	dev_info(&client->dev, "%Zd byte %s EEPROM%s\n",
> > > +		at24->bin.size, client->name,
> > > +		writable ? " (writable)" : "");
> > 
> > What about explicitly saying " (read-only)" for read-only EEPROMs?
> 
> Instead of "(writable)"?  Probably makes sense; the bias here
> is to provide full access, and messages should call out what's
> uncommon, not what's common.

I was thinking: in addition to "(writable)". You have writable EEPROMs
in mind but users may not, so I think it's better to just write what we
have explicitly. It's not that expensive.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

One thing I forgot in my initial review:

On Thu, 15 May 2008 22:36:39 +0200, Wolfram Sang wrote:
> +	/* Use I2C operations unless we're stuck with SMBus extensions. */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		if (chip->flags & AT24_EE_ADDR2) {
> +			err = -ECOMM;
> +			goto fail;
> +		}
> +		if (!i2c_check_functionality(client->adapter,
> +				I2C_FUNC_SMBUS_I2C_BLOCK)) {
> +			err = -EBADMSG;
> +			goto fail;
> +		}
> +		use_smbus = true;
> +	}

For a read-only EEPROM, you only need I2C_FUNC_SMBUS_READ_I2C_BLOCK,
not I2C_FUNC_SMBUS_I2C_BLOCK. It would matter if the underlying adapter
supports I2C block read but not I2C block write.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Friday 23 May 2008, Jean Delvare wrote:
> > > Preliminary note: I'm curious if sysfs is the right interface for
> > > writable EEPROMs. Isn't /dev/mtd supposed to be used for this purpose?
> > 
> > It's never been used for that before.  Consider also all the
> > various /sys/class/rtc/rtc0/device/nvram files,
> 
> Isn't there /dev/nvram too?

There is such a legacy driver; I don't believe it's much
used.  The ATARI bits, for example.  Seems to me the main
point of that driver is the /proc/driver/nvram file, which
parses some of the better understood fields.

Note that the "nvram" contents are extremely specific to
the platform ... and that on non-PC hardware there might
be more than one device with NVRAM.  The /dev/nvram stuff
does not allow multiple NVRAM files.


> > as well as what the current "eeprom" driver does.
> 
> Whatever the "eeprom" driver does is not necessarily meaningful. It
> uses sysfs because it was considered a "sensor" driver for a long time
> and hardware monitoring drivers use sysfs.

The existing model for EEPROMs *should* be meaningful!
If for no other reason than not needlessly exposing a
different interface to the same SPD EEPROM.

It wouldn't necessarily be determinative, of course.
What might be determinative would be a compelling reason
that a binary sysfs file is the wrong model.  And there
don't seem to be any such reasons.


> > Plus, it doesn't support the MTD model ... which, as someone
> > pointed out, is more attuned to filesystems (lots of data,
> > vs a relative handful of bytes).
>
> I'm not insisting... I just seemed to remember that binary files
> in /sysfs were only tolerated and should be avoided when possible. I
> don't remember where I read that though.

It would be the wrong model for e.g. exposing a register bank;
that's where I recall specific pushback.


> > > Another difference is that the eeprom driver caches the data for 5
> > > minutes, while the at24 driver doesn't.
> > 
> > Which IMO is kind of a waste of energy ... and I think you didn't
> > say it right.  It caches the data forever, but refreshes it if it's
> > older than 5 minutes.
> 
> What difference does that make?

Memory consumption.  When it's only the 256 bytes of the old
EEPROM driver, that may not matter.  But when you're talking
about needlessly tying down several pages (e.g. 64 KB for the
24c512 EEPROMs found on some of my ARM boards) virtually
forever ... the fact that there's no cache purging (or any
intelligence about how much to cache) can be a big deal.


> >                       If you're going to talk about performance,
> > it's probably worth just saying that "at24" will usually be faster
> > since it uses block I/O primitives.
> 
> The slowness of the eeprom driver is exactly the reason why we made it
> cache the data.

How much does that matter though, for SMBus systems where the
only reason to read the SPD data is to see what kind of memory
is available?  (In more detail than the DMI tables expose.)

It'd be rare to need that data more than a few times, ever.
(Ergo "waste of energy".)

Plus, caching presumes the system isn't multi-master, where
another master could change the data (without any way to
invalidate such a local cache).  SMBus restricts itself to
a single master, but I2C doesn't.


> Your at24 driver is faster, but it will fail to work on 
> most SMBus adapters (I2C block transaction support is a rare feature.)

Well, a quick check shows that the i801 SMBus support on most
recent motherboards with Intel chipsets can do it.  So it's
hardly "rare"; more accurate would be "common".  (I suspect
some of the other SMBus drivers could do those transactions,
but their drivers don't yet know how.)

But it's true that it's not universal.  Also true that it's
very fixable ... and that using "word" (16 bits not 32/64/...)
would be an easy 100+% performance boost over byte-at-a-time
reads/writes!


> > > > +	unsigned offset = (unsigned) off;
> > > 
> > > Why don't you simply make the offset parameter an unsigned int, if
> > > that's what you want?
> > 
> > The sysfs API says it's "loff_t"; can't change that.
> 
> at24_ee_write() is a helper function, not a sysfs callback function, so
> we are free to use whatever parameter types we want. As a matter of
> fact, at24_ee_read() has its offset passed as an unsigned.

Forgot about that.  OK, changing that to "unsigned" would be fine.


> > > > +	dev_err(&client->dev, "write %zd@%d, timeout %ld ticks\n",
> > > > +			count, offset, jiffies
> > > > +			- (timeout - msecs_to_jiffies(write_timeout)));
> > > 
> > > I'm not sure what value there is to print the number of ticks. We
> > > already know that it'll be the next possible value after write_timeout.
> > 
> > For debugging it was nice to know just how long it waited.
> 
> So it can go away now? Or be moved to a dev_dbg().

Sure.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 2612 bytes --]

Hi Jean,

thanks for your review! Just a few comments from me, otherwise I'll
change the things as suggested by you or David.

> > As we now support device_ids, probably most of the chip data will
> > come back into the driver. This is yet to be done. Tested on two
> > platforms by me and another one by David. Builds also on x86.
> We have to make a decision about the strategy now, and stick to it.
> Changing it after the driver is upstream will cause a lot of
> confusion.
ACK. I was thinking about the following way to support the standard
eeprom sizes without bloating the driver too much with chip data: In a
MODULE_DEVICE_TABLE, we have a kernel_ulong_t available for each entry
of a supported device. All parameters of the standard eeprom chips are
powers of 2. If we use log2 of these parameters, they will easily fit
into a kernel_ulong_t. We'd need one macro to create these magic numbers
from the provided chip data (AT24_DATA_*) and just a tiny bit of code to
create a proper platform_data struct from it.
A general "at24" entry will expect the platform data to be provided, of
course.

> > +	struct i2c_client *client[];
> Isn't this supposed to be
> 	struct i2c_client *client[0];
IIRC I found both versions in kernel code and chose this because of
ANSI-C.

> > +	at24->bin.attr.name = "eeprom";
> > +	at24->bin.attr.mode = S_IRUSR;
> This makes the data only readable by root, as the comment says. The
> eeprom driver makes (most of) the data world-readable. I think it would
> be good to at least have the option to make the data world-readable,
> for example for SPD or EDID data. Otherwise we will never be able to
> drop the eeprom driver.
I'd go for David's solution here to introduce another flag for this
purpose.

> > +	u8		i2c_addr_mask;		/* for multi-addr chips */
> This notion of i2c_addr_mask seems more restrictive and easy to get
> wrong than it needs to be. What you really have is a number of slave
> I2C addresses, that's more intuitive IMHO, and using this would save a
> couple "+ 1" in the code. As a matter of fact, that's what you
> described in the comment above.
> Oh, BTW, can't you compute this value yourself from byte_len and (flags
> & AT24_EE_ADDR2)? I think so...
I was about to address this issue in the next version. Still need to
think about side-effects and corner-cases in a quiet minute.

I guess some more comments will arise while updating the driver...

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi Wolfram,

On Fri, 30 May 2008 11:15:34 +0200, Wolfram Sang wrote:
> thanks for your review! Just a few comments from me, otherwise I'll
> change the things as suggested by you or David.
> 
> > > As we now support device_ids, probably most of the chip data will
> > > come back into the driver. This is yet to be done. Tested on two
> > > platforms by me and another one by David. Builds also on x86.
> > We have to make a decision about the strategy now, and stick to it.
> > Changing it after the driver is upstream will cause a lot of
> > confusion.
> ACK. I was thinking about the following way to support the standard
> eeprom sizes without bloating the driver too much with chip data: In a
> MODULE_DEVICE_TABLE, we have a kernel_ulong_t available for each entry
> of a supported device. All parameters of the standard eeprom chips are
> powers of 2. If we use log2 of these parameters, they will easily fit
> into a kernel_ulong_t. We'd need one macro to create these magic numbers
> from the provided chip data (AT24_DATA_*) and just a tiny bit of code to
> create a proper platform_data struct from it.
> A general "at24" entry will expect the platform data to be provided, of
> course.

That's fine with me.

> > > +	at24->bin.attr.name = "eeprom";
> > > +	at24->bin.attr.mode = S_IRUSR;
> > This makes the data only readable by root, as the comment says. The
> > eeprom driver makes (most of) the data world-readable. I think it would
> > be good to at least have the option to make the data world-readable,
> > for example for SPD or EDID data. Otherwise we will never be able to
> > drop the eeprom driver.
> I'd go for David's solution here to introduce another flag for this
> purpose.

OK.

> > > +	u8		i2c_addr_mask;		/* for multi-addr chips */
> > This notion of i2c_addr_mask seems more restrictive and easy to get
> > wrong than it needs to be. What you really have is a number of slave
> > I2C addresses, that's more intuitive IMHO, and using this would save a
> > couple "+ 1" in the code. As a matter of fact, that's what you
> > described in the comment above.
> > Oh, BTW, can't you compute this value yourself from byte_len and (flags
> > & AT24_EE_ADDR2)? I think so...
> I was about to address this issue in the next version. Still need to
> think about side-effects and corner-cases in a quiet minute.

I'd rather see this addressed now rather than later, as struct
at24_platform_data is public, changing it later would be non-trivial.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 645 bytes --]

Hi Jean,

On Fri, May 30, 2008 at 11:51:31AM +0200, Jean Delvare wrote:
> > I was about to address this issue in the next version. Still need to
> > think about side-effects and corner-cases in a quiet minute.
> I'd rather see this addressed now rather than later, as struct
> at24_platform_data is public, changing it later would be non-trivial.
I agree. With "next version" I did not mean "next version after it went
upstream" but "next revision after the version I sent lately". I am
already on it!

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 6047 bytes --]

Hi Jean,

some more comments while working on the driver:

> > +static struct i2c_client *at24_ee_address(struct at24_data *at24, u16 *addr,
> > +		unsigned *offset)
> > +{
> > +	struct at24_platform_data *chip = &at24->chip;
> > +	unsigned i;
> > +
> > +	if (*offset >= chip->byte_len)
> > +		return NULL;
> > +
> > +	if (chip->flags & AT24_EE_ADDR2) {
> > +		i = *offset >> 16;
> > +		*offset &= 0xffff;
> > +	} else {
> > +		i = *offset >> 8;
> > +		*offset &= 0xff;
> > +	}
> > +
> > +	if (unlikely(i > chip->i2c_addr_mask)) {
> > +		dev_err(&at24->client[0]->dev,
> > +			"requested client %u not available!\n", i);
> > +		return NULL;
> > +	}
> 
> That's not just unlikely... that would be a bug in the driver, right?
> This could be protected by #ifdef DEBUG, to not slow down the driver
> uselessly.
This error case can happen, if the supplied 'byte_len' and
'i2c_addr_mask' don't fit. I will put this check to the probe-function,
this should also do. Calculating i2c_addr_mask from byte_len still gives
me headaches, although I'd also like this to happen (see later).

> > +	/*
> > +	 * When we have a better choice than SMBus calls, use a combined
> > +	 * I2C message.  Write address; then read up to io_limit data bytes.
> > +	 * Note that read page rollover helps us here (unlike writes).
> > +	 */
> > +	msg[0].buf = addr;
> > +	addr[1] = (u8) offset;
> 
> Please use a proper mask instead of a cast, it's clearer what you're
> doing.
> 
> > +	addr[0] = (u8) (offset >> 8);
> 
> Cast is not needed.

I tend to change it like this:

	offset_be = cpu_to_be16(offset);
        msg[0].buf = (u8 *) &offset_be;

> > +static ssize_t at24_ee_write(struct at24_data *at24, char *buf,
> > loff_t off, +		size_t count) +{ +	struct
> > i2c_client *client; +	struct i2c_msg  msg;
> 
> Doubled space.
> 
> > +	unsigned offset = (unsigned) off;
> 
> Why don't you simply make the offset parameter an unsigned int, if
> that's what you want?
I changed the parameter to unsigned. But I think I need to have another
look, not that we are bitten by some cast-sideeffects.

> > +	/* buffer big enough to stick the address at the beginning */
> > +	at24->writebuf = kmalloc(at24->write_max + 2, GFP_KERNEL);
> > +	if (!at24->writebuf) {
> > +		retval = -ENOMEM;
> > +		count = 0;
> > +	}
> 
> You could move this to before taking the mutex (using a temporary
> pointer), so that you can return with an error immediately if
> allocation fails. But more importantly: do you really need to allocate
> and free a new buffer for each write? You serialize the writes, and the
> size of the buffer doesn't depend on the actual write operation, so you
> might as well allocate the buffer as part of at24_probe(), this will
> make write operations faster and will avoid useless memory
> fragmentation.
> 
> If you are really worried about the memory waste in case the user
> doesn't actually write to the EEPROM, you could alternatively allocate
> the buffer on the first write, and free it in the .remove() method.
Now, I allocate it in probe when the EEPROM is selected to be writable.

> > +	/* Use I2C operations unless we're stuck with SMBus extensions. */
> > +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > +		if (chip->flags & AT24_EE_ADDR2) {
> > +			err = -ECOMM;
> > +			goto fail;
> > +		}
> > +		if (!i2c_check_functionality(client->adapter,
> > +				I2C_FUNC_SMBUS_I2C_BLOCK)) {
> > +			err = -EBADMSG;
> > +			goto fail;
> > +		}
> 
> These are definitely not the correct error values. I also see no
> reason to return different error values for the two cases as the
> problem is exactly the same: underlying adapter doesn't support the
> transaction types needed to talk to the EEPROM. What about -EOPNOTSUPP?
> Or -ENODEV.
Ah, errno-fun. -EPFNOSUPPORT?

> > +	/* use dummy devices for multiple-address chips */
> > +	if (chip->i2c_addr_mask) {
> > +		for (i = 1; i <= chip->i2c_addr_mask; i++) {
> > +			c = i2c_new_dummy(client->adapter, client->addr + i);
> > +			if (!c) {
> > +				dev_err(&client->dev, "addr %d unavail\n",
> 
> Please spell words in error messages completely. Address should be
> printed in hexadecimal, that's what developers and users are used to.
> 
> > +						client->addr + i);
> > +				err = -ENOCSI;
> 
> Again a funky error value, completely unrelated with the error that
> occurred. -EBUSY?
-EADDRINUSE?

> > +cleanup:
> > +	if (chip->i2c_addr_mask) {
> 
> Note that this test is not needed: the for loop below won't do anything
> if i2c_addr_mask == 0.
I thought it would be cleaner to skip the for-loop if it is not needed.
Otherwise it may look like a bug easily. Then again, a comment would
also help.

> 
> > +		for (i = 1; i <= chip->i2c_addr_mask; i++) {
> > +			c = at24->client[i];
> > +			if (c)
> > +				i2c_unregister_device(c);
> > + *	- What write page size does it support?
> > + */
> > +
> > +struct at24_platform_data {
> > +	u32		byte_len;		/* size (sum of all addr) */
> > +	u16		page_size;		/* for writes */
> > +	u8		i2c_addr_mask;		/* for multi-addr chips */
> 
> This notion of i2c_addr_mask seems more restrictive and easy to get
> wrong than it needs to be. What you really have is a number of slave
> I2C addresses, that's more intuitive IMHO, and using this would save a
> couple "+ 1" in the code. As a matter of fact, that's what you
> described in the comment above.
> 
> Oh, BTW, can't you compute this value yourself from byte_len and (flags
> & AT24_EE_ADDR2)? I think so...
There is at least one exception already (24c00) which covers eight
addresses but actually just needs one. This spoils the calculation of
i2c_addr_mask (and if there is one case, there will be others) :( I
agree, that num_addresses might be more apropriate than i2c_addr_mask.

Kind regards,

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi Wolfram,

On Mon, 2 Jun 2008 18:21:54 +0200, Wolfram Sang wrote:
> some more comments while working on the driver:

I concur with everything, except:

> > Oh, BTW, can't you compute this value yourself from byte_len and (flags
> > & AT24_EE_ADDR2)? I think so...
>
> There is at least one exception already (24c00) which covers eight
> addresses but actually just needs one. This spoils the calculation of
> i2c_addr_mask (and if there is one case, there will be others) :( I
> agree, that num_addresses might be more apropriate than i2c_addr_mask.

Not really. The 24C00 might answer to 8 I2C addresses, but how do you
care? You only need one address to access the whole data range.
Registering the extra clients is a waste of time and memory, so just
don't do it.

Problem solved :)

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Monday 02 June 2008, Jean Delvare wrote:
> Not really. The 24C00 might answer to 8 I2C addresses, but how do you
> care? You only need one address to access the whole data range.
> Registering the extra clients is a waste of time and memory, so just
> don't do it.

One reason the driver claims all the EEPROM addresses used by
each chip is to address review feedback from Jean Delvare.
ISTR the point was safety:  letting other drivers potentially
access the device was a bad idea.  ;)


> Problem solved :)

Not really.  The "eeprom" driver, or various other legacy
drivers knowing about the 0x50..0x57 address range, could
bind to those addresses.  That problem would not be solved.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

On Mon, 2 Jun 2008 12:33:46 -0700, David Brownell wrote:
> On Monday 02 June 2008, Jean Delvare wrote:
> > Not really. The 24C00 might answer to 8 I2C addresses, but how do you
> > care? You only need one address to access the whole data range.
> > Registering the extra clients is a waste of time and memory, so just
> > don't do it.
> 
> One reason the driver claims all the EEPROM addresses used by
> each chip is to address review feedback from Jean Delvare.

You shall not listen to what that guy says. He's a notoriously clueless
idiot! ;)

> ISTR the point was safety:  letting other drivers potentially
> access the device was a bad idea.  ;)

I'm not even sure what problem it could cause in practice...

> > Problem solved :)
> 
> Not really.  The "eeprom" driver, or various other legacy
> drivers knowing about the 0x50..0x57 address range, could
> bind to those addresses.  That problem would not be solved.

If you're using the at24 driver on that bus, you're most certainly
using only new-style drivers and not even setting a "class" for legacy
i2c drivers to probe the i2c adapter. So, nothing will attach to the
extra addresses unless you explicitly ask for it - and then you're on
your own.

Yes, the eeprom driver currently doesn't check for i2c_adapter.class,
and yes, it's a bug. We need to define a class flag for it
(I2C_CLASS_SPD?), set it in all PC SMBus host drivers, and have the
eeprom check for (I2C_CLASS_SPD | I2C_CLASS_DDC).

Once this is fixed, I really see zero benefit in requesting all the
addresses the 24C00 replies to in the at24 driver.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Monday 02 June 2008, Jean Delvare wrote:
> > ISTR the point was safety:  letting other drivers potentially
> > access the device was a bad idea.  ;)
> 
> I'm not even sure what problem it could cause in practice...

Who knows what protocol the "other drivers" expect to work.

It might happen to look like writing to arbitrary addresses.
Not unlike "read with 16-bit address" starts out with bytes
that look just like "write to 8-bit address"...


> > > Problem solved :)
> > 
> > Not really.  The "eeprom" driver, or various other legacy
> > drivers knowing about the 0x50..0x57 address range, could
> > bind to those addresses.  That problem would not be solved.
> 
> If you're using the at24 driver on that bus, you're most certainly
> using only new-style drivers and not even setting a "class" for legacy
> i2c drivers to probe the i2c adapter.

Not yet true.  There are systems that still need to use legacy
drivers.  In a few years, I hope you're correct about this.


> Yes, the eeprom driver currently doesn't check for i2c_adapter.class,
> and yes, it's a bug. We need to define a class flag for it
> (I2C_CLASS_SPD?), set it in all PC SMBus host drivers, and have the
> eeprom check for (I2C_CLASS_SPD | I2C_CLASS_DDC).
> 
> Once this is fixed, I really see zero benefit in requesting all the
> addresses the 24C00 replies to in the at24 driver.

I personally won't worry much about 24c00 chips; I have some for
testing, but haven't seen new systems using them for some time.
It was news for me that they exist on DDC links!


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi David,

On Tue, 3 Jun 2008 13:36:35 -0700, David Brownell wrote:
> On Monday 02 June 2008, Jean Delvare wrote:
> > > ISTR the point was safety:  letting other drivers potentially
> > > access the device was a bad idea.  ;)
> > 
> > I'm not even sure what problem it could cause in practice...
> 
> Who knows what protocol the "other drivers" expect to work.
> 
> It might happen to look like writing to arbitrary addresses.
> Not unlike "read with 16-bit address" starts out with bytes
> that look just like "write to 8-bit address"...

That's correct but irrelevant here. You can't rely on driver A picking
addresses it doesn't even use before legacy driver B has a chance to
probe them because that probing could do something wrong. B could
"load" before A, or the kernel could have driver B but not driver A.
The only safe approach is to only use "safe" transactions (basically:
SMBus read byte transactions) during probing in legacy drivers, and
that's what we actually do.

> > > > Problem solved :)
> > > 
> > > Not really.  The "eeprom" driver, or various other legacy
> > > drivers knowing about the 0x50..0x57 address range, could
> > > bind to those addresses.  That problem would not be solved.
> > 
> > If you're using the at24 driver on that bus, you're most certainly
> > using only new-style drivers and not even setting a "class" for legacy
> > i2c drivers to probe the i2c adapter.
> 
> Not yet true.  There are systems that still need to use legacy
> drivers.  In a few years, I hope you're correct about this.

But then they tell which type of legacy drivers can probe them. I
pretty much doubt that you'll allow the legacy eeprom driver to probe
your bus if you are also using the at24 driver. And the eeprom driver
is possibly the only legacy driver which probes addresses 0x51-0x57.

I agree that in theory something wrong could happen in the current
state of things if the extra addresses of the 24C00 chip are not
"protected" by the at24 driver, but in practice I very much doubt that
it'll happen, and if it does, it would be easy to fix. So I reiterate
that the at24 driver doesn't need to handle this.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 974 bytes --]

On Tue, Jun 03, 2008 at 11:19:27PM +0200, Jean Delvare wrote:
> I agree that in theory something wrong could happen in the current
> state of things if the extra addresses of the 24C00 chip are not
> "protected" by the at24 driver, but in practice I very much doubt that
> it'll happen, and if it does, it would be easy to fix. So I reiterate
> that the at24 driver doesn't need to handle this.
After removing i2c_addr_mask, it turned out that it just needed one
if-case and anonther flag to handle the 24c00-quirk. Hoping that this
will be the only case needing such treatment ;), I think it is worth the
price.

Sidenote: I just finished hacking in all points from my to-do-list. Now,
I want to validate the implicit casts and give the driver some really
good testing as some parts had to be rewritten for v3.

All the best,

   Wolfram
-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[Jean Delvare]

Hi David,

On Tue, 3 Jun 2008 13:36:35 -0700, David Brownell wrote:
> I personally won't worry much about 24c00 chips; I have some for
> testing, but haven't seen new systems using them for some time.
> It was news for me that they exist on DDC links!

Actually I don't think there are 24C00 chips on DDC links. These chips
only provide 128 bit (16 bytes) of storage space, which is insufficient
to hold EDID data. So I think these are 24C01 chips (1 kb) which
behave like the 24C00 as far as I2C addressing is concerned (i.e.
answering to all addresses 0x50-0x57.) Apparently Atmel make such
chips (the datasheet doesn't mention any I2C address, and the 3 EEPROM
pins which are typically used for address selection are labelled "NC".)

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] Add a new-style driver for most I2C EEPROMs

[David Brownell]

On Sunday 08 June 2008, Jean Delvare wrote:
> On Tue, 3 Jun 2008 13:36:35 -0700, David Brownell wrote:
> > I personally won't worry much about 24c00 chips; I have some for
> > testing, but haven't seen new systems using them for some time.
> > It was news for me that they exist on DDC links!
> 
> Actually I don't think there are 24C00 chips on DDC links. These chips
> only provide 128 bit (16 bytes) of storage space, which is insufficient
> to hold EDID data. So I think these are 24C01 chips (1 kb) which
> behave like the 24C00 as far as I2C addressing is concerned (i.e.
> answering to all addresses 0x50-0x57.) Apparently Atmel make such
> chips (the datasheet doesn't mention any I2C address, and the 3 EEPROM
> pins which are typically used for address selection are labelled "NC".)

I see.  The only data sheet I had handy lists 24c01A parts,
which *do* have address pins, not marked as NC ("No Connect").
But I see the 24c01 is as you describe, though it's "not for
new designs".

And I'm a bit puzzled how the SOT23-5 package for the 24c01B
parts will be addressed ... those signals aren't pinned out
there (couldn't be!), and the address doesn't appear to be an
option which is exposed in the part number.

- Dave

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c