[PATCH 0/4] eeprom-93xx46: Add support for Atmel AT93C56 and AT93C66

[PATCH 0/4] eeprom-93xx46: Add support for Atmel AT93C56 and AT93C66

[Emmanuel Gil Peyrot]

These two devices differ from the AT93C46 by their storage size,
respectively 2 Kib and 4 Kib, while the AT93C46 contains 1 Kib.

The driver was currently hardcoding that addrlen == 7 means 8-bit words,
and anything else means 16-bit words.  This is obviously not going to
work with more storage and thus more bits spent on the address (for
instance on the AT93C66 in 8-bit words mode, addrlen == 9 since there
are 512 bytes to address), so I’m now doing those checks based on the
word size specified in the device tree.

It might make sense to rename this driver now that it supports all three
sizes, but I don’t know what would be a good name, eeprom_93xxx6 doesn’t
sound very nice.

I have tested this series on a Nintendo Wii U, on the downstream
linux-wiiu kernel based on 4.19, and thus only with a AT93C66.  You can
find this work here if you want to test it:
https://gitlab.com/linux-wiiu/linux-wiiu/-/merge_requests/16

Emmanuel Gil Peyrot (4):
  misc: eeprom_93xx46: Add new at93c56 and at93c66 compatible strings
  misc: eeprom_93xx46: set size and addrlen according to the dts
  misc: eeprom_93xx46: Compute bits based on addrlen
  misc: eeprom_93xx46: Switch based on word size, not addrlen

 .../bindings/misc/eeprom-93xx46.txt           |  3 +
 drivers/misc/eeprom/eeprom_93xx46.c           | 85 ++++++++++++++-----
 include/linux/eeprom_93xx46.h                 |  3 +
 3 files changed, 68 insertions(+), 23 deletions(-)

-- 
2.31.1

[PATCH 1/4] misc: eeprom_93xx46: Add new at93c56 and at93c66 compatible strings

[Emmanuel Gil Peyrot]

These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 .../bindings/misc/eeprom-93xx46.txt           |  3 +++
 drivers/misc/eeprom/eeprom_93xx46.c           | 21 ++++++++++++++++++-
 include/linux/eeprom_93xx46.h                 |  3 +++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
index 7b636b7a8311..72ea0af368d4 100644
--- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
+++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
@@ -2,7 +2,10 @@ EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
 
 Required properties:
 - compatible : shall be one of:
+    "atmel,at93c46"
     "atmel,at93c46d"
+    "atmel,at93c56"
+    "atmel,at93c66"
     "eeprom-93xx46"
     "microchip,93lc46b"
 - data-size : number of data bits per word (either 8 or 16)
diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 80114f4c80ad..64dd76f66463 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -28,14 +28,29 @@
 
 struct eeprom_93xx46_devtype_data {
 	unsigned int quirks;
+	unsigned char flags;
+};
+
+static const struct eeprom_93xx46_devtype_data at93c46_data = {
+	.flags = EE_SIZE1K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c56_data = {
+	.flags = EE_SIZE2K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c66_data = {
+	.flags = EE_SIZE4K,
 };
 
 static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
 		  EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
 };
 
 static const struct eeprom_93xx46_devtype_data microchip_93lc46b_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_EXTRA_READ_CYCLE,
 };
 
@@ -375,8 +390,11 @@ static void select_deassert(void *context)
 }
 
 static const struct of_device_id eeprom_93xx46_of_table[] = {
-	{ .compatible = "eeprom-93xx46", },
+	{ .compatible = "eeprom-93xx46", .data = &at93c46_data, },
+	{ .compatible = "atmel,at93c46", .data = &at93c46_data, },
 	{ .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
+	{ .compatible = "atmel,at93c56", .data = &at93c56_data, },
+	{ .compatible = "atmel,at93c66", .data = &at93c66_data, },
 	{ .compatible = "microchip,93lc46b", .data = &microchip_93lc46b_data, },
 	{}
 };
@@ -425,6 +443,7 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
 	if (of_id->data) {
 		const struct eeprom_93xx46_devtype_data *data = of_id->data;
 
+		pd->flags |= data->flags;
 		pd->quirks = data->quirks;
 	}
 
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index 99580c22f91a..34c2175e6a1e 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -10,6 +10,9 @@ struct eeprom_93xx46_platform_data {
 #define EE_ADDR8	0x01		/*  8 bit addr. cfg */
 #define EE_ADDR16	0x02		/* 16 bit addr. cfg */
 #define EE_READONLY	0x08		/* forbid writing */
+#define EE_SIZE1K	0x10		/* 1 kb of data, that is a 93xx46 */
+#define EE_SIZE2K	0x20		/* 2 kb of data, that is a 93xx56 */
+#define EE_SIZE4K	0x40		/* 4 kb of data, that is a 93xx66 */
 
 	unsigned int	quirks;
 /* Single word read transfers only; no sequential read. */
-- 
2.31.1

[PATCH 2/4] misc: eeprom_93xx46: set size and addrlen according to the dts

[Emmanuel Gil Peyrot]

This can then be used by the rest of the driver to use the correct
commands on 93c56 and 93c66.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 64dd76f66463..39375255e22a 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -18,6 +18,7 @@
 #include <linux/spi/spi.h>
 #include <linux/nvmem-provider.h>
 #include <linux/eeprom_93xx46.h>
+#include <linux/log2.h>
 
 #define OP_START	0x4
 #define OP_WRITE	(OP_START | 0x1)
@@ -474,10 +475,22 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (!edev)
 		return -ENOMEM;
 
+	if (pd->flags & EE_SIZE1K)
+		edev->size = 128;
+	else if (pd->flags & EE_SIZE2K)
+		edev->size = 256;
+	else if (pd->flags & EE_SIZE4K)
+		edev->size = 512;
+	else {
+		dev_err(&spi->dev, "unspecified size\n");
+		err = -EINVAL;
+		goto fail;
+	}
+
 	if (pd->flags & EE_ADDR8)
-		edev->addrlen = 7;
+		edev->addrlen = ilog2(edev->size);
 	else if (pd->flags & EE_ADDR16)
-		edev->addrlen = 6;
+		edev->addrlen = ilog2(edev->size) - 1;
 	else {
 		dev_err(&spi->dev, "unspecified address type\n");
 		return -EINVAL;
@@ -488,7 +501,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	edev->spi = spi;
 	edev->pdata = pd;
 
-	edev->size = 128;
 	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
 	edev->nvmem_config.name = dev_name(&spi->dev);
 	edev->nvmem_config.dev = &spi->dev;
@@ -508,8 +520,9 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (IS_ERR(edev->nvmem))
 		return PTR_ERR(edev->nvmem);
 
-	dev_info(&spi->dev, "%d-bit eeprom %s\n",
+	dev_info(&spi->dev, "%d-bit eeprom containing %d bytes %s\n",
 		(pd->flags & EE_ADDR8) ? 8 : 16,
+		edev->size,
 		(pd->flags & EE_READONLY) ? "(readonly)" : "");
 
 	if (!(pd->flags & EE_READONLY)) {
-- 
2.31.1

[PATCH 3/4] misc: eeprom_93xx46: Compute bits based on addrlen

[Emmanuel Gil Peyrot]

In the read case, this also moves it out of the loop.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 39375255e22a..2f4b39417873 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -86,6 +86,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	struct eeprom_93xx46_dev *edev = priv;
 	char *buf = val;
 	int err = 0;
+	int bits;
 
 	if (unlikely(off >= edev->size))
 		return 0;
@@ -99,21 +100,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	if (edev->pdata->prepare)
 		edev->pdata->prepare(edev);
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	while (count) {
 		struct spi_message m;
 		struct spi_transfer t[2] = { { 0 } };
 		u16 cmd_addr = OP_READ << edev->addrlen;
 		size_t nbytes = count;
-		int bits;
 
 		if (edev->addrlen == 7) {
 			cmd_addr |= off & 0x7f;
-			bits = 10;
 			if (has_quirk_single_word_read(edev))
 				nbytes = 1;
 		} else {
 			cmd_addr |= (off >> 1) & 0x3f;
-			bits = 9;
 			if (has_quirk_single_word_read(edev))
 				nbytes = 2;
 		}
@@ -168,13 +169,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
 	if (edev->addrlen == 7) {
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
-		bits = 10;
 	} else {
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
-		bits = 9;
 	}
 
 	if (has_quirk_instruction_length(edev)) {
@@ -221,15 +223,16 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
 	int bits, data_len, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_WRITE << edev->addrlen;
 
 	if (edev->addrlen == 7) {
 		cmd_addr |= off & 0x7f;
-		bits = 10;
 		data_len = 1;
 	} else {
 		cmd_addr |= (off >> 1) & 0x3f;
-		bits = 9;
 		data_len = 2;
 	}
 
@@ -311,13 +314,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
 	if (edev->addrlen == 7) {
 		cmd_addr |= ADDR_ERAL << 1;
-		bits = 10;
 	} else {
 		cmd_addr |= ADDR_ERAL;
-		bits = 9;
 	}
 
 	if (has_quirk_instruction_length(edev)) {
-- 
2.31.1

[PATCH 4/4] misc: eeprom_93xx46: Switch based on word size, not addrlen

[Emmanuel Gil Peyrot]

This avoids using magic numbers based on the total size and length of an
address, where we only want to differentiate between 8-bit and 16-bit,
and finally makes 93c56 and 93c66 usable!

If the two pointer indirections is too much, we could move the flags to
the main struct instead, but I doubt it’s going to make any sensible
difference.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 2f4b39417873..afa89f71a390 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -109,12 +109,12 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 		u16 cmd_addr = OP_READ << edev->addrlen;
 		size_t nbytes = count;
 
-		if (edev->addrlen == 7) {
-			cmd_addr |= off & 0x7f;
+		if (edev->pdata->flags & EE_ADDR8) {
+			cmd_addr |= off;
 			if (has_quirk_single_word_read(edev))
 				nbytes = 1;
 		} else {
-			cmd_addr |= (off >> 1) & 0x3f;
+			cmd_addr |= (off >> 1);
 			if (has_quirk_single_word_read(edev))
 				nbytes = 2;
 		}
@@ -173,7 +173,7 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
 	bits = edev->addrlen + 3;
 
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8) {
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
 	} else {
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
@@ -223,16 +223,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
 	int bits, data_len, ret;
 	u16 cmd_addr;
 
+	if (unlikely(off >= edev->size))
+		return -EINVAL;
+
 	/* The opcode in front of the address is three bits. */
 	bits = edev->addrlen + 3;
 
 	cmd_addr = OP_WRITE << edev->addrlen;
 
-	if (edev->addrlen == 7) {
-		cmd_addr |= off & 0x7f;
+	if (edev->pdata->flags & EE_ADDR8) {
+		cmd_addr |= off;
 		data_len = 1;
 	} else {
-		cmd_addr |= (off >> 1) & 0x3f;
+		cmd_addr |= (off >> 1);
 		data_len = 2;
 	}
 
@@ -272,7 +275,7 @@ static int eeprom_93xx46_write(void *priv, unsigned int off,
 		return count;
 
 	/* only write even number of bytes on 16-bit devices */
-	if (edev->addrlen == 6) {
+	if (edev->pdata->flags & EE_ADDR16) {
 		step = 2;
 		count &= ~1;
 	}
@@ -318,7 +321,7 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
 	bits = edev->addrlen + 3;
 
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8) {
 		cmd_addr |= ADDR_ERAL << 1;
 	} else {
 		cmd_addr |= ADDR_ERAL;
-- 
2.31.1

Re: [PATCH 1/4] misc: eeprom_93xx46: Add new at93c56 and at93c66 compatible strings

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1551 bytes --]

On Sat, Apr 24, 2021 at 02:30:30PM +0200, Emmanuel Gil Peyrot wrote:
> These two devices have respectively 2048 and 4096 bits of storage,
> compared to 1024 for the 93c46.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  .../bindings/misc/eeprom-93xx46.txt           |  3 +++
>  drivers/misc/eeprom/eeprom_93xx46.c           | 21 ++++++++++++++++++-
>  include/linux/eeprom_93xx46.h                 |  3 +++
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> index 7b636b7a8311..72ea0af368d4 100644
> --- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> @@ -2,7 +2,10 @@ EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
>  
>  Required properties:
>  - compatible : shall be one of:
> +    "atmel,at93c46"
>      "atmel,at93c46d"
> +    "atmel,at93c56"
> +    "atmel,at93c66"
>      "eeprom-93xx46"
>      "microchip,93lc46b"
>  - data-size : number of data bits per word (either 8 or 16)

The DT binding patch should ideally be separate from the driver patch.

> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 80114f4c80ad..64dd76f66463 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c


other than that, the patch looks fine to me, AFAICT.


Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] misc: eeprom_93xx46: set size and addrlen according to the dts

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2663 bytes --]

Hi,


> [PATCH 2/4] misc: eeprom_93xx46: set size and addrlen according to the dts

This patch doesn't really deal with the devicetree, so this subject line
seems a bit mismatched.

On Sat, Apr 24, 2021 at 02:30:31PM +0200, Emmanuel Gil Peyrot wrote:
> This can then be used by the rest of the driver to use the correct
> commands on 93c56 and 93c66.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

Ah hmmm. Does this mean that with the previous patch, the driver will be
instanciated for 93c56 and 93c66 but send the wrong commands? I think
you should avoid this pitfall by rearranging (or squashing) the patches.

>  drivers/misc/eeprom/eeprom_93xx46.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 64dd76f66463..39375255e22a 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -18,6 +18,7 @@
>  #include <linux/spi/spi.h>
>  #include <linux/nvmem-provider.h>
>  #include <linux/eeprom_93xx46.h>
> +#include <linux/log2.h>
>  
>  #define OP_START	0x4
>  #define OP_WRITE	(OP_START | 0x1)
> @@ -474,10 +475,22 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	if (!edev)
>  		return -ENOMEM;
>  
> +	if (pd->flags & EE_SIZE1K)
> +		edev->size = 128;
> +	else if (pd->flags & EE_SIZE2K)
> +		edev->size = 256;
> +	else if (pd->flags & EE_SIZE4K)
> +		edev->size = 512;
> +	else {
> +		dev_err(&spi->dev, "unspecified size\n");
> +		err = -EINVAL;
> +		goto fail;
> +	}
> +
>  	if (pd->flags & EE_ADDR8)
> -		edev->addrlen = 7;
> +		edev->addrlen = ilog2(edev->size);
>  	else if (pd->flags & EE_ADDR16)
> -		edev->addrlen = 6;
> +		edev->addrlen = ilog2(edev->size) - 1;
>  	else {
>  		dev_err(&spi->dev, "unspecified address type\n");
>  		return -EINVAL;
> @@ -488,7 +501,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	edev->spi = spi;
>  	edev->pdata = pd;
>  
> -	edev->size = 128;
>  	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
>  	edev->nvmem_config.name = dev_name(&spi->dev);
>  	edev->nvmem_config.dev = &spi->dev;
> @@ -508,8 +520,9 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	if (IS_ERR(edev->nvmem))
>  		return PTR_ERR(edev->nvmem);
>  
> -	dev_info(&spi->dev, "%d-bit eeprom %s\n",
> +	dev_info(&spi->dev, "%d-bit eeprom containing %d bytes %s\n",
>  		(pd->flags & EE_ADDR8) ? 8 : 16,
> +		edev->size,
>  		(pd->flags & EE_READONLY) ? "(readonly)" : "");


The logic itself looks good though.

Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/4] misc: eeprom_93xx46: Compute bits based on addrlen

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 3790 bytes --]

> [PATCH 3/4] misc: eeprom_93xx46: Compute bits based on addrlen

It's not obvious what "bits" and "addrlen" mean, without reading the
code first — can you perhaps rephrase this in a more meaningful (to the
uninitiated) way?

Maybe:   Compute command length based on address length

On Sat, Apr 24, 2021 at 02:30:32PM +0200, Emmanuel Gil Peyrot wrote:
> In the read case, this also moves it out of the loop.

I think this patch could use a slightly longer description:

- What's the rough aim of it?
- Is it purely a refactoring, or does it result in different observable
  behavior?

> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  drivers/misc/eeprom/eeprom_93xx46.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 39375255e22a..2f4b39417873 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -86,6 +86,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	struct eeprom_93xx46_dev *edev = priv;
>  	char *buf = val;
>  	int err = 0;
> +	int bits;
>  
>  	if (unlikely(off >= edev->size))
>  		return 0;
> @@ -99,21 +100,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	if (edev->pdata->prepare)
>  		edev->pdata->prepare(edev);
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	while (count) {
>  		struct spi_message m;
>  		struct spi_transfer t[2] = { { 0 } };
>  		u16 cmd_addr = OP_READ << edev->addrlen;
>  		size_t nbytes = count;
> -		int bits;
>  
>  		if (edev->addrlen == 7) {
>  			cmd_addr |= off & 0x7f;
> -			bits = 10;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 1;
>  		} else {
>  			cmd_addr |= (off >> 1) & 0x3f;
> -			bits = 9;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 2;
>  		}

The if/else looks bogus as there are now more than two different address
lengths. This if/else seems to conflate two things:

- how the command/address bits should be shifted around to form a proper
  command
- whether we're dealing with 8-bit or 16-bit words (nbytes = ...)

> @@ -168,13 +169,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
> -		bits = 10;
>  	} else {
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
> -		bits = 9;
>  	}

dito.

>  
>  	if (has_quirk_instruction_length(edev)) {
> @@ -221,15 +223,16 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>  	int bits, data_len, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_WRITE << edev->addrlen;
>  
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= off & 0x7f;
> -		bits = 10;
>  		data_len = 1;
>  	} else {
>  		cmd_addr |= (off >> 1) & 0x3f;
> -		bits = 9;
>  		data_len = 2;
>  	}

dito.

>  
> @@ -311,13 +314,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
>  	if (edev->addrlen == 7) {
>  		cmd_addr |= ADDR_ERAL << 1;
> -		bits = 10;
>  	} else {
>  		cmd_addr |= ADDR_ERAL;
> -		bits = 9;
>  	}

dito.



Thank you for cleaning up this driver!

Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 4/4] misc: eeprom_93xx46: Switch based on word size, not addrlen

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1017 bytes --]

On Sat, Apr 24, 2021 at 02:30:33PM +0200, Emmanuel Gil Peyrot wrote:
> This avoids using magic numbers based on the total size and length of an
> address, where we only want to differentiate between 8-bit and 16-bit,
> and finally makes 93c56 and 93c66 usable!
> 
> If the two pointer indirections is too much, we could move the flags to
> the main struct instead, but I doubt it’s going to make any sensible
> difference.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

Ah, this somewhat addresses my reply to the previous patch.
I think by rearranging and/or squashing the patches, they could tell a
more coherent story, and cause less confusion.

(Basically: avoid creating conditions where the code is wrong — if a
 later patch is needed in order to make a previous patch correct, but the
 later patch alone wouldn't make the code incorrect, swap them. If there's
 breakage either way and you can't tease them apart, squash them together.)



Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] misc: eeprom_93xx46: set size and addrlen according to the dts

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 11716 bytes --]

Hi Emmanuel,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on soc/for-next robh/for-next linux/master linus/master v5.12-rc8 next-20210423]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Emmanuel-Gil-Peyrot/eeprom-93xx46-Add-support-for-Atmel-AT93C56-and-AT93C66/20210424-204020
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git e2cb6b891ad2b8caa9131e3be70f45243df82a80
config: riscv-randconfig-r036-20210424 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project 3d1aecbd285709f58168b3ad65c06da4b42132a9)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/46b220709479fc35862b671390a11c6da2ec4d08
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Emmanuel-Gil-Peyrot/eeprom-93xx46-Add-support-for-Atmel-AT93C56-and-AT93C66/20210424-204020
        git checkout 46b220709479fc35862b671390a11c6da2ec4d08
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:556:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inb(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:55:76: note: expanded from macro 'inb'
   #define inb(c)          ({ u8  __v; __io_pbr(); __v = readb_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:87:48: note: expanded from macro 'readb_cpu'
   #define readb_cpu(c)            ({ u8  __r = __raw_readb(c); __r; })
                                                            ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:564:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inw(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:56:76: note: expanded from macro 'inw'
   #define inw(c)          ({ u16 __v; __io_pbr(); __v = readw_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:88:76: note: expanded from macro 'readw_cpu'
   #define readw_cpu(c)            ({ u16 __r = le16_to_cpu((__force __le16)__raw_readw(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:36:51: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
                                                     ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:572:9: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return inl(addr);
                  ^~~~~~~~~
   arch/riscv/include/asm/io.h:57:76: note: expanded from macro 'inl'
   #define inl(c)          ({ u32 __v; __io_pbr(); __v = readl_cpu((void*)(PCI_IOBASE + (c))); __io_par(__v); __v; })
                                                                           ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:89:76: note: expanded from macro 'readl_cpu'
   #define readl_cpu(c)            ({ u32 __r = le32_to_cpu((__force __le32)__raw_readl(c)); __r; })
                                                                                        ^
   include/uapi/linux/byteorder/little_endian.h:34:51: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
                                                     ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:580:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outb(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:59:68: note: expanded from macro 'outb'
   #define outb(v,c)       ({ __io_pbw(); writeb_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:91:52: note: expanded from macro 'writeb_cpu'
   #define writeb_cpu(v, c)        ((void)__raw_writeb((v), (c)))
                                                             ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:588:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outw(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:60:68: note: expanded from macro 'outw'
   #define outw(v,c)       ({ __io_pbw(); writew_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:92:76: note: expanded from macro 'writew_cpu'
   #define writew_cpu(v, c)        ((void)__raw_writew((__force u16)cpu_to_le16(v), (c)))
                                                                                     ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:596:2: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           outl(value, addr);
           ^~~~~~~~~~~~~~~~~
   arch/riscv/include/asm/io.h:61:68: note: expanded from macro 'outl'
   #define outl(v,c)       ({ __io_pbw(); writel_cpu((v),(void*)(PCI_IOBASE + (c))); __io_paw(); })
                                                                 ~~~~~~~~~~ ^
   arch/riscv/include/asm/mmio.h:93:76: note: expanded from macro 'writel_cpu'
   #define writel_cpu(v, c)        ((void)__raw_writel((__force u32)cpu_to_le32(v), (c)))
                                                                                     ^
   In file included from drivers/misc/eeprom/eeprom_93xx46.c:16:
   In file included from include/linux/of_gpio.h:14:
   In file included from include/linux/gpio/driver.h:7:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:13:
   In file included from arch/riscv/include/asm/io.h:149:
   include/asm-generic/io.h:1005:55: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           return (port > MMIO_UPPER_LIMIT) ? NULL : PCI_IOBASE + port;
                                                     ~~~~~~~~~~ ^
>> drivers/misc/eeprom/eeprom_93xx46.c:487:8: error: use of undeclared label 'fail'
                   goto fail;
                        ^
   7 warnings and 1 error generated.


vim +/fail +487 drivers/misc/eeprom/eeprom_93xx46.c

   455	
   456	static int eeprom_93xx46_probe(struct spi_device *spi)
   457	{
   458		struct eeprom_93xx46_platform_data *pd;
   459		struct eeprom_93xx46_dev *edev;
   460		int err;
   461	
   462		if (spi->dev.of_node) {
   463			err = eeprom_93xx46_probe_dt(spi);
   464			if (err < 0)
   465				return err;
   466		}
   467	
   468		pd = spi->dev.platform_data;
   469		if (!pd) {
   470			dev_err(&spi->dev, "missing platform data\n");
   471			return -ENODEV;
   472		}
   473	
   474		edev = devm_kzalloc(&spi->dev, sizeof(*edev), GFP_KERNEL);
   475		if (!edev)
   476			return -ENOMEM;
   477	
   478		if (pd->flags & EE_SIZE1K)
   479			edev->size = 128;
   480		else if (pd->flags & EE_SIZE2K)
   481			edev->size = 256;
   482		else if (pd->flags & EE_SIZE4K)
   483			edev->size = 512;
   484		else {
   485			dev_err(&spi->dev, "unspecified size\n");
   486			err = -EINVAL;
 > 487			goto fail;
   488		}
   489	
   490		if (pd->flags & EE_ADDR8)
   491			edev->addrlen = ilog2(edev->size);
   492		else if (pd->flags & EE_ADDR16)
   493			edev->addrlen = ilog2(edev->size) - 1;
   494		else {
   495			dev_err(&spi->dev, "unspecified address type\n");
   496			return -EINVAL;
   497		}
   498	
   499		mutex_init(&edev->lock);
   500	
   501		edev->spi = spi;
   502		edev->pdata = pd;
   503	
   504		edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
   505		edev->nvmem_config.name = dev_name(&spi->dev);
   506		edev->nvmem_config.dev = &spi->dev;
   507		edev->nvmem_config.read_only = pd->flags & EE_READONLY;
   508		edev->nvmem_config.root_only = true;
   509		edev->nvmem_config.owner = THIS_MODULE;
   510		edev->nvmem_config.compat = true;
   511		edev->nvmem_config.base_dev = &spi->dev;
   512		edev->nvmem_config.reg_read = eeprom_93xx46_read;
   513		edev->nvmem_config.reg_write = eeprom_93xx46_write;
   514		edev->nvmem_config.priv = edev;
   515		edev->nvmem_config.stride = 4;
   516		edev->nvmem_config.word_size = 1;
   517		edev->nvmem_config.size = edev->size;
   518	
   519		edev->nvmem = devm_nvmem_register(&spi->dev, &edev->nvmem_config);
   520		if (IS_ERR(edev->nvmem))
   521			return PTR_ERR(edev->nvmem);
   522	
   523		dev_info(&spi->dev, "%d-bit eeprom containing %d bytes %s\n",
   524			(pd->flags & EE_ADDR8) ? 8 : 16,
   525			edev->size,
   526			(pd->flags & EE_READONLY) ? "(readonly)" : "");
   527	
   528		if (!(pd->flags & EE_READONLY)) {
   529			if (device_create_file(&spi->dev, &dev_attr_erase))
   530				dev_err(&spi->dev, "can't create erase interface\n");
   531		}
   532	
   533		spi_set_drvdata(spi, edev);
   534		return 0;
   535	}
   536	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 31637 bytes --]

[PATCH v2 0/3] eeprom-93xx46: Add support for Atmel AT93C56 and AT93C66

[Emmanuel Gil Peyrot]

These two devices differ from the AT93C46 by their storage size,
respectively 2 Kib and 4 Kib, while the AT93C46 contains 1 Kib.

The driver was currently hardcoding that addrlen == 7 means 8-bit words,
and anything else means 16-bit words.  This is obviously not going to
work with more storage and thus more bits spent on the address (for
instance on the AT93C66 in 8-bit words mode, addrlen == 9 since there
are 512 bytes to address), so I’m now doing those checks based on the
word size specified in the device tree.

It might make sense to rename this driver now that it supports all three
sizes, but I don’t know what would be a good name, eeprom_93xxx6 doesn’t
sound very nice.

I have tested this series on a Nintendo Wii U, on the downstream
linux-wiiu kernel based on 4.19, and thus only with a AT93C66.  You can
find this work here if you want to test it:
https://gitlab.com/linux-wiiu/linux-wiiu/-/merge_requests/16

Changes since v1:
- Reordered and squashed patches.
- Split out the dts addition.
- Removed a bogus goto.
- Improved commit messages to make what they do more explicit.

Emmanuel Gil Peyrot (3):
  misc: eeprom_93xx46: Remove hardcoded bit lengths
  misc: eeprom_93xx46: Add new 93c56 and 93c66 compatible strings
  dts: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

 .../bindings/misc/eeprom-93xx46.txt           |  3 +
 drivers/misc/eeprom/eeprom_93xx46.c           | 90 +++++++++++++------
 include/linux/eeprom_93xx46.h                 |  3 +
 3 files changed, 69 insertions(+), 27 deletions(-)

-- 
2.31.1

[PATCH v2 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths

[Emmanuel Gil Peyrot]

This avoids using magic numbers based on the length of an address or a
command, while we only want to differentiate between 8-bit and 16-bit.

The driver was previously wrapping around the offset in the write
operation, this now returns -EINVAL instead (but should never happen in
the first place).

If two pointer indirections are too many, we could move the flags to the
main struct instead, but I doubt it’s going to make any sensible
difference on any hardware.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 57 ++++++++++++++++-------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 80114f4c80ad..ffdb8e5a26e0 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
@@ -70,6 +71,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	struct eeprom_93xx46_dev *edev = priv;
 	char *buf = val;
 	int err = 0;
+	int bits;
 
 	if (unlikely(off >= edev->size))
 		return 0;
@@ -83,21 +85,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	if (edev->pdata->prepare)
 		edev->pdata->prepare(edev);
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	while (count) {
 		struct spi_message m;
 		struct spi_transfer t[2] = { { 0 } };
 		u16 cmd_addr = OP_READ << edev->addrlen;
 		size_t nbytes = count;
-		int bits;
 
-		if (edev->addrlen == 7) {
-			cmd_addr |= off & 0x7f;
-			bits = 10;
+		if (edev->pdata->flags & EE_ADDR8) {
+			cmd_addr |= off;
 			if (has_quirk_single_word_read(edev))
 				nbytes = 1;
 		} else {
-			cmd_addr |= (off >> 1) & 0x3f;
-			bits = 9;
+			cmd_addr |= (off >> 1);
 			if (has_quirk_single_word_read(edev))
 				nbytes = 2;
 		}
@@ -152,14 +154,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8)
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
-		bits = 10;
-	} else {
+	else
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
-		bits = 9;
-	}
 
 	if (has_quirk_instruction_length(edev)) {
 		cmd_addr <<= 2;
@@ -205,15 +207,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
 	int bits, data_len, ret;
 	u16 cmd_addr;
 
+	if (unlikely(off >= edev->size))
+		return -EINVAL;
+
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_WRITE << edev->addrlen;
 
-	if (edev->addrlen == 7) {
-		cmd_addr |= off & 0x7f;
-		bits = 10;
+	if (edev->pdata->flags & EE_ADDR8) {
+		cmd_addr |= off;
 		data_len = 1;
 	} else {
-		cmd_addr |= (off >> 1) & 0x3f;
-		bits = 9;
+		cmd_addr |= (off >> 1);
 		data_len = 2;
 	}
 
@@ -253,7 +259,7 @@ static int eeprom_93xx46_write(void *priv, unsigned int off,
 		return count;
 
 	/* only write even number of bytes on 16-bit devices */
-	if (edev->addrlen == 6) {
+	if (edev->pdata->flags & EE_ADDR16) {
 		step = 2;
 		count &= ~1;
 	}
@@ -295,14 +301,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8)
 		cmd_addr |= ADDR_ERAL << 1;
-		bits = 10;
-	} else {
+	else
 		cmd_addr |= ADDR_ERAL;
-		bits = 9;
-	}
 
 	if (has_quirk_instruction_length(edev)) {
 		cmd_addr <<= 2;
@@ -455,10 +461,12 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (!edev)
 		return -ENOMEM;
 
+	edev->size = 128;
+
 	if (pd->flags & EE_ADDR8)
-		edev->addrlen = 7;
+		edev->addrlen = ilog2(edev->size);
 	else if (pd->flags & EE_ADDR16)
-		edev->addrlen = 6;
+		edev->addrlen = ilog2(edev->size) - 1;
 	else {
 		dev_err(&spi->dev, "unspecified address type\n");
 		return -EINVAL;
@@ -469,7 +477,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	edev->spi = spi;
 	edev->pdata = pd;
 
-	edev->size = 128;
 	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
 	edev->nvmem_config.name = dev_name(&spi->dev);
 	edev->nvmem_config.dev = &spi->dev;
-- 
2.31.1

[PATCH v2 2/3] misc: eeprom_93xx46: Add new 93c56 and 93c66 compatible strings

[Emmanuel Gil Peyrot]

These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 35 ++++++++++++++++++++++++++---
 include/linux/eeprom_93xx46.h       |  3 +++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index ffdb8e5a26e0..29d8971ec558 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -29,14 +29,29 @@
 
 struct eeprom_93xx46_devtype_data {
 	unsigned int quirks;
+	unsigned char flags;
+};
+
+static const struct eeprom_93xx46_devtype_data at93c46_data = {
+	.flags = EE_SIZE1K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c56_data = {
+	.flags = EE_SIZE2K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c66_data = {
+	.flags = EE_SIZE4K,
 };
 
 static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
 		  EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
 };
 
 static const struct eeprom_93xx46_devtype_data microchip_93lc46b_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_EXTRA_READ_CYCLE,
 };
 
@@ -381,8 +396,11 @@ static void select_deassert(void *context)
 }
 
 static const struct of_device_id eeprom_93xx46_of_table[] = {
-	{ .compatible = "eeprom-93xx46", },
+	{ .compatible = "eeprom-93xx46", .data = &at93c46_data, },
+	{ .compatible = "atmel,at93c46", .data = &at93c46_data, },
 	{ .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
+	{ .compatible = "atmel,at93c56", .data = &at93c56_data, },
+	{ .compatible = "atmel,at93c66", .data = &at93c66_data, },
 	{ .compatible = "microchip,93lc46b", .data = &microchip_93lc46b_data, },
 	{}
 };
@@ -432,6 +450,7 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
 		const struct eeprom_93xx46_devtype_data *data = of_id->data;
 
 		pd->quirks = data->quirks;
+		pd->flags |= data->flags;
 	}
 
 	spi->dev.platform_data = pd;
@@ -461,7 +480,16 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (!edev)
 		return -ENOMEM;
 
-	edev->size = 128;
+	if (pd->flags & EE_SIZE1K)
+		edev->size = 128;
+	else if (pd->flags & EE_SIZE2K)
+		edev->size = 256;
+	else if (pd->flags & EE_SIZE4K)
+		edev->size = 512;
+	else {
+		dev_err(&spi->dev, "unspecified size\n");
+		return -EINVAL;
+	}
 
 	if (pd->flags & EE_ADDR8)
 		edev->addrlen = ilog2(edev->size);
@@ -496,8 +524,9 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (IS_ERR(edev->nvmem))
 		return PTR_ERR(edev->nvmem);
 
-	dev_info(&spi->dev, "%d-bit eeprom %s\n",
+	dev_info(&spi->dev, "%d-bit eeprom containing %d bytes %s\n",
 		(pd->flags & EE_ADDR8) ? 8 : 16,
+		edev->size,
 		(pd->flags & EE_READONLY) ? "(readonly)" : "");
 
 	if (!(pd->flags & EE_READONLY)) {
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index 99580c22f91a..34c2175e6a1e 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -10,6 +10,9 @@ struct eeprom_93xx46_platform_data {
 #define EE_ADDR8	0x01		/*  8 bit addr. cfg */
 #define EE_ADDR16	0x02		/* 16 bit addr. cfg */
 #define EE_READONLY	0x08		/* forbid writing */
+#define EE_SIZE1K	0x10		/* 1 kb of data, that is a 93xx46 */
+#define EE_SIZE2K	0x20		/* 2 kb of data, that is a 93xx56 */
+#define EE_SIZE4K	0x40		/* 4 kb of data, that is a 93xx66 */
 
 	unsigned int	quirks;
 /* Single word read transfers only; no sequential read. */
-- 
2.31.1

[PATCH v2 3/3] dts: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

[Emmanuel Gil Peyrot]

These devices differ by the size of their storage, which is why they
have different compatible strings.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
---
 Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
index 7b636b7a8311..72ea0af368d4 100644
--- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
+++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
@@ -2,7 +2,10 @@ EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
 
 Required properties:
 - compatible : shall be one of:
+    "atmel,at93c46"
     "atmel,at93c46d"
+    "atmel,at93c56"
+    "atmel,at93c66"
     "eeprom-93xx46"
     "microchip,93lc46b"
 - data-size : number of data bits per word (either 8 or 16)
-- 
2.31.1

Re: [PATCH v2 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 5227 bytes --]

On Sat, Apr 24, 2021 at 11:25:41PM +0200, Emmanuel Gil Peyrot wrote:
> This avoids using magic numbers based on the length of an address or a
> command, while we only want to differentiate between 8-bit and 16-bit.
> 
> The driver was previously wrapping around the offset in the write
> operation, this now returns -EINVAL instead (but should never happen in
> the first place).
> 
> If two pointer indirections are too many, we could move the flags to the
> main struct instead, but I doubt it’s going to make any sensible
> difference on any hardware.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

I'm not an EEPROM driver expert, but FWIW:

Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>


>  drivers/misc/eeprom/eeprom_93xx46.c | 57 ++++++++++++++++-------------
>  1 file changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
> index 80114f4c80ad..ffdb8e5a26e0 100644
> --- a/drivers/misc/eeprom/eeprom_93xx46.c
> +++ b/drivers/misc/eeprom/eeprom_93xx46.c
> @@ -9,6 +9,7 @@
>  #include <linux/device.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/kernel.h>
> +#include <linux/log2.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/of.h>
> @@ -70,6 +71,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	struct eeprom_93xx46_dev *edev = priv;
>  	char *buf = val;
>  	int err = 0;
> +	int bits;
>  
>  	if (unlikely(off >= edev->size))
>  		return 0;
> @@ -83,21 +85,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
>  	if (edev->pdata->prepare)
>  		edev->pdata->prepare(edev);
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	while (count) {
>  		struct spi_message m;
>  		struct spi_transfer t[2] = { { 0 } };
>  		u16 cmd_addr = OP_READ << edev->addrlen;
>  		size_t nbytes = count;
> -		int bits;
>  
> -		if (edev->addrlen == 7) {
> -			cmd_addr |= off & 0x7f;
> -			bits = 10;
> +		if (edev->pdata->flags & EE_ADDR8) {
> +			cmd_addr |= off;
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 1;
>  		} else {
> -			cmd_addr |= (off >> 1) & 0x3f;
> -			bits = 9;
> +			cmd_addr |= (off >> 1);
>  			if (has_quirk_single_word_read(edev))
>  				nbytes = 2;
>  		}
> @@ -152,14 +154,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
> -	if (edev->addrlen == 7) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
> -		bits = 10;
> -	} else {
> +	else
>  		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
> -		bits = 9;
> -	}
>  
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<= 2;
> @@ -205,15 +207,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
>  	int bits, data_len, ret;
>  	u16 cmd_addr;
>  
> +	if (unlikely(off >= edev->size))
> +		return -EINVAL;
> +
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_WRITE << edev->addrlen;
>  
> -	if (edev->addrlen == 7) {
> -		cmd_addr |= off & 0x7f;
> -		bits = 10;
> +	if (edev->pdata->flags & EE_ADDR8) {
> +		cmd_addr |= off;
>  		data_len = 1;
>  	} else {
> -		cmd_addr |= (off >> 1) & 0x3f;
> -		bits = 9;
> +		cmd_addr |= (off >> 1);
>  		data_len = 2;
>  	}
>  
> @@ -253,7 +259,7 @@ static int eeprom_93xx46_write(void *priv, unsigned int off,
>  		return count;
>  
>  	/* only write even number of bytes on 16-bit devices */
> -	if (edev->addrlen == 6) {
> +	if (edev->pdata->flags & EE_ADDR16) {
>  		step = 2;
>  		count &= ~1;
>  	}
> @@ -295,14 +301,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
>  	int bits, ret;
>  	u16 cmd_addr;
>  
> +	/* The opcode in front of the address is three bits. */
> +	bits = edev->addrlen + 3;
> +
>  	cmd_addr = OP_START << edev->addrlen;
> -	if (edev->addrlen == 7) {
> +	if (edev->pdata->flags & EE_ADDR8)
>  		cmd_addr |= ADDR_ERAL << 1;
> -		bits = 10;
> -	} else {
> +	else
>  		cmd_addr |= ADDR_ERAL;
> -		bits = 9;
> -	}
>  
>  	if (has_quirk_instruction_length(edev)) {
>  		cmd_addr <<= 2;
> @@ -455,10 +461,12 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	if (!edev)
>  		return -ENOMEM;
>  
> +	edev->size = 128;
> +
>  	if (pd->flags & EE_ADDR8)
> -		edev->addrlen = 7;
> +		edev->addrlen = ilog2(edev->size);
>  	else if (pd->flags & EE_ADDR16)
> -		edev->addrlen = 6;
> +		edev->addrlen = ilog2(edev->size) - 1;
>  	else {
>  		dev_err(&spi->dev, "unspecified address type\n");
>  		return -EINVAL;
> @@ -469,7 +477,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
>  	edev->spi = spi;
>  	edev->pdata = pd;
>  
> -	edev->size = 128;
>  	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
>  	edev->nvmem_config.name = dev_name(&spi->dev);
>  	edev->nvmem_config.dev = &spi->dev;
> -- 
> 2.31.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] misc: eeprom_93xx46: Add new 93c56 and 93c66 compatible strings

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 803 bytes --]

On Sat, Apr 24, 2021 at 11:25:42PM +0200, Emmanuel Gil Peyrot wrote:
> These two devices have respectively 2048 and 4096 bits of storage,
> compared to 1024 for the 93c46.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---

Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>


>  drivers/misc/eeprom/eeprom_93xx46.c | 35 ++++++++++++++++++++++++++---
>  include/linux/eeprom_93xx46.h       |  3 +++
>  2 files changed, 35 insertions(+), 3 deletions(-)

One thing, so it doesn't go unmentioned: There is another driver for
these EEPROMs, drivers/misc/eeprom/eeprom_93cx6.c, but it isn't
stand-alone, it doesn't even have a probe function AFAICT.
In that sense, I agree with the decision to extend this driver instead
of the other one.


Thanks,
Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] dts: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

> [PATCH v2 3/3] dts: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

The usual prefix for devicetree binding patches is "dt-bindings", not
"dts".

Other than that, I think the patch looks good.

Thanks,
Jonathan

On Sat, Apr 24, 2021 at 11:25:43PM +0200, Emmanuel Gil Peyrot wrote:
> These devices differ by the size of their storage, which is why they
> have different compatible strings.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> index 7b636b7a8311..72ea0af368d4 100644
> --- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> +++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
> @@ -2,7 +2,10 @@ EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
>  
>  Required properties:
>  - compatible : shall be one of:
> +    "atmel,at93c46"
>      "atmel,at93c46d"
> +    "atmel,at93c56"
> +    "atmel,at93c66"
>      "eeprom-93xx46"
>      "microchip,93lc46b"
>  - data-size : number of data bits per word (either 8 or 16)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] dts: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

[Rob Herring]

On Sat, 24 Apr 2021 23:25:43 +0200, Emmanuel Gil Peyrot wrote:
> These devices differ by the size of their storage, which is why they
> have different compatible strings.
> 
> Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
> ---
>  Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 3 +++
>  1 file changed, 3 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v3 0/3] eeprom-93xx46: Add support for Atmel AT93C56 and AT93C66

[Emmanuel Gil Peyrot]

These two devices differ from the AT93C46 by their storage size,
respectively 2 Kib and 4 Kib, while the AT93C46 contains 1 Kib.

The driver was currently hardcoding that addrlen == 7 means 8-bit words,
and anything else means 16-bit words.  This is obviously not going to
work with more storage and thus more bits spent on the address (for
instance on the AT93C66 in 8-bit words mode, addrlen == 9 since there
are 512 bytes to address), so I’m now doing those checks based on the
word size specified in the device tree.

It might make sense to rename this driver now that it supports all three
sizes, but I don’t know what would be a good name, eeprom_93xxx6 doesn’t
sound very nice.

I have tested this series on a Nintendo Wii U, on the downstream
linux-wiiu kernel based on 4.19, and thus only with a AT93C66.  You can
find this work here if you want to test it:
https://gitlab.com/linux-wiiu/linux-wiiu/-/merge_requests/16

Changes since v1:
- Reordered and squashed patches.
- Split out the dts addition.
- Removed a bogus goto.
- Improved commit messages to make what they do more explicit.

Changes since v2:
- Renamed the dts patch.
- Included the R-b and A-b these got.

Emmanuel Gil Peyrot (3):
  misc: eeprom_93xx46: Remove hardcoded bit lengths
  misc: eeprom_93xx46: Add new 93c56 and 93c66 compatible strings
  dt-bindings: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

 .../bindings/misc/eeprom-93xx46.txt           |  3 +
 drivers/misc/eeprom/eeprom_93xx46.c           | 90 +++++++++++++------
 include/linux/eeprom_93xx46.h                 |  3 +
 3 files changed, 69 insertions(+), 27 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] misc: eeprom_93xx46: Remove hardcoded bit lengths

[Emmanuel Gil Peyrot]

This avoids using magic numbers based on the length of an address or a
command, while we only want to differentiate between 8-bit and 16-bit.

The driver was previously wrapping around the offset in the write
operation, this now returns -EINVAL instead (but should never happen in
the first place).

If two pointer indirections are too many, we could move the flags to the
main struct instead, but I doubt it’s going to make any sensible
difference on any hardware.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 57 ++++++++++++++++-------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index 80114f4c80ad..ffdb8e5a26e0 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -9,6 +9,7 @@
 #include <linux/device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
+#include <linux/log2.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/of.h>
@@ -70,6 +71,7 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	struct eeprom_93xx46_dev *edev = priv;
 	char *buf = val;
 	int err = 0;
+	int bits;
 
 	if (unlikely(off >= edev->size))
 		return 0;
@@ -83,21 +85,21 @@ static int eeprom_93xx46_read(void *priv, unsigned int off,
 	if (edev->pdata->prepare)
 		edev->pdata->prepare(edev);
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	while (count) {
 		struct spi_message m;
 		struct spi_transfer t[2] = { { 0 } };
 		u16 cmd_addr = OP_READ << edev->addrlen;
 		size_t nbytes = count;
-		int bits;
 
-		if (edev->addrlen == 7) {
-			cmd_addr |= off & 0x7f;
-			bits = 10;
+		if (edev->pdata->flags & EE_ADDR8) {
+			cmd_addr |= off;
 			if (has_quirk_single_word_read(edev))
 				nbytes = 1;
 		} else {
-			cmd_addr |= (off >> 1) & 0x3f;
-			bits = 9;
+			cmd_addr |= (off >> 1);
 			if (has_quirk_single_word_read(edev))
 				nbytes = 2;
 		}
@@ -152,14 +154,14 @@ static int eeprom_93xx46_ew(struct eeprom_93xx46_dev *edev, int is_on)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8)
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS) << 1;
-		bits = 10;
-	} else {
+	else
 		cmd_addr |= (is_on ? ADDR_EWEN : ADDR_EWDS);
-		bits = 9;
-	}
 
 	if (has_quirk_instruction_length(edev)) {
 		cmd_addr <<= 2;
@@ -205,15 +207,19 @@ eeprom_93xx46_write_word(struct eeprom_93xx46_dev *edev,
 	int bits, data_len, ret;
 	u16 cmd_addr;
 
+	if (unlikely(off >= edev->size))
+		return -EINVAL;
+
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_WRITE << edev->addrlen;
 
-	if (edev->addrlen == 7) {
-		cmd_addr |= off & 0x7f;
-		bits = 10;
+	if (edev->pdata->flags & EE_ADDR8) {
+		cmd_addr |= off;
 		data_len = 1;
 	} else {
-		cmd_addr |= (off >> 1) & 0x3f;
-		bits = 9;
+		cmd_addr |= (off >> 1);
 		data_len = 2;
 	}
 
@@ -253,7 +259,7 @@ static int eeprom_93xx46_write(void *priv, unsigned int off,
 		return count;
 
 	/* only write even number of bytes on 16-bit devices */
-	if (edev->addrlen == 6) {
+	if (edev->pdata->flags & EE_ADDR16) {
 		step = 2;
 		count &= ~1;
 	}
@@ -295,14 +301,14 @@ static int eeprom_93xx46_eral(struct eeprom_93xx46_dev *edev)
 	int bits, ret;
 	u16 cmd_addr;
 
+	/* The opcode in front of the address is three bits. */
+	bits = edev->addrlen + 3;
+
 	cmd_addr = OP_START << edev->addrlen;
-	if (edev->addrlen == 7) {
+	if (edev->pdata->flags & EE_ADDR8)
 		cmd_addr |= ADDR_ERAL << 1;
-		bits = 10;
-	} else {
+	else
 		cmd_addr |= ADDR_ERAL;
-		bits = 9;
-	}
 
 	if (has_quirk_instruction_length(edev)) {
 		cmd_addr <<= 2;
@@ -455,10 +461,12 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (!edev)
 		return -ENOMEM;
 
+	edev->size = 128;
+
 	if (pd->flags & EE_ADDR8)
-		edev->addrlen = 7;
+		edev->addrlen = ilog2(edev->size);
 	else if (pd->flags & EE_ADDR16)
-		edev->addrlen = 6;
+		edev->addrlen = ilog2(edev->size) - 1;
 	else {
 		dev_err(&spi->dev, "unspecified address type\n");
 		return -EINVAL;
@@ -469,7 +477,6 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	edev->spi = spi;
 	edev->pdata = pd;
 
-	edev->size = 128;
 	edev->nvmem_config.type = NVMEM_TYPE_EEPROM;
 	edev->nvmem_config.name = dev_name(&spi->dev);
 	edev->nvmem_config.dev = &spi->dev;
-- 
2.31.1

[PATCH v3 2/3] misc: eeprom_93xx46: Add new 93c56 and 93c66 compatible strings

[Emmanuel Gil Peyrot]

These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/misc/eeprom/eeprom_93xx46.c | 35 ++++++++++++++++++++++++++---
 include/linux/eeprom_93xx46.h       |  3 +++
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/eeprom_93xx46.c b/drivers/misc/eeprom/eeprom_93xx46.c
index ffdb8e5a26e0..29d8971ec558 100644
--- a/drivers/misc/eeprom/eeprom_93xx46.c
+++ b/drivers/misc/eeprom/eeprom_93xx46.c
@@ -29,14 +29,29 @@
 
 struct eeprom_93xx46_devtype_data {
 	unsigned int quirks;
+	unsigned char flags;
+};
+
+static const struct eeprom_93xx46_devtype_data at93c46_data = {
+	.flags = EE_SIZE1K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c56_data = {
+	.flags = EE_SIZE2K,
+};
+
+static const struct eeprom_93xx46_devtype_data at93c66_data = {
+	.flags = EE_SIZE4K,
 };
 
 static const struct eeprom_93xx46_devtype_data atmel_at93c46d_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_SINGLE_WORD_READ |
 		  EEPROM_93XX46_QUIRK_INSTRUCTION_LENGTH,
 };
 
 static const struct eeprom_93xx46_devtype_data microchip_93lc46b_data = {
+	.flags = EE_SIZE1K,
 	.quirks = EEPROM_93XX46_QUIRK_EXTRA_READ_CYCLE,
 };
 
@@ -381,8 +396,11 @@ static void select_deassert(void *context)
 }
 
 static const struct of_device_id eeprom_93xx46_of_table[] = {
-	{ .compatible = "eeprom-93xx46", },
+	{ .compatible = "eeprom-93xx46", .data = &at93c46_data, },
+	{ .compatible = "atmel,at93c46", .data = &at93c46_data, },
 	{ .compatible = "atmel,at93c46d", .data = &atmel_at93c46d_data, },
+	{ .compatible = "atmel,at93c56", .data = &at93c56_data, },
+	{ .compatible = "atmel,at93c66", .data = &at93c66_data, },
 	{ .compatible = "microchip,93lc46b", .data = &microchip_93lc46b_data, },
 	{}
 };
@@ -432,6 +450,7 @@ static int eeprom_93xx46_probe_dt(struct spi_device *spi)
 		const struct eeprom_93xx46_devtype_data *data = of_id->data;
 
 		pd->quirks = data->quirks;
+		pd->flags |= data->flags;
 	}
 
 	spi->dev.platform_data = pd;
@@ -461,7 +480,16 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (!edev)
 		return -ENOMEM;
 
-	edev->size = 128;
+	if (pd->flags & EE_SIZE1K)
+		edev->size = 128;
+	else if (pd->flags & EE_SIZE2K)
+		edev->size = 256;
+	else if (pd->flags & EE_SIZE4K)
+		edev->size = 512;
+	else {
+		dev_err(&spi->dev, "unspecified size\n");
+		return -EINVAL;
+	}
 
 	if (pd->flags & EE_ADDR8)
 		edev->addrlen = ilog2(edev->size);
@@ -496,8 +524,9 @@ static int eeprom_93xx46_probe(struct spi_device *spi)
 	if (IS_ERR(edev->nvmem))
 		return PTR_ERR(edev->nvmem);
 
-	dev_info(&spi->dev, "%d-bit eeprom %s\n",
+	dev_info(&spi->dev, "%d-bit eeprom containing %d bytes %s\n",
 		(pd->flags & EE_ADDR8) ? 8 : 16,
+		edev->size,
 		(pd->flags & EE_READONLY) ? "(readonly)" : "");
 
 	if (!(pd->flags & EE_READONLY)) {
diff --git a/include/linux/eeprom_93xx46.h b/include/linux/eeprom_93xx46.h
index 99580c22f91a..34c2175e6a1e 100644
--- a/include/linux/eeprom_93xx46.h
+++ b/include/linux/eeprom_93xx46.h
@@ -10,6 +10,9 @@ struct eeprom_93xx46_platform_data {
 #define EE_ADDR8	0x01		/*  8 bit addr. cfg */
 #define EE_ADDR16	0x02		/* 16 bit addr. cfg */
 #define EE_READONLY	0x08		/* forbid writing */
+#define EE_SIZE1K	0x10		/* 1 kb of data, that is a 93xx46 */
+#define EE_SIZE2K	0x20		/* 2 kb of data, that is a 93xx56 */
+#define EE_SIZE4K	0x40		/* 4 kb of data, that is a 93xx66 */
 
 	unsigned int	quirks;
 /* Single word read transfers only; no sequential read. */
-- 
2.31.1

[PATCH v3 3/3] dt-bindings: eeprom-93xx46: Add support for 93C46, 93C56 and 93C66

[Emmanuel Gil Peyrot]

These devices differ by the size of their storage, which is why they
have different compatible strings.

Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
index 7b636b7a8311..72ea0af368d4 100644
--- a/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
+++ b/Documentation/devicetree/bindings/misc/eeprom-93xx46.txt
@@ -2,7 +2,10 @@ EEPROMs (SPI) compatible with Microchip Technology 93xx46 family.
 
 Required properties:
 - compatible : shall be one of:
+    "atmel,at93c46"
     "atmel,at93c46d"
+    "atmel,at93c56"
+    "atmel,at93c66"
     "eeprom-93xx46"
     "microchip,93lc46b"
 - data-size : number of data bits per word (either 8 or 16)
-- 
2.31.1