This patch adds an interface by which other kernel code can read/write
persistent memory such as i2c or SPI EEPROMs.
In additon, the at24 EEPROM driver is updated to use this interface.
In the case of at24 EEPROM, the platform code registers a 'setup'
callback with the at24_platform_data. When the at24 driver detects an
EEPROM, it fills out the read and write functions of the
memory_accessor and calls the setup callback passing the
memory_accessor struct. The platform code can then use the read/write
functions in the memory_accessor struct for reading and writing the
EEPROM.
Original idea, review and updates by David Brownell <[email protected]>
Cc: David Brownell <[email protected]>
Signed-off-by: Kevin Hilman <[email protected]>
---
Applies against v2.6.29-rc7
drivers/misc/eeprom/at24.c | 42 +++++++++++++++++++++++++++++++++++-------
include/linux/i2c/at24.h | 4 ++++
include/linux/memory.h | 11 +++++++++++
3 files changed, 50 insertions(+), 7 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index d477552..80bf7ab 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -53,6 +53,7 @@
struct at24_data {
struct at24_platform_data chip;
+ struct memory_accessor macc;
bool use_smbus;
/*
@@ -264,13 +265,6 @@ static ssize_t at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
/*
- * 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.
@@ -386,6 +380,30 @@ static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
/*-------------------------------------------------------------------------*/
+/*
+ * This lets other kernel code access the eeprom data. For example, it
+ * might hold a board's Ethernet address, or board-specific calibration
+ * data generated on the manufacturing floor.
+ */
+
+static ssize_t at24_read(struct memory_accessor *macc, char *buf,
+ off_t offset, size_t count)
+{
+ struct at24_data *at24 = container_of(macc, struct at24_data, macc);
+
+ return at24_eeprom_read(at24, buf, offset, count);
+}
+
+static ssize_t at24_write(struct memory_accessor *macc, char *buf,
+ off_t offset, size_t count)
+{
+ struct at24_data *at24 = container_of(macc, struct at24_data, macc);
+
+ return at24_eeprom_write(at24, buf, offset, count);
+}
+
+/*-------------------------------------------------------------------------*/
+
static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
{
struct at24_platform_data chip;
@@ -413,6 +431,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
* is recommended anyhow.
*/
chip.page_size = 1;
+
+ chip.setup = NULL;
+ chip.context = NULL;
}
if (!is_power_of_2(chip.byte_len))
@@ -449,6 +470,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
goto err_out;
}
+ at24->macc.read = at24_read;
+ at24->macc.write = at24_write;
+
mutex_init(&at24->lock);
at24->use_smbus = use_smbus;
at24->chip = chip;
@@ -520,6 +544,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->write_max,
use_smbus ? ", use_smbus" : "");
+ /* export data to kernel code */
+ if (chip.setup)
+ chip.setup(&at24->macc, chip.context);
+
return 0;
err_clients:
diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
index f6edd52..85b4f7e 100644
--- a/include/linux/i2c/at24.h
+++ b/include/linux/i2c/at24.h
@@ -2,6 +2,7 @@
#define _LINUX_AT24_H
#include <linux/types.h>
+#include <linux/memory.h>
/*
* As seen through Linux I2C, differences between the most common types of I2C
@@ -23,6 +24,9 @@ struct at24_platform_data {
#define AT24_FLAG_READONLY 0x40 /* sysfs-entry will be read-only */
#define AT24_FLAG_IRUGO 0x20 /* sysfs-entry will be world-readable */
#define AT24_FLAG_TAKE8ADDR 0x10 /* take always 8 addresses (24c00) */
+
+ int (*setup)(struct memory_accessor *, void *context);
+ void *context;
};
#endif /* _LINUX_AT24_H */
diff --git a/include/linux/memory.h b/include/linux/memory.h
index 3fdc108..aa97724 100644
--- a/include/linux/memory.h
+++ b/include/linux/memory.h
@@ -99,4 +99,15 @@ enum mem_add_context { BOOT, HOTPLUG };
#define hotplug_memory_notifier(fn, pri) do { } while (0)
#endif
+/*
+ * 'struct memory_accessor' is a generic interface to provide
+ * in-kernel access to persistent memory such as i2c or SPI EEPROMs
+ */
+struct memory_accessor {
+ ssize_t (*read)(struct memory_accessor *, char *buf, off_t offset,
+ size_t count);
+ ssize_t (*write)(struct memory_accessor *, char *buf, off_t offset,
+ size_t count);
+};
+
#endif /* _LINUX_MEMORY_H_ */
--
1.6.1.3
On Thursday 05 March 2009, Kevin Hilman wrote:
> This patch adds an interface by which other kernel code can read/write
> persistent memory such as i2c or SPI EEPROMs.
Use cases include storage of board-specific configuration
data like Ethernet addresses and sensor calibrations, as
you noted in the at24 updates ... probably worth mentioning
that in the interface patch, since that's as far as many
folk will ever read.
The same interface can work with NVRAM, which a lot of
RTC chips provide. Another use case would be supporting
a more platform-agnostic solution for what PM_TRACE_RTC
does for x86 ... and without trashing RTC state and thus
screwing up reboots.
> In additon, the at24 EEPROM driver is updated to use this interface.
>
> In the case of at24 EEPROM, the platform code registers a 'setup'
> callback with the at24_platform_data. When the at24 driver detects an
> EEPROM, it fills out the read and write functions of the
> memory_accessor and calls the setup callback passing the
> memory_accessor struct. The platform code can then use the read/write
> functions in the memory_accessor struct for reading and writing the
> EEPROM.
I don't have any particular issue seeing the interface and
its first implementation in the same patch, but $SUBJECT
should say so if that's what you're doing. :)
Better would be a short set of patches. I'll send an at25
driver update in a moment, which could be the third in that
little series.
> Original idea, review and updates by David Brownell <[email protected]>
>
> Cc: David Brownell <[email protected]>
> Signed-off-by: Kevin Hilman <[email protected]>
I have no particular issues with this interface, but
there are a few ways the at24 part could be improved;
see below. At any rate, for the interface part:
Acked-by: David Brownell <[email protected]>
> ---
> Applies against v2.6.29-rc7
>
> drivers/misc/eeprom/at24.c | 42 +++++++++++++++++++++++++++++++++++-------
> include/linux/i2c/at24.h | 4 ++++
> include/linux/memory.h | 11 +++++++++++
> 3 files changed, 50 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index d477552..80bf7ab 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -53,6 +53,7 @@
>
> struct at24_data {
> struct at24_platform_data chip;
> + struct memory_accessor macc;
> bool use_smbus;
>
> /*
> @@ -264,13 +265,6 @@ static ssize_t at24_bin_read(struct kobject *kobj, struct bin_attribute *attr,
>
>
> /*
> - * 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.
> @@ -386,6 +380,30 @@ static ssize_t at24_bin_write(struct kobject *kobj, struct bin_attribute *attr,
>
> /*-------------------------------------------------------------------------*/
>
> +/*
> + * This lets other kernel code access the eeprom data. For example, it
> + * might hold a board's Ethernet address, or board-specific calibration
> + * data generated on the manufacturing floor.
> + */
> +
> +static ssize_t at24_read(struct memory_accessor *macc, char *buf,
> + off_t offset, size_t count)
> +{
> + struct at24_data *at24 = container_of(macc, struct at24_data, macc);
> +
> + return at24_eeprom_read(at24, buf, offset, count);
The at24_bin_read() routine does two things that this doesn't:
- compensate at24_eeprom_read() possibly returning just a
byte (e.g. on SMBUS-only hardware) by issuing many reads
- range checking the parameters
I suspect both the sysfs and in-kernel accessor should call a
common routine, which handles those issues.
> +}
> +
> +static ssize_t at24_write(struct memory_accessor *macc, char *buf,
> + off_t offset, size_t count)
> +{
> + struct at24_data *at24 = container_of(macc, struct at24_data, macc);
> +
> + return at24_eeprom_write(at24, buf, offset, count);
Same kind of issue here, too. Although for writes there are
some extra reasons for the lower level routine not to handle
the whole buffer: EEPROM page sizes can be a lot smaller
than the data chunks, and data can cross page boundaries.
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> {
> struct at24_platform_data chip;
> @@ -413,6 +431,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> * is recommended anyhow.
> */
> chip.page_size = 1;
> +
> + chip.setup = NULL;
> + chip.context = NULL;
> }
>
> if (!is_power_of_2(chip.byte_len))
> @@ -449,6 +470,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> goto err_out;
> }
>
> + at24->macc.read = at24_read;
> + at24->macc.write = at24_write;
> +
> mutex_init(&at24->lock);
> at24->use_smbus = use_smbus;
> at24->chip = chip;
> @@ -520,6 +544,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
> at24->write_max,
> use_smbus ? ", use_smbus" : "");
>
> + /* export data to kernel code */
> + if (chip.setup)
> + chip.setup(&at24->macc, chip.context);
Since you're not using the return value of setup() ...
> +
> return 0;
>
> err_clients:
> diff --git a/include/linux/i2c/at24.h b/include/linux/i2c/at24.h
> index f6edd52..85b4f7e 100644
> --- a/include/linux/i2c/at24.h
> +++ b/include/linux/i2c/at24.h
> @@ -2,6 +2,7 @@
> #define _LINUX_AT24_H
>
> #include <linux/types.h>
> +#include <linux/memory.h>
>
> /*
> * As seen through Linux I2C, differences between the most common types of I2C
> @@ -23,6 +24,9 @@ struct at24_platform_data {
> #define AT24_FLAG_READONLY 0x40 /* sysfs-entry will be read-only */
> #define AT24_FLAG_IRUGO 0x20 /* sysfs-entry will be world-readable */
> #define AT24_FLAG_TAKE8ADDR 0x10 /* take always 8 addresses (24c00) */
> +
> + int (*setup)(struct memory_accessor *, void *context);
... might as well declare its value as "void".
> + void *context;
> };
>
> #endif /* _LINUX_AT24_H */
> diff --git a/include/linux/memory.h b/include/linux/memory.h
> index 3fdc108..aa97724 100644
> --- a/include/linux/memory.h
> +++ b/include/linux/memory.h
> @@ -99,4 +99,15 @@ enum mem_add_context { BOOT, HOTPLUG };
> #define hotplug_memory_notifier(fn, pri) do { } while (0)
> #endif
>
> +/*
> + * 'struct memory_accessor' is a generic interface to provide
> + * in-kernel access to persistent memory such as i2c or SPI EEPROMs
Capitalize I2C.
> + */
> +struct memory_accessor {
> + ssize_t (*read)(struct memory_accessor *, char *buf, off_t offset,
> + size_t count);
> + ssize_t (*write)(struct memory_accessor *, char *buf, off_t offset,
> + size_t count);
> +};
That interface suits my minimalist tendancies nicely. ;)
Though maybe the write() should take a pointer to const data.
> +
> #endif /* _LINUX_MEMORY_H_ */
> --
> 1.6.1.3
>
>
From: David Brownell <[email protected]>
Implement the new memory_accessor interfaces for SPI EEPROMs:
- Define new setup() hook to export the accessor
- Implement accessor methods
Moves some error checking out of the sysfs interface code
into the layer below it, which is now shared by both sysfs
and memory access code.
Signed-off-by: David Brownell <[email protected]>
---
Can't have this only for I2C ... SPI may need it too. :)
drivers/misc/eeprom/at25.c | 58 ++++++++++++++++++++++++++++++++-----------
include/linux/spi/eeprom.h | 6 ++++
2 files changed, 50 insertions(+), 14 deletions(-)
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -30,6 +30,7 @@
struct at25_data {
struct spi_device *spi;
+ struct memory_accessor mem;
struct mutex lock;
struct spi_eeprom chip;
struct bin_attribute bin;
@@ -75,6 +76,13 @@ at25_ee_read(
struct spi_transfer t[2];
struct spi_message m;
+ if (unlikely(offset >= at25->bin.size))
+ return 0;
+ if ((offset + count) > at25->bin.size)
+ count = at25->bin.size - offset;
+ if (unlikely(!count))
+ return count;
+
cp = command;
*cp++ = AT25_READ;
@@ -127,13 +135,6 @@ at25_bin_read(struct kobject *kobj, stru
dev = container_of(kobj, struct device, kobj);
at25 = dev_get_drvdata(dev);
- if (unlikely(off >= at25->bin.size))
- return 0;
- if ((off + count) > at25->bin.size)
- count = at25->bin.size - off;
- if (unlikely(!count))
- return count;
-
return at25_ee_read(at25, buf, off, count);
}
@@ -146,6 +147,13 @@ at25_ee_write(struct at25_data *at25, ch
unsigned buf_size;
u8 *bounce;
+ if (unlikely(off >= at25->bin.size))
+ return -EFBIG;
+ if ((off + count) > at25->bin.size)
+ count = at25->bin.size - off;
+ if (unlikely(!count))
+ return count;
+
/* Temp buffer starts with command and address */
buf_size = at25->chip.page_size;
if (buf_size > io_limit)
@@ -253,18 +261,31 @@ at25_bin_write(struct kobject *kobj, str
dev = container_of(kobj, struct device, kobj);
at25 = dev_get_drvdata(dev);
- if (unlikely(off >= at25->bin.size))
- return -EFBIG;
- if ((off + count) > at25->bin.size)
- count = at25->bin.size - off;
- if (unlikely(!count))
- return count;
-
return at25_ee_write(at25, buf, off, count);
}
/*-------------------------------------------------------------------------*/
+/* Let in-kernel code access the eeprom data. */
+
+static ssize_t at25_mem_read(struct memory_accessor *mem, char *buf,
+ off_t offset, size_t count)
+{
+ struct at25_data *at25 = container_of(mem, struct at25_data, mem);
+
+ return at25_ee_read(at25, buf, offset, count);
+}
+
+static ssize_t at25_mem_write(struct memory_accessor *mem, char *buf,
+ off_t offset, size_t count)
+{
+ struct at25_data *at25 = container_of(mem, struct at25_data, mem);
+
+ return at25_ee_write(at25, buf, offset, count);
+}
+
+/*-------------------------------------------------------------------------*/
+
static int at25_probe(struct spi_device *spi)
{
struct at25_data *at25 = NULL;
@@ -317,6 +338,10 @@ static int at25_probe(struct spi_device
at25->addrlen = addrlen;
/* Export the EEPROM bytes through sysfs, since that's convenient.
+ * And maybe to other kernel code; it might hold a board's Ethernet
+ * address, or board-specific calibration data generated on the
+ * manufacturing floor.
+ *
* Default to root-only access to the data; EEPROMs often hold data
* that's sensitive for read and/or write, like ethernet addresses,
* security codes, board-specific manufacturing calibrations, etc.
@@ -324,17 +349,22 @@ static int at25_probe(struct spi_device
at25->bin.attr.name = "eeprom";
at25->bin.attr.mode = S_IRUSR;
at25->bin.read = at25_bin_read;
+ at25->mem.read = at25_mem_read;
at25->bin.size = at25->chip.byte_len;
if (!(chip->flags & EE_READONLY)) {
at25->bin.write = at25_bin_write;
at25->bin.attr.mode |= S_IWUSR;
+ at25->mem.write = at25_mem_write;
}
err = sysfs_create_bin_file(&spi->dev.kobj, &at25->bin);
if (err)
goto fail;
+ if (chip->setup)
+ chip->setup(&at25->mem, chip->context);
+
dev_info(&spi->dev, "%Zd %s %s eeprom%s, pagesize %u\n",
(at25->bin.size < 1024)
? at25->bin.size
--- a/include/linux/spi/eeprom.h
+++ b/include/linux/spi/eeprom.h
@@ -1,6 +1,8 @@
#ifndef __LINUX_SPI_EEPROM_H
#define __LINUX_SPI_EEPROM_H
+#include <linux/memory.h>
+
/*
* Put one of these structures in platform_data for SPI EEPROMS handled
* by the "at25" driver. On SPI, most EEPROMS understand the same core
@@ -17,6 +19,10 @@ struct spi_eeprom {
#define EE_ADDR2 0x0002 /* 16 bit addrs */
#define EE_ADDR3 0x0004 /* 24 bit addrs */
#define EE_READONLY 0x0008 /* disallow writes */
+
+ /* for exporting this chip's data to other kernel code */
+ void (*setup)(struct memory_accessor *mem, void *context);
+ void *context;
};
#endif /* __LINUX_SPI_EEPROM_H */