Chips from the at24cs EEPROM series have an additional read-only
memory area containing a factory pre-programmed serial number. In
order to access it, a dummy write must be executed before reading
the serial number bytes.
Chips from the at24mac familiy, apart from the serial number, have
a second special memory area containing a factory programmed
EUI-48/EUI64 mac address.
The read-only serial/mac memory area is accessible on a different i2c
slave address (addr + 0x08). This patchset makes it possible to
instantiate a separate at24 device on this second address and access
the read-only area through the regular eeprom sysfs attribute or the
new nvmem subsystem.
This series also contains several patches intoducing some minor tweaks
and refactoring.
Tested with at24cs32 and at24cs02 chips (for both 16 and 8 bit address
pointers). I have no means of testing the support for at24mac chips, I
relied solely on the datasheet.
v2:
- fixed the 'assignment from incompatible pointer type' bug reported
by kbuild
v1: https://lkml.org/lkml/2016/3/24/337
Bartosz Golaszewski (13):
eeprom: at24: remove a reduntant if
eeprom: at24: improve the device_id table readability
eeprom: at24: platform_data: use BIT() macro
eeprom: at24: make locking more fine-grained
eeprom: at24: replace msleep() with usleep_range()
eeprom: at24: add serial number flag
eeprom: at24: support reading of the serial number
eeprom: at24: call read and write routines via function pointers
eeprom: at24: use at24cs_serial_read()
eeprom: at24: add the at24cs series to the list of supported devices
eeprom: at24: add at24mac series flag
eeprom: at24: add support for at24mac series
eeprom: at24: add at24mac chips to the list of supported devices
drivers/misc/eeprom/at24.c | 201 ++++++++++++++++++++++++++++---------
include/linux/platform_data/at24.h | 10 +-
2 files changed, 162 insertions(+), 49 deletions(-)
--
2.7.4
We cannot expect msleep(1) to actually sleep for a period shorter than
20 ms. Replace all calls to msleep() with usleep_range().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 9e01428..bc5be1e 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -240,8 +240,7 @@ static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
if (status == count)
return count;
- /* REVISIT: at HZ=100, this is sloooow */
- msleep(1);
+ usleep_range(1000, 1500);
} while (time_before(read_time, timeout));
return -ETIMEDOUT;
@@ -355,8 +354,7 @@ static ssize_t at24_eeprom_write(struct at24_data *at24, const char *buf,
if (status == count)
return count;
- /* REVISIT: at HZ=100, this is sloooow */
- msleep(1);
+ usleep_range(1000, 1500);
} while (time_before(write_time, timeout));
return -ETIMEDOUT;
--
2.7.4
The at24cs series EEPROM chips have an additional read-only memory area
containing a factory pre-programmed serial number. In order to access
it, one has to perform a dummy write before reading the serial number
bytes.
Add a function that allows to access the serial number.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index bc5be1e..fda5c56 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -159,6 +159,65 @@ static struct i2c_client *at24_translate_offset(struct at24_data *at24,
return at24->client[i];
}
+static ssize_t __attribute__((unused))
+at24cs_serial_read(struct at24_data *at24,
+ char *buf, loff_t off, size_t count)
+{
+ unsigned long timeout, read_time;
+ struct i2c_client *client;
+ unsigned int offset = off;
+ struct i2c_msg msg[2];
+ u8 addrbuf[2];
+ int status;
+
+ client = at24_translate_offset(at24, &offset);
+
+ memset(msg, 0, sizeof(msg));
+ msg[0].addr = client->addr;
+ msg[0].buf = addrbuf;
+
+ /*
+ * The address pointer of the device is shared between the regular
+ * EEPROM array and the serial number block. The dummy write (part of
+ * the sequential read protocol) ensures the address pointer is reset
+ * to the desired position.
+ */
+ if (at24->chip.flags & AT24_FLAG_ADDR16) {
+ /*
+ * For 16 bit address pointers, the word address must contain
+ * a '10' sequence in bits 11 and 10 regardless of the
+ * intended position of the address pointer.
+ */
+ addrbuf[0] = 0x08;
+ addrbuf[1] = offset;
+ msg[0].len = 2;
+ } else {
+ /*
+ * Otherwise the word address must begin with a '10' sequence,
+ * regardless of the intended address.
+ */
+ addrbuf[0] = 0x80 + offset;
+ msg[0].len = 1;
+ }
+
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].buf = buf;
+ msg[1].len = count;
+
+ timeout = jiffies + msecs_to_jiffies(write_timeout);
+ do {
+ read_time = jiffies;
+ status = i2c_transfer(client->adapter, msg, 2);
+ if (status == 2)
+ return count;
+
+ usleep_range(1000, 1500);
+ } while (time_before(read_time, timeout));
+
+ return -ETIMEDOUT;
+}
+
static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
unsigned offset, size_t count)
{
--
2.7.4
Now with the infrastructue for reading the factory-programmed mac
address in place, add the two available chips from the at24mac
family: at24mac402 and at24mac602 to the device ID list.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index edec236..558b498 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -120,6 +120,10 @@ static const struct i2c_device_id at24_ids[] = {
{ "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
{ "24cs02", AT24_DEVICE_MAGIC(128 / 8,
AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+ { "24mac402", AT24_DEVICE_MAGIC(48 / 8,
+ AT24_FLAG_MAC | AT24_FLAG_READONLY) },
+ { "24mac602", AT24_DEVICE_MAGIC(64 / 8,
+ AT24_FLAG_MAC | AT24_FLAG_READONLY) },
/* spd is a 24c02 in memory DIMMs */
{ "spd", AT24_DEVICE_MAGIC(2048 / 8,
AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
--
2.7.4
Add a new read function to the at24 driver allowing to retrieve the
factory-programmed mac address embedded in chips from the at24mac
family.
These chips can be instantiated similarily to the at24cs family,
except that there's no way of having access to both the serial number
and the mac address at the same time - the user must instantiate
either an at24cs or at24mac device as both special memory areas are
accessible on the same slave address.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++++++++++++++++++++++
1 file changed, 43 insertions(+)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 281dd48..edec236 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -239,6 +239,41 @@ static ssize_t at24cs_serial_read(struct at24_data *at24,
return -ETIMEDOUT;
}
+static ssize_t at24mac_mac_read(struct at24_data *at24,
+ char *buf, loff_t off, size_t count)
+{
+ unsigned long timeout, read_time;
+ struct i2c_client *client;
+ unsigned int offset = off;
+ struct i2c_msg msg[2];
+ u8 addrbuf[2];
+ int status;
+
+ client = at24_translate_offset(at24, &offset);
+
+ memset(msg, 0, sizeof(msg));
+ msg[0].addr = client->addr;
+ msg[0].buf = addrbuf;
+ addrbuf[0] = 0x90 + offset;
+ msg[0].len = 1;
+ msg[1].addr = client->addr;
+ msg[1].flags = I2C_M_RD;
+ msg[1].buf = buf;
+ msg[1].len = count;
+
+ timeout = jiffies + msecs_to_jiffies(write_timeout);
+ do {
+ read_time = jiffies;
+ status = i2c_transfer(client->adapter, msg, 2);
+ if (status == 2)
+ return count;
+
+ usleep_range(1000, 1500);
+ } while (time_before(read_time, timeout));
+
+ return -ETIMEDOUT;
+}
+
static ssize_t at24_eeprom_read(struct at24_data *at24, char *buf,
unsigned offset, size_t count)
{
@@ -632,8 +667,16 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->chip = chip;
at24->num_addresses = num_addresses;
+ if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
+ dev_err(&client->dev,
+ "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
+ return -EINVAL;
+ }
+
if (chip.flags & AT24_FLAG_SERIAL) {
at24->read_func = at24cs_serial_read;
+ } else if (chip.flags & AT24_FLAG_MAC) {
+ at24->read_func = at24mac_mac_read;
} else {
at24->read_func = at24_read;
at24->write_func = at24_write;
--
2.7.4
As part of supporting the at24mac series add a new flag to the at24
platform data. When set, it indicates that this chip exposes the
factory-programmed EUI-48 or EUI-64 address.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
include/linux/platform_data/at24.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
index 5bc02fe..fa89605 100644
--- a/include/linux/platform_data/at24.h
+++ b/include/linux/platform_data/at24.h
@@ -48,6 +48,7 @@ struct at24_platform_data {
#define AT24_FLAG_IRUGO BIT(5) /* sysfs-entry will be world-readable */
#define AT24_FLAG_TAKE8ADDR BIT(4) /* take always 8 addresses (24c00) */
#define AT24_FLAG_SERIAL BIT(3) /* factory-programmed serial number */
+#define AT24_FLAG_MAC BIT(2) /* factory-programmed mac address */
void (*setup)(struct nvmem_device *nvmem, void *context);
void *context;
--
2.7.4
The infrastructure for reading of the factory-programmed serial number
for at24cs EEPROM series is now in place. Add the chips that are actually
equipped with the serial number memory area to the list of supported
devices.
The chips from the at24cs family have two memory areas - a regular
read-write block and a read-only area containing the serial number.
The latter is visible on a different slave address (the address of the
wr memory block + 0x08). In order to access both blocks the user needs
to instantiate a regular at24c device for the wr block address and a
corresponding at24cs device on the serial number block address.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index d3e4d66..281dd48 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -115,16 +115,34 @@ static const struct i2c_device_id at24_ids[] = {
{ "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
/* old variants can't be handled with this generic entry! */
{ "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
+ { "24cs01", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
{ "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
+ { "24cs02", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
/* spd is a 24c02 in memory DIMMs */
{ "spd", AT24_DEVICE_MAGIC(2048 / 8,
AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
{ "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
+ { "24cs04", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
/* 24rf08 quirk is handled at i2c-core */
{ "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
+ { "24cs08", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
{ "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
+ { "24cs16", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
{ "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
+ { "24cs32", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_ADDR16 |
+ AT24_FLAG_SERIAL |
+ AT24_FLAG_READONLY) },
{ "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
+ { "24cs64", AT24_DEVICE_MAGIC(128 / 8,
+ AT24_FLAG_ADDR16 |
+ AT24_FLAG_SERIAL |
+ AT24_FLAG_READONLY) },
{ "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
{ "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
{ "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
--
2.7.4
Assign at24cs_serial_read() to at24->read_func if the chip allows serial
number read operation.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 4c868a2..d3e4d66 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -163,9 +163,8 @@ static struct i2c_client *at24_translate_offset(struct at24_data *at24,
return at24->client[i];
}
-static ssize_t __attribute__((unused))
-at24cs_serial_read(struct at24_data *at24,
- char *buf, loff_t off, size_t count)
+static ssize_t at24cs_serial_read(struct at24_data *at24,
+ char *buf, loff_t off, size_t count)
{
unsigned long timeout, read_time;
struct i2c_client *client;
@@ -615,8 +614,12 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->chip = chip;
at24->num_addresses = num_addresses;
- at24->read_func = at24_read;
- at24->write_func = at24_write;
+ if (chip.flags & AT24_FLAG_SERIAL) {
+ at24->read_func = at24cs_serial_read;
+ } else {
+ at24->read_func = at24_read;
+ at24->write_func = at24_write;
+ }
writable = !(chip.flags & AT24_FLAG_READONLY);
if (writable) {
--
2.7.4
In order to support non-standard read/write functions (as part of the
at24cs series support) introduce two function pointers in struct
at24_data to which different implementations can be assigned.
For now we continue to use the regular read/write routines by default.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index fda5c56..4c868a2 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -59,6 +59,10 @@ struct at24_data {
int use_smbus;
int use_smbus_write;
+ ssize_t (*read_func)(struct at24_data *, char *, loff_t, size_t);
+ ssize_t (*write_func)(struct at24_data *,
+ const char *, loff_t, size_t);
+
u8 *writebuf;
struct mutex wrbuf_lock;
unsigned write_max;
@@ -458,7 +462,7 @@ static int at24_regmap_read(void *context, const void *reg, size_t reg_size,
off_t offset = *(u32 *)reg;
int err;
- err = at24_read(at24, val, offset, val_size);
+ err = at24->read_func(at24, val, offset, val_size);
if (err)
return err;
return 0;
@@ -476,7 +480,7 @@ static int at24_regmap_write(void *context, const void *data, size_t count)
buf = (const char *)data + sizeof(offset);
len = count - sizeof(offset);
- err = at24_write(at24, buf, offset, len);
+ err = at24->write_func(at24, buf, offset, len);
if (err)
return err;
return 0;
@@ -611,6 +615,9 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
at24->chip = chip;
at24->num_addresses = num_addresses;
+ at24->read_func = at24_read;
+ at24->write_func = at24_write;
+
writable = !(chip.flags & AT24_FLAG_READONLY);
if (writable) {
if (!use_smbus || use_smbus_write) {
--
2.7.4
In preparation for supporting the at24cs EEPROM series add a new flag
to platform data. When set, it should tell the driver that the chip
has an additional read-only memory area that holds a factory
pre-programmed serial number.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
include/linux/platform_data/at24.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
index a543b93..5bc02fe 100644
--- a/include/linux/platform_data/at24.h
+++ b/include/linux/platform_data/at24.h
@@ -47,6 +47,7 @@ struct at24_platform_data {
#define AT24_FLAG_READONLY BIT(6) /* sysfs-entry will be read-only */
#define AT24_FLAG_IRUGO BIT(5) /* sysfs-entry will be world-readable */
#define AT24_FLAG_TAKE8ADDR BIT(4) /* take always 8 addresses (24c00) */
+#define AT24_FLAG_SERIAL BIT(3) /* factory-programmed serial number */
void (*setup)(struct nvmem_device *nvmem, void *context);
void *context;
--
2.7.4
Use BIT() macro to replace the 0xXX constants in platform_data flags
definitions.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
include/linux/platform_data/at24.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/platform_data/at24.h b/include/linux/platform_data/at24.h
index dc9a13e..a543b93 100644
--- a/include/linux/platform_data/at24.h
+++ b/include/linux/platform_data/at24.h
@@ -43,10 +43,10 @@ struct at24_platform_data {
u32 byte_len; /* size (sum of all addr) */
u16 page_size; /* for writes */
u8 flags;
-#define AT24_FLAG_ADDR16 0x80 /* address pointer is 16 bit */
-#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) */
+#define AT24_FLAG_ADDR16 BIT(7) /* address pointer is 16 bit */
+#define AT24_FLAG_READONLY BIT(6) /* sysfs-entry will be read-only */
+#define AT24_FLAG_IRUGO BIT(5) /* sysfs-entry will be world-readable */
+#define AT24_FLAG_TAKE8ADDR BIT(4) /* take always 8 addresses (24c00) */
void (*setup)(struct nvmem_device *nvmem, void *context);
void *context;
--
2.7.4
The only field in struct at24_data that needs locking in the module
code is u8 *writebuf. Other data is already protected by i2c core.
Rename the lock in at24_data to wrbuf_lock and only use it where
writebuf is accessed.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 28 +++++-----------------------
1 file changed, 5 insertions(+), 23 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 744c526..9e01428 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -59,13 +59,8 @@ struct at24_data {
int use_smbus;
int use_smbus_write;
- /*
- * Lock protects against activities from other Linux tasks,
- * but not from changes by other I2C masters.
- */
- struct mutex lock;
-
u8 *writebuf;
+ struct mutex wrbuf_lock;
unsigned write_max;
unsigned num_addresses;
@@ -260,12 +255,6 @@ static ssize_t at24_read(struct at24_data *at24,
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;
@@ -281,8 +270,6 @@ static ssize_t at24_read(struct at24_data *at24,
retval += status;
}
- mutex_unlock(&at24->lock);
-
return retval;
}
@@ -322,6 +309,8 @@ static ssize_t at24_eeprom_write(struct at24_data *at24, const char *buf,
msg.addr = client->addr;
msg.flags = 0;
+ mutex_lock(&at24->wrbuf_lock);
+
/* msg.buf is u8 and casts will mask the values */
msg.buf = at24->writebuf;
if (at24->chip.flags & AT24_FLAG_ADDR16)
@@ -356,6 +345,7 @@ static ssize_t at24_eeprom_write(struct at24_data *at24, const char *buf,
status = count;
} else {
status = i2c_transfer(client->adapter, &msg, 1);
+ mutex_unlock(&at24->wrbuf_lock);
if (status == 1)
status = count;
}
@@ -380,12 +370,6 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
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;
@@ -401,8 +385,6 @@ static ssize_t at24_write(struct at24_data *at24, const char *buf, loff_t off,
retval += status;
}
- mutex_unlock(&at24->lock);
-
return retval;
}
@@ -566,7 +548,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
if (!at24)
return -ENOMEM;
- mutex_init(&at24->lock);
+ mutex_init(&at24->wrbuf_lock);
at24->use_smbus = use_smbus;
at24->use_smbus_write = use_smbus_write;
at24->chip = chip;
--
2.7.4
It seems as if the second check for I2C_FUNC_I2C functionality had
been introduced accidentally during a merge. Tt's reduntant, so
remove it.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 089d694..001a9af 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -544,10 +544,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
} else {
return -EPFNOSUPPORT;
}
- }
- /* Use I2C operations unless we're stuck with SMBus extensions. */
- if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
if (i2c_check_functionality(client->adapter,
I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
use_smbus_write = I2C_SMBUS_I2C_BLOCK_DATA;
--
2.7.4
As part of the preparation for introducing support for more chips,
improve the readability of the device table by separating columns
with tabs.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/misc/eeprom/at24.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 001a9af..744c526 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -113,23 +113,23 @@ MODULE_PARM_DESC(write_timeout, "Time (in ms) to try writes (default 25)");
static const struct i2c_device_id at24_ids[] = {
/* needs 8 addresses as A0-A2 are ignored */
- { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
+ { "24c00", AT24_DEVICE_MAGIC(128 / 8, AT24_FLAG_TAKE8ADDR) },
/* old variants can't be handled with this generic entry! */
- { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
- { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
+ { "24c01", AT24_DEVICE_MAGIC(1024 / 8, 0) },
+ { "24c02", AT24_DEVICE_MAGIC(2048 / 8, 0) },
/* spd is a 24c02 in memory DIMMs */
- { "spd", AT24_DEVICE_MAGIC(2048 / 8,
- AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
- { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
+ { "spd", AT24_DEVICE_MAGIC(2048 / 8,
+ AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
+ { "24c04", AT24_DEVICE_MAGIC(4096 / 8, 0) },
/* 24rf08 quirk is handled at i2c-core */
- { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
- { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
- { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
- { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
- { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
- { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
- { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
- { "24c1024", AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) },
+ { "24c08", AT24_DEVICE_MAGIC(8192 / 8, 0) },
+ { "24c16", AT24_DEVICE_MAGIC(16384 / 8, 0) },
+ { "24c32", AT24_DEVICE_MAGIC(32768 / 8, AT24_FLAG_ADDR16) },
+ { "24c64", AT24_DEVICE_MAGIC(65536 / 8, AT24_FLAG_ADDR16) },
+ { "24c128", AT24_DEVICE_MAGIC(131072 / 8, AT24_FLAG_ADDR16) },
+ { "24c256", AT24_DEVICE_MAGIC(262144 / 8, AT24_FLAG_ADDR16) },
+ { "24c512", AT24_DEVICE_MAGIC(524288 / 8, AT24_FLAG_ADDR16) },
+ { "24c1024", AT24_DEVICE_MAGIC(1048576 / 8, AT24_FLAG_ADDR16) },
{ "at24", 0 },
{ /* END OF LIST */ }
};
--
2.7.4
On Mon, Apr 11, 2016 at 11:57:17AM -0700, Bartosz Golaszewski wrote:
> It seems as if the second check for I2C_FUNC_I2C functionality had
> been introduced accidentally during a merge. Tt's reduntant, so
> remove it.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Reworded commit message (there was no merge conflict) and applied to
for-next, thanks!
On Mon, Apr 11, 2016 at 11:57:20AM -0700, Bartosz Golaszewski wrote:
> The only field in struct at24_data that needs locking in the module
> code is u8 *writebuf. Other data is already protected by i2c core.
>
> Rename the lock in at24_data to wrbuf_lock and only use it where
> writebuf is accessed.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Did you run some tests to verify our assumption?
On Mon, Apr 11, 2016 at 11:57:21AM -0700, Bartosz Golaszewski wrote:
> We cannot expect msleep(1) to actually sleep for a period shorter than
> 20 ms. Replace all calls to msleep() with usleep_range().
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
Applied to for-next, thanks!
Hi,
On Mon, Apr 11, 2016 at 11:57:16AM -0700, Bartosz Golaszewski wrote:
> Chips from the at24cs EEPROM series have an additional read-only
> memory area containing a factory pre-programmed serial number. In
> order to access it, a dummy write must be executed before reading
> the serial number bytes.
>
> Chips from the at24mac familiy, apart from the serial number, have
> a second special memory area containing a factory programmed
> EUI-48/EUI64 mac address.
>
> The read-only serial/mac memory area is accessible on a different i2c
> slave address (addr + 0x08). This patchset makes it possible to
> instantiate a separate at24 device on this second address and access
> the read-only area through the regular eeprom sysfs attribute or the
> new nvmem subsystem.
>
> This series also contains several patches intoducing some minor tweaks
> and refactoring.
>
> Tested with at24cs32 and at24cs02 chips (for both 16 and 8 bit address
> pointers). I have no means of testing the support for at24mac chips, I
> relied solely on the datasheet.
Thanks for adding at24mac nonetheless!
I still don't understand why you want to add this to at24 instead of
doing a seperate driver for it? You basically tie a read_function to an
nvmem device. You don't need the special smbus handling, cutting large
requests into chunks, write support, size flags and all this from at24.
And with the function pointers, it makes the already complex code a bit
more complex.
IMO, this could be solved by a simple, seperate driver. You could start
with at24 and remove unneeded stuff. I think there should be mainly
nvmem-init code and your read routines left. I also think this new
driver should be allowed to set the "compat" option of the nvmem
framework, so you'll get the "eeprom" file, too.
Or am I overlooking something?
2016-04-16 23:17 GMT+02:00 Wolfram Sang <[email protected]>:
> Hi,
>
> On Mon, Apr 11, 2016 at 11:57:16AM -0700, Bartosz Golaszewski wrote:
>> Chips from the at24cs EEPROM series have an additional read-only
>> memory area containing a factory pre-programmed serial number. In
>> order to access it, a dummy write must be executed before reading
>> the serial number bytes.
>>
>> Chips from the at24mac familiy, apart from the serial number, have
>> a second special memory area containing a factory programmed
>> EUI-48/EUI64 mac address.
>>
>> The read-only serial/mac memory area is accessible on a different i2c
>> slave address (addr + 0x08). This patchset makes it possible to
>> instantiate a separate at24 device on this second address and access
>> the read-only area through the regular eeprom sysfs attribute or the
>> new nvmem subsystem.
>>
>> This series also contains several patches intoducing some minor tweaks
>> and refactoring.
>>
>> Tested with at24cs32 and at24cs02 chips (for both 16 and 8 bit address
>> pointers). I have no means of testing the support for at24mac chips, I
>> relied solely on the datasheet.
>
> Thanks for adding at24mac nonetheless!
>
> I still don't understand why you want to add this to at24 instead of
> doing a seperate driver for it? You basically tie a read_function to an
> nvmem device. You don't need the special smbus handling, cutting large
> requests into chunks, write support, size flags and all this from at24.
> And with the function pointers, it makes the already complex code a bit
> more complex.
>
Hi Wolfram,
while you're right that this makes the code more complex, I can see
some space for overall improvement of the driver's code quality with
function pointers.
The read and write routines are quite complex and decide which
operations to use (smbus or i2c) on the fly every time they're called.
Why not split them into several simpler functions (each implementing a
single way of talking to EEPROM) that would be assigned in probe()
depending on the chip type and adapter's functionalities? Would you be
willing to accept a series that does it?
> IMO, this could be solved by a simple, seperate driver. You could start
> with at24 and remove unneeded stuff. I think there should be mainly
> nvmem-init code and your read routines left. I also think this new
> driver should be allowed to set the "compat" option of the nvmem
> framework, so you'll get the "eeprom" file, too.
>
> Or am I overlooking something?
>
I think creating two new drivers for something as simple as that is
overkill. Especially when it's a backwards compatible sub-family of
chips we're talking about. How about leaving this in at24 and instead
cleaning the driver up a bit with the approach I suggested above?
Best regards,
Bartosz Golaszewski