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
In the read case, this also moves it out of the loop.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
---
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
These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
---
.../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 = µchip_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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
> ---
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
> [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 <[email protected]>
> ---
> 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
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 <[email protected]>
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/[email protected]
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
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 <[email protected]>
---
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
These devices differ by the size of their storage, which is why they
have different compatible strings.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
---
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
These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
---
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 = µchip_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
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 <[email protected]>
> ---
I'm not an EEPROM driver expert, but FWIW:
Reviewed-by: Jonathan Neuschäfer <[email protected]>
> 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
>
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 <[email protected]>
> ---
Reviewed-by: Jonathan Neuschäfer <[email protected]>
> 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
> [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 <[email protected]>
> ---
> 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)
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 <[email protected]>
> ---
> Documentation/devicetree/bindings/misc/eeprom-93xx46.txt | 3 +++
> 1 file changed, 3 insertions(+)
>
Acked-by: Rob Herring <[email protected]>
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
These two devices have respectively 2048 and 4096 bits of storage,
compared to 1024 for the 93c46.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
Reviewed-by: Jonathan Neuschäfer <[email protected]>
---
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 = µchip_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
These devices differ by the size of their storage, which is why they
have different compatible strings.
Signed-off-by: Emmanuel Gil Peyrot <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Jonathan Neuschäfer <[email protected]>
---
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