[PATCH v4 0/7] eeprom: at24: switch driver to regmap_i2c

[PATCH v4 0/7] eeprom: at24: switch driver to regmap_i2c

[Heiner Kallweit]

Using regmap_i2c allows us to get rid of dealing with the low-level
differences between I2C and SMBUS. As a result the code can be
simplified a lot.

This patchset was successfully tested with a 24C32 on a I2C adapter.

The patchset was submitted first in Aug 2017 and was sitting in
the review queue until now. Resubmitted version is rebased due to
recent changes to at24.

Changes in v3:
- Small changes based on review comments
- Introduce at24_adjust_read_offset to avoid some overhead
Changes in v4:
- Small improvement in patch 5

Heiner Kallweit (7):
  eeprom: at24: add basic regmap_i2c support
  eeprom: at24: change at24_translate_offset return type
  eeprom: at24: add regmap-based write function
  eeprom: at24: remove old write functions
  eeprom: at24: add regmap-based read functions
  eeprom: at24: remove old read functions
  eeprom: at24: remove now unneeded smbus-related code

 drivers/misc/eeprom/Kconfig |   1 +
 drivers/misc/eeprom/at24.c  | 426 +++++++++++---------------------------------
 2 files changed, 105 insertions(+), 322 deletions(-)

-- 
2.15.0

[PATCH v4 1/7] eeprom: at24: add basic regmap_i2c support

[Heiner Kallweit]

This patch adds basic regmap support to be used by subsequent
patches of this series.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- no changes
v4:
- no changes
---
 drivers/misc/eeprom/Kconfig |  1 +
 drivers/misc/eeprom/at24.c  | 53 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index de5876209..68a1ac929 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -4,6 +4,7 @@ config EEPROM_AT24
 	tristate "I2C EEPROMs / RAMs / ROMs from most vendors"
 	depends on I2C && SYSFS
 	select NVMEM
+	select REGMAP_I2C
 	help
 	  Enable this driver to get read/write support to most I2C EEPROMs
 	  and compatible devices like FRAMs, SRAMs, ROMs etc. After you
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index e0b4b36ef..911cce8ec 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -24,6 +24,7 @@
 #include <linux/acpi.h>
 #include <linux/i2c.h>
 #include <linux/nvmem-provider.h>
+#include <linux/regmap.h>
 #include <linux/platform_data/at24.h>
 #include <linux/pm_runtime.h>
 
@@ -55,6 +56,11 @@
  * which won't work on pure SMBus systems.
  */
 
+struct at24_client {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
 struct at24_data {
 	struct at24_platform_data chip;
 	int use_smbus;
@@ -81,7 +87,7 @@ struct at24_data {
 	 * 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[];
+	struct at24_client client[];
 };
 
 /*
@@ -274,7 +280,7 @@ static struct i2c_client *at24_translate_offset(struct at24_data *at24,
 		*offset &= 0xff;
 	}
 
-	return at24->client[i];
+	return at24->client[i].client;
 }
 
 static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
@@ -673,6 +679,16 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 	}
 }
 
+static const struct regmap_config regmap_config_8 = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static const struct regmap_config regmap_config_16 = {
+	.reg_bits = 16,
+	.val_bits = 8,
+};
+
 static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct at24_platform_data chip;
@@ -683,6 +699,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	struct at24_data *at24;
 	int err;
 	unsigned i, num_addresses;
+	const struct regmap_config *config;
 	u8 test_byte;
 
 	if (client->dev.platform_data) {
@@ -764,8 +781,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		num_addresses =	DIV_ROUND_UP(chip.byte_len,
 			(chip.flags & AT24_FLAG_ADDR16) ? 65536 : 256);
 
+	if (chip.flags & AT24_FLAG_ADDR16)
+		config = &regmap_config_16;
+	else
+		config = &regmap_config_8;
+
 	at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
-		num_addresses * sizeof(struct i2c_client *), GFP_KERNEL);
+		num_addresses * sizeof(struct at24_client), GFP_KERNEL);
 	if (!at24)
 		return -ENOMEM;
 
@@ -775,6 +797,11 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	at24->chip = chip;
 	at24->num_addresses = num_addresses;
 
+	at24->client[0].client = client;
+	at24->client[0].regmap = devm_regmap_init_i2c(client, config);
+	if (IS_ERR(at24->client[0].regmap))
+		return PTR_ERR(at24->client[0].regmap);
+
 	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.");
@@ -822,18 +849,22 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		}
 	}
 
-	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]) {
+		at24->client[i].client = i2c_new_dummy(client->adapter,
+						       client->addr + i);
+		if (!at24->client[i].client) {
 			dev_err(&client->dev, "address 0x%02x unavailable\n",
 					client->addr + i);
 			err = -EADDRINUSE;
 			goto err_clients;
 		}
+		at24->client[i].regmap = devm_regmap_init_i2c(
+					at24->client[i].client, config);
+		if (IS_ERR(at24->client[i].regmap)) {
+			err = PTR_ERR(at24->client[i].regmap);
+			goto err_clients;
+		}
 	}
 
 	i2c_set_clientdata(client, at24);
@@ -892,8 +923,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 err_clients:
 	for (i = 1; i < num_addresses; i++)
-		if (at24->client[i])
-			i2c_unregister_device(at24->client[i]);
+		if (at24->client[i].client)
+			i2c_unregister_device(at24->client[i].client);
 
 	pm_runtime_disable(&client->dev);
 
@@ -910,7 +941,7 @@ static int at24_remove(struct i2c_client *client)
 	nvmem_unregister(at24->nvmem);
 
 	for (i = 1; i < at24->num_addresses; i++)
-		i2c_unregister_device(at24->client[i]);
+		i2c_unregister_device(at24->client[i].client);
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
-- 
2.15.0

[PATCH v4 2/7] eeprom: at24: change at24_translate_offset return type

[Heiner Kallweit]

Change return type of at24_translate_offset to *at24_client to make
member regmap accessible for subsequent patches of this series.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- improve readability
v4:
- no changes
---
 drivers/misc/eeprom/at24.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 911cce8ec..fe4393ad6 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -267,8 +267,8 @@ MODULE_DEVICE_TABLE(acpi, at24_acpi_ids);
  * one "eeprom" file not four, but larger reads would fail when
  * they crossed certain pages.
  */
-static struct i2c_client *at24_translate_offset(struct at24_data *at24,
-						unsigned int *offset)
+static struct at24_client *at24_translate_offset(struct at24_data *at24,
+						 unsigned int *offset)
 {
 	unsigned i;
 
@@ -280,17 +280,19 @@ static struct i2c_client *at24_translate_offset(struct at24_data *at24,
 		*offset &= 0xff;
 	}
 
-	return at24->client[i].client;
+	return &at24->client[i];
 }
 
 static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
 				      unsigned int offset, size_t count)
 {
 	unsigned long timeout, read_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 
 	if (count > io_limit)
 		count = io_limit;
@@ -318,13 +320,15 @@ static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
 				    unsigned int offset, size_t count)
 {
 	unsigned long timeout, read_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	struct i2c_msg msg[2];
 	int status, i;
 	u8 msgbuf[2];
 
 	memset(msg, 0, sizeof(msg));
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 
 	if (count > io_limit)
 		count = io_limit;
@@ -368,12 +372,14 @@ static ssize_t at24_eeprom_read_serial(struct at24_data *at24, char *buf,
 				       unsigned int offset, size_t count)
 {
 	unsigned long timeout, read_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	struct i2c_msg msg[2];
 	u8 addrbuf[2];
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 
 	memset(msg, 0, sizeof(msg));
 	msg[0].addr = client->addr;
@@ -421,12 +427,14 @@ static ssize_t at24_eeprom_read_mac(struct at24_data *at24, char *buf,
 				    unsigned int offset, size_t count)
 {
 	unsigned long timeout, read_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	struct i2c_msg msg[2];
 	u8 addrbuf[2];
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 
 	memset(msg, 0, sizeof(msg));
 	msg[0].addr = client->addr;
@@ -478,10 +486,12 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24,
 					     unsigned int offset, size_t count)
 {
 	unsigned long timeout, write_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	ssize_t status = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 	count = at24_adjust_write_count(at24, offset, count);
 
 	loop_until_timeout(timeout, write_time) {
@@ -505,10 +515,12 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24,
 					    unsigned int offset, size_t count)
 {
 	unsigned long timeout, write_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	ssize_t status = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 
 	loop_until_timeout(timeout, write_time) {
 		status = i2c_smbus_write_byte_data(client, offset, buf[0]);
@@ -529,12 +541,14 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 				     unsigned int offset, size_t count)
 {
 	unsigned long timeout, write_time;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	struct i2c_msg msg;
 	ssize_t status = 0;
 	int i = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	at24_client = at24_translate_offset(at24, &offset);
+	client = at24_client->client;
 	count = at24_adjust_write_count(at24, offset, count);
 
 	msg.addr = client->addr;
@@ -567,6 +581,7 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 {
 	struct at24_data *at24 = priv;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	char *buf = val;
 	int ret;
@@ -574,7 +589,8 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 	if (unlikely(!count))
 		return count;
 
-	client = at24_translate_offset(at24, &off);
+	at24_client = at24_translate_offset(at24, &off);
+	client = at24_client->client;
 
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret < 0) {
@@ -612,6 +628,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 {
 	struct at24_data *at24 = priv;
+	struct at24_client *at24_client;
 	struct i2c_client *client;
 	char *buf = val;
 	int ret;
@@ -619,7 +636,8 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	if (unlikely(!count))
 		return -EINVAL;
 
-	client = at24_translate_offset(at24, &off);
+	at24_client = at24_translate_offset(at24, &off);
+	client = at24_client->client;
 
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret < 0) {
-- 
2.15.0

[PATCH v4 3/7] eeprom: at24: add regmap-based write function

[Heiner Kallweit]

Add a regmap-based write function.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- improve readability
- re-introduce debug message
v4:
- no changes
---
 drivers/misc/eeprom/at24.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index fe4393ad6..86a36482a 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -537,6 +537,31 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24,
 	return -ETIMEDOUT;
 }
 
+static ssize_t at24_regmap_write(struct at24_data *at24, const char *buf,
+				 unsigned int offset, size_t count)
+{
+	unsigned long timeout, write_time;
+	struct at24_client *at24_client;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	int ret;
+
+	at24_client = at24_translate_offset(at24, &offset);
+	regmap = at24_client->regmap;
+	client = at24_client->client;
+	count = at24_adjust_write_count(at24, offset, count);
+
+	loop_until_timeout(timeout, write_time) {
+		ret = regmap_bulk_write(regmap, offset, buf, count);
+		dev_dbg(&client->dev, "write %zu@%d --> %d (%ld)\n",
+			count, offset, ret, jiffies);
+		if (!ret)
+			return count;
+	}
+
+	return -ETIMEDOUT;
+}
+
 static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 				     unsigned int offset, size_t count)
 {
@@ -654,7 +679,7 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	while (count) {
 		int status;
 
-		status = at24->write_func(at24, buf, off, count);
+		status = at24_regmap_write(at24, buf, off, count);
 		if (status < 0) {
 			mutex_unlock(&at24->lock);
 			pm_runtime_put(&client->dev);
-- 
2.15.0

[PATCH v4 4/7] eeprom: at24: remove old write functions

[Heiner Kallweit]

Remove the old and now unused write functions.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- rebased
v4:
- no changes
---
 drivers/misc/eeprom/at24.c | 117 ---------------------------------------------
 1 file changed, 117 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 86a36482a..493e2b646 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -64,11 +64,8 @@ struct at24_client {
 struct at24_data {
 	struct at24_platform_data chip;
 	int use_smbus;
-	int use_smbus_write;
 
 	ssize_t (*read_func)(struct at24_data *, char *, unsigned int, size_t);
-	ssize_t (*write_func)(struct at24_data *,
-			      const char *, unsigned int, size_t);
 
 	/*
 	 * Lock protects against activities from other Linux tasks,
@@ -76,7 +73,6 @@ struct at24_data {
 	 */
 	struct mutex lock;
 
-	u8 *writebuf;
 	unsigned write_max;
 	unsigned num_addresses;
 
@@ -481,62 +477,6 @@ static size_t at24_adjust_write_count(struct at24_data *at24,
 	return count;
 }
 
-static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24,
-					     const char *buf,
-					     unsigned int offset, size_t count)
-{
-	unsigned long timeout, write_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	ssize_t status = 0;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-	count = at24_adjust_write_count(at24, offset, count);
-
-	loop_until_timeout(timeout, write_time) {
-		status = i2c_smbus_write_i2c_block_data(client,
-							offset, count, buf);
-		if (status == 0)
-			status = count;
-
-		dev_dbg(&client->dev, "write %zu@%d --> %zd (%ld)\n",
-				count, offset, status, jiffies);
-
-		if (status == count)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
-static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24,
-					    const char *buf,
-					    unsigned int offset, size_t count)
-{
-	unsigned long timeout, write_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	ssize_t status = 0;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-
-	loop_until_timeout(timeout, write_time) {
-		status = i2c_smbus_write_byte_data(client, offset, buf[0]);
-		if (status == 0)
-			status = count;
-
-		dev_dbg(&client->dev, "write %zu@%d --> %zd (%ld)\n",
-				count, offset, status, jiffies);
-
-		if (status == count)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
 static ssize_t at24_regmap_write(struct at24_data *at24, const char *buf,
 				 unsigned int offset, size_t count)
 {
@@ -562,47 +502,6 @@ static ssize_t at24_regmap_write(struct at24_data *at24, const char *buf,
 	return -ETIMEDOUT;
 }
 
-static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
-				     unsigned int offset, size_t count)
-{
-	unsigned long timeout, write_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	struct i2c_msg msg;
-	ssize_t status = 0;
-	int i = 0;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-	count = at24_adjust_write_count(at24, offset, count);
-
-	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;
-
-	loop_until_timeout(timeout, write_time) {
-		status = i2c_transfer(client->adapter, &msg, 1);
-		if (status == 1)
-			status = count;
-
-		dev_dbg(&client->dev, "write %zu@%d --> %zd (%ld)\n",
-				count, offset, status, jiffies);
-
-		if (status == count)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
 static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 {
 	struct at24_data *at24 = priv;
@@ -836,7 +735,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	mutex_init(&at24->lock);
 	at24->use_smbus = use_smbus;
-	at24->use_smbus_write = use_smbus_write;
 	at24->chip = chip;
 	at24->num_addresses = num_addresses;
 
@@ -860,15 +758,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 						  : at24_eeprom_read_i2c;
 	}
 
-	if (at24->use_smbus) {
-		if (at24->use_smbus_write == I2C_SMBUS_I2C_BLOCK_DATA)
-			at24->write_func = at24_eeprom_write_smbus_block;
-		else
-			at24->write_func = at24_eeprom_write_smbus_byte;
-	} else {
-		at24->write_func = at24_eeprom_write_i2c;
-	}
-
 	writable = !(chip.flags & AT24_FLAG_READONLY);
 	if (writable) {
 		if (!use_smbus || use_smbus_write) {
@@ -880,12 +769,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 			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 = devm_kzalloc(&client->dev,
-				write_max + 2, GFP_KERNEL);
-			if (!at24->writebuf)
-				return -ENOMEM;
 		} else {
 			dev_warn(&client->dev,
 				"cannot write due to controller restrictions.");
-- 
2.15.0

[PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit]

Add regmap-based read function and instead of using three different
read functions (standard, mac, serial) use just one and factor out the
read offset adjustment for mac and serial to at24_adjust_read_offset.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- improve readability
- re-introduce debug message
- introduce at24_adjust_read_offset
v4:
- move offset adjustment calculation to probe function
---
 drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 493e2b646..c16a9a495 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -75,6 +75,7 @@ struct at24_data {
 
 	unsigned write_max;
 	unsigned num_addresses;
+	unsigned int offset_adj;
 
 	struct nvmem_config nvmem_config;
 	struct nvmem_device *nvmem;
@@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
 	return -ETIMEDOUT;
 }
 
+static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
+				unsigned int offset, size_t count)
+{
+	unsigned long timeout, read_time;
+	struct at24_client *at24_client;
+	struct i2c_client *client;
+	struct regmap *regmap;
+	int ret;
+
+	at24_client = at24_translate_offset(at24, &offset);
+	regmap = at24_client->regmap;
+	client = at24_client->client;
+
+	if (count > io_limit)
+		count = io_limit;
+
+	/* adjust offset for mac and serial read ops */
+	offset += at24->offset_adj;
+
+	loop_until_timeout(timeout, read_time) {
+		ret = regmap_bulk_read(regmap, offset, buf, count);
+		dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
+			count, offset, ret, jiffies);
+		if (!ret)
+			return count;
+	}
+
+	return -ETIMEDOUT;
+}
+
 static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
 				    unsigned int offset, size_t count)
 {
@@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 	while (count) {
 		int	status;
 
-		status = at24->read_func(at24, buf, off, count);
+		status = at24_regmap_read(at24, buf, off, count);
 		if (status < 0) {
 			mutex_unlock(&at24->lock);
 			pm_runtime_put(&client->dev);
@@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
 	}
 }
 
+static unsigned int at24_get_offset_adj(u8 flags)
+{
+	if (flags & AT24_FLAG_MAC)
+		return 0x90;
+	else if (flags & AT24_FLAG_SERIAL && 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.
+		 */
+		return BIT(11);
+	else if (flags & AT24_FLAG_SERIAL)
+		/*
+		 * Otherwise the word address must begin with a '10' sequence,
+		 * regardless of the intended address.
+		 */
+		return BIT(7);
+	else
+		return 0;
+}
+
 static const struct regmap_config regmap_config_8 = {
 	.reg_bits = 8,
 	.val_bits = 8,
@@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -EINVAL;
 	}
 
+	at24->offset_adj = at24_get_offset_adj(chip.flags);
+
 	if (chip.flags & AT24_FLAG_SERIAL) {
 		at24->read_func = at24_eeprom_read_serial;
 	} else if (chip.flags & AT24_FLAG_MAC) {
-- 
2.15.0

[PATCH v4 6/7] eeprom: at24: remove old read functions

[Heiner Kallweit]

Remove the old and now unused read functions.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- rebased
v4:
- rebased
---
 drivers/misc/eeprom/at24.c | 185 ---------------------------------------------
 1 file changed, 185 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c16a9a495..c65169fdf 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -63,9 +63,6 @@ struct at24_client {
 
 struct at24_data {
 	struct at24_platform_data chip;
-	int use_smbus;
-
-	ssize_t (*read_func)(struct at24_data *, char *, unsigned int, size_t);
 
 	/*
 	 * Lock protects against activities from other Linux tasks,
@@ -280,39 +277,6 @@ static struct at24_client *at24_translate_offset(struct at24_data *at24,
 	return &at24->client[i];
 }
 
-static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
-				      unsigned int offset, size_t count)
-{
-	unsigned long timeout, read_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	int status;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-
-	if (count > io_limit)
-		count = io_limit;
-
-	/* Smaller eeproms can work given some SMBus extension calls */
-	if (count > I2C_SMBUS_BLOCK_MAX)
-		count = I2C_SMBUS_BLOCK_MAX;
-
-	loop_until_timeout(timeout, read_time) {
-		status = i2c_smbus_read_i2c_block_data_or_emulated(client,
-								   offset,
-								   count, buf);
-
-		dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
-				count, offset, status, jiffies);
-
-		if (status == count)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
 static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
 				unsigned int offset, size_t count)
 {
@@ -343,145 +307,6 @@ static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
 	return -ETIMEDOUT;
 }
 
-static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
-				    unsigned int offset, size_t count)
-{
-	unsigned long timeout, read_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	struct i2c_msg msg[2];
-	int status, i;
-	u8 msgbuf[2];
-
-	memset(msg, 0, sizeof(msg));
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-
-	if (count > io_limit)
-		count = io_limit;
-
-	/*
-	 * 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.
-	 */
-	i = 0;
-	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;
-
-	loop_until_timeout(timeout, read_time) {
-		status = i2c_transfer(client->adapter, msg, 2);
-		if (status == 2)
-			status = count;
-
-		dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
-				count, offset, status, jiffies);
-
-		if (status == count)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
-static ssize_t at24_eeprom_read_serial(struct at24_data *at24, char *buf,
-				       unsigned int offset, size_t count)
-{
-	unsigned long timeout, read_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	struct i2c_msg msg[2];
-	u8 addrbuf[2];
-	int status;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-
-	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;
-
-	loop_until_timeout(timeout, read_time) {
-		status = i2c_transfer(client->adapter, msg, 2);
-		if (status == 2)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
-static ssize_t at24_eeprom_read_mac(struct at24_data *at24, char *buf,
-				    unsigned int offset, size_t count)
-{
-	unsigned long timeout, read_time;
-	struct at24_client *at24_client;
-	struct i2c_client *client;
-	struct i2c_msg msg[2];
-	u8 addrbuf[2];
-	int status;
-
-	at24_client = at24_translate_offset(at24, &offset);
-	client = at24_client->client;
-
-	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;
-
-	loop_until_timeout(timeout, read_time) {
-		status = i2c_transfer(client->adapter, msg, 2);
-		if (status == 2)
-			return count;
-	}
-
-	return -ETIMEDOUT;
-}
-
 /*
  * Note that if the hardware write-protect pin is pulled high, the whole
  * chip is normally write protected. But there are plenty of product
@@ -786,7 +611,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -ENOMEM;
 
 	mutex_init(&at24->lock);
-	at24->use_smbus = use_smbus;
 	at24->chip = chip;
 	at24->num_addresses = num_addresses;
 
@@ -803,15 +627,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	at24->offset_adj = at24_get_offset_adj(chip.flags);
 
-	if (chip.flags & AT24_FLAG_SERIAL) {
-		at24->read_func = at24_eeprom_read_serial;
-	} else if (chip.flags & AT24_FLAG_MAC) {
-		at24->read_func = at24_eeprom_read_mac;
-	} else {
-		at24->read_func = at24->use_smbus ? at24_eeprom_read_smbus
-						  : at24_eeprom_read_i2c;
-	}
-
 	writable = !(chip.flags & AT24_FLAG_READONLY);
 	if (writable) {
 		if (!use_smbus || use_smbus_write) {
-- 
2.15.0

[PATCH v4 7/7] eeprom: at24: remove now unneeded smbus-related code

[Heiner Kallweit]

Remove remaining now unneeded code dealing with SMBUS details.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
v3:
- rebased
v4:
- rebased
---
 drivers/misc/eeprom/at24.c | 52 ++++------------------------------------------
 1 file changed, 4 insertions(+), 48 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c65169fdf..d80f36d70 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -513,8 +513,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	struct at24_platform_data chip;
 	kernel_ulong_t magic = 0;
 	bool writable;
-	int use_smbus = 0;
-	int use_smbus_write = 0;
 	struct at24_data *at24;
 	int err;
 	unsigned i, num_addresses;
@@ -566,33 +564,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		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)
-			return -EPFNOSUPPORT;
-
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
 		if (i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
-			use_smbus = I2C_SMBUS_I2C_BLOCK_DATA;
-		} else if (i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_READ_WORD_DATA)) {
-			use_smbus = I2C_SMBUS_WORD_DATA;
-		} else if (i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
-			use_smbus = I2C_SMBUS_BYTE_DATA;
-		} else {
-			return -EPFNOSUPPORT;
-		}
-
-		if (i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_WRITE_I2C_BLOCK)) {
-			use_smbus_write = I2C_SMBUS_I2C_BLOCK_DATA;
-		} else if (i2c_check_functionality(client->adapter,
-				I2C_FUNC_SMBUS_WRITE_BYTE_DATA)) {
-			use_smbus_write = I2C_SMBUS_BYTE_DATA;
+				I2C_FUNC_SMBUS_WRITE_BYTE_DATA))
 			chip.page_size = 1;
-		}
-	}
 
 	if (chip.flags & AT24_FLAG_TAKE8ADDR)
 		num_addresses = 8;
@@ -628,21 +603,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	at24->offset_adj = at24_get_offset_adj(chip.flags);
 
 	writable = !(chip.flags & AT24_FLAG_READONLY);
-	if (writable) {
-		if (!use_smbus || use_smbus_write) {
-
-			unsigned write_max = chip.page_size;
-
-			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;
-		} else {
-			dev_warn(&client->dev,
-				"cannot write due to controller restrictions.");
-		}
-	}
+	if (writable)
+		at24->write_max = min_t(unsigned int, chip.page_size, io_limit);
 
 	/* use dummy devices for multiple-address chips */
 	for (i = 1; i < num_addresses; i++) {
@@ -703,12 +665,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	dev_info(&client->dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
 		chip.byte_len, client->name,
 		writable ? "writable" : "read-only", at24->write_max);
-	if (use_smbus == I2C_SMBUS_WORD_DATA ||
-	    use_smbus == I2C_SMBUS_BYTE_DATA) {
-		dev_notice(&client->dev, "Falling back to %s reads, "
-			   "performance will suffer\n", use_smbus ==
-			   I2C_SMBUS_WORD_DATA ? "word" : "byte");
-	}
 
 	/* export data to kernel code */
 	if (chip.setup)
-- 
2.15.0

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Bartosz Golaszewski]

2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Add regmap-based read function and instead of using three different
> read functions (standard, mac, serial) use just one and factor out the
> read offset adjustment for mac and serial to at24_adjust_read_offset.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rebased
> v3:
> - improve readability
> - re-introduce debug message
> - introduce at24_adjust_read_offset
> v4:
> - move offset adjustment calculation to probe function
> ---
>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 55 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 493e2b646..c16a9a495 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -75,6 +75,7 @@ struct at24_data {
>
>         unsigned write_max;
>         unsigned num_addresses;
> +       unsigned int offset_adj;
>
>         struct nvmem_config nvmem_config;
>         struct nvmem_device *nvmem;
> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>         return -ETIMEDOUT;
>  }
>

OK this looks better. The series is almost ready - just a couple more
nits I'd like to see fixed and we're done.

> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
> +                               unsigned int offset, size_t count)
> +{
> +       unsigned long timeout, read_time;
> +       struct at24_client *at24_client;
> +       struct i2c_client *client;
> +       struct regmap *regmap;
> +       int ret;
> +
> +       at24_client = at24_translate_offset(at24, &offset);
> +       regmap = at24_client->regmap;
> +       client = at24_client->client;
> +
> +       if (count > io_limit)
> +               count = io_limit;
> +
> +       /* adjust offset for mac and serial read ops */
> +       offset += at24->offset_adj;

Let's use '|=' here as it's safer (doesn't shift the bit if it's set
in both sides).

> +
> +       loop_until_timeout(timeout, read_time) {
> +               ret = regmap_bulk_read(regmap, offset, buf, count);
> +               dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
> +                       count, offset, ret, jiffies);
> +               if (!ret)
> +                       return count;
> +       }
> +
> +       return -ETIMEDOUT;
> +}
> +
>  static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
>                                     unsigned int offset, size_t count)
>  {
> @@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
>         while (count) {
>                 int     status;
>
> -               status = at24->read_func(at24, buf, off, count);
> +               status = at24_regmap_read(at24, buf, off, count);
>                 if (status < 0) {
>                         mutex_unlock(&at24->lock);
>                         pm_runtime_put(&client->dev);
> @@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
>         }
>  }
>
> +static unsigned int at24_get_offset_adj(u8 flags)
> +{
> +       if (flags & AT24_FLAG_MAC)
> +               return 0x90;

Let's stay consistent here and do BIT(4) | BIT(7)

> +       else if (flags & AT24_FLAG_SERIAL && 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.
> +                */
> +               return BIT(11);

Even though we only have a single line of code here and below, the
multi-line comment makes it seem like a block of code. Please wrap it
in braces so that nobody ever falls into a trap should someone want to
modify in the future.

> +       else if (flags & AT24_FLAG_SERIAL)
> +               /*
> +                * Otherwise the word address must begin with a '10' sequence,
> +                * regardless of the intended address.
> +                */
> +               return BIT(7);
> +       else
> +               return 0;
> +}
> +
>  static const struct regmap_config regmap_config_8 = {
>         .reg_bits = 8,
>         .val_bits = 8,
> @@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 return -EINVAL;
>         }
>
> +       at24->offset_adj = at24_get_offset_adj(chip.flags);
> +
>         if (chip.flags & AT24_FLAG_SERIAL) {
>                 at24->read_func = at24_eeprom_read_serial;
>         } else if (chip.flags & AT24_FLAG_MAC) {
> --
> 2.15.0
>
>

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit]

Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Add regmap-based read function and instead of using three different
>> read functions (standard, mac, serial) use just one and factor out the
>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> v2:
>> - rebased
>> v3:
>> - improve readability
>> - re-introduce debug message
>> - introduce at24_adjust_read_offset
>> v4:
>> - move offset adjustment calculation to probe function
>> ---
>>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index 493e2b646..c16a9a495 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -75,6 +75,7 @@ struct at24_data {
>>
>>         unsigned write_max;
>>         unsigned num_addresses;
>> +       unsigned int offset_adj;
>>
>>         struct nvmem_config nvmem_config;
>>         struct nvmem_device *nvmem;
>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>         return -ETIMEDOUT;
>>  }
>>
> 
> OK this looks better. The series is almost ready - just a couple more
> nits I'd like to see fixed and we're done.
> 
>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>> +                               unsigned int offset, size_t count)
>> +{
>> +       unsigned long timeout, read_time;
>> +       struct at24_client *at24_client;
>> +       struct i2c_client *client;
>> +       struct regmap *regmap;
>> +       int ret;
>> +
>> +       at24_client = at24_translate_offset(at24, &offset);
>> +       regmap = at24_client->regmap;
>> +       client = at24_client->client;
>> +
>> +       if (count > io_limit)
>> +               count = io_limit;
>> +
>> +       /* adjust offset for mac and serial read ops */
>> +       offset += at24->offset_adj;
> 
> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
> in both sides).
> 
To build an opinion on |= vs. += I checked the code in more detail plus
some datasheets, what lead to quite some question marks ..

Major issue is that offset and size in at24_read/write are not checked
currently. So we completely rely on the calling subsystem (nvmem).
The nvmem sysfs interface does such checking. However nvmem_device_read
does not. So maybe the nvmem core should be changed to do checking in
all cases. I add Srinivas as nvmem maintainer to the conversation
to hear his opinion.

If we have such checks then in general |= and += deliver the same result,
it's just a question of taste.

According to the at24mac602/at24mac402 datasheet the MAC is provided at:
24mac402 / EUI-48 -> position 0x9a - 0x9f
24mac602 / EUI-64 -> position 0x98 - 0x9f

Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
So the caller has to provide offset 0x08 to read the mac what is
greater than the chip size of 6 bytes.
So reading the mac via nvmem sysfs interface shouldn't be possible.

I saw that you submitted the 24macx02 code, did you test the driver
with one of these chips and I miss something?

Most likely we would have to change the driver so that the caller can
read the mac from offset 0.

Rgds, Heiner

>> +
>> +       loop_until_timeout(timeout, read_time) {
>> +               ret = regmap_bulk_read(regmap, offset, buf, count);
>> +               dev_dbg(&client->dev, "read %zu@%d --> %d (%ld)\n",
>> +                       count, offset, ret, jiffies);
>> +               if (!ret)
>> +                       return count;
>> +       }
>> +
>> +       return -ETIMEDOUT;
>> +}
>> +
>>  static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
>>                                     unsigned int offset, size_t count)
>>  {
>> @@ -531,7 +562,7 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
>>         while (count) {
>>                 int     status;
>>
>> -               status = at24->read_func(at24, buf, off, count);
>> +               status = at24_regmap_read(at24, buf, off, count);
>>                 if (status < 0) {
>>                         mutex_unlock(&at24->lock);
>>                         pm_runtime_put(&client->dev);
>> @@ -621,6 +652,27 @@ static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
>>         }
>>  }
>>
>> +static unsigned int at24_get_offset_adj(u8 flags)
>> +{
>> +       if (flags & AT24_FLAG_MAC)
>> +               return 0x90;
> 
> Let's stay consistent here and do BIT(4) | BIT(7)
> 
>> +       else if (flags & AT24_FLAG_SERIAL && 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.
>> +                */
>> +               return BIT(11);
> 
> Even though we only have a single line of code here and below, the
> multi-line comment makes it seem like a block of code. Please wrap it
> in braces so that nobody ever falls into a trap should someone want to
> modify in the future.
> 
>> +       else if (flags & AT24_FLAG_SERIAL)
>> +               /*
>> +                * Otherwise the word address must begin with a '10' sequence,
>> +                * regardless of the intended address.
>> +                */
>> +               return BIT(7);
>> +       else
>> +               return 0;
>> +}
>> +
>>  static const struct regmap_config regmap_config_8 = {
>>         .reg_bits = 8,
>>         .val_bits = 8,
>> @@ -749,6 +801,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>                 return -EINVAL;
>>         }
>>
>> +       at24->offset_adj = at24_get_offset_adj(chip.flags);
>> +
>>         if (chip.flags & AT24_FLAG_SERIAL) {
>>                 at24->read_func = at24_eeprom_read_serial;
>>         } else if (chip.flags & AT24_FLAG_MAC) {
>> --
>> 2.15.0
>>
>>
> 

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Bartosz Golaszewski]

2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>> Add regmap-based read function and instead of using three different
>>> read functions (standard, mac, serial) use just one and factor out the
>>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>> v2:
>>> - rebased
>>> v3:
>>> - improve readability
>>> - re-introduce debug message
>>> - introduce at24_adjust_read_offset
>>> v4:
>>> - move offset adjustment calculation to probe function
>>> ---
>>>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>>  1 file changed, 55 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index 493e2b646..c16a9a495 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -75,6 +75,7 @@ struct at24_data {
>>>
>>>         unsigned write_max;
>>>         unsigned num_addresses;
>>> +       unsigned int offset_adj;
>>>
>>>         struct nvmem_config nvmem_config;
>>>         struct nvmem_device *nvmem;
>>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>>         return -ETIMEDOUT;
>>>  }
>>>
>>
>> OK this looks better. The series is almost ready - just a couple more
>> nits I'd like to see fixed and we're done.
>>
>>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>>> +                               unsigned int offset, size_t count)
>>> +{
>>> +       unsigned long timeout, read_time;
>>> +       struct at24_client *at24_client;
>>> +       struct i2c_client *client;
>>> +       struct regmap *regmap;
>>> +       int ret;
>>> +
>>> +       at24_client = at24_translate_offset(at24, &offset);
>>> +       regmap = at24_client->regmap;
>>> +       client = at24_client->client;
>>> +
>>> +       if (count > io_limit)
>>> +               count = io_limit;
>>> +
>>> +       /* adjust offset for mac and serial read ops */
>>> +       offset += at24->offset_adj;
>>
>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>> in both sides).
>>
> To build an opinion on |= vs. += I checked the code in more detail plus
> some datasheets, what lead to quite some question marks ..
>
> Major issue is that offset and size in at24_read/write are not checked
> currently. So we completely rely on the calling subsystem (nvmem).
> The nvmem sysfs interface does such checking. However nvmem_device_read
> does not. So maybe the nvmem core should be changed to do checking in
> all cases. I add Srinivas as nvmem maintainer to the conversation
> to hear his opinion.
>
> If we have such checks then in general |= and += deliver the same result,
> it's just a question of taste.
>
> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
> 24mac402 / EUI-48 -> position 0x9a - 0x9f
> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>
> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
> So the caller has to provide offset 0x08 to read the mac what is
> greater than the chip size of 6 bytes.
> So reading the mac via nvmem sysfs interface shouldn't be possible.
>
> I saw that you submitted the 24macx02 code, did you test the driver
> with one of these chips and I miss something?
>

At the time when I submitted the support for at24cs (which I had
tested both for 8- and 16-bit addresses), Wolfram suggested that I
include support for at24mac too, but since I don't have such a chip, I
could not really test it. Looking at the note on page 21 of the
relevant datasheet, it's obvious it can't work. I must have missed
that at the time of writing the code.

Also: there's this patch[1] which looks like a workaround for this
problem. I'm Cc'ing the author.

@Claudiu: is that the case or do you actually have an EEPROM chip with
the MAC at a different offset? Could you by any chance test the
patch[2] from Heiner?

> Most likely we would have to change the driver so that the caller can
> read the mac from offset 0.
>
> Rgds, Heiner
>

Best regards,
Bartosz Golaszewski

[1] http://patchwork.ozlabs.org/patch/785106/
[2] http://patchwork.ozlabs.org/patch/840958/

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Claudiu Beznea]

On 24.11.2017 13:00, Bartosz Golaszewski wrote:
> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>> Add regmap-based read function and instead of using three different
>>>> read functions (standard, mac, serial) use just one and factor out the
>>>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>>>
>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>> ---
>>>> v2:
>>>> - rebased
>>>> v3:
>>>> - improve readability
>>>> - re-introduce debug message
>>>> - introduce at24_adjust_read_offset
>>>> v4:
>>>> - move offset adjustment calculation to probe function
>>>> ---
>>>>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 55 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>> index 493e2b646..c16a9a495 100644
>>>> --- a/drivers/misc/eeprom/at24.c
>>>> +++ b/drivers/misc/eeprom/at24.c
>>>> @@ -75,6 +75,7 @@ struct at24_data {
>>>>
>>>>         unsigned write_max;
>>>>         unsigned num_addresses;
>>>> +       unsigned int offset_adj;
>>>>
>>>>         struct nvmem_config nvmem_config;
>>>>         struct nvmem_device *nvmem;
>>>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>>>         return -ETIMEDOUT;
>>>>  }
>>>>
>>>
>>> OK this looks better. The series is almost ready - just a couple more
>>> nits I'd like to see fixed and we're done.
>>>
>>>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>>>> +                               unsigned int offset, size_t count)
>>>> +{
>>>> +       unsigned long timeout, read_time;
>>>> +       struct at24_client *at24_client;
>>>> +       struct i2c_client *client;
>>>> +       struct regmap *regmap;
>>>> +       int ret;
>>>> +
>>>> +       at24_client = at24_translate_offset(at24, &offset);
>>>> +       regmap = at24_client->regmap;
>>>> +       client = at24_client->client;
>>>> +
>>>> +       if (count > io_limit)
>>>> +               count = io_limit;
>>>> +
>>>> +       /* adjust offset for mac and serial read ops */
>>>> +       offset += at24->offset_adj;
>>>
>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>> in both sides).
>>>
>> To build an opinion on |= vs. += I checked the code in more detail plus
>> some datasheets, what lead to quite some question marks ..
>>
>> Major issue is that offset and size in at24_read/write are not checked
>> currently. So we completely rely on the calling subsystem (nvmem).
>> The nvmem sysfs interface does such checking. However nvmem_device_read
>> does not. So maybe the nvmem core should be changed to do checking in
>> all cases. I add Srinivas as nvmem maintainer to the conversation
>> to hear his opinion.
>>
>> If we have such checks then in general |= and += deliver the same result,
>> it's just a question of taste.
>>
>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>
>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
Moreover, if I remember good, in the initialization of the 24mac402 the
size is truncated at something which is power of 2. I don't if this is
for some historical reasons or not so that you can only read 4 bytes
instead of 6 for the EUI-48.

>> So the caller has to provide offset 0x08 to read the mac what is
>> greater than the chip size of 6 bytes.
>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>
>> I saw that you submitted the 24macx02 code, did you test the driver
>> with one of these chips and I miss something?
>>
> 
> At the time when I submitted the support for at24cs (which I had
> tested both for 8- and 16-bit addresses), Wolfram suggested that I
> include support for at24mac too, but since I don't have such a chip, I
> could not really test it. Looking at the note on page 21 of the
> relevant datasheet, it's obvious it can't work. I must have missed
> that at the time of writing the code.
> 
> Also: there's this patch[1] which looks like a workaround for this
> problem. I'm Cc'ing the author.
I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
and providing this offset via device tree was my first option in order
to not broke the initial functionality. Anyway, the device tree approach
as not accepted at that time, the usage of another DT binding was proposed
to me at that time but I didn't found that feasible, said about it on
mailing list but I didn't received any other inputs.

> 
> @Claudiu: is that the case or do you actually have an EEPROM chip with
> the MAC at a different offset? Could you by any chance test the
> patch[2] from Heiner?
I have chip at [3] with MAC at 0xfa.

Regarding the testing of patch [2], at this moment I haven't a board
with at24mac602 EEPROM. I will come back later to this thread as soon as
I will have one. Regarding the changes, if I remember good, the
at24->chip.byte_len is truncated at something which is power of 2,
in case of 24mac402 will be 4 not 6 as expected, so it should return
only 4 LSB bytes of MAC. Other than this it looks OK from my point
of view.

Thanks,
Claudiu

[3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
> 
>> Most likely we would have to change the driver so that the caller can
>> read the mac from offset 0.
>>
>> Rgds, Heiner
>>
> 
> Best regards,
> Bartosz Golaszewski
> 
> [1] http://patchwork.ozlabs.org/patch/785106/
> [2] http://patchwork.ozlabs.org/patch/840958/
> 

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Srinivas Kandagatla]

On 23/11/17 21:31, Heiner Kallweit wrote:
> To build an opinion on |= vs. += I checked the code in more detail plus
> some datasheets, what lead to quite some question marks ..
> 
> Major issue is that offset and size in at24_read/write are not checked
> currently. So we completely rely on the calling subsystem (nvmem).
> The nvmem sysfs interface does such checking. However nvmem_device_read
> does not. So maybe the nvmem core should be changed to do checking in
> all cases. I add Srinivas as nvmem maintainer to the conversation
> to hear his opinion.w.r.t nvmem I can see these sanity check are missing form read/writes.

we should probably add the check when we add the cell itself something 
like this below should do the job..

I will send a proper patch after testing..
------------------------>cut<--------------------------
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index d12e5de78e70..8ae865765754 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -320,11 +320,26 @@ static void nvmem_device_remove_all_cells(const 
struct nvmem_device *nvmem)
         }
  }

-static void nvmem_cell_add(struct nvmem_cell *cell)
+static int nvmem_cell_add(struct nvmem_cell *cell)
  {
+       struct nvmem_device *nvmem = cell->nvmem;
+
+       if (cell->offset >= nvmem->size)
+               return -EINVAL;
+
+       if (cell->bytes < nvmem->word_size)
+               return -EINVAL;
+
+       if (cell->offset + cell->bytes > nvmem->size)
+               cell->bytes = nvmem->size - cell->offset;
+
+       cell->bytes = round_down(cell->bytes, nvmem->word_size);
+
         mutex_lock(&nvmem_cells_mutex);
         list_add_tail(&cell->node, &nvmem_cells);
         mutex_unlock(&nvmem_cells_mutex);
+
+       return 0;
  }

  static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
@@ -377,7 +392,11 @@ static int nvmem_add_cells(struct nvmem_device *nvmem,
                         goto err;
                 }

-               nvmem_cell_add(cells[i]);
+               rval = nvmem_cell_add(cells[i]);
+               if (rval) {
+                       kfree(cells[i]);
+                       goto err;
+               }
         }

         nvmem->ncells = cfg->ncells;
@@ -830,7 +849,9 @@ struct nvmem_cell *of_nvmem_cell_get(struct 
device_node *np,
                 goto err_sanity;
         }

-       nvmem_cell_add(cell);
+       rval = nvmem_cell_add(cell);
+       if (rval)
+               goto err_sanity;

         return cell;
------------------------>cut<--------------------------

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit]

Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
> 
> 
> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>> Add regmap-based read function and instead of using three different
>>>>> read functions (standard, mac, serial) use just one and factor out the
>>>>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>>>>
>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>> ---
>>>>> v2:
>>>>> - rebased
>>>>> v3:
>>>>> - improve readability
>>>>> - re-introduce debug message
>>>>> - introduce at24_adjust_read_offset
>>>>> v4:
>>>>> - move offset adjustment calculation to probe function
>>>>> ---
>>>>>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>  1 file changed, 55 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>>> index 493e2b646..c16a9a495 100644
>>>>> --- a/drivers/misc/eeprom/at24.c
>>>>> +++ b/drivers/misc/eeprom/at24.c
>>>>> @@ -75,6 +75,7 @@ struct at24_data {
>>>>>
>>>>>         unsigned write_max;
>>>>>         unsigned num_addresses;
>>>>> +       unsigned int offset_adj;
>>>>>
>>>>>         struct nvmem_config nvmem_config;
>>>>>         struct nvmem_device *nvmem;
>>>>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>>>>         return -ETIMEDOUT;
>>>>>  }
>>>>>
>>>>
>>>> OK this looks better. The series is almost ready - just a couple more
>>>> nits I'd like to see fixed and we're done.
>>>>
>>>>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>>>>> +                               unsigned int offset, size_t count)
>>>>> +{
>>>>> +       unsigned long timeout, read_time;
>>>>> +       struct at24_client *at24_client;
>>>>> +       struct i2c_client *client;
>>>>> +       struct regmap *regmap;
>>>>> +       int ret;
>>>>> +
>>>>> +       at24_client = at24_translate_offset(at24, &offset);
>>>>> +       regmap = at24_client->regmap;
>>>>> +       client = at24_client->client;
>>>>> +
>>>>> +       if (count > io_limit)
>>>>> +               count = io_limit;
>>>>> +
>>>>> +       /* adjust offset for mac and serial read ops */
>>>>> +       offset += at24->offset_adj;
>>>>
>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>> in both sides).
>>>>
>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>> some datasheets, what lead to quite some question marks ..
>>>
>>> Major issue is that offset and size in at24_read/write are not checked
>>> currently. So we completely rely on the calling subsystem (nvmem).
>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>> does not. So maybe the nvmem core should be changed to do checking in
>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>> to hear his opinion.
>>>
>>> If we have such checks then in general |= and += deliver the same result,
>>> it's just a question of taste.
>>>
>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>
>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
> Moreover, if I remember good, in the initialization of the 24mac402 the
> size is truncated at something which is power of 2. I don't if this is
> for some historical reasons or not so that you can only read 4 bytes
> instead of 6 for the EUI-48.
> 
Very good point! Actually I don't see any real need for this check.
I thin we lose nothing if we simply remove it.

>>> So the caller has to provide offset 0x08 to read the mac what is
>>> greater than the chip size of 6 bytes.
>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>
>>> I saw that you submitted the 24macx02 code, did you test the driver
>>> with one of these chips and I miss something?
>>>
>>
>> At the time when I submitted the support for at24cs (which I had
>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>> include support for at24mac too, but since I don't have such a chip, I
>> could not really test it. Looking at the note on page 21 of the
>> relevant datasheet, it's obvious it can't work. I must have missed
>> that at the time of writing the code.
>>
>> Also: there's this patch[1] which looks like a workaround for this
>> problem. I'm Cc'ing the author.
> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
> and providing this offset via device tree was my first option in order
> to not broke the initial functionality. Anyway, the device tree approach
> as not accepted at that time, the usage of another DT binding was proposed
> to me at that time but I didn't found that feasible, said about it on
> mailing list but I didn't received any other inputs.
> 

My patch works for the two MAC EEPROM's currently supported by the driver,
but not for others like the one mentioned by you (Microchip 24AA02E48 and
friends) because they have other start addresses for the MAC.

To deal with this situation we would have to add the MAC start address to
the chip config data. In addition the proposed DT parameter would helpful
in case chips are used which are not yet supported by the driver
(similar to the recently introduced "size" parameter).


Bartosz, can we first go with the additional sanity checking in
at24_read/write (if fine with you) and my i2c refactoring
(will resubmit with the last small change)?
Then the driver is somewhat smaller and simpler what makes further
improvements easier.

Kind regards,
Heiner

>>
>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>> the MAC at a different offset? Could you by any chance test the
>> patch[2] from Heiner?
> I have chip at [3] with MAC at 0xfa.
> 
> Regarding the testing of patch [2], at this moment I haven't a board
> with at24mac602 EEPROM. I will come back later to this thread as soon as
> I will have one. Regarding the changes, if I remember good, the
> at24->chip.byte_len is truncated at something which is power of 2,
> in case of 24mac402 will be 4 not 6 as expected, so it should return
> only 4 LSB bytes of MAC. Other than this it looks OK from my point
> of view.
> 
> Thanks,
> Claudiu
> 
> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>
>>> Most likely we would have to change the driver so that the caller can
>>> read the mac from offset 0.
>>>
>>> Rgds, Heiner
>>>
>>
>> Best regards,
>> Bartosz Golaszewski
>>
>> [1] http://patchwork.ozlabs.org/patch/785106/
>> [2] http://patchwork.ozlabs.org/patch/840958/
>>
> 

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit]

Am 24.11.2017 um 22:17 schrieb Heiner Kallweit:
> Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
>>
>>
>> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>>> 2017-11-22 22:12 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>>> Add regmap-based read function and instead of using three different
>>>>>> read functions (standard, mac, serial) use just one and factor out the
>>>>>> read offset adjustment for mac and serial to at24_adjust_read_offset.
>>>>>>
>>>>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>>>>> ---
>>>>>> v2:
>>>>>> - rebased
>>>>>> v3:
>>>>>> - improve readability
>>>>>> - re-introduce debug message
>>>>>> - introduce at24_adjust_read_offset
>>>>>> v4:
>>>>>> - move offset adjustment calculation to probe function
>>>>>> ---
>>>>>>  drivers/misc/eeprom/at24.c | 56 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>>>  1 file changed, 55 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>>>>> index 493e2b646..c16a9a495 100644
>>>>>> --- a/drivers/misc/eeprom/at24.c
>>>>>> +++ b/drivers/misc/eeprom/at24.c
>>>>>> @@ -75,6 +75,7 @@ struct at24_data {
>>>>>>
>>>>>>         unsigned write_max;
>>>>>>         unsigned num_addresses;
>>>>>> +       unsigned int offset_adj;
>>>>>>
>>>>>>         struct nvmem_config nvmem_config;
>>>>>>         struct nvmem_device *nvmem;
>>>>>> @@ -312,6 +313,36 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>>>>>>         return -ETIMEDOUT;
>>>>>>  }
>>>>>>
>>>>>
>>>>> OK this looks better. The series is almost ready - just a couple more
>>>>> nits I'd like to see fixed and we're done.
>>>>>
>>>>>> +static ssize_t at24_regmap_read(struct at24_data *at24, char *buf,
>>>>>> +                               unsigned int offset, size_t count)
>>>>>> +{
>>>>>> +       unsigned long timeout, read_time;
>>>>>> +       struct at24_client *at24_client;
>>>>>> +       struct i2c_client *client;
>>>>>> +       struct regmap *regmap;
>>>>>> +       int ret;
>>>>>> +
>>>>>> +       at24_client = at24_translate_offset(at24, &offset);
>>>>>> +       regmap = at24_client->regmap;
>>>>>> +       client = at24_client->client;
>>>>>> +
>>>>>> +       if (count > io_limit)
>>>>>> +               count = io_limit;
>>>>>> +
>>>>>> +       /* adjust offset for mac and serial read ops */
>>>>>> +       offset += at24->offset_adj;
>>>>>
>>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>>> in both sides).
>>>>>
>>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>>> some datasheets, what lead to quite some question marks ..
>>>>
>>>> Major issue is that offset and size in at24_read/write are not checked
>>>> currently. So we completely rely on the calling subsystem (nvmem).
>>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>>> does not. So maybe the nvmem core should be changed to do checking in
>>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>>> to hear his opinion.
>>>>
>>>> If we have such checks then in general |= and += deliver the same result,
>>>> it's just a question of taste.
>>>>
>>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>>
>>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
>> Moreover, if I remember good, in the initialization of the 24mac402 the
>> size is truncated at something which is power of 2. I don't if this is
>> for some historical reasons or not so that you can only read 4 bytes
>> instead of 6 for the EUI-48.
>>
> Very good point! Actually I don't see any real need for this check.
> I thin we lose nothing if we simply remove it.
> 
Just see that this check only prints a warning, the actual issue comes
from the ilog2 in AT24_DEVICE_MAGIC.
When we need one more config parameter anyway (for the MAC start address),
then IMO it would make sense to convert the magic to a proper struct.
I'll spend a few thoughts on that.

>>>> So the caller has to provide offset 0x08 to read the mac what is
>>>> greater than the chip size of 6 bytes.
>>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>>
>>>> I saw that you submitted the 24macx02 code, did you test the driver
>>>> with one of these chips and I miss something?
>>>>
>>>
>>> At the time when I submitted the support for at24cs (which I had
>>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>>> include support for at24mac too, but since I don't have such a chip, I
>>> could not really test it. Looking at the note on page 21 of the
>>> relevant datasheet, it's obvious it can't work. I must have missed
>>> that at the time of writing the code.
>>>
>>> Also: there's this patch[1] which looks like a workaround for this
>>> problem. I'm Cc'ing the author.
>> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
>> and providing this offset via device tree was my first option in order
>> to not broke the initial functionality. Anyway, the device tree approach
>> as not accepted at that time, the usage of another DT binding was proposed
>> to me at that time but I didn't found that feasible, said about it on
>> mailing list but I didn't received any other inputs.
>>
> 
> My patch works for the two MAC EEPROM's currently supported by the driver,
> but not for others like the one mentioned by you (Microchip 24AA02E48 and
> friends) because they have other start addresses for the MAC.
> 
> To deal with this situation we would have to add the MAC start address to
> the chip config data. In addition the proposed DT parameter would helpful
> in case chips are used which are not yet supported by the driver
> (similar to the recently introduced "size" parameter).
> 
> 
> Bartosz, can we first go with the additional sanity checking in
> at24_read/write (if fine with you) and my i2c refactoring
> (will resubmit with the last small change)?
> Then the driver is somewhat smaller and simpler what makes further
> improvements easier.
> 
> Kind regards,
> Heiner
> 
>>>
>>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>>> the MAC at a different offset? Could you by any chance test the
>>> patch[2] from Heiner?
>> I have chip at [3] with MAC at 0xfa.
>>
>> Regarding the testing of patch [2], at this moment I haven't a board
>> with at24mac602 EEPROM. I will come back later to this thread as soon as
>> I will have one. Regarding the changes, if I remember good, the
>> at24->chip.byte_len is truncated at something which is power of 2,
>> in case of 24mac402 will be 4 not 6 as expected, so it should return
>> only 4 LSB bytes of MAC. Other than this it looks OK from my point
>> of view.
>>
>> Thanks,
>> Claudiu
>>
>> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>>
>>>> Most likely we would have to change the driver so that the caller can
>>>> read the mac from offset 0.
>>>>
>>>> Rgds, Heiner
>>>>
>>>
>>> Best regards,
>>> Bartosz Golaszewski
>>>
>>> [1] http://patchwork.ozlabs.org/patch/785106/
>>> [2] http://patchwork.ozlabs.org/patch/840958/
>>>
>>
> 

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Bartosz Golaszewski]

2017-11-24 23:13 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 24.11.2017 um 22:17 schrieb Heiner Kallweit:
>> Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
>>>
>>>
>>> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>>>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>>>> in both sides).
>>>>>>
>>>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>>>> some datasheets, what lead to quite some question marks ..
>>>>>
>>>>> Major issue is that offset and size in at24_read/write are not checked
>>>>> currently. So we completely rely on the calling subsystem (nvmem).
>>>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>>>> does not. So maybe the nvmem core should be changed to do checking in
>>>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>>>> to hear his opinion.
>>>>>
>>>>> If we have such checks then in general |= and += deliver the same result,
>>>>> it's just a question of taste.
>>>>>
>>>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>>>
>>>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
>>> Moreover, if I remember good, in the initialization of the 24mac402 the
>>> size is truncated at something which is power of 2. I don't if this is
>>> for some historical reasons or not so that you can only read 4 bytes
>>> instead of 6 for the EUI-48.
>>>
>> Very good point! Actually I don't see any real need for this check.
>> I thin we lose nothing if we simply remove it.
>>
> Just see that this check only prints a warning, the actual issue comes
> from the ilog2 in AT24_DEVICE_MAGIC.
> When we need one more config parameter anyway (for the MAC start address),
> then IMO it would make sense to convert the magic to a proper struct.
> I'll spend a few thoughts on that.
>

One thing that bothers me is that we now have a feature in the kernel
(reading the MAC address from at24mac402/at24mac604) which doesn't
work and any patch fixing it, that at the same time significantly
changes the code would likely not make its way into the stable
branches.

I see it like this: I would like to merge commit ("eeprom: at24: fix
reading from 24MAC402/24MAC602") first: that would at least fix the
at24mac402 case. For at24mac602 it would be ok to create the MAGIC
manually without the call to ilog2(). Such changes would then be
submitted for linux-stable.

After these two patches, I will merge the regmap conversion patches
and we would base any subsequent development (e.g. magic -> struct
conversion) on top of that. How about that?

Best regards,
Bartosz Golaszewski

>>>>> So the caller has to provide offset 0x08 to read the mac what is
>>>>> greater than the chip size of 6 bytes.
>>>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>>>
>>>>> I saw that you submitted the 24macx02 code, did you test the driver
>>>>> with one of these chips and I miss something?
>>>>>
>>>>
>>>> At the time when I submitted the support for at24cs (which I had
>>>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>>>> include support for at24mac too, but since I don't have such a chip, I
>>>> could not really test it. Looking at the note on page 21 of the
>>>> relevant datasheet, it's obvious it can't work. I must have missed
>>>> that at the time of writing the code.
>>>>
>>>> Also: there's this patch[1] which looks like a workaround for this
>>>> problem. I'm Cc'ing the author.
>>> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
>>> and providing this offset via device tree was my first option in order
>>> to not broke the initial functionality. Anyway, the device tree approach
>>> as not accepted at that time, the usage of another DT binding was proposed
>>> to me at that time but I didn't found that feasible, said about it on
>>> mailing list but I didn't received any other inputs.
>>>
>>
>> My patch works for the two MAC EEPROM's currently supported by the driver,
>> but not for others like the one mentioned by you (Microchip 24AA02E48 and
>> friends) because they have other start addresses for the MAC.
>>
>> To deal with this situation we would have to add the MAC start address to
>> the chip config data. In addition the proposed DT parameter would helpful
>> in case chips are used which are not yet supported by the driver
>> (similar to the recently introduced "size" parameter).
>>
>>
>> Bartosz, can we first go with the additional sanity checking in
>> at24_read/write (if fine with you) and my i2c refactoring
>> (will resubmit with the last small change)?
>> Then the driver is somewhat smaller and simpler what makes further
>> improvements easier.
>>
>> Kind regards,
>> Heiner
>>
>>>>
>>>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>>>> the MAC at a different offset? Could you by any chance test the
>>>> patch[2] from Heiner?
>>> I have chip at [3] with MAC at 0xfa.
>>>
>>> Regarding the testing of patch [2], at this moment I haven't a board
>>> with at24mac602 EEPROM. I will come back later to this thread as soon as
>>> I will have one. Regarding the changes, if I remember good, the
>>> at24->chip.byte_len is truncated at something which is power of 2,
>>> in case of 24mac402 will be 4 not 6 as expected, so it should return
>>> only 4 LSB bytes of MAC. Other than this it looks OK from my point
>>> of view.
>>>
>>> Thanks,
>>> Claudiu
>>>
>>> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>>>
>>>>> Most likely we would have to change the driver so that the caller can
>>>>> read the mac from offset 0.
>>>>>
>>>>> Rgds, Heiner
>>>>>
>>>>
>>>> Best regards,
>>>> Bartosz Golaszewski
>>>>
>>>> [1] http://patchwork.ozlabs.org/patch/785106/
>>>> [2] http://patchwork.ozlabs.org/patch/840958/
>>>>
>>>
>>
>

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Heiner Kallweit]

Am 26.11.2017 um 21:27 schrieb Bartosz Golaszewski:
> 2017-11-24 23:13 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Am 24.11.2017 um 22:17 schrieb Heiner Kallweit:
>>> Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
>>>>
>>>>
>>>> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>>>>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>>>>> in both sides).
>>>>>>>
>>>>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>>>>> some datasheets, what lead to quite some question marks ..
>>>>>>
>>>>>> Major issue is that offset and size in at24_read/write are not checked
>>>>>> currently. So we completely rely on the calling subsystem (nvmem).
>>>>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>>>>> does not. So maybe the nvmem core should be changed to do checking in
>>>>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>>>>> to hear his opinion.
>>>>>>
>>>>>> If we have such checks then in general |= and += deliver the same result,
>>>>>> it's just a question of taste.
>>>>>>
>>>>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>>>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>>>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>>>>
>>>>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>>>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
>>>> Moreover, if I remember good, in the initialization of the 24mac402 the
>>>> size is truncated at something which is power of 2. I don't if this is
>>>> for some historical reasons or not so that you can only read 4 bytes
>>>> instead of 6 for the EUI-48.
>>>>
>>> Very good point! Actually I don't see any real need for this check.
>>> I thin we lose nothing if we simply remove it.
>>>
>> Just see that this check only prints a warning, the actual issue comes
>> from the ilog2 in AT24_DEVICE_MAGIC.
>> When we need one more config parameter anyway (for the MAC start address),
>> then IMO it would make sense to convert the magic to a proper struct.
>> I'll spend a few thoughts on that.
>>
> 
> One thing that bothers me is that we now have a feature in the kernel
> (reading the MAC address from at24mac402/at24mac604) which doesn't
> work and any patch fixing it, that at the same time significantly
> changes the code would likely not make its way into the stable
> branches.
> 
> I see it like this: I would like to merge commit ("eeprom: at24: fix
> reading from 24MAC402/24MAC602") first: that would at least fix the
> at24mac402 case. For at24mac602 it would be ok to create the MAGIC
> manually without the call to ilog2(). Such changes would then be
> submitted for linux-stable.
> 
I think it's opposite, 24MAC402 causes the trouble due to byte_len = 6.
Approach is fine with me, are you going to prepare the fix for
replacing the magic for 24MAC402 ?

> After these two patches, I will merge the regmap conversion patches
> and we would base any subsequent development (e.g. magic -> struct
> conversion) on top of that. How about that?
> 
Also fine with me, then I will send the (hopefully) final series
tonight incl. the last small requested change.

> Best regards,
> Bartosz Golaszewski
> 
>>>>>> So the caller has to provide offset 0x08 to read the mac what is
>>>>>> greater than the chip size of 6 bytes.
>>>>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>>>>
>>>>>> I saw that you submitted the 24macx02 code, did you test the driver
>>>>>> with one of these chips and I miss something?
>>>>>>
>>>>>
>>>>> At the time when I submitted the support for at24cs (which I had
>>>>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>>>>> include support for at24mac too, but since I don't have such a chip, I
>>>>> could not really test it. Looking at the note on page 21 of the
>>>>> relevant datasheet, it's obvious it can't work. I must have missed
>>>>> that at the time of writing the code.
>>>>>
>>>>> Also: there's this patch[1] which looks like a workaround for this
>>>>> problem. I'm Cc'ing the author.
>>>> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
>>>> and providing this offset via device tree was my first option in order
>>>> to not broke the initial functionality. Anyway, the device tree approach
>>>> as not accepted at that time, the usage of another DT binding was proposed
>>>> to me at that time but I didn't found that feasible, said about it on
>>>> mailing list but I didn't received any other inputs.
>>>>
>>>
>>> My patch works for the two MAC EEPROM's currently supported by the driver,
>>> but not for others like the one mentioned by you (Microchip 24AA02E48 and
>>> friends) because they have other start addresses for the MAC.
>>>
>>> To deal with this situation we would have to add the MAC start address to
>>> the chip config data. In addition the proposed DT parameter would helpful
>>> in case chips are used which are not yet supported by the driver
>>> (similar to the recently introduced "size" parameter).
>>>
>>>
>>> Bartosz, can we first go with the additional sanity checking in
>>> at24_read/write (if fine with you) and my i2c refactoring
>>> (will resubmit with the last small change)?
>>> Then the driver is somewhat smaller and simpler what makes further
>>> improvements easier.
>>>
>>> Kind regards,
>>> Heiner
>>>
>>>>>
>>>>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>>>>> the MAC at a different offset? Could you by any chance test the
>>>>> patch[2] from Heiner?
>>>> I have chip at [3] with MAC at 0xfa.
>>>>
>>>> Regarding the testing of patch [2], at this moment I haven't a board
>>>> with at24mac602 EEPROM. I will come back later to this thread as soon as
>>>> I will have one. Regarding the changes, if I remember good, the
>>>> at24->chip.byte_len is truncated at something which is power of 2,
>>>> in case of 24mac402 will be 4 not 6 as expected, so it should return
>>>> only 4 LSB bytes of MAC. Other than this it looks OK from my point
>>>> of view.
>>>>
>>>> Thanks,
>>>> Claudiu
>>>>
>>>> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>>>>
>>>>>> Most likely we would have to change the driver so that the caller can
>>>>>> read the mac from offset 0.
>>>>>>
>>>>>> Rgds, Heiner
>>>>>>
>>>>>
>>>>> Best regards,
>>>>> Bartosz Golaszewski
>>>>>
>>>>> [1] http://patchwork.ozlabs.org/patch/785106/
>>>>> [2] http://patchwork.ozlabs.org/patch/840958/
>>>>>
>>>>
>>>
>>
> 

Re: [PATCH v4 5/7] eeprom: at24: add regmap-based read function

[Bartosz Golaszewski]

2017-11-27 7:24 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 26.11.2017 um 21:27 schrieb Bartosz Golaszewski:
>> 2017-11-24 23:13 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>> Am 24.11.2017 um 22:17 schrieb Heiner Kallweit:
>>>> Am 24.11.2017 um 18:35 schrieb Claudiu Beznea:
>>>>>
>>>>>
>>>>> On 24.11.2017 13:00, Bartosz Golaszewski wrote:
>>>>>> 2017-11-23 22:31 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>>>>>> Am 23.11.2017 um 17:40 schrieb Bartosz Golaszewski:
>>>>>>>> Let's use '|=' here as it's safer (doesn't shift the bit if it's set
>>>>>>>> in both sides).
>>>>>>>>
>>>>>>> To build an opinion on |= vs. += I checked the code in more detail plus
>>>>>>> some datasheets, what lead to quite some question marks ..
>>>>>>>
>>>>>>> Major issue is that offset and size in at24_read/write are not checked
>>>>>>> currently. So we completely rely on the calling subsystem (nvmem).
>>>>>>> The nvmem sysfs interface does such checking. However nvmem_device_read
>>>>>>> does not. So maybe the nvmem core should be changed to do checking in
>>>>>>> all cases. I add Srinivas as nvmem maintainer to the conversation
>>>>>>> to hear his opinion.
>>>>>>>
>>>>>>> If we have such checks then in general |= and += deliver the same result,
>>>>>>> it's just a question of taste.
>>>>>>>
>>>>>>> According to the at24mac602/at24mac402 datasheet the MAC is provided at:
>>>>>>> 24mac402 / EUI-48 -> position 0x9a - 0x9f
>>>>>>> 24mac602 / EUI-64 -> position 0x98 - 0x9f
>>>>>>>
>>>>>>> Size of the 24mac402 is defined as 48 bit = 6 byte and the effective
>>>>>>> offset in at24_eeprom_read_mac is 0x90 + offset provided by caller.
>>>>> Moreover, if I remember good, in the initialization of the 24mac402 the
>>>>> size is truncated at something which is power of 2. I don't if this is
>>>>> for some historical reasons or not so that you can only read 4 bytes
>>>>> instead of 6 for the EUI-48.
>>>>>
>>>> Very good point! Actually I don't see any real need for this check.
>>>> I thin we lose nothing if we simply remove it.
>>>>
>>> Just see that this check only prints a warning, the actual issue comes
>>> from the ilog2 in AT24_DEVICE_MAGIC.
>>> When we need one more config parameter anyway (for the MAC start address),
>>> then IMO it would make sense to convert the magic to a proper struct.
>>> I'll spend a few thoughts on that.
>>>
>>
>> One thing that bothers me is that we now have a feature in the kernel
>> (reading the MAC address from at24mac402/at24mac604) which doesn't
>> work and any patch fixing it, that at the same time significantly
>> changes the code would likely not make its way into the stable
>> branches.
>>
>> I see it like this: I would like to merge commit ("eeprom: at24: fix
>> reading from 24MAC402/24MAC602") first: that would at least fix the
>> at24mac402 case. For at24mac602 it would be ok to create the MAGIC
>> manually without the call to ilog2(). Such changes would then be
>> submitted for linux-stable.
>>
> I think it's opposite, 24MAC402 causes the trouble due to byte_len = 6.

Indeed.

> Approach is fine with me, are you going to prepare the fix for
> replacing the magic for 24MAC402 ?
>

Yes, I'll post it later today.

>> After these two patches, I will merge the regmap conversion patches
>> and we would base any subsequent development (e.g. magic -> struct
>> conversion) on top of that. How about that?
>>
> Also fine with me, then I will send the (hopefully) final series
> tonight incl. the last small requested change.
>

Ok, I'll give it a last testing spin on top of 4.15-rc1 and hopefully
we can have it in by Wednesday.

Thanks,
Bartosz

>> Best regards,
>> Bartosz Golaszewski
>>
>>>>>>> So the caller has to provide offset 0x08 to read the mac what is
>>>>>>> greater than the chip size of 6 bytes.
>>>>>>> So reading the mac via nvmem sysfs interface shouldn't be possible.
>>>>>>>
>>>>>>> I saw that you submitted the 24macx02 code, did you test the driver
>>>>>>> with one of these chips and I miss something?
>>>>>>>
>>>>>>
>>>>>> At the time when I submitted the support for at24cs (which I had
>>>>>> tested both for 8- and 16-bit addresses), Wolfram suggested that I
>>>>>> include support for at24mac too, but since I don't have such a chip, I
>>>>>> could not really test it. Looking at the note on page 21 of the
>>>>>> relevant datasheet, it's obvious it can't work. I must have missed
>>>>>> that at the time of writing the code.
>>>>>>
>>>>>> Also: there's this patch[1] which looks like a workaround for this
>>>>>> problem. I'm Cc'ing the author.
>>>>> I tried to make this driver work for chip at [3] which EUI-48 is located at 0xfa
>>>>> and providing this offset via device tree was my first option in order
>>>>> to not broke the initial functionality. Anyway, the device tree approach
>>>>> as not accepted at that time, the usage of another DT binding was proposed
>>>>> to me at that time but I didn't found that feasible, said about it on
>>>>> mailing list but I didn't received any other inputs.
>>>>>
>>>>
>>>> My patch works for the two MAC EEPROM's currently supported by the driver,
>>>> but not for others like the one mentioned by you (Microchip 24AA02E48 and
>>>> friends) because they have other start addresses for the MAC.
>>>>
>>>> To deal with this situation we would have to add the MAC start address to
>>>> the chip config data. In addition the proposed DT parameter would helpful
>>>> in case chips are used which are not yet supported by the driver
>>>> (similar to the recently introduced "size" parameter).
>>>>
>>>>
>>>> Bartosz, can we first go with the additional sanity checking in
>>>> at24_read/write (if fine with you) and my i2c refactoring
>>>> (will resubmit with the last small change)?
>>>> Then the driver is somewhat smaller and simpler what makes further
>>>> improvements easier.
>>>>
>>>> Kind regards,
>>>> Heiner
>>>>
>>>>>>
>>>>>> @Claudiu: is that the case or do you actually have an EEPROM chip with
>>>>>> the MAC at a different offset? Could you by any chance test the
>>>>>> patch[2] from Heiner?
>>>>> I have chip at [3] with MAC at 0xfa.
>>>>>
>>>>> Regarding the testing of patch [2], at this moment I haven't a board
>>>>> with at24mac602 EEPROM. I will come back later to this thread as soon as
>>>>> I will have one. Regarding the changes, if I remember good, the
>>>>> at24->chip.byte_len is truncated at something which is power of 2,
>>>>> in case of 24mac402 will be 4 not 6 as expected, so it should return
>>>>> only 4 LSB bytes of MAC. Other than this it looks OK from my point
>>>>> of view.
>>>>>
>>>>> Thanks,
>>>>> Claudiu
>>>>>
>>>>> [3] http://ww1.microchip.com/downloads/en/DeviceDoc/20002124G.pdf
>>>>>>
>>>>>>> Most likely we would have to change the driver so that the caller can
>>>>>>> read the mac from offset 0.
>>>>>>>
>>>>>>> Rgds, Heiner
>>>>>>>
>>>>>>
>>>>>> Best regards,
>>>>>> Bartosz Golaszewski
>>>>>>
>>>>>> [1] http://patchwork.ozlabs.org/patch/785106/
>>>>>> [2] http://patchwork.ozlabs.org/patch/840958/
>>>>>>
>>>>>
>>>>
>>>
>>
>