[PATCH 0/7] eeprom: at24: series with smaller improvements

[PATCH 0/7] eeprom: at24: series with smaller improvements

[Heiner Kallweit]

Before doing bigger changes, e.g. replacing the magic config values with
a struct, here comes series with smaller improvements.

Heiner Kallweit (7):
  eeprom: at24: don't explicitely include header files which are implicitely included
  eeprom: at24: consider that SERIAL and MAC flags imply read-only
  eeprom: at24: simplify probe a little by replacing &client->dev
  eeprom: at24: simplify functions at24_read/write a little
  eeprom: at24: zero-initialize variable chip in probe
  eeprom: at24: don't check chip.byte_len for power of two
  eeprom: at24: don't check page_size for read-only chips and reorder checks

 drivers/misc/eeprom/at24.c | 154 +++++++++++++++++++--------------------------
 1 file changed, 64 insertions(+), 90 deletions(-)

-- 
2.15.0

[PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included

[Heiner Kallweit]

Don't explicitely include header files which are implicitely included.

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

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index cd87f1b21..c75bb9b45 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -10,15 +10,10 @@
  * (at your option) any later version.
  */
 #include <linux/kernel.h>
-#include <linux/init.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
-#include <linux/mutex.h>
-#include <linux/mod_devicetable.h>
-#include <linux/log2.h>
-#include <linux/bitops.h>
 #include <linux/jiffies.h>
 #include <linux/property.h>
 #include <linux/acpi.h>
-- 
2.15.0

[PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only

[Heiner Kallweit]

Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
Therefore handle this in the code instead of specifying
AT24_FLAG_READONLY in the config data in these cases.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index c75bb9b45..90fefd1cf 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
 	{ "24c00",	AT24_DEVICE_MAGIC(128 / 8,	AT24_FLAG_TAKE8ADDR) },
 	/* old variants can't be handled with this generic entry! */
 	{ "24c01",	AT24_DEVICE_MAGIC(1024 / 8,	0) },
-	{ "24cs01",	AT24_DEVICE_MAGIC(16,
-				AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+	{ "24cs01",	AT24_DEVICE_MAGIC(16,		AT24_FLAG_SERIAL) },
 	{ "24c02",	AT24_DEVICE_MAGIC(2048 / 8,	0) },
-	{ "24cs02",	AT24_DEVICE_MAGIC(16,
-				AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
-	{ "24mac402",	AT24_DEVICE_MAGIC(48 / 8,
-				AT24_FLAG_MAC | AT24_FLAG_READONLY) },
-	{ "24mac602",	AT24_DEVICE_MAGIC(64 / 8,
-				AT24_FLAG_MAC | AT24_FLAG_READONLY) },
+	{ "24cs02",	AT24_DEVICE_MAGIC(16,		AT24_FLAG_SERIAL) },
+	{ "24mac402",	AT24_DEVICE_MAGIC(48 / 8,	AT24_FLAG_MAC) },
+	{ "24mac602",	AT24_DEVICE_MAGIC(64 / 8,	AT24_FLAG_MAC) },
 	/* spd is a 24c02 in memory DIMMs */
 	{ "spd",	AT24_DEVICE_MAGIC(2048 / 8,
 				AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
 	{ "24c04",	AT24_DEVICE_MAGIC(4096 / 8,	0) },
-	{ "24cs04",	AT24_DEVICE_MAGIC(16,
-				AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+	{ "24cs04",	AT24_DEVICE_MAGIC(16,	AT24_FLAG_SERIAL) },
 	/* 24rf08 quirk is handled at i2c-core */
 	{ "24c08",	AT24_DEVICE_MAGIC(8192 / 8,	0) },
-	{ "24cs08",	AT24_DEVICE_MAGIC(16,
-				AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+	{ "24cs08",	AT24_DEVICE_MAGIC(16,	AT24_FLAG_SERIAL) },
 	{ "24c16",	AT24_DEVICE_MAGIC(16384 / 8,	0) },
-	{ "24cs16",	AT24_DEVICE_MAGIC(16,
-				AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
+	{ "24cs16",	AT24_DEVICE_MAGIC(16,		AT24_FLAG_SERIAL) },
 	{ "24c32",	AT24_DEVICE_MAGIC(32768 / 8,	AT24_FLAG_ADDR16) },
 	{ "24cs32",	AT24_DEVICE_MAGIC(16,
 				AT24_FLAG_ADDR16 |
-				AT24_FLAG_SERIAL |
-				AT24_FLAG_READONLY) },
+				AT24_FLAG_SERIAL) },
 	{ "24c64",	AT24_DEVICE_MAGIC(65536 / 8,	AT24_FLAG_ADDR16) },
 	{ "24cs64",	AT24_DEVICE_MAGIC(16,
 				AT24_FLAG_ADDR16 |
-				AT24_FLAG_SERIAL |
-				AT24_FLAG_READONLY) },
+				AT24_FLAG_SERIAL) },
 	{ "24c128",	AT24_DEVICE_MAGIC(131072 / 8,	AT24_FLAG_ADDR16) },
 	{ "24c256",	AT24_DEVICE_MAGIC(262144 / 8,	AT24_FLAG_ADDR16) },
 	{ "24c512",	AT24_DEVICE_MAGIC(524288 / 8,	AT24_FLAG_ADDR16) },
@@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.context = NULL;
 	}
 
+	/* both flags imply read-only */
+	if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
+		chip.flags |= AT24_FLAG_READONLY;
+
 	if (!is_power_of_2(chip.byte_len))
 		dev_warn(&client->dev,
 			"byte_len looks suspicious (no power of 2)!\n");
-- 
2.15.0

[PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev

[Heiner Kallweit]

We have lots of places in probe where &client->dev is used, replace it
with a variable to simplify code a little.

In addition remove redundant check for client->dev.of_node when using
of_match_device, this function can deal with a NULL of_node.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/misc/eeprom/at24.c | 48 +++++++++++++++++++++-------------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 90fefd1cf..d56be71f1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -511,26 +511,25 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	int err;
 	unsigned i, num_addresses;
 	const struct regmap_config *config;
+	struct device *dev = &client->dev;
 	u8 test_byte;
 
-	if (client->dev.platform_data) {
-		chip = *(struct at24_platform_data *)client->dev.platform_data;
+	if (dev->platform_data) {
+		chip = *(struct at24_platform_data *)dev->platform_data;
 	} else {
 		/*
 		 * The I2C core allows OF nodes compatibles to match against the
 		 * I2C device ID table as a fallback, so check not only if an OF
 		 * node is present but also if it matches an OF device ID entry.
 		 */
-		if (client->dev.of_node &&
-		    of_match_device(at24_of_match, &client->dev)) {
-			magic = (kernel_ulong_t)
-				of_device_get_match_data(&client->dev);
+		if (of_match_device(at24_of_match, dev)) {
+			magic = (kernel_ulong_t) of_device_get_match_data(dev);
 		} else if (id) {
 			magic = id->driver_data;
 		} else {
 			const struct acpi_device_id *aid;
 
-			aid = acpi_match_device(at24_acpi_ids, &client->dev);
+			aid = acpi_match_device(at24_acpi_ids, dev);
 			if (aid)
 				magic = aid->driver_data;
 		}
@@ -541,7 +540,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		magic >>= AT24_SIZE_BYTELEN;
 		chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
 
-		at24_get_pdata(&client->dev, &chip);
+		at24_get_pdata(dev, &chip);
 
 		chip.setup = NULL;
 		chip.context = NULL;
@@ -552,15 +551,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.flags |= AT24_FLAG_READONLY;
 
 	if (!is_power_of_2(chip.byte_len))
-		dev_warn(&client->dev,
-			"byte_len looks suspicious (no power of 2)!\n");
+		dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
 	if (!chip.page_size) {
-		dev_err(&client->dev, "page_size must not be 0!\n");
+		dev_err(dev, "page_size must not be 0!\n");
 		return -EINVAL;
 	}
 	if (!is_power_of_2(chip.page_size))
-		dev_warn(&client->dev,
-			"page_size looks suspicious (no power of 2)!\n");
+		dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
 
 	/*
 	 * REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
@@ -588,7 +585,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	else
 		config = &regmap_config_8;
 
-	at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
+	at24 = devm_kzalloc(dev, sizeof(struct at24_data) +
 		num_addresses * sizeof(struct at24_client), GFP_KERNEL);
 	if (!at24)
 		return -ENOMEM;
@@ -604,8 +601,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		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.");
+		dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
 		return -EINVAL;
 	}
 
@@ -622,8 +618,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		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);
+			dev_err(dev, "address 0x%02x unavailable\n",
+				client->addr + i);
 			err = -EADDRINUSE;
 			goto err_clients;
 		}
@@ -638,27 +634,27 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	i2c_set_clientdata(client, at24);
 
 	/* enable runtime pm */
-	pm_runtime_set_active(&client->dev);
-	pm_runtime_enable(&client->dev);
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
 
 	/*
 	 * Perform a one-byte test read to verify that the
 	 * chip is functional.
 	 */
 	err = at24_read(at24, 0, &test_byte, 1);
-	pm_runtime_idle(&client->dev);
+	pm_runtime_idle(dev);
 	if (err) {
 		err = -ENODEV;
 		goto err_clients;
 	}
 
-	at24->nvmem_config.name = dev_name(&client->dev);
-	at24->nvmem_config.dev = &client->dev;
+	at24->nvmem_config.name = dev_name(dev);
+	at24->nvmem_config.dev = dev;
 	at24->nvmem_config.read_only = !writable;
 	at24->nvmem_config.root_only = true;
 	at24->nvmem_config.owner = THIS_MODULE;
 	at24->nvmem_config.compat = true;
-	at24->nvmem_config.base_dev = &client->dev;
+	at24->nvmem_config.base_dev = dev;
 	at24->nvmem_config.reg_read = at24_read;
 	at24->nvmem_config.reg_write = at24_write;
 	at24->nvmem_config.priv = at24;
@@ -673,7 +669,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		goto err_clients;
 	}
 
-	dev_info(&client->dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
+	dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
 		chip.byte_len, client->name,
 		writable ? "writable" : "read-only", at24->write_max);
 
@@ -688,7 +684,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		if (at24->client[i].client)
 			i2c_unregister_device(at24->client[i].client);
 
-	pm_runtime_disable(&client->dev);
+	pm_runtime_disable(dev);
 
 	return err;
 }
-- 
2.15.0

[PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little

[Heiner Kallweit]

Simplify functions at24_read/write a little.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/misc/eeprom/at24.c | 40 ++++++++++++++++------------------------
 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index d56be71f1..837f1d88c 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -374,24 +374,20 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
 	mutex_lock(&at24->lock);
 
 	while (count) {
-		int	status;
+		ret = at24_regmap_read(at24, buf, off, count);
+		if (ret < 0)
+			goto out;
 
-		status = at24_regmap_read(at24, buf, off, count);
-		if (status < 0) {
-			mutex_unlock(&at24->lock);
-			pm_runtime_put(&client->dev);
-			return status;
-		}
-		buf += status;
-		off += status;
-		count -= status;
+		buf += ret;
+		off += ret;
+		count -= ret;
 	}
-
+out:
 	mutex_unlock(&at24->lock);
 
 	pm_runtime_put(&client->dev);
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static int at24_write(void *priv, unsigned int off, void *val, size_t count)
@@ -424,24 +420,20 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
 	mutex_lock(&at24->lock);
 
 	while (count) {
-		int status;
+		ret = at24_regmap_write(at24, buf, off, count);
+		if (ret < 0)
+			goto out;
 
-		status = at24_regmap_write(at24, buf, off, count);
-		if (status < 0) {
-			mutex_unlock(&at24->lock);
-			pm_runtime_put(&client->dev);
-			return status;
-		}
-		buf += status;
-		off += status;
-		count -= status;
+		buf += ret;
+		off += ret;
+		count -= ret;
 	}
-
+out:
 	mutex_unlock(&at24->lock);
 
 	pm_runtime_put(&client->dev);
 
-	return 0;
+	return ret < 0 ? ret : 0;
 }
 
 static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
-- 
2.15.0

[PATCH 5/7] eeprom: at24: zero-initialize variable chip in probe

[Heiner Kallweit]

Zero-initialize variable chip to be on the safe side. This also
allows ro remove some explicit NULL initializations.

Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/misc/eeprom/at24.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 837f1d88c..8fef6d5a8 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -496,7 +496,7 @@ static const struct regmap_config regmap_config_16 = {
 
 static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
-	struct at24_platform_data chip;
+	struct at24_platform_data chip = {};
 	kernel_ulong_t magic = 0;
 	bool writable;
 	struct at24_data *at24;
@@ -533,9 +533,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
 
 		at24_get_pdata(dev, &chip);
-
-		chip.setup = NULL;
-		chip.context = NULL;
 	}
 
 	/* both flags imply read-only */
-- 
2.15.0

[PATCH 6/7] eeprom: at24: don't check chip.byte_len for power of two

[Heiner Kallweit]

When using AT24_FLAG_SERIAL and AT24_FLAG_MAC we expose just parts of
the chip. These parts can have arbitrary size (e.g. a 6 byte MAC),
so remove the check for power of two.

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

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 8fef6d5a8..74d2347a1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -539,8 +539,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
 		chip.flags |= AT24_FLAG_READONLY;
 
-	if (!is_power_of_2(chip.byte_len))
-		dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
 	if (!chip.page_size) {
 		dev_err(dev, "page_size must not be 0!\n");
 		return -EINVAL;
-- 
2.15.0

[PATCH 7/7] eeprom: at24: don't check page_size for read-only chips and reorder checks

[Heiner Kallweit]

Currently we check for a proper page_size value also for read-only chips
where this value isn't used. Therefore remove these checks for read-only
chips.

In addition reorder checks to do sanity checking first. 
 
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
 drivers/misc/eeprom/at24.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 74d2347a1..2348da953 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -535,16 +535,28 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		at24_get_pdata(dev, &chip);
 	}
 
+	if ((chip.flags & AT24_FLAG_SERIAL) && (chip.flags & AT24_FLAG_MAC)) {
+		dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
+		return -EINVAL;
+	}
+
 	/* both flags imply read-only */
 	if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
 		chip.flags |= AT24_FLAG_READONLY;
 
-	if (!chip.page_size) {
-		dev_err(dev, "page_size must not be 0!\n");
-		return -EINVAL;
+	writable = !(chip.flags & AT24_FLAG_READONLY);
+	if (writable) {
+		if (!chip.page_size) {
+			dev_err(dev, "page_size must not be 0!\n");
+			return -EINVAL;
+		}
+		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
+		    !i2c_check_functionality(client->adapter,
+					     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
+			chip.page_size = 1;
+		if (!is_power_of_2(chip.page_size))
+			dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
 	}
-	if (!is_power_of_2(chip.page_size))
-		dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
 
 	/*
 	 * REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
@@ -556,11 +568,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (chip.flags & AT24_FLAG_MAC && chip.byte_len == 4)
 		chip.byte_len = 6;
 
-	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
-	    !i2c_check_functionality(client->adapter,
-				     I2C_FUNC_SMBUS_WRITE_I2C_BLOCK))
-		chip.page_size = 1;
-
 	if (chip.flags & AT24_FLAG_TAKE8ADDR)
 		num_addresses = 8;
 	else
@@ -587,12 +594,6 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	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(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
-		return -EINVAL;
-	}
-
-	writable = !(chip.flags & AT24_FLAG_READONLY);
 	if (writable) {
 		at24->write_max = min_t(unsigned int, chip.page_size, io_limit);
 		if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C) &&
-- 
2.15.0

Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included

[Peter Rosin]

On 2017-11-30 07:48, Heiner Kallweit wrote:
> Don't explicitely include header files which are implicitely included.

That's backwards. It hinders future header rearrangement. Things that
are used directly (e.g. the BIT macro) should have its respective
include specified (bitops.h in that case).

It's not a competition for the shortest file, the goal is easy
maintenance.

Cheers,
Peter

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/misc/eeprom/at24.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index cd87f1b21..c75bb9b45 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -10,15 +10,10 @@
>   * (at your option) any later version.
>   */
>  #include <linux/kernel.h>
> -#include <linux/init.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
>  #include <linux/slab.h>
>  #include <linux/delay.h>
> -#include <linux/mutex.h>
> -#include <linux/mod_devicetable.h>
> -#include <linux/log2.h>
> -#include <linux/bitops.h>
>  #include <linux/jiffies.h>
>  #include <linux/property.h>
>  #include <linux/acpi.h>
> 

Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included

[Heiner Kallweit]

Am 30.11.2017 um 16:56 schrieb Peter Rosin:
> On 2017-11-30 07:48, Heiner Kallweit wrote:
>> Don't explicitely include header files which are implicitely included.
> 
> That's backwards. It hinders future header rearrangement. Things that
> are used directly (e.g. the BIT macro) should have its respective
> include specified (bitops.h in that case).
> 
> It's not a competition for the shortest file, the goal is easy
> maintenance.
> 
Understood ..

@Bartosz: then you can ignore this one.

Rgds, Heiner


> Cheers,
> Peter
> 
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/misc/eeprom/at24.c | 5 -----
>>  1 file changed, 5 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index cd87f1b21..c75bb9b45 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -10,15 +10,10 @@
>>   * (at your option) any later version.
>>   */
>>  #include <linux/kernel.h>
>> -#include <linux/init.h>
>>  #include <linux/module.h>
>>  #include <linux/of_device.h>
>>  #include <linux/slab.h>
>>  #include <linux/delay.h>
>> -#include <linux/mutex.h>
>> -#include <linux/mod_devicetable.h>
>> -#include <linux/log2.h>
>> -#include <linux/bitops.h>
>>  #include <linux/jiffies.h>
>>  #include <linux/property.h>
>>  #include <linux/acpi.h>
>>
> 
> 

Re: [PATCH 1/7] eeprom: at24: don't explicitely include header files which are implicitely included

[Bartosz Golaszewski]

2017-11-30 20:32 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 30.11.2017 um 16:56 schrieb Peter Rosin:
>> On 2017-11-30 07:48, Heiner Kallweit wrote:
>>> Don't explicitely include header files which are implicitely included.
>>
>> That's backwards. It hinders future header rearrangement. Things that
>> are used directly (e.g. the BIT macro) should have its respective
>> include specified (bitops.h in that case).
>>
>> It's not a competition for the shortest file, the goal is easy
>> maintenance.
>>
> Understood ..
>
> @Bartosz: then you can ignore this one.
>
> Rgds, Heiner
>
>
>> Cheers,
>> Peter
>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/misc/eeprom/at24.c | 5 -----
>>>  1 file changed, 5 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index cd87f1b21..c75bb9b45 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -10,15 +10,10 @@
>>>   * (at your option) any later version.
>>>   */
>>>  #include <linux/kernel.h>
>>> -#include <linux/init.h>
>>>  #include <linux/module.h>
>>>  #include <linux/of_device.h>
>>>  #include <linux/slab.h>
>>>  #include <linux/delay.h>
>>> -#include <linux/mutex.h>
>>> -#include <linux/mod_devicetable.h>
>>> -#include <linux/log2.h>
>>> -#include <linux/bitops.h>
>>>  #include <linux/jiffies.h>
>>>  #include <linux/property.h>
>>>  #include <linux/acpi.h>
>>>
>>
>>
>

Yep, whenever we use anything from some header, we want to have it
included directly. Otherwise any rearrangement would cause us pain.

Thanks,
Bartosz

Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only

[Bartosz Golaszewski]

2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
> Therefore handle this in the code instead of specifying
> AT24_FLAG_READONLY in the config data in these cases.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index c75bb9b45..90fefd1cf 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>         { "24c00",      AT24_DEVICE_MAGIC(128 / 8,      AT24_FLAG_TAKE8ADDR) },
>         /* old variants can't be handled with this generic entry! */
>         { "24c01",      AT24_DEVICE_MAGIC(1024 / 8,     0) },
> -       { "24cs01",     AT24_DEVICE_MAGIC(16,
> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> +       { "24cs01",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>         { "24c02",      AT24_DEVICE_MAGIC(2048 / 8,     0) },
> -       { "24cs02",     AT24_DEVICE_MAGIC(16,
> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> -       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,
> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
> -       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,
> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
> +       { "24cs02",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
> +       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,       AT24_FLAG_MAC) },
> +       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,       AT24_FLAG_MAC) },
>         /* spd is a 24c02 in memory DIMMs */
>         { "spd",        AT24_DEVICE_MAGIC(2048 / 8,
>                                 AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>         { "24c04",      AT24_DEVICE_MAGIC(4096 / 8,     0) },
> -       { "24cs04",     AT24_DEVICE_MAGIC(16,
> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> +       { "24cs04",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>         /* 24rf08 quirk is handled at i2c-core */
>         { "24c08",      AT24_DEVICE_MAGIC(8192 / 8,     0) },
> -       { "24cs08",     AT24_DEVICE_MAGIC(16,
> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> +       { "24cs08",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>         { "24c16",      AT24_DEVICE_MAGIC(16384 / 8,    0) },
> -       { "24cs16",     AT24_DEVICE_MAGIC(16,
> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
> +       { "24cs16",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>         { "24c32",      AT24_DEVICE_MAGIC(32768 / 8,    AT24_FLAG_ADDR16) },
>         { "24cs32",     AT24_DEVICE_MAGIC(16,
>                                 AT24_FLAG_ADDR16 |
> -                               AT24_FLAG_SERIAL |
> -                               AT24_FLAG_READONLY) },
> +                               AT24_FLAG_SERIAL) },
>         { "24c64",      AT24_DEVICE_MAGIC(65536 / 8,    AT24_FLAG_ADDR16) },
>         { "24cs64",     AT24_DEVICE_MAGIC(16,
>                                 AT24_FLAG_ADDR16 |
> -                               AT24_FLAG_SERIAL |
> -                               AT24_FLAG_READONLY) },
> +                               AT24_FLAG_SERIAL) },
>         { "24c128",     AT24_DEVICE_MAGIC(131072 / 8,   AT24_FLAG_ADDR16) },
>         { "24c256",     AT24_DEVICE_MAGIC(262144 / 8,   AT24_FLAG_ADDR16) },
>         { "24c512",     AT24_DEVICE_MAGIC(524288 / 8,   AT24_FLAG_ADDR16) },
> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 chip.context = NULL;
>         }
>
> +       /* both flags imply read-only */
> +       if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
> +               chip.flags |= AT24_FLAG_READONLY;
> +
>         if (!is_power_of_2(chip.byte_len))
>                 dev_warn(&client->dev,
>                         "byte_len looks suspicious (no power of 2)!\n");
> --
> 2.15.0
>
>

Just as with the header patch: this doesn't save us any code other
than visual. We want the definition to scream that this chip is
read-only. Nacked.

Thanks,
Bartosz

Re: [PATCH 3/7] eeprom: at24: simplify probe a little by replacing &client->dev

[Bartosz Golaszewski]

2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> We have lots of places in probe where &client->dev is used, replace it
> with a variable to simplify code a little.
>
> In addition remove redundant check for client->dev.of_node when using
> of_match_device, this function can deal with a NULL of_node.

This belongs in a separate patch. Please split it for v2.

>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/misc/eeprom/at24.c | 48 +++++++++++++++++++++-------------------------
>  1 file changed, 22 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 90fefd1cf..d56be71f1 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -511,26 +511,25 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>         int err;
>         unsigned i, num_addresses;
>         const struct regmap_config *config;
> +       struct device *dev = &client->dev;
>         u8 test_byte;
>
> -       if (client->dev.platform_data) {
> -               chip = *(struct at24_platform_data *)client->dev.platform_data;
> +       if (dev->platform_data) {
> +               chip = *(struct at24_platform_data *)dev->platform_data;
>         } else {
>                 /*
>                  * The I2C core allows OF nodes compatibles to match against the
>                  * I2C device ID table as a fallback, so check not only if an OF
>                  * node is present but also if it matches an OF device ID entry.
>                  */
> -               if (client->dev.of_node &&
> -                   of_match_device(at24_of_match, &client->dev)) {
> -                       magic = (kernel_ulong_t)
> -                               of_device_get_match_data(&client->dev);
> +               if (of_match_device(at24_of_match, dev)) {
> +                       magic = (kernel_ulong_t) of_device_get_match_data(dev);
>                 } else if (id) {
>                         magic = id->driver_data;
>                 } else {
>                         const struct acpi_device_id *aid;
>
> -                       aid = acpi_match_device(at24_acpi_ids, &client->dev);
> +                       aid = acpi_match_device(at24_acpi_ids, dev);
>                         if (aid)
>                                 magic = aid->driver_data;
>                 }
> @@ -541,7 +540,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 magic >>= AT24_SIZE_BYTELEN;
>                 chip.flags = magic & AT24_BITMASK(AT24_SIZE_FLAGS);
>
> -               at24_get_pdata(&client->dev, &chip);
> +               at24_get_pdata(dev, &chip);
>
>                 chip.setup = NULL;
>                 chip.context = NULL;
> @@ -552,15 +551,13 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 chip.flags |= AT24_FLAG_READONLY;
>
>         if (!is_power_of_2(chip.byte_len))
> -               dev_warn(&client->dev,
> -                       "byte_len looks suspicious (no power of 2)!\n");
> +               dev_warn(dev, "byte_len looks suspicious (no power of 2)!\n");
>         if (!chip.page_size) {
> -               dev_err(&client->dev, "page_size must not be 0!\n");
> +               dev_err(dev, "page_size must not be 0!\n");
>                 return -EINVAL;
>         }
>         if (!is_power_of_2(chip.page_size))
> -               dev_warn(&client->dev,
> -                       "page_size looks suspicious (no power of 2)!\n");
> +               dev_warn(dev, "page_size looks suspicious (no power of 2)!\n");
>
>         /*
>          * REVISIT: the size of the EUI-48 byte array is 6 in at24mac402, while
> @@ -588,7 +585,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>         else
>                 config = &regmap_config_8;
>
> -       at24 = devm_kzalloc(&client->dev, sizeof(struct at24_data) +
> +       at24 = devm_kzalloc(dev, sizeof(struct at24_data) +
>                 num_addresses * sizeof(struct at24_client), GFP_KERNEL);
>         if (!at24)
>                 return -ENOMEM;
> @@ -604,8 +601,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 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.");
> +               dev_err(dev, "invalid device data - cannot have both AT24_FLAG_SERIAL & AT24_FLAG_MAC.");
>                 return -EINVAL;
>         }
>
> @@ -622,8 +618,8 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 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);
> +                       dev_err(dev, "address 0x%02x unavailable\n",
> +                               client->addr + i);
>                         err = -EADDRINUSE;
>                         goto err_clients;
>                 }
> @@ -638,27 +634,27 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>         i2c_set_clientdata(client, at24);
>
>         /* enable runtime pm */
> -       pm_runtime_set_active(&client->dev);
> -       pm_runtime_enable(&client->dev);
> +       pm_runtime_set_active(dev);
> +       pm_runtime_enable(dev);
>
>         /*
>          * Perform a one-byte test read to verify that the
>          * chip is functional.
>          */
>         err = at24_read(at24, 0, &test_byte, 1);
> -       pm_runtime_idle(&client->dev);
> +       pm_runtime_idle(dev);
>         if (err) {
>                 err = -ENODEV;
>                 goto err_clients;
>         }
>
> -       at24->nvmem_config.name = dev_name(&client->dev);
> -       at24->nvmem_config.dev = &client->dev;
> +       at24->nvmem_config.name = dev_name(dev);
> +       at24->nvmem_config.dev = dev;
>         at24->nvmem_config.read_only = !writable;
>         at24->nvmem_config.root_only = true;
>         at24->nvmem_config.owner = THIS_MODULE;
>         at24->nvmem_config.compat = true;
> -       at24->nvmem_config.base_dev = &client->dev;
> +       at24->nvmem_config.base_dev = dev;
>         at24->nvmem_config.reg_read = at24_read;
>         at24->nvmem_config.reg_write = at24_write;
>         at24->nvmem_config.priv = at24;
> @@ -673,7 +669,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 goto err_clients;
>         }
>
> -       dev_info(&client->dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
> +       dev_info(dev, "%u byte %s EEPROM, %s, %u bytes/write\n",
>                 chip.byte_len, client->name,
>                 writable ? "writable" : "read-only", at24->write_max);
>
> @@ -688,7 +684,7 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>                 if (at24->client[i].client)
>                         i2c_unregister_device(at24->client[i].client);
>
> -       pm_runtime_disable(&client->dev);
> +       pm_runtime_disable(dev);
>
>         return err;
>  }
> --
> 2.15.0
>
>

Re: [PATCH 4/7] eeprom: at24: simplify functions at24_read/write a little

[Bartosz Golaszewski]

2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Simplify functions at24_read/write a little.
>

Please be more specific in your commit message - it's not obvious at
first glance what you're actually simplifying.

Thanks,
Bartosz

> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/misc/eeprom/at24.c | 40 ++++++++++++++++------------------------
>  1 file changed, 16 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index d56be71f1..837f1d88c 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -374,24 +374,20 @@ static int at24_read(void *priv, unsigned int off, void *val, size_t count)
>         mutex_lock(&at24->lock);
>
>         while (count) {
> -               int     status;
> +               ret = at24_regmap_read(at24, buf, off, count);
> +               if (ret < 0)
> +                       goto out;
>
> -               status = at24_regmap_read(at24, buf, off, count);
> -               if (status < 0) {
> -                       mutex_unlock(&at24->lock);
> -                       pm_runtime_put(&client->dev);
> -                       return status;
> -               }
> -               buf += status;
> -               off += status;
> -               count -= status;
> +               buf += ret;
> +               off += ret;
> +               count -= ret;
>         }
> -
> +out:
>         mutex_unlock(&at24->lock);
>
>         pm_runtime_put(&client->dev);
>
> -       return 0;
> +       return ret < 0 ? ret : 0;
>  }
>
>  static int at24_write(void *priv, unsigned int off, void *val, size_t count)
> @@ -424,24 +420,20 @@ static int at24_write(void *priv, unsigned int off, void *val, size_t count)
>         mutex_lock(&at24->lock);
>
>         while (count) {
> -               int status;
> +               ret = at24_regmap_write(at24, buf, off, count);
> +               if (ret < 0)
> +                       goto out;
>
> -               status = at24_regmap_write(at24, buf, off, count);
> -               if (status < 0) {
> -                       mutex_unlock(&at24->lock);
> -                       pm_runtime_put(&client->dev);
> -                       return status;
> -               }
> -               buf += status;
> -               off += status;
> -               count -= status;
> +               buf += ret;
> +               off += ret;
> +               count -= ret;
>         }
> -
> +out:
>         mutex_unlock(&at24->lock);
>
>         pm_runtime_put(&client->dev);
>
> -       return 0;
> +       return ret < 0 ? ret : 0;
>  }
>
>  static void at24_get_pdata(struct device *dev, struct at24_platform_data *chip)
> --
> 2.15.0
>
>

Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only

[Heiner Kallweit]

Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
>> Therefore handle this in the code instead of specifying
>> AT24_FLAG_READONLY in the config data in these cases.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>>  1 file changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>> index c75bb9b45..90fefd1cf 100644
>> --- a/drivers/misc/eeprom/at24.c
>> +++ b/drivers/misc/eeprom/at24.c
>> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>>         { "24c00",      AT24_DEVICE_MAGIC(128 / 8,      AT24_FLAG_TAKE8ADDR) },
>>         /* old variants can't be handled with this generic entry! */
>>         { "24c01",      AT24_DEVICE_MAGIC(1024 / 8,     0) },
>> -       { "24cs01",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs01",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>         { "24c02",      AT24_DEVICE_MAGIC(2048 / 8,     0) },
>> -       { "24cs02",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> -       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,
>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> -       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,
>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>> +       { "24cs02",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>> +       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,       AT24_FLAG_MAC) },
>> +       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,       AT24_FLAG_MAC) },
>>         /* spd is a 24c02 in memory DIMMs */
>>         { "spd",        AT24_DEVICE_MAGIC(2048 / 8,
>>                                 AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>>         { "24c04",      AT24_DEVICE_MAGIC(4096 / 8,     0) },
>> -       { "24cs04",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs04",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>         /* 24rf08 quirk is handled at i2c-core */
>>         { "24c08",      AT24_DEVICE_MAGIC(8192 / 8,     0) },
>> -       { "24cs08",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs08",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>         { "24c16",      AT24_DEVICE_MAGIC(16384 / 8,    0) },
>> -       { "24cs16",     AT24_DEVICE_MAGIC(16,
>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>> +       { "24cs16",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>         { "24c32",      AT24_DEVICE_MAGIC(32768 / 8,    AT24_FLAG_ADDR16) },
>>         { "24cs32",     AT24_DEVICE_MAGIC(16,
>>                                 AT24_FLAG_ADDR16 |
>> -                               AT24_FLAG_SERIAL |
>> -                               AT24_FLAG_READONLY) },
>> +                               AT24_FLAG_SERIAL) },
>>         { "24c64",      AT24_DEVICE_MAGIC(65536 / 8,    AT24_FLAG_ADDR16) },
>>         { "24cs64",     AT24_DEVICE_MAGIC(16,
>>                                 AT24_FLAG_ADDR16 |
>> -                               AT24_FLAG_SERIAL |
>> -                               AT24_FLAG_READONLY) },
>> +                               AT24_FLAG_SERIAL) },
>>         { "24c128",     AT24_DEVICE_MAGIC(131072 / 8,   AT24_FLAG_ADDR16) },
>>         { "24c256",     AT24_DEVICE_MAGIC(262144 / 8,   AT24_FLAG_ADDR16) },
>>         { "24c512",     AT24_DEVICE_MAGIC(524288 / 8,   AT24_FLAG_ADDR16) },
>> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>                 chip.context = NULL;
>>         }
>>
>> +       /* both flags imply read-only */
>> +       if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
>> +               chip.flags |= AT24_FLAG_READONLY;
>> +
>>         if (!is_power_of_2(chip.byte_len))
>>                 dev_warn(&client->dev,
>>                         "byte_len looks suspicious (no power of 2)!\n");
>> --
>> 2.15.0
>>
>>
> 
> Just as with the header patch: this doesn't save us any code other
> than visual. We want the definition to scream that this chip is
> read-only. Nacked.
> 
I see your point. My concern is that so far we may have platform-data-
configured devices setting flag SERIAL or MAC and not READONLY and we
have no control over such (IMO) misconfigurations.

So my primary question would be whether we agree on SERIAL and MAC
implying READONLY. If yes then I think we should reflect this
relationship in the code.
Instead of silently setting flag READONLY we could also reject such
invalid flag configurations. Or print at least a message/warning
when setting READONLY.

Rgds, Heiner

> Thanks,
> Bartosz
> 

Re: [PATCH 2/7] eeprom: at24: consider that SERIAL and MAC flags imply read-only

[Bartosz Golaszewski]

2017-12-02 23:00 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
> Am 01.12.2017 um 11:10 schrieb Bartosz Golaszewski:
>> 2017-11-30 7:49 GMT+01:00 Heiner Kallweit <hkallweit1@gmail.com>:
>>> Flags AT24_FLAG_SERIAL and AT24_FLAG_MAC imply read-only.
>>> Therefore handle this in the code instead of specifying
>>> AT24_FLAG_READONLY in the config data in these cases.
>>>
>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>>> ---
>>>  drivers/misc/eeprom/at24.c | 31 +++++++++++++------------------
>>>  1 file changed, 13 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
>>> index c75bb9b45..90fefd1cf 100644
>>> --- a/drivers/misc/eeprom/at24.c
>>> +++ b/drivers/misc/eeprom/at24.c
>>> @@ -131,38 +131,29 @@ static const struct i2c_device_id at24_ids[] = {
>>>         { "24c00",      AT24_DEVICE_MAGIC(128 / 8,      AT24_FLAG_TAKE8ADDR) },
>>>         /* old variants can't be handled with this generic entry! */
>>>         { "24c01",      AT24_DEVICE_MAGIC(1024 / 8,     0) },
>>> -       { "24cs01",     AT24_DEVICE_MAGIC(16,
>>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> +       { "24cs01",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>>         { "24c02",      AT24_DEVICE_MAGIC(2048 / 8,     0) },
>>> -       { "24cs02",     AT24_DEVICE_MAGIC(16,
>>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> -       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,
>>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>>> -       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,
>>> -                               AT24_FLAG_MAC | AT24_FLAG_READONLY) },
>>> +       { "24cs02",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>> +       { "24mac402",   AT24_DEVICE_MAGIC(48 / 8,       AT24_FLAG_MAC) },
>>> +       { "24mac602",   AT24_DEVICE_MAGIC(64 / 8,       AT24_FLAG_MAC) },
>>>         /* spd is a 24c02 in memory DIMMs */
>>>         { "spd",        AT24_DEVICE_MAGIC(2048 / 8,
>>>                                 AT24_FLAG_READONLY | AT24_FLAG_IRUGO) },
>>>         { "24c04",      AT24_DEVICE_MAGIC(4096 / 8,     0) },
>>> -       { "24cs04",     AT24_DEVICE_MAGIC(16,
>>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> +       { "24cs04",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>>         /* 24rf08 quirk is handled at i2c-core */
>>>         { "24c08",      AT24_DEVICE_MAGIC(8192 / 8,     0) },
>>> -       { "24cs08",     AT24_DEVICE_MAGIC(16,
>>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> +       { "24cs08",     AT24_DEVICE_MAGIC(16,   AT24_FLAG_SERIAL) },
>>>         { "24c16",      AT24_DEVICE_MAGIC(16384 / 8,    0) },
>>> -       { "24cs16",     AT24_DEVICE_MAGIC(16,
>>> -                               AT24_FLAG_SERIAL | AT24_FLAG_READONLY) },
>>> +       { "24cs16",     AT24_DEVICE_MAGIC(16,           AT24_FLAG_SERIAL) },
>>>         { "24c32",      AT24_DEVICE_MAGIC(32768 / 8,    AT24_FLAG_ADDR16) },
>>>         { "24cs32",     AT24_DEVICE_MAGIC(16,
>>>                                 AT24_FLAG_ADDR16 |
>>> -                               AT24_FLAG_SERIAL |
>>> -                               AT24_FLAG_READONLY) },
>>> +                               AT24_FLAG_SERIAL) },
>>>         { "24c64",      AT24_DEVICE_MAGIC(65536 / 8,    AT24_FLAG_ADDR16) },
>>>         { "24cs64",     AT24_DEVICE_MAGIC(16,
>>>                                 AT24_FLAG_ADDR16 |
>>> -                               AT24_FLAG_SERIAL |
>>> -                               AT24_FLAG_READONLY) },
>>> +                               AT24_FLAG_SERIAL) },
>>>         { "24c128",     AT24_DEVICE_MAGIC(131072 / 8,   AT24_FLAG_ADDR16) },
>>>         { "24c256",     AT24_DEVICE_MAGIC(262144 / 8,   AT24_FLAG_ADDR16) },
>>>         { "24c512",     AT24_DEVICE_MAGIC(524288 / 8,   AT24_FLAG_ADDR16) },
>>> @@ -556,6 +547,10 @@ static int at24_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>>                 chip.context = NULL;
>>>         }
>>>
>>> +       /* both flags imply read-only */
>>> +       if (chip.flags & AT24_FLAG_SERIAL || chip.flags & AT24_FLAG_MAC)
>>> +               chip.flags |= AT24_FLAG_READONLY;
>>> +
>>>         if (!is_power_of_2(chip.byte_len))
>>>                 dev_warn(&client->dev,
>>>                         "byte_len looks suspicious (no power of 2)!\n");
>>> --
>>> 2.15.0
>>>
>>>
>>
>> Just as with the header patch: this doesn't save us any code other
>> than visual. We want the definition to scream that this chip is
>> read-only. Nacked.
>>
> I see your point. My concern is that so far we may have platform-data-
> configured devices setting flag SERIAL or MAC and not READONLY and we
> have no control over such (IMO) misconfigurations.
>
> So my primary question would be whether we agree on SERIAL and MAC
> implying READONLY. If yes then I think we should reflect this
> relationship in the code.
> Instead of silently setting flag READONLY we could also reject such
> invalid flag configurations. Or print at least a message/warning
> when setting READONLY.
>
> Rgds, Heiner
>
>> Thanks,
>> Bartosz
>>
>

OK, this makes more sense, let's check if READONLY is set for MAC and
CS and if not: let's set it ourselves while also printing a warning.

Thanks,
Bartosz