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

[PATCH 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.

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  | 385 +++++++++-----------------------------------
 2 files changed, 79 insertions(+), 307 deletions(-)

-- 
2.15.0

[PATCH v2 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
---
 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 v2 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
---
 drivers/misc/eeprom/at24.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 911cce8ec..1411fa029 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,7 +280,7 @@ 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,
@@ -290,7 +290,7 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
 	struct i2c_client *client;
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 
 	if (count > io_limit)
 		count = io_limit;
@@ -324,7 +324,7 @@ static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
 	u8 msgbuf[2];
 
 	memset(msg, 0, sizeof(msg));
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 
 	if (count > io_limit)
 		count = io_limit;
@@ -373,7 +373,7 @@ static ssize_t at24_eeprom_read_serial(struct at24_data *at24, char *buf,
 	u8 addrbuf[2];
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 
 	memset(msg, 0, sizeof(msg));
 	msg[0].addr = client->addr;
@@ -426,7 +426,7 @@ static ssize_t at24_eeprom_read_mac(struct at24_data *at24, char *buf,
 	u8 addrbuf[2];
 	int status;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 
 	memset(msg, 0, sizeof(msg));
 	msg[0].addr = client->addr;
@@ -481,7 +481,7 @@ static ssize_t at24_eeprom_write_smbus_block(struct at24_data *at24,
 	struct i2c_client *client;
 	ssize_t status = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 	count = at24_adjust_write_count(at24, offset, count);
 
 	loop_until_timeout(timeout, write_time) {
@@ -508,7 +508,7 @@ static ssize_t at24_eeprom_write_smbus_byte(struct at24_data *at24,
 	struct i2c_client *client;
 	ssize_t status = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 
 	loop_until_timeout(timeout, write_time) {
 		status = i2c_smbus_write_byte_data(client, offset, buf[0]);
@@ -534,7 +534,7 @@ static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 	ssize_t status = 0;
 	int i = 0;
 
-	client = at24_translate_offset(at24, &offset);
+	client = at24_translate_offset(at24, &offset)->client;
 	count = at24_adjust_write_count(at24, offset, count);
 
 	msg.addr = client->addr;
@@ -574,7 +574,7 @@ 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);
+	client = at24_translate_offset(at24, &off)->client;
 
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret < 0) {
@@ -619,7 +619,7 @@ 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);
+	client = at24_translate_offset(at24, &off)->client;
 
 	ret = pm_runtime_get_sync(&client->dev);
 	if (ret < 0) {
-- 
2.15.0

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

[Bartosz Golaszewski]

2017-11-16 21:26 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> 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
> ---
>  drivers/misc/eeprom/at24.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 911cce8ec..1411fa029 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,7 +280,7 @@ 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,
> @@ -290,7 +290,7 @@ static ssize_t at24_eeprom_read_smbus(struct at24_data *at24, char *buf,
>         struct i2c_client *client;
>         int status;
>
> -       client = at24_translate_offset(at24, &offset);
> +       client = at24_translate_offset(at24, &offset)->client;

Please do something like:

struct at24_client *at24;
struct i2c_client *client;

at24 = at24_translate_offset(at24, &offset);
client = at24->client;

I find it more readable like that and we don't hide the type of the
return value of at24_translate_offset().

Thanks,
Bartosz

[PATCH v2 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
---
 drivers/misc/eeprom/at24.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 1411fa029..c0c59575e 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -525,6 +525,22 @@ 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 regmap *regmap;
+
+	regmap = at24_translate_offset(at24, &offset)->regmap;
+	count = at24_adjust_write_count(at24, offset, count);
+
+	loop_until_timeout(timeout, write_time)
+		if (!regmap_bulk_write(regmap, offset, buf, count))
+			return count;
+
+	return -ETIMEDOUT;
+}
+
 static ssize_t at24_eeprom_write_i2c(struct at24_data *at24, const char *buf,
 				     unsigned int offset, size_t count)
 {
@@ -636,7 +652,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

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

[Bartosz Golaszewski]

2017-11-16 21:26 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Add a regmap-based write function.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rebased
> ---
>  drivers/misc/eeprom/at24.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 1411fa029..c0c59575e 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -525,6 +525,22 @@ 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 regmap *regmap;
> +
> +       regmap = at24_translate_offset(at24, &offset)->regmap;
> +       count = at24_adjust_write_count(at24, offset, count);
> +
> +       loop_until_timeout(timeout, write_time)
> +               if (!regmap_bulk_write(regmap, offset, buf, count))
> +                       return count;
> +

Please do this:

int rv;

rv = regmap_bulk_write(...);
if (rv == 0)
    return count;

I'd like to stay as readable as possible and making the return value
type visible is always good.

Also: it would be nice if you could keep the dev_dbg() call from the
original write function.

Thanks,
Bartosz

[PATCH v2 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
---
 drivers/misc/eeprom/at24.c | 111 ---------------------------------------------
 1 file changed, 111 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c0c59575e..28ea1a950 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;
 
@@ -473,58 +469,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 i2c_client *client;
-	ssize_t status = 0;
-
-	client = at24_translate_offset(at24, &offset)->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 i2c_client *client;
-	ssize_t status = 0;
-
-	client = at24_translate_offset(at24, &offset)->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)
 {
@@ -541,45 +485,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 i2c_client *client;
-	struct i2c_msg msg;
-	ssize_t status = 0;
-	int i = 0;
-
-	client = at24_translate_offset(at24, &offset)->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;
@@ -809,7 +714,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;
 
@@ -833,15 +737,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) {
@@ -853,12 +748,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 v2 5/7] eeprom: at24: add regmap-based read functions

[Heiner Kallweit]

Add regmap-based read functions.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- rebased
---
 drivers/misc/eeprom/at24.c | 58 +++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 28ea1a950..391e0d998 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -310,6 +310,59 @@ 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 regmap *regmap;
+
+	regmap = at24_translate_offset(at24, &offset)->regmap;
+
+	if (count > io_limit)
+		count = io_limit;
+
+	if (at24->chip.flags & AT24_FLAG_MAC)
+		offset += 0x90;
+
+	loop_until_timeout(timeout, read_time)
+		if (!regmap_bulk_read(regmap, offset, buf, count))
+			return count;
+
+	return -ETIMEDOUT;
+}
+
+static ssize_t at24_regmap_read_serial(struct at24_data *at24, char *buf,
+				       unsigned int offset, size_t count)
+{
+	unsigned long timeout, read_time;
+	struct regmap *regmap;
+
+	regmap = at24_translate_offset(at24, &offset)->regmap;
+
+	if (count > io_limit)
+		count = io_limit;
+
+	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.
+		 */
+		offset |= BIT(11);
+	else
+		/*
+		 * Otherwise the word address must begin with a '10' sequence,
+		 * regardless of the intended address.
+		 */
+		offset |= BIT(7);
+
+	loop_until_timeout(timeout, read_time)
+		if (!regmap_bulk_read(regmap, offset, buf, count))
+			return count;
+
+	return -ETIMEDOUT;
+}
+
 static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
 				    unsigned int offset, size_t count)
 {
@@ -512,7 +565,10 @@ 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);
+		if (at24->chip.flags & AT24_FLAG_SERIAL)
+			status = at24_regmap_read_serial(at24, buf, off, count);
+		else
+			status = at24_regmap_read(at24, buf, off, count);
 		if (status < 0) {
 			mutex_unlock(&at24->lock);
 			pm_runtime_put(&client->dev);
-- 
2.15.0

Re: [PATCH v2 5/7] eeprom: at24: add regmap-based read functions

[Bartosz Golaszewski]

2017-11-16 21:26 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Add regmap-based read functions.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> v2:
> - rebased
> ---
>  drivers/misc/eeprom/at24.c | 58 +++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 28ea1a950..391e0d998 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -310,6 +310,59 @@ 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 regmap *regmap;
> +
> +       regmap = at24_translate_offset(at24, &offset)->regmap;
> +
> +       if (count > io_limit)
> +               count = io_limit;
> +
> +       if (at24->chip.flags & AT24_FLAG_MAC)
> +               offset += 0x90;
> +
> +       loop_until_timeout(timeout, read_time)
> +               if (!regmap_bulk_read(regmap, offset, buf, count))
> +                       return count;
> +

Same as in previous patches: don't hide the return value type.

> +       return -ETIMEDOUT;
> +}
> +
> +static ssize_t at24_regmap_read_serial(struct at24_data *at24, char *buf,
> +                                      unsigned int offset, size_t count)
> +{
> +       unsigned long timeout, read_time;
> +       struct regmap *regmap;
> +
> +       regmap = at24_translate_offset(at24, &offset)->regmap;
> +
> +       if (count > io_limit)
> +               count = io_limit;
> +
> +       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.
> +                */
> +               offset |= BIT(11);
> +       else
> +               /*
> +                * Otherwise the word address must begin with a '10' sequence,
> +                * regardless of the intended address.
> +                */
> +               offset |= BIT(7);
> +
> +       loop_until_timeout(timeout, read_time)
> +               if (!regmap_bulk_read(regmap, offset, buf, count))
> +                       return count;
> +
> +       return -ETIMEDOUT;
> +}
> +
>  static ssize_t at24_eeprom_read_i2c(struct at24_data *at24, char *buf,
>                                     unsigned int offset, size_t count)
>  {
> @@ -512,7 +565,10 @@ 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);
> +               if (at24->chip.flags & AT24_FLAG_SERIAL)
> +                       status = at24_regmap_read_serial(at24, buf, off, count);
> +               else
> +                       status = at24_regmap_read(at24, buf, off, count);
>                 if (status < 0) {
>                         mutex_unlock(&at24->lock);
>                         pm_runtime_put(&client->dev);

I think the support for reading the MAC address for at24mac* series is missing?

Thanks,
Bartosz

[PATCH v2 6/7] eeprom: at24: 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
---
 drivers/misc/eeprom/at24.c | 177 ---------------------------------------------
 1 file changed, 177 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 391e0d998..a4e157b6b 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,
@@ -279,37 +276,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 i2c_client *client;
-	int status;
-
-	client = at24_translate_offset(at24, &offset)->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)
 {
@@ -363,139 +329,6 @@ static ssize_t at24_regmap_read_serial(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 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)->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 i2c_client *client;
-	struct i2c_msg msg[2];
-	u8 addrbuf[2];
-	int status;
-
-	client = at24_translate_offset(at24, &offset)->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 i2c_client *client;
-	struct i2c_msg msg[2];
-	u8 addrbuf[2];
-	int status;
-
-	client = at24_translate_offset(at24, &offset)->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
@@ -769,7 +602,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;
 
@@ -784,15 +616,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		return -EINVAL;
 	}
 
-	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 v2 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
---
 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 a4e157b6b..89eaa85b1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -504,8 +504,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;
@@ -557,33 +555,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;
@@ -617,21 +592,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 
 	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++) {
@@ -692,12 +654,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 0/7] eeprom: at24: switch driver to regmap_i2c

[Bartosz Golaszewski]

2017-11-16 21:16 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> 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.
>
> 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  | 385 +++++++++-----------------------------------
>  2 files changed, 79 insertions(+), 307 deletions(-)
>
> --
> 2.15.0
>

Hi Heiner,

These changes are a very good idea, but if I'm not mistaken, the
support for reading the MAC address in at24mac* eeprom series is
missing. Unfortunately I don't have access to one of those ATM to make
sure.

I've tested the series on a couple of chips and it works fine -
including the serial number read on at24cs32 and at24cs02. I'll test
it again on top of 4.15-rc1.

The patches look good code-wise - just a couple nits here and there.

Best regards,
Bartosz Golaszewski

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

[Heiner Kallweit]

Am 21.11.2017 um 11:14 schrieb Bartosz Golaszewski:
> 2017-11-16 21:16 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> 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.
>>
>> 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  | 385 +++++++++-----------------------------------
>>  2 files changed, 79 insertions(+), 307 deletions(-)
>>
>> --
>> 2.15.0
>>
> 
> Hi Heiner,
> 
Hi Bartosz,

> These changes are a very good idea, but if I'm not mistaken, the
> support for reading the MAC address in at24mac* eeprom series is
> missing. Unfortunately I don't have access to one of those ATM to make
> sure.
> 
The only difference when reading the MAC is that the offset is relative
to position 0x90. Therefore I merged reading a MAC into the standard
read function. See following part in patch 5:

if (at24->chip.flags & AT24_FLAG_MAC)
	offset += 0x90;

> I've tested the series on a couple of chips and it works fine -
> including the serial number read on at24cs32 and at24cs02. I'll test
> it again on top of 4.15-rc1.
> 
> The patches look good code-wise - just a couple nits here and there.
> 
Will have a look at it.


> Best regards,
> Bartosz Golaszewski
> 
Rgds, Heiner