[PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Wolfram Sang]

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


New-style I2C and SMBus EEPROM driver (with device_ids)

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

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

Note: I am not in my office this week, so I can't do all the tests I usually
do. Please check carefully! David: I hope it is OK with you that I added myself
as another author of this module.

Updates since last version:

 - added device_ids for common eeprom types (parameters encoded in
   a 'magic' driver_data value)
 - removed platform_data entry 'i2c_addr_mask' and calculated
   its value from other parameters
 - added 24c00-quirk flag (it covers 8 addresses)
 - added a flag to make eeproms world-readable (used for spd)
 - rewrote code that adds an i2c-address to an i2c-message
 - rewrote code which truncates to page_size
 - removed 'addr'-variable from eeprom-functions; i2c-address is
   now taken from the corresponding client-structure
 - write buffer now allocated once in probe
 - removed some sanity checks for file offsets as they are handled at
   the sysfs-layer already.
 - fixed typos and corrected spellings in comments and Kconfig
 - renamed some functions to be more self-explanatory
 - added includes
 - further cleanups and simplifications
 - added myself as another author

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

 drivers/i2c/chips/Kconfig  |   26 ++
 drivers/i2c/chips/Makefile |    1 
 drivers/i2c/chips/at24.c   |  574 +++++++++++++++++++++++++++++++++++++++++++++
 include/linux/i2c/at24.h   |  104 ++++++++
 4 files changed, 705 insertions(+)
 create mode 100644 drivers/i2c/chips/at24.c
 create mode 100644 include/linux/i2c/at24.h

Index: linux-2.6.26-rc4/drivers/i2c/chips/Kconfig
===================================================================
--- linux-2.6.26-rc4.orig/drivers/i2c/chips/Kconfig
+++ linux-2.6.26-rc4/drivers/i2c/chips/Kconfig
@@ -14,6 +14,32 @@
 	  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 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
+	  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 kbit) or larger is NOT really a
+	  24c16 (16 kbit) or smaller, and vice versa. Marking the chip
+	  as read-only won't help recover from this. Also, if your chip
+	  has any software write-protect mechanism you may want to review the
+	  code 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 kByte).
+
+	  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
Index: linux-2.6.26-rc4/drivers/i2c/chips/Makefile
===================================================================
--- linux-2.6.26-rc4.orig/drivers/i2c/chips/Makefile
+++ linux-2.6.26-rc4/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
Index: linux-2.6.26-rc4/drivers/i2c/chips/at24.c
===================================================================
--- /dev/null
+++ linux-2.6.26-rc4/drivers/i2c/chips/at24.c
@@ -0,0 +1,574 @@
+/*
+ * 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/mutex.h>
+#include <linux/sysfs.h>
+#include <linux/mod_devicetable.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.
+ *
+ * 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 addresses,
+ * 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;
+	unsigned num_addresses;
+
+	/*
+	 * Some chips tie up multiple I2C addresses; dummy devices reserve
+	 * them for us, 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 addresses. It
+ * computes the addressing information to be used for a given r/w request.
+ * Assumes that sanity checks for offset happened at sysfs-layer.
+ */
+static struct i2c_client *at24_translate_offset(struct at24_data *at24,
+		unsigned *offset)
+{
+	unsigned i;
+
+	if (at24->chip.flags & AT24_FLAG_ADDR16) {
+		i = *offset >> 16;
+		*offset &= 0xffff;
+	} else {
+		i = *offset >> 8;
+		*offset &= 0xff;
+	}
+
+	return at24->client[i];
+}
+
+static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
+		unsigned offset, size_t count)
+{
+	struct i2c_msg msg[2];
+	u8 msgbuf[2];
+	struct i2c_client *client;
+	int status, i = 0;
+
+	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.
+	 */
+
+	/*
+	 * 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_translate_offset(at24, &offset);
+
+	if (count > io_limit)
+		count = io_limit;
+
+	/* Smaller eproms can work given some SMBus extension calls */
+	if (at24->use_smbus) {
+		if (count > I2C_SMBUS_BLOCK_MAX)
+			count = I2C_SMBUS_BLOCK_MAX;
+		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).
+	 * msgbuf is u8 and will cast to our needs.
+	 */
+	if (at24->chip.flags & AT24_FLAG_ADDR16)
+		msgbuf[i++] = offset >> 8;
+	msgbuf[i++] = offset;
+
+	msg[0].addr = client->addr;
+	msg[0].buf = msgbuf;
+	msg[0].len = i;
+
+	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 at24_data *at24;
+	ssize_t retval = 0;
+
+	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
+
+	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);
+
+	while (count) {
+		ssize_t	status;
+
+		status = at24_eeprom_read(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+	}
+
+	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_eeprom_write(struct at24_data *at24, char *buf,
+		unsigned offset, size_t count)
+{
+	struct i2c_client *client;
+	struct i2c_msg msg;
+	ssize_t status;
+	unsigned long timeout, write_time;
+	unsigned next_page;
+
+	/* Get corresponding i2c address and adjust offset */
+	client = at24_translate_offset(at24, &offset);
+
+	/* 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 */
+	next_page = roundup(offset + 1, at24->chip.page_size);
+	if (offset + count > next_page)
+		count = next_page - offset;
+
+	/* If we'll use i2c calls for I/O, set up the message */
+	if (!at24->use_smbus) {
+		int i = 0;
+
+		msg.addr = client->addr;
+		msg.flags = 0;
+
+		/* msg.buf is u8 and casts will mask the values */
+		msg.buf = at24->writebuf;
+		if (at24->chip.flags & AT24_FLAG_ADDR16)
+			msg.buf[i++] = offset >> 8;
+
+		msg.buf[i++] = offset;
+		memcpy(&msg.buf[i], buf, count);
+		msg.len = i + count;
+	}
+
+	/*
+	 * Writes fail if the previous one didn't complete yet. We may
+	 * 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));
+
+	return -ETIMEDOUT;
+}
+
+static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct at24_data *at24;
+	ssize_t retval = 0;
+
+	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
+
+	if (unlikely(!count))
+		return count;
+
+	/*
+	 * Write data to chip, protecting against concurrent updates
+	 * from this host ... but not from other i2c masters.
+	 */
+	mutex_lock(&at24->lock);
+
+	while (count) {
+		ssize_t	status;
+
+		status = at24_eeprom_write(at24, buf, off, count);
+		if (status <= 0) {
+			if (retval == 0)
+				retval = status;
+			break;
+		}
+		buf += status;
+		off += status;
+		count -= status;
+		retval += status;
+	}
+
+	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, num_addresses;
+	kernel_ulong_t magic;
+
+	if (id->driver_data) {
+		chip = kmalloc(sizeof(struct at24_platform_data), GFP_KERNEL);
+		if (!chip) {
+			err = -ENOMEM;
+			goto err_out;
+		}
+		magic = id->driver_data;
+		chip->byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
+		magic >>= AT24_SIZE_BYTELEN;
+		chip->page_size = BIT(magic & AT24_BITMASK(AT24_SIZE_PAGESIZE));
+		magic >>= AT24_SIZE_PAGESIZE;
+		chip->flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
+	} else {
+		chip = client->dev.platform_data;
+		if (!chip) {
+			err = -ENODEV;
+			goto err_out;
+		}
+	}
+
+	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");
+
+	/* Use I2C operations unless we're stuck with SMBus extensions. */
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		if (chip->flags & AT24_FLAG_ADDR16) {
+			err = -EPFNOSUPPORT;
+			goto err_chip;
+		}
+		if (!i2c_check_functionality(client->adapter,
+				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
+			err = -EPFNOSUPPORT;
+			goto err_chip;
+		}
+		use_smbus = true;
+	}
+
+	if (chip->flags & AT24_FLAG_24C00)
+		num_addresses = 8;
+	else
+		num_addresses = (chip->byte_len >>
+				(chip->flags & AT24_FLAG_ADDR16 ? 16 : 8)) + 1;
+
+	at24 = kzalloc(sizeof(struct at24_data) +
+		num_addresses * sizeof(struct i2c_client *), GFP_KERNEL);
+	if (!at24) {
+		err = -ENOMEM;
+		goto err_chip;
+	}
+
+	mutex_init(&at24->lock);
+	at24->use_smbus = use_smbus;
+	at24->chip = *chip;
+	at24->num_addresses = num_addresses;
+
+	/*
+	 * 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 = chip->flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
+	at24->bin.attr.owner = THIS_MODULE;
+	at24->bin.read = at24_bin_read;
+	at24->bin.size = chip->byte_len;
+
+	writable = !(chip->flags & AT24_FLAG_READONLY);
+	if (writable) {
+		if (!use_smbus || i2c_check_functionality(client->adapter,
+				I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
+
+			unsigned write_max = 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 > I2C_SMBUS_BLOCK_MAX)
+				write_max = I2C_SMBUS_BLOCK_MAX;
+			at24->write_max = write_max;
+
+			/* buffer (data + address at the beginning) */
+			at24->writebuf = kmalloc(write_max + 2, GFP_KERNEL);
+			if (!at24->writebuf) {
+				err = -ENOMEM;
+				goto err_struct;
+			}
+		} else {
+			dev_warn(&client->dev,
+				"cannot write due to controller restrictions.");
+		}
+	}
+
+	at24->client[0] = client;
+
+	/* use dummy devices for multiple-address chips */
+	for (i = 1; i < num_addresses; i++) {
+		at24->client[i] = i2c_new_dummy(client->adapter,
+					client->addr + i);
+		if (!at24->client[i]) {
+			dev_err(&client->dev, "address 0x%04x unavailable\n",
+					client->addr + i);
+			err = -EADDRINUSE;
+			goto err_clients;
+		}
+	}
+
+	err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
+	if (err)
+		goto err_clients;
+
+	i2c_set_clientdata(client, at24);
+
+	dev_info(&client->dev, "%Zd byte %s EEPROM %s\n",
+		at24->bin.size, client->name,
+		writable ? "(writable)" : "(read-only)");
+	dev_dbg(&client->dev,
+		"page_size %d, num_addresses %d, write_max %d%s\n",
+		chip->page_size, num_addresses,
+		at24->write_max,
+		use_smbus ? ", use_smbus" : "");
+
+	if (id->driver_data)
+		 kfree(chip);
+	return 0;
+
+err_clients:
+	for (i = 1; i < num_addresses; i++)
+		if (at24->client[i])
+			i2c_unregister_device(at24->client[i]);
+
+	kfree(at24->writebuf);
+err_struct:
+	kfree(at24);
+err_chip:
+	if (id->driver_data)
+		kfree(chip);
+err_out:
+	dev_dbg(&client->dev, "probe error %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);
+
+	for (i = 1; i < at24->num_addresses; i++)
+		if (at24->client[i])
+			i2c_unregister_device(at24->client[i]);
+
+	kfree(at24->writebuf);
+	kfree(at24);
+	i2c_set_clientdata(client, NULL);
+	return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+static const struct i2c_device_id at24_ids[] = {
+	{ "24c00", AT24_DEVICE_MAGIC(AT24_DATA_24C00) },
+	{ "24c01", AT24_DEVICE_MAGIC(AT24_DATA_24C01) },
+	{ "24c02", AT24_DEVICE_MAGIC(AT24_DATA_24C02) },
+	{ "spd", AT24_DEVICE_MAGIC(AT24_DATA_SPD) },
+	{ "pcf8570", AT24_DEVICE_MAGIC(AT24_DATA_PCF8570) },
+	{ "24c04", AT24_DEVICE_MAGIC(AT24_DATA_24C04) },
+	{ "24c08", AT24_DEVICE_MAGIC(AT24_DATA_24C08) },
+	{ "24c16", AT24_DEVICE_MAGIC(AT24_DATA_24C16) },
+	{ "24c32", AT24_DEVICE_MAGIC(AT24_DATA_24C32) },
+	{ "24c64", AT24_DEVICE_MAGIC(AT24_DATA_24C64) },
+	{ "24c128", AT24_DEVICE_MAGIC(AT24_DATA_24C128) },
+	{ "24c256", AT24_DEVICE_MAGIC(AT24_DATA_24C256) },
+	{ "24c512", AT24_DEVICE_MAGIC(AT24_DATA_24C512) },
+	{ "24c1024", AT24_DEVICE_MAGIC(AT24_DATA_24C1024) },
+	{ "at24", 0 },
+	{ /* END OF LIST */ }
+};
+MODULE_DEVICE_TABLE(i2c, at24_ids);
+
+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 and Wolfram Sang");
+MODULE_LICENSE("GPL");
Index: linux-2.6.26-rc4/include/linux/i2c/at24.h
===================================================================
--- /dev/null
+++ linux-2.6.26-rc4/include/linux/i2c/at24.h
@@ -0,0 +1,104 @@
+#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 much memory is available (usually specified in bit)?
+ * - What write page size does it support?
+ * - Special flags (16 bit addresses, read_only, world readable)?
+ *
+ * If you 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! A typical custom type declaration would look similar to this:
+ *
+ *	struct const at24_platform_data my_eeprom_data {
+ *		AT24_PLATFORM_DATA(byte_len, page_size, flags);
+ *	};
+ *
+ */
+struct at24_platform_data {
+	u32		byte_len;		/* size (sum of all addr) */
+	u16		page_size;		/* for writes */
+	u8		flags;
+#define AT24_FLAG_ADDR16	0x80
+#define AT24_FLAG_READONLY	0x40
+#define AT24_FLAG_IRUGO		0x20
+#define AT24_FLAG_24C00		0x10
+};
+
+#define AT24_SIZE_BYTELEN 5
+#define AT24_SIZE_PAGESIZE 4
+#define AT24_SIZE_FLAGS 8
+
+#define AT24_BITMASK(x) ((1UL << x) - 1)
+
+/* nest macros to enforce expansion of macros containing parameters */
+#define AT24_PLATFORM_DATA(x) _AT24_PLATFORM_DATA(x)
+
+#define _AT24_PLATFORM_DATA(_len, _page, _flags) \
+	.byte_len = (_len), .page_size = (_page), .flags = (_flags)
+
+/* create non-zero magic value for given eeprom parameters */
+#define AT24_DEVICE_MAGIC(x) _AT24_DEVICE_MAGIC(x)
+
+#define _AT24_DEVICE_MAGIC(_len, _page, _flags) 	\
+	(((1 << AT24_SIZE_FLAGS | _flags) 		\
+		<< AT24_SIZE_PAGESIZE | ilog2(_page)) 	\
+		<< AT24_SIZE_BYTELEN | ilog2(_len))
+
+/*
+ * Chip data. Parameters are byte_len, page_size and flags
+ */
+
+/* 128 bit chip, I2C A0-A2 ignored */
+#define AT24_DATA_24C00 128 / 8, 1, AT24_FLAG_24C00
+
+/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
+#define AT24_DATA_24C01 1024 / 8, 8, 0
+
+/* 2 Kbit chip, some have 16 byte pages: */
+#define AT24_DATA_24C02 2048 / 8, 8, 0
+
+/* 2 Kbit chip, 24c02 in memory DIMMs, some have 16 byte pages */
+#define AT24_DATA_SPD 2048 / 8, 8, AT24_FLAG_READONLY | AT24_FLAG_IRUGO
+
+/* 2 Kbit chip, SRAM, not EEPROM!, no page size issues, write it all at once */
+#define AT24_DATA_PCF8570 2048 / 8, 2048 / 8, 0
+
+/* 4 Kbit chip, I2C A0 is MEM A8 */
+#define AT24_DATA_24C04 4096 / 8, 16, 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_DATA_24C08 8192 / 8, 16, 0
+
+/* 16 Kbit chip, I2C A2-A0 is MEM A10-A8 */
+#define AT24_DATA_24C16 16384 / 8, 16, 0
+
+/* this second block of EEPROMs uses 16 bit memory addressing */
+
+/* 32 Kbits */
+#define AT24_DATA_24C32 32768 / 8, 32, AT24_FLAG_ADDR16
+
+/* 64 Kbits */
+#define AT24_DATA_24C64 65536 / 8, 32, AT24_FLAG_ADDR16
+
+/* 128 Kbits */
+#define AT24_DATA_24C128 131072 / 8, 64, AT24_FLAG_ADDR16
+
+/* 256 Kbits */
+#define AT24_DATA_24C256 262144 / 8, 64, AT24_FLAG_ADDR16
+
+/* 512 Kbits */
+#define AT24_DATA_24C512 524288 / 8, 128, AT24_FLAG_ADDR16
+
+/* 1 Mbits, I2C A0 is MEM A16 */
+#define AT24_DATA_24C1024 1048576 / 8, 256, AT24_FLAG_ADDR16
+
+#endif /* _LINUX_AT24_H */
-- 
  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 v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

Hi Wolfram,

On Thu, 5 Jun 2008 21:31:03 +0200, Wolfram Sang wrote:
> 
> New-style I2C and SMBus EEPROM driver (with device_ids)
> 
> Add a new-style driver for most I2C EEPROMs, giving sysfs read/write
> access to their data. Tested with various chips and clock rates.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---

It's getting better :) One more (and hopefully last) review:

> 
> Note: I am not in my office this week, so I can't do all the tests I usually
> do. Please check carefully! David: I hope it is OK with you that I added myself
> as another author of this module.
> 
> Updates since last version:
> 
>  - added device_ids for common eeprom types (parameters encoded in
>    a 'magic' driver_data value)
>  - removed platform_data entry 'i2c_addr_mask' and calculated
>    its value from other parameters
>  - added 24c00-quirk flag (it covers 8 addresses)

As said before, I don't think it's needed, but if you and David insist
on picking all addresses of the 24C00, so be it.

>  - added a flag to make eeproms world-readable (used for spd)
>  - rewrote code that adds an i2c-address to an i2c-message
>  - rewrote code which truncates to page_size
>  - removed 'addr'-variable from eeprom-functions; i2c-address is
>    now taken from the corresponding client-structure
>  - write buffer now allocated once in probe
>  - removed some sanity checks for file offsets as they are handled at
>    the sysfs-layer already.
>  - fixed typos and corrected spellings in comments and Kconfig
>  - renamed some functions to be more self-explanatory
>  - added includes
>  - further cleanups and simplifications
>  - added myself as another author
> 
> 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
> 
>  drivers/i2c/chips/Kconfig  |   26 ++
>  drivers/i2c/chips/Makefile |    1 
>  drivers/i2c/chips/at24.c   |  574 +++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/i2c/at24.h   |  104 ++++++++
>  4 files changed, 705 insertions(+)
>  create mode 100644 drivers/i2c/chips/at24.c
>  create mode 100644 include/linux/i2c/at24.h
> 
> Index: linux-2.6.26-rc4/drivers/i2c/chips/Kconfig
> ===================================================================
> --- linux-2.6.26-rc4.orig/drivers/i2c/chips/Kconfig
> +++ linux-2.6.26-rc4/drivers/i2c/chips/Kconfig
> @@ -14,6 +14,32 @@
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called ds1682.
>  
> +config I2C_AT24

Just AT24 please.

> +	tristate "EEPROMs from most vendors"
> +	depends on 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
> +	  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 kbit) or larger is NOT really a
> +	  24c16 (16 kbit) or smaller, and vice versa. Marking the chip
> +	  as read-only won't help recover from this. Also, if your chip
> +	  has any software write-protect mechanism you may want to review the
> +	  code 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

Typo: available.

> +	  supported (24c16 and below, max 4 kByte).
> +
> +	  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
> Index: linux-2.6.26-rc4/drivers/i2c/chips/Makefile
> ===================================================================
> --- linux-2.6.26-rc4.orig/drivers/i2c/chips/Makefile
> +++ linux-2.6.26-rc4/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
> Index: linux-2.6.26-rc4/drivers/i2c/chips/at24.c
> ===================================================================
> --- /dev/null
> +++ linux-2.6.26-rc4/drivers/i2c/chips/at24.c
> @@ -0,0 +1,574 @@
> +/*
> + * 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/mutex.h>
> +#include <linux/sysfs.h>
> +#include <linux/mod_devicetable.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.
> + *
> + * 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 addresses,
> + * 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;
> +	unsigned num_addresses;
> +
> +	/*
> +	 * Some chips tie up multiple I2C addresses; dummy devices reserve
> +	 * them for us, 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 addresses. It
> + * computes the addressing information to be used for a given r/w request.
> + * Assumes that sanity checks for offset happened at sysfs-layer.
> + */
> +static struct i2c_client *at24_translate_offset(struct at24_data *at24,
> +		unsigned *offset)
> +{
> +	unsigned i;
> +
> +	if (at24->chip.flags & AT24_FLAG_ADDR16) {
> +		i = *offset >> 16;
> +		*offset &= 0xffff;
> +	} else {
> +		i = *offset >> 8;
> +		*offset &= 0xff;
> +	}
> +
> +	return at24->client[i];
> +}
> +
> +static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
> +		unsigned offset, size_t count)
> +{
> +	struct i2c_msg msg[2];
> +	u8 msgbuf[2];
> +	struct i2c_client *client;
> +	int status, i = 0;

I would feel safer if the initialization of "i" was moved...

> +
> +	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.
> +	 */
> +
> +	/*
> +	 * 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_translate_offset(at24, &offset);
> +
> +	if (count > io_limit)
> +		count = io_limit;
> +
> +	/* Smaller eproms can work given some SMBus extension calls */
> +	if (at24->use_smbus) {
> +		if (count > I2C_SMBUS_BLOCK_MAX)
> +			count = I2C_SMBUS_BLOCK_MAX;
> +		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).
> +	 * msgbuf is u8 and will cast to our needs.
> +	 */

... down there, where you actually use it.

> +	if (at24->chip.flags & AT24_FLAG_ADDR16)
> +		msgbuf[i++] = offset >> 8;
> +	msgbuf[i++] = offset;
> +
> +	msg[0].addr = client->addr;
> +	msg[0].buf = msgbuf;
> +	msg[0].len = i;
> +
> +	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 at24_data *at24;
> +	ssize_t retval = 0;
> +
> +	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> +
> +	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);
> +
> +	while (count) {
> +		ssize_t	status;
> +
> +		status = at24_eeprom_read(at24, buf, off, count);
> +		if (status <= 0) {
> +			if (retval == 0)
> +				retval = status;
> +			break;
> +		}
> +		buf += status;
> +		off += status;
> +		count -= status;
> +		retval += status;
> +	}
> +
> +	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_eeprom_write(struct at24_data *at24, char *buf,
> +		unsigned offset, size_t count)
> +{
> +	struct i2c_client *client;
> +	struct i2c_msg msg;
> +	ssize_t status;
> +	unsigned long timeout, write_time;
> +	unsigned next_page;
> +
> +	/* Get corresponding i2c address and adjust offset */
> +	client = at24_translate_offset(at24, &offset);
> +
> +	/* 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 */
> +	next_page = roundup(offset + 1, at24->chip.page_size);
> +	if (offset + count > next_page)
> +		count = next_page - offset;
> +
> +	/* If we'll use i2c calls for I/O, set up the message */
> +	if (!at24->use_smbus) {
> +		int i = 0;
> +
> +		msg.addr = client->addr;
> +		msg.flags = 0;
> +
> +		/* msg.buf is u8 and casts will mask the values */
> +		msg.buf = at24->writebuf;
> +		if (at24->chip.flags & AT24_FLAG_ADDR16)
> +			msg.buf[i++] = offset >> 8;
> +
> +		msg.buf[i++] = offset;
> +		memcpy(&msg.buf[i], buf, count);
> +		msg.len = i + count;
> +	}
> +
> +	/*
> +	 * Writes fail if the previous one didn't complete yet. We may
> +	 * 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));
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
> +		char *buf, loff_t off, size_t count)
> +{
> +	struct at24_data *at24;
> +	ssize_t retval = 0;
> +
> +	at24 = dev_get_drvdata(container_of(kobj, struct device, kobj));
> +
> +	if (unlikely(!count))
> +		return count;
> +
> +	/*
> +	 * Write data to chip, protecting against concurrent updates
> +	 * from this host ... but not from other i2c masters.
> +	 */
> +	mutex_lock(&at24->lock);
> +
> +	while (count) {
> +		ssize_t	status;
> +
> +		status = at24_eeprom_write(at24, buf, off, count);
> +		if (status <= 0) {
> +			if (retval == 0)
> +				retval = status;
> +			break;
> +		}
> +		buf += status;
> +		off += status;
> +		count -= status;
> +		retval += status;
> +	}
> +
> +	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, num_addresses;
> +	kernel_ulong_t magic;
> +
> +	if (id->driver_data) {
> +		chip = kmalloc(sizeof(struct at24_platform_data), GFP_KERNEL);
> +		if (!chip) {

You allocate this just to copy it to another structure and free it
immediately. Allocating the other structure (struct at24_data) earlier
would save this temporary allocation. Alternatively, just put the struct
at24_platform_data on the stack, it's small enough to do so.

> +			err = -ENOMEM;
> +			goto err_out;
> +		}
> +		magic = id->driver_data;
> +		chip->byte_len = BIT(magic & AT24_BITMASK(AT24_SIZE_BYTELEN));
> +		magic >>= AT24_SIZE_BYTELEN;
> +		chip->page_size = BIT(magic & AT24_BITMASK(AT24_SIZE_PAGESIZE));
> +		magic >>= AT24_SIZE_PAGESIZE;

You need to include <linux/bitops.h> for BIT().

> +		chip->flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
> +	} else {
> +		chip = client->dev.platform_data;
> +		if (!chip) {
> +			err = -ENODEV;
> +			goto err_out;
> +		}
> +	}
> +
> +	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");
> +
> +	/* Use I2C operations unless we're stuck with SMBus extensions. */
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		if (chip->flags & AT24_FLAG_ADDR16) {
> +			err = -EPFNOSUPPORT;
> +			goto err_chip;
> +		}
> +		if (!i2c_check_functionality(client->adapter,
> +				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> +			err = -EPFNOSUPPORT;
> +			goto err_chip;
> +		}
> +		use_smbus = true;
> +	}
> +
> +	if (chip->flags & AT24_FLAG_24C00)
> +		num_addresses = 8;
> +	else
> +		num_addresses = (chip->byte_len >>
> +				(chip->flags & AT24_FLAG_ADDR16 ? 16 : 8)) + 1;

This doesn't look correct to me. For a 24C02, byte_len is 256, 256 >> 8
= 1, 1 + 1 = 2. So you're allocating 2 clients when you need only one.

> +
> +	at24 = kzalloc(sizeof(struct at24_data) +
> +		num_addresses * sizeof(struct i2c_client *), GFP_KERNEL);
> +	if (!at24) {
> +		err = -ENOMEM;
> +		goto err_chip;
> +	}
> +
> +	mutex_init(&at24->lock);
> +	at24->use_smbus = use_smbus;
> +	at24->chip = *chip;
> +	at24->num_addresses = num_addresses;
> +
> +	/*
> +	 * 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 = chip->flags & AT24_FLAG_IRUGO ? S_IRUGO : S_IRUSR;
> +	at24->bin.attr.owner = THIS_MODULE;
> +	at24->bin.read = at24_bin_read;
> +	at24->bin.size = chip->byte_len;
> +
> +	writable = !(chip->flags & AT24_FLAG_READONLY);
> +	if (writable) {
> +		if (!use_smbus || i2c_check_functionality(client->adapter,
> +				I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
> +
> +			unsigned write_max = 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 > I2C_SMBUS_BLOCK_MAX)
> +				write_max = I2C_SMBUS_BLOCK_MAX;
> +			at24->write_max = write_max;
> +
> +			/* buffer (data + address at the beginning) */
> +			at24->writebuf = kmalloc(write_max + 2, GFP_KERNEL);
> +			if (!at24->writebuf) {
> +				err = -ENOMEM;
> +				goto err_struct;
> +			}
> +		} else {
> +			dev_warn(&client->dev,
> +				"cannot write due to controller restrictions.");
> +		}
> +	}
> +
> +	at24->client[0] = client;
> +
> +	/* use dummy devices for multiple-address chips */
> +	for (i = 1; i < num_addresses; i++) {
> +		at24->client[i] = i2c_new_dummy(client->adapter,
> +					client->addr + i);
> +		if (!at24->client[i]) {
> +			dev_err(&client->dev, "address 0x%04x unavailable\n",

These are 7-bit addresses, so: 0x%02x.

> +					client->addr + i);
> +			err = -EADDRINUSE;
> +			goto err_clients;
> +		}
> +	}
> +
> +	err = sysfs_create_bin_file(&client->dev.kobj, &at24->bin);
> +	if (err)
> +		goto err_clients;
> +
> +	i2c_set_clientdata(client, at24);
> +
> +	dev_info(&client->dev, "%Zd byte %s EEPROM %s\n",
> +		at24->bin.size, client->name,
> +		writable ? "(writable)" : "(read-only)");
> +	dev_dbg(&client->dev,
> +		"page_size %d, num_addresses %d, write_max %d%s\n",
> +		chip->page_size, num_addresses,
> +		at24->write_max,
> +		use_smbus ? ", use_smbus" : "");
> +
> +	if (id->driver_data)
> +		 kfree(chip);
> +	return 0;
> +
> +err_clients:
> +	for (i = 1; i < num_addresses; i++)
> +		if (at24->client[i])
> +			i2c_unregister_device(at24->client[i]);
> +
> +	kfree(at24->writebuf);
> +err_struct:
> +	kfree(at24);
> +err_chip:
> +	if (id->driver_data)
> +		kfree(chip);
> +err_out:
> +	dev_dbg(&client->dev, "probe error %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);
> +
> +	for (i = 1; i < at24->num_addresses; i++)
> +		if (at24->client[i])

This test will always be true (otherwise the device probe would have
failed.)

> +			i2c_unregister_device(at24->client[i]);
> +
> +	kfree(at24->writebuf);
> +	kfree(at24);
> +	i2c_set_clientdata(client, NULL);
> +	return 0;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static const struct i2c_device_id at24_ids[] = {
> +	{ "24c00", AT24_DEVICE_MAGIC(AT24_DATA_24C00) },
> +	{ "24c01", AT24_DEVICE_MAGIC(AT24_DATA_24C01) },
> +	{ "24c02", AT24_DEVICE_MAGIC(AT24_DATA_24C02) },
> +	{ "spd", AT24_DEVICE_MAGIC(AT24_DATA_SPD) },
> +	{ "pcf8570", AT24_DEVICE_MAGIC(AT24_DATA_PCF8570) },
> +	{ "24c04", AT24_DEVICE_MAGIC(AT24_DATA_24C04) },
> +	{ "24c08", AT24_DEVICE_MAGIC(AT24_DATA_24C08) },
> +	{ "24c16", AT24_DEVICE_MAGIC(AT24_DATA_24C16) },
> +	{ "24c32", AT24_DEVICE_MAGIC(AT24_DATA_24C32) },
> +	{ "24c64", AT24_DEVICE_MAGIC(AT24_DATA_24C64) },
> +	{ "24c128", AT24_DEVICE_MAGIC(AT24_DATA_24C128) },
> +	{ "24c256", AT24_DEVICE_MAGIC(AT24_DATA_24C256) },
> +	{ "24c512", AT24_DEVICE_MAGIC(AT24_DATA_24C512) },
> +	{ "24c1024", AT24_DEVICE_MAGIC(AT24_DATA_24C1024) },
> +	{ "at24", 0 },
> +	{ /* END OF LIST */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, at24_ids);
> +
> +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 and Wolfram Sang");
> +MODULE_LICENSE("GPL");
> Index: linux-2.6.26-rc4/include/linux/i2c/at24.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.26-rc4/include/linux/i2c/at24.h
> @@ -0,0 +1,104 @@
> +#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 much memory is available (usually specified in bit)?
> + * - What write page size does it support?
> + * - Special flags (16 bit addresses, read_only, world readable)?
> + *
> + * If you 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! A typical custom type declaration would look similar to this:
> + *
> + *	struct const at24_platform_data my_eeprom_data {
> + *		AT24_PLATFORM_DATA(byte_len, page_size, flags);
> + *	};

Actually, a typical declaration would use actual numbers and flags.

> + *
> + */
> +struct at24_platform_data {
> +	u32		byte_len;		/* size (sum of all addr) */
> +	u16		page_size;		/* for writes */
> +	u8		flags;
> +#define AT24_FLAG_ADDR16	0x80
> +#define AT24_FLAG_READONLY	0x40
> +#define AT24_FLAG_IRUGO		0x20
> +#define AT24_FLAG_24C00		0x10
> +};
> +
> +#define AT24_SIZE_BYTELEN 5
> +#define AT24_SIZE_PAGESIZE 4
> +#define AT24_SIZE_FLAGS 8

These values (and all macros related to them) should be internal to the
at24 driver. There's no reason why anybody else would need to know
about them, and you may want to change the encoding at some point in
the future, for example if you add flags or support for larger EEPROMs.

> +#define AT24_BITMASK(x) ((1UL << x) - 1)

No reason to export this either.

> +/* nest macros to enforce expansion of macros containing parameters */
> +#define AT24_PLATFORM_DATA(x) _AT24_PLATFORM_DATA(x)
> +
> +#define _AT24_PLATFORM_DATA(_len, _page, _flags) \
> +	.byte_len = (_len), .page_size = (_page), .flags = (_flags)

I see little point in this macro (let alone the nesting...) We usually
define such macros only for widely used structures living in arrays
most of the time. That's not the case here. Using such macros has a
risk for the user to put the parameters in the wrong order, and you
really don't want this to happen here.

> +
> +/* create non-zero magic value for given eeprom parameters */
> +#define AT24_DEVICE_MAGIC(x) _AT24_DEVICE_MAGIC(x)
> +
> +#define _AT24_DEVICE_MAGIC(_len, _page, _flags) 	\
> +	(((1 << AT24_SIZE_FLAGS | _flags) 		\

Missing parentheses around _flags.

> +		<< AT24_SIZE_PAGESIZE | ilog2(_page)) 	\
> +		<< AT24_SIZE_BYTELEN | ilog2(_len))
> +
> +/*
> + * Chip data. Parameters are byte_len, page_size and flags
> + */
> +
> +/* 128 bit chip, I2C A0-A2 ignored */
> +#define AT24_DATA_24C00 128 / 8, 1, AT24_FLAG_24C00
> +
> +/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
> +#define AT24_DATA_24C01 1024 / 8, 8, 0

The Atmel 24C01 datasheet says page size is 4 bytes, and the Microchip
24C01A datasheet says 2 bytes. So defaulting to 8 doesn't look safe.

> +
> +/* 2 Kbit chip, some have 16 byte pages: */
> +#define AT24_DATA_24C02 2048 / 8, 8, 0

Microchip says 2-byte pages.

> +
> +/* 2 Kbit chip, 24c02 in memory DIMMs, some have 16 byte pages */
> +#define AT24_DATA_SPD 2048 / 8, 8, AT24_FLAG_READONLY | AT24_FLAG_IRUGO
> +
> +/* 2 Kbit chip, SRAM, not EEPROM!, no page size issues, write it all at once */
> +#define AT24_DATA_PCF8570 2048 / 8, 2048 / 8, 0
> +
> +/* 4 Kbit chip, I2C A0 is MEM A8 */
> +#define AT24_DATA_24C04 4096 / 8, 16, 0

Microchip says 8-byte pages.

> +
> +/*
> + * 8 Kbit chip, I2C A1-A0 is MEM A9-A8, works also with 24RF08
> + * (its quirk is handled at i2c-core-level)
> + */
> +#define AT24_DATA_24C08 8192 / 8, 16, 0
> +
> +/* 16 Kbit chip, I2C A2-A0 is MEM A10-A8 */
> +#define AT24_DATA_24C16 16384 / 8, 16, 0
> +
> +/* this second block of EEPROMs uses 16 bit memory addressing */
> +
> +/* 32 Kbits */
> +#define AT24_DATA_24C32 32768 / 8, 32, AT24_FLAG_ADDR16
> +
> +/* 64 Kbits */
> +#define AT24_DATA_24C64 65536 / 8, 32, AT24_FLAG_ADDR16
> +
> +/* 128 Kbits */
> +#define AT24_DATA_24C128 131072 / 8, 64, AT24_FLAG_ADDR16
> +
> +/* 256 Kbits */
> +#define AT24_DATA_24C256 262144 / 8, 64, AT24_FLAG_ADDR16
> +
> +/* 512 Kbits */
> +#define AT24_DATA_24C512 524288 / 8, 128, AT24_FLAG_ADDR16
> +
> +/* 1 Mbits, I2C A0 is MEM A16 */
> +#define AT24_DATA_24C1024 1048576 / 8, 256, AT24_FLAG_ADDR16

You're going to quite some extent to obfuscate simple things ;) All
these defines are for the sole internal purpose of the at24 driver
(custom eeprom types would use platform data instead) and should
not be in the public header file... if they should defined at all.
There's only one place where you use them (the id_table of the driver)
and I think it would much clearer if we would see the actual parameters
there, instead of having to search for the macro definitions. This
would also remove the need of macro nesting as I understand it.

(FWIW, checkpatch complains loudly about the defines above.)

In the end, the only things that must go in at24.h are the definition
of struct at24_platform_data and its flags. All the rest is internal to
the driver and should go in at24.c.

> +
> +#endif /* _LINUX_AT24_H */

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

And some more nit-picking for the road...

On Thu, 5 Jun 2008 21:31:03 +0200, Wolfram Sang wrote:
> --- /dev/null
> +++ linux-2.6.26-rc4/include/linux/i2c/at24.h
> @@ -0,0 +1,104 @@
> (...)
> + * If you set up a custom eeprom type, please make sure you
> + * have double-checked the parameters. Especially page_size needs extra care,

"Make sure" and "double-checked" are somewhat redundant.

> + * as you risk data loss if your value is bigger than what the chip actually
> + * supports! A typical custom type declaration would look similar to this:
> + *
> + *	struct const at24_platform_data my_eeprom_data {

const struct, in this order.

> + *		AT24_PLATFORM_DATA(byte_len, page_size, flags);
> + *	};

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Wolfram Sang]

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

Hi Jean,

thanks again for your review!

> As said before, I don't think it's needed, but if you and David insist
> on picking all addresses of the 24C00, so be it.

A new issue is now that some 24C01 also have this behaviour, so I guess the
flag should be renamed from AT24_FLAG_24C00 to AT24_FLAG_TAKE8ADDR or
something.

> > +/*-------------------------------------------------------------------------*/
> > +
> > +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, num_addresses;
> > +	kernel_ulong_t magic;
> > +
> > +	if (id->driver_data) {
> > +		chip = kmalloc(sizeof(struct at24_platform_data), GFP_KERNEL);
> > +		if (!chip) {
>
> You allocate this just to copy it to another structure and free it
> immediately. Allocating the other structure (struct at24_data) earlier
> would save this temporary allocation. Alternatively, just put the struct
> at24_platform_data on the stack, it's small enough to do so.

I can't allocate at24_data beforehand, as I need to know num_addresses,
so I can then calculate the actual size of at24_data. Will use the stack.

> > +		if (!at24->client[i]) {
> > +			dev_err(&client->dev, "address 0x%04x unavailable\n",
> 
> These are 7-bit addresses, so: 0x%02x.

I was unsure because of the 10-bit-addresses (and %03x looked even more
unusual). Will revert to %02x.

> > +/*
> > + * Chip data. Parameters are byte_len, page_size and flags
> > + */
> > +
> > +/* 128 bit chip, I2C A0-A2 ignored */
> > +#define AT24_DATA_24C00 128 / 8, 1, AT24_FLAG_24C00
> > +
> > +/* 1 Kbit chip, some have 16 byte pages: 24lc014, ... */
> > +#define AT24_DATA_24C01 1024 / 8, 8, 0
> 
> The Atmel 24C01 datasheet says page size is 4 bytes, and the Microchip
> 24C01A datasheet says 2 bytes. So defaulting to 8 doesn't look safe.

Will go back to 2 because of Microchip. But 24C01 seems to have lots of
variants, which makes a generic entry difficult. Some would need
AT24_FLAG_24C00 (doesn't really matter), and AT24C01 needs 128
addresses?? (please, someone, prove me wrong)

> You're going to quite some extent to obfuscate simple things ;) All

I wanted to make the transition from at24_v2 to at24_v3 easier by
reusing the defines containing chip data. I agree though, that the
benefit is mainly for those who already dared to use this driver (is
there anyone besides David and me?), and not for upstream. Will try to
make it more simple.

> In the end, the only things that must go in at24.h are the definition
> of struct at24_platform_data and its flags. All the rest is internal to
> the driver and should go in at24.c.

I wanted to have the AT24_SIZE_* flags next to the struct, so I won't
forget to change their size if anything inside the struct will change.
Then again, I might work with sizeof here; the result will probably look
a bit nasty, too...

All the best,

   Wolfram

-- 
  Dipl.-Ing. Wolfralfum 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 v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

On Tue, 10 Jun 2008 15:43:47 +0200, Wolfram Sang wrote:
> > The Atmel 24C01 datasheet says page size is 4 bytes, and the Microchip
> > 24C01A datasheet says 2 bytes. So defaulting to 8 doesn't look safe.
> 
> Will go back to 2 because of Microchip. But 24C01 seems to have lots of
> variants, which makes a generic entry difficult. Some would need

Feel free to not make a generic entry at all if you think it's not
worth it.

> AT24_FLAG_24C00 (doesn't really matter), and AT24C01 needs 128
> addresses?? (please, someone, prove me wrong)

Why do you think so? My personal guess is that they simply forgot to
mention the address in the datasheet. A chip responding to all
addresses would prevent any other chip from being connected to the bus,
that's impractical enough to be reasonably certain that no manufacturer
did this.

> > In the end, the only things that must go in at24.h are the definition
> > of struct at24_platform_data and its flags. All the rest is internal to
> > the driver and should go in at24.c.
> 
> I wanted to have the AT24_SIZE_* flags next to the struct, so I won't
> forget to change their size if anything inside the struct will change.
> Then again, I might work with sizeof here; the result will probably look
> a bit nasty, too...

I see, it makes some sense to keep these flags around then. But then
please add a warning that these are for the driver internal use only
and shouldn't be considered stable.

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[David Brownell]

On Sunday 08 June 2008, Jean Delvare wrote:
> You need to include <linux/bitops.h> for BIT().

That's handled by <linux/kernel.h> ... I rather think it's OK
to rely on a few basics like that.


> You're going to quite some extent to obfuscate simple things ;) All
> these defines are for the sole internal purpose of the at24 driver
> (custom eeprom types would use platform data instead) and should
> not be in the public header file... if they should defined at all.

The original notion was to get the driver out of the business of
holding a large table of device parameters including worst-case
pagesizes (e.g. Microchip pages being half or a quarter the size
of Atmel pages) and address consumption (e.g. Atmel 24c01 vs 24c01a,
or the SOT23 versions doing who-knows-undocumented-what).

So I think those #defines are somewhat a legacy of having had to
change direction mid-steam to cope with the new "i2c_device_id"
and its expectation that drivers *would* have such large tables
with worst-case parameters.

Just so you know.  :)

- Dave
 

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

Hi David,

On Tue, 10 Jun 2008 14:02:44 -0700, David Brownell wrote:
> On Sunday 08 June 2008, Jean Delvare wrote:
> > You're going to quite some extent to obfuscate simple things ;) All
> > these defines are for the sole internal purpose of the at24 driver
> > (custom eeprom types would use platform data instead) and should
> > not be in the public header file... if they should defined at all.
> 
> The original notion was to get the driver out of the business of
> holding a large table of device parameters including worst-case
> pagesizes (e.g. Microchip pages being half or a quarter the size
> of Atmel pages) and address consumption (e.g. Atmel 24c01 vs 24c01a,
> or the SOT23 versions doing who-knows-undocumented-what).
> 
> So I think those #defines are somewhat a legacy of having had to
> change direction mid-steam to cope with the new "i2c_device_id"
> and its expectation that drivers *would* have such large tables
> with worst-case parameters.
> 
> Just so you know.  :)

I understand the history behind the code.

Having only dealt with read-only EEPROMs so far, I wasn't aware that
different incarnations of otherwise compatible EEPROMs could have
different page sizes. This is indeed a problem. If you think that this
makes default types useless, then I am fine not having such default
types (meaning that platform data would be mandatory, except for SPD
EEPROMs.) But then you wouldn't include arbitrary example platform data
structures either, contrary to what your original proposal did.

That being said... If almost compatible EEPROMs can be used in a
hardware design, differing only by the write page size, can the
platform code author, in practice, really assume a page size greater
than the minimum? That would assume the same specific incarnation of the
EEPROM is always used. Is is common to have such a certainty? I think
that our decision whether to have default definitions in the driver,
depends on the answer to this question (and you'll know better than me.)

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Wolfram Sang]

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

> > AT24_FLAG_24C00 (doesn't really matter), and AT24C01 needs 128
> > addresses?? (please, someone, prove me wrong)
> 
> Why do you think so? My personal guess is that they simply forgot to
> mention the address in the datasheet. A chip responding to all
> addresses would prevent any other chip from being connected to the bus,
> that's impractical enough to be reasonably certain that no manufacturer
> did this.

No I2C-address is mentioned in the whole datasheet; all the timing
diagrams put the memory offset to the place where one would expect the
I2C-address; the pins usually named A0-A2 are not connected...

Maybe it once made the chip one cent cheaper, I dunno, but I wouldn't be
that surprised :) Doesn't really matter for at24, luckily.

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 v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

On Wed, 11 Jun 2008 09:33:25 +0200, Wolfram Sang wrote:
> > > AT24_FLAG_24C00 (doesn't really matter), and AT24C01 needs 128
> > > addresses?? (please, someone, prove me wrong)
> > 
> > Why do you think so? My personal guess is that they simply forgot to
> > mention the address in the datasheet. A chip responding to all
> > addresses would prevent any other chip from being connected to the bus,
> > that's impractical enough to be reasonably certain that no manufacturer
> > did this.
> 
> No I2C-address is mentioned in the whole datasheet; all the timing
> diagrams put the memory offset to the place where one would expect the
> I2C-address; the pins usually named A0-A2 are not connected...

Ooch. I didn't notice the timing diagrams... OMG.

> Maybe it once made the chip one cent cheaper, I dunno, but I wouldn't be
> that surprised :) Doesn't really matter for at24, luckily.

Indeed. If this chip is the crazy thing you suspect (and I now fear you
are correct), it's not compatible with the other EEPROMs, so we don't
really care about it.

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[David Brownell]

On Wednesday 11 June 2008, Jean Delvare wrote:
> On Wed, 11 Jun 2008 09:33:25 +0200, Wolfram Sang wrote:
> > > > AT24_FLAG_24C00 (doesn't really matter), and AT24C01 needs 128
> > > > addresses?? (please, someone, prove me wrong)
> > > 
> > > Why do you think so? ...
> > 
> > No I2C-address is mentioned in the whole datasheet; all the timing
> > diagrams put the memory offset to the place where one would expect the
> > I2C-address; the pins usually named A0-A2 are not connected...
>
> Ooch. I didn't notice the timing diagrams... OMG.

You mean, the obsolete 24c01, rather than the 24c01b or newer?


> > Maybe it once made the chip one cent cheaper, I dunno, but I wouldn't be
> > that surprised :) Doesn't really matter for at24, luckily.
> 
> Indeed. If this chip is the crazy thing you suspect (and I now fear you
> are correct), it's not compatible with the other EEPROMs, so we don't
> really care about it.

Right ... worth a comment in the chip ID table that 24c01b (and newer)
entries really mean that, since the original 24c01 parts were broken by
design (no on-wire chip address byte).

- Dave

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[David Brownell]

On Wednesday 11 June 2008, Jean Delvare wrote:
> Having only dealt with read-only EEPROMs so far, I wasn't aware that
> different incarnations of otherwise compatible EEPROMs could have
> different page sizes. This is indeed a problem.

In terms of getting the best bulk write performance, yes;
it's easily worth a factor of two or four.  I don't think
that will matter on all systems though.


> If you think that this 
> makes default types useless, then I am fine not having such default
> types (meaning that platform data would be mandatory, except for SPD
> EEPROMs.) 

By "default types" presumably you mean what I meant when I said
"least common denominator" settings?  Well, no -- not useless.

But it does ensure that some systems will want to provide more
specific settings.  The read-only flag has the same issue:  some
systems won't want to make it too easy for end-users to clobber
calibrations recorded during manufacturing.


> But then you wouldn't include arbitrary example platform data 
> structures either, contrary to what your original proposal did.

Which version of "original" do you mean?  :)

The idea you refer to was a simplification:  the driver wouldn't
need to maintain lots of chip tables *plus* provide a per-chip
override mechanism.  There'd be only platform_data, in all cases.

The easy way would be to reference a predefined struct, which
could easily be cloned and modified to address pagesize and
write protection issues.  (Also the various EEPROMs that were
not listed in the current tables.)

The switch to use i2c_device_id seems to throw a monkey wrench
in that plan.  Though I suppose it doesn't need to, except for
the powerpc model whereby *only* client->name is available to
distinguish parts, and platform_data is in the "not mentioned
in polite society" category.  ;)

 
> That being said... If almost compatible EEPROMs can be used in a 
> hardware design, differing only by the write page size, can the
> platform code author, in practice, really assume a page size greater
> than the minimum? That would assume the same specific incarnation of the
> EEPROM is always used. Is is common to have such a certainty?

In my observation, yes.  I don't happen to have observed anyone
swapping one vendor's part for another vendor's, even in newer
board revisions.  A bill-of-materials rarely changes much.

But that's more of a manufacturing issue than a software issue,
and I can easily *imagine* boards that are handled differently.

Some boards are designed to allow certain part substitutions,
and in theory that could be done for EEPROMs as well as flash
or RAM.  Or for discretes (resistors, capacitors, inductors,
diodes, FETs, etc).  Such capabilities should be designed in,
so a platform code author would know what the options are.


> I think 
> that our decision whether to have default definitions in the driver,
> depends on the answer to this question (and you'll know better than me.)

I don't quite follow your decision tree there.  Elaborate please?

- Dave



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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

Hi David,

On Wed, 11 Jun 2008 10:25:08 -0700, David Brownell wrote:
> On Wednesday 11 June 2008, Jean Delvare wrote:
> > But then you wouldn't include arbitrary example platform data 
> > structures either, contrary to what your original proposal did.
> 
> Which version of "original" do you mean?  :)

I was thinking of
http://lists.lm-sensors.org/pipermail/i2c/2008-April/003307.html
but now I realize that it had already changes from Wolfram, so I was
confused, sorry.

> The idea you refer to was a simplification:  the driver wouldn't
> need to maintain lots of chip tables *plus* provide a per-chip
> override mechanism.  There'd be only platform_data, in all cases.

Such devices can never be created from user-space then (or maybe using
configfs? I really need to look into that). The nice thing about i2c
device names is that it makes devices easy to instantiate.

> The easy way would be to reference a predefined struct, which
> could easily be cloned and modified to address pagesize and
> write protection issues.  (Also the various EEPROMs that were
> not listed in the current tables.)

That's what I would like to avoid. I think that providing predefined
structures is dangerous. You have to look at two places to know what
you have exactly. If platform data is provided then I would like all of
it to be spelled out explicitly by the caller. Otherwise I expect some
confusion.

> The switch to use i2c_device_id seems to throw a monkey wrench
> in that plan.  Though I suppose it doesn't need to, except for
> the powerpc model whereby *only* client->name is available to
> distinguish parts, and platform_data is in the "not mentioned
> in polite society" category.  ;)

The switch to i2c_device_id has little to do here. We already had i2c
device names acting as sub-types. All it did was moving the name string
comparison from individual drivers to i2c-core.

> > I think 
> > that our decision whether to have default definitions in the driver,
> > depends on the answer to this question (and you'll know better than me.)
> 
> I don't quite follow your decision tree there.  Elaborate please?

My point is: if EEPROM parts can be swapped easily and manufacturers
do that, then platform code has to play it safe and always use the
smallest possible page size. That is, do exactly what plan on doing with
i2c_device_id entries. So it makes sense to have these entries
(platform code will use them.)

OTOH, if EEPROM parts are usually not swappable and the platform code
authors really knows what is there, then they will want to optimize the
platform data to use the best possible page size, meaning that the
i2c_device_id entries will, in general, not be used, and thus might not
be worth even defining.

An alternative approach would be to make all i2c_device_id entries
read-only EEPROMs (so the page size no longer matters) and anyone who
wants write access has to provide custom platform data.

As I wrote above, I never had to deal with writable EEPROMs myself, so
I'm really not the right person to make the decision. Leaving it up to
you and Wolfram.

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[David Brownell]

On Wednesday 11 June 2008, Jean Delvare wrote:
> > The idea you refer to was a simplification:  the driver wouldn't
> > need to maintain lots of chip tables *plus* provide a per-chip
> > override mechanism.  There'd be only platform_data, in all cases.
> 
> Such devices can never be created from user-space then (or maybe using
> configfs? I really need to look into that). The nice thing about i2c
> device names is that it makes devices easy to instantiate.

But they do *NOT* provide enough information to let any given
device connect in non-trivial ways to the rest of the kernel.

Examples include "which IRQ does it use", callbacks that may
be needed to configure things that implicitly rely on that
I2C device, calibration data, and more.

I'm just not a fan of creating devices from userspace, it seems.
The main reason to support such mechanisms is historical.  But
all the I2C systems I work with haven't needed such stuff ...
plus, those legacy configuration schemes required drivers to have
lots of board-specific hacks.  Those hacks went away when we
could finally rely on platform_data.


> > The easy way would be to reference a predefined struct, which
> > could easily be cloned and modified to address pagesize and
> > write protection issues.  (Also the various EEPROMs that were
> > not listed in the current tables.)
> 
> That's what I would like to avoid. I think that providing predefined
> structures is dangerous. You have to look at two places to know what
> you have exactly. If platform data is provided then I would like all of
> it to be spelled out explicitly by the caller. Otherwise I expect some
> confusion.

Thing is, we know developers *WILL* cut'n'paste such stuff.
Many of them won't dig up the data sheets.  Some confusion
is unavoidable ... the issue is how to minimize it.

As I said, my thought there is to make it safe for most
developers to just say "24c32" (or whatever) and have a
sane default ... while making sure that they *always* have
a way to set chips up to be readonly, or provide a better
page size (if they need better bulk write speeds).


> > > I think 
> > > that our decision whether to have default definitions in the driver,
> > > depends on the answer to this question (and you'll know better than me.)
> > 
> > I don't quite follow your decision tree there.  Elaborate please?
> 
> My point is: if EEPROM parts can be swapped easily and manufacturers
> do that, then platform code has to play it safe and always use the
> smallest possible page size. That is, do exactly what plan on doing with
> i2c_device_id entries. So it makes sense to have these entries
> (platform code will use them.)

Smallest size among the parts qualified for that design.  That's
a manufacturing and design question; we don't know whether the
qualified parts will ever include one of the small-page chips.


> OTOH, if EEPROM parts are usually not swappable and the platform code
> authors really knows what is there, then they will want to optimize the
> platform data to use the best possible page size, meaning that the
> i2c_device_id entries will, in general, not be used, and thus might not
> be worth even defining.

Some designs will use one hand, one will use the other.  I can't
see this as an either/or situation myself.  Both examples will fit
some real-world produts.


> An alternative approach would be to make all i2c_device_id entries
> read-only EEPROMs (so the page size no longer matters) and anyone who
> wants write access has to provide custom platform data.

Ugh.  That'd be a PROM driver, not an EEPROM driver.  ;)

 
> As I wrote above, I never had to deal with writable EEPROMs myself, so
> I'm really not the right person to make the decision. Leaving it up to
> you and Wolfram.

I don't know what Wolframe thinks about this ...

- Dave

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

Hi David,

On Thu, 12 Jun 2008 14:17:51 -0700, David Brownell wrote:
> On Wednesday 11 June 2008, Jean Delvare wrote:
> > > The idea you refer to was a simplification:  the driver wouldn't
> > > need to maintain lots of chip tables *plus* provide a per-chip
> > > override mechanism.  There'd be only platform_data, in all cases.
> > 
> > Such devices can never be created from user-space then (or maybe using
> > configfs? I really need to look into that). The nice thing about i2c
> > device names is that it makes devices easy to instantiate.
> 
> But they do *NOT* provide enough information to let any given
> device connect in non-trivial ways to the rest of the kernel.
> 
> Examples include "which IRQ does it use", callbacks that may
> be needed to configure things that implicitly rely on that
> I2C device, calibration data, and more.

The IRQ could be easily provided from user-space, as it's a field in
struct i2c_client. What cannot be easily provided are driver-specific
settings, I agree. But I still have to check if configfs could help
there.

My point was that having a way to instantiate read-only 128 or 256-byte
EEPROMs (as used for EDID and SPD, respectively) from user-space would
be valuable. And having an i2c_device_id entry for that, should help.
But I agree that, as long as the mechanism to actually instantiate I2C
devices from user-space isn't implemented, any discussion about how
drivers must be designed to support it, is moot. Let's postpone it to
later.

> I'm just not a fan of creating devices from userspace, it seems.
> The main reason to support such mechanisms is historical.  But

Such mechanisms existed because they were useful, and they still are.
You can't at the same time dislike automatic probing of I2C devices in
the kernel, and say that a way to instantiate I2C devices from
user-space is not needed. We need at least one of these mechanisms
(and, for the time being, the two of them.)

> all the I2C systems I work with haven't needed such stuff ...
> plus, those legacy configuration schemes required drivers to have
> lots of board-specific hacks.  Those hacks went away when we
> could finally rely on platform_data.

For the cases I have at hand (hardware monitoring devices, eeproms...)
there is no board-specific hacks involved at all.

> > > The easy way would be to reference a predefined struct, which
> > > could easily be cloned and modified to address pagesize and
> > > write protection issues.  (Also the various EEPROMs that were
> > > not listed in the current tables.)
> > 
> > That's what I would like to avoid. I think that providing predefined
> > structures is dangerous. You have to look at two places to know what
> > you have exactly. If platform data is provided then I would like all of
> > it to be spelled out explicitly by the caller. Otherwise I expect some
> > confusion.
> 
> Thing is, we know developers *WILL* cut'n'paste such stuff.
> Many of them won't dig up the data sheets.  Some confusion
> is unavoidable ... the issue is how to minimize it.
> 
> As I said, my thought there is to make it safe for most
> developers to just say "24c32" (or whatever) and have a
> sane default ... while making sure that they *always* have
> a way to set chips up to be readonly, or provide a better
> page size (if they need better bulk write speeds).

I agree on the principle. But the question remains: what is a sane
default in this context? Using the smallest page size amongst commonly
used EEPROM models? Using the smallest page size amongst known models?
Using the smallest possible page size (that would be 1 byte for all
EEPROM sizes as I understand it)? This 3rd possibility would probably
be no better than defaulting to read-only, as the write performance
would be so bad that every developer would specify custom platform data.

> > (...)
> > An alternative approach would be to make all i2c_device_id entries
> > read-only EEPROMs (so the page size no longer matters) and anyone who
> > wants write access has to provide custom platform data.
> 
> Ugh.  That'd be a PROM driver, not an EEPROM driver.  ;)

With what I wrote above, this still sounds like a sane thing to do,
though. Plus, if the default is read-only, developers who need write
access will have to pay some attention to the page size they set.
Hopefully this should minimize the risk of them accidentally using the
wrong page size.

-- 
Jean Delvare

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

Re: [PATCH v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Wolfram Sang]

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

Hi Jean,

hopefully, I now have the time to finish at24 for good. Do you think it
can go through the next merge window?

> > As I said, my thought there is to make it safe for most
> > developers to just say "24c32" (or whatever) and have a
> > sane default ... while making sure that they *always* have
> > a way to set chips up to be readonly, or provide a better
> > page size (if they need better bulk write speeds).
> 
> I agree on the principle. But the question remains: what is a sane
> default in this context? Using the smallest page size amongst commonly
> used EEPROM models? Using the smallest page size amongst known models?
> Using the smallest possible page size (that would be 1 byte for all
> EEPROM sizes as I understand it)? This 3rd possibility would probably
> be no better than defaulting to read-only, as the write performance
> would be so bad that every developer would specify custom platform data.

Hmm, I tend to set all page_sizes to 1. It is a bit awkward to write
"double check those parameters" and then provide values you cannot be
absolutely sure of. Still, a minimal write-possibility may come in
handy, I think. I agree with Jean here, that it will be so slow, that
developers surely will create their own platform_devices.

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 v3] New-style I2C and SMBus EEPROM driver (with device_ids)

[Jean Delvare]

Hi Wolfram,

On Tue, 1 Jul 2008 15:20:51 +0200, Wolfram Sang wrote:
> hopefully, I now have the time to finish at24 for good. Do you think it
> can go through the next merge window?

Yes, it can and should.

> > > As I said, my thought there is to make it safe for most
> > > developers to just say "24c32" (or whatever) and have a
> > > sane default ... while making sure that they *always* have
> > > a way to set chips up to be readonly, or provide a better
> > > page size (if they need better bulk write speeds).
> > 
> > I agree on the principle. But the question remains: what is a sane
> > default in this context? Using the smallest page size amongst commonly
> > used EEPROM models? Using the smallest page size amongst known models?
> > Using the smallest possible page size (that would be 1 byte for all
> > EEPROM sizes as I understand it)? This 3rd possibility would probably
> > be no better than defaulting to read-only, as the write performance
> > would be so bad that every developer would specify custom platform data.
> 
> Hmm, I tend to set all page_sizes to 1. It is a bit awkward to write
> "double check those parameters" and then provide values you cannot be
> absolutely sure of. Still, a minimal write-possibility may come in
> handy, I think. I agree with Jean here, that it will be so slow, that
> developers surely will create their own platform_devices.

-- 
Jean Delvare

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