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

[Wolfram Sang]

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+***@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 */

[Jean Delvare]

Hi Wolfram,

Post by Wolfram Sang
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.
---
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.
- 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.

Post by Wolfram Sang
- 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
- 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
- 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.

Post by Wolfram Sang
+ 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
+
+ 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.

Post by Wolfram Sang
+ 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...

Post by Wolfram Sang
+
+ 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
+ * 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);
+ 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.

Post by Wolfram Sang
+ 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);
+ 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;
+ }
+ 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.

Post by Wolfram Sang
+ 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().

Post by Wolfram Sang
+ 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.

Post by Wolfram Sang
+
+ 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.

Post by Wolfram Sang
+ 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;
+
+ for (i = 1; i < num_addresses; i++)
+ if (at24->client[i])
+ i2c_unregister_device(at24->client[i]);
+
+ kfree(at24->writebuf);
+ kfree(at24);
+ if (id->driver_data)
+ kfree(chip);
+ 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.)

Post by Wolfram Sang
+ 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
+ * - 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
+ *
+ * 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.

Post by Wolfram Sang
+ *
+ */
+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.

Post by Wolfram Sang
+#define AT24_BITMASK(x) ((1UL << x) - 1)

No reason to export this either.

Post by Wolfram Sang
+/* 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.

Post by Wolfram Sang
+
+/* 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.

Post by Wolfram Sang
+ << 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.

Post by Wolfram Sang
+
+/* 2 Kbit chip, some have 16 byte pages: */
+#define AT24_DATA_24C02 2048 / 8, 8, 0

Microchip says 2-byte pages.

Post by Wolfram Sang
+
+/* 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.

Post by Wolfram Sang
+
+/*
+ * 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.

Post by Wolfram Sang
+
+#endif /* _LINUX_AT24_H */

[Wolfram Sang]

Hi Jean,

thanks again for your review!

Post by Jean Delvare
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.

Post by Jean Delvare

Post by Wolfram Sang
+/*-------------------------------------------------------------------------*/
+
+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.

Post by Jean Delvare

Post by Wolfram Sang
+ 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.

Post by Jean Delvare

Post by Wolfram Sang
+/*
+ * 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)

Post by Jean Delvare
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.

Post by Jean Delvare
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

[Jean Delvare]

Post by Wolfram Sang

Post by Jean Delvare
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.

Post by Wolfram Sang
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.

Post by Wolfram Sang

Post by Jean Delvare
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.

[Wolfram Sang]

Post by Jean Delvare

Post by Wolfram Sang
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

[Jean Delvare]

Post by Wolfram Sang

Post by Jean Delvare

Post by Wolfram Sang
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.

Post by Wolfram Sang
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.

[David Brownell]

Post by Jean Delvare

Post by Wolfram Sang

Post by Wolfram Sang
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?

Post by Jean Delvare

Post by Wolfram Sang
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

[David Brownell]

Post by Jean Delvare
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.

Post by Jean Delvare
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

[Jean Delvare]

Hi David,

Post by David Brownell

Post by Jean Delvare
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.)

[David Brownell]

Post by Jean Delvare
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.

Post by Jean Delvare
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.

Post by Jean Delvare
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. ;)

Post by Jean Delvare
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.

Post by Jean Delvare
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

[Jean Delvare]

Hi David,

Post by David Brownell

Post by Jean Delvare
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.

Post by David Brownell
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.

Post by David Brownell
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.

Post by David Brownell
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.

Post by David Brownell

Post by Jean Delvare
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.

[David Brownell]

Post by Jean Delvare

Post by David Brownell
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.

Post by Jean Delvare

Post by David Brownell
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).

Post by Jean Delvare

Post by David Brownell

Post by Jean Delvare
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.

Post by Jean Delvare
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.

Post by Jean Delvare
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. ;)

Post by Jean Delvare
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

[Jean Delvare]

Hi David,

Post by David Brownell

Post by Jean Delvare

Post by David Brownell
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.

Post by David Brownell
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.)

Post by David Brownell
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.

Post by David Brownell

Post by Jean Delvare

Post by David Brownell
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.

Post by David Brownell

Post by Jean Delvare
(...)
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.

[Wolfram Sang]

Hi Jean,

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

Post by Jean Delvare

Post by David Brownell
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

[Jean Delvare]

Hi Wolfram,

Post by Wolfram Sang
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.

Post by Wolfram Sang

Post by Jean Delvare

Post by David Brownell
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]

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

Post by Wolfram Sang
--- /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.

Post by Wolfram Sang
+ * as you risk data loss if your value is bigger than what the chip actually
+ *
+ * struct const at24_platform_data my_eeprom_data {

const struct, in this order.

Post by Wolfram Sang
+ * AT24_PLATFORM_DATA(byte_len, page_size, flags);
+ * };