[PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Geert Uytterhoeven]

	Hi all,

This patch series contains two improvements for the AT25 SPI EEPROM
driver, related to SPI transfers.

Changes compared to v1:
  - Merge "off" and "offset" into a single variable instead of just
    killing the cast, as suggested by Arnd,
  - Add Acked-by,
  - Dropped "[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers
    to optimize DMA", as this is better implemented in the SPI
    controller driver (cfr. e.g. "[v2 PATCH 2/2] spi: sh-msiof: Use DMA
    if possible",
    https://lore.kernel.org/linux-renesas-soc/1547803771-9564-2-git-send-email-na-hoan@jinso.co.jp/)

Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
and a 25LC040 EEPROM.

Thanks!

Geert Uytterhoeven (2):
  eeprom: at25: Merge "off" and "offset" in at25_ee_write()
  eeprom: at25: Use spi_message_init_with_transfers() instead of open
    coding

 drivers/misc/eeprom/at25.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/2] eeprom: at25: Merge "off" and "offset" in at25_ee_write()

[Geert Uytterhoeven]

Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
dependency") changed the type of "off" from "loff_t" to "unsigned int",
"off" and "offset" are now the same type, and can be merged into a
single variable.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Merge "off" and "offset" into a single variable instead of just
    killing the cast, as suggested by Arnd.
---
 drivers/misc/eeprom/at25.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 99de6939cd5ac2e3..2d8e5388e574710d 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -130,7 +130,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
 	return status;
 }
 
-static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
+static int at25_ee_write(void *priv, unsigned int offset, void *val,
+			 size_t count)
 {
 	struct at25_data *at25 = priv;
 	const char *buf = val;
@@ -138,10 +139,10 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 	unsigned		buf_size;
 	u8			*bounce;
 
-	if (unlikely(off >= at25->chip.byte_len))
+	if (unlikely(offset >= at25->chip.byte_len))
 		return -EFBIG;
-	if ((off + count) > at25->chip.byte_len)
-		count = at25->chip.byte_len - off;
+	if ((offset + count) > at25->chip.byte_len)
+		count = at25->chip.byte_len - offset;
 	if (unlikely(!count))
 		return -EINVAL;
 
@@ -160,7 +161,6 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 	do {
 		unsigned long	timeout, retries;
 		unsigned	segment;
-		unsigned	offset = (unsigned) off;
 		u8		*cp = bounce;
 		int		sr;
 		u8		instr;
@@ -234,7 +234,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 			break;
 		}
 
-		off += segment;
+		offset += segment;
 		buf += segment;
 		count -= segment;
 
-- 
2.17.1

[PATCH v2 2/2] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding

[Geert Uytterhoeven]

Reduce code duplication in at25_ee_read() by using the
spi_message_init_with_transfers() helper.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
---
v2:
  - Add Acked-by.
---
 drivers/misc/eeprom/at25.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 2d8e5388e574710d..18a093e14517ab45 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset,
 		*cp++ = offset >> 0;
 	}
 
-	spi_message_init(&m);
 	memset(t, 0, sizeof(t));
 
 	t[0].tx_buf = command;
 	t[0].len = at25->addrlen + 1;
-	spi_message_add_tail(&t[0], &m);
 
 	t[1].rx_buf = buf;
 	t[1].len = count;
-	spi_message_add_tail(&t[1], &m);
+
+	spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
 
 	mutex_lock(&at25->lock);
 
-- 
2.17.1

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Boris Brezillon]

Hi Geert,

On Fri, 18 Jan 2019 15:05:23 +0100
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> 	Hi all,
> 
> This patch series contains two improvements for the AT25 SPI EEPROM
> driver, related to SPI transfers.
> 
> Changes compared to v1:
>   - Merge "off" and "offset" into a single variable instead of just
>     killing the cast, as suggested by Arnd,
>   - Add Acked-by,
>   - Dropped "[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers
>     to optimize DMA", as this is better implemented in the SPI
>     controller driver (cfr. e.g. "[v2 PATCH 2/2] spi: sh-msiof: Use DMA
>     if possible",
>     https://lore.kernel.org/linux-renesas-soc/1547803771-9564-2-git-send-email-na-hoan@jinso.co.jp/)
> 
> Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
> and a 25LC040 EEPROM.

Did you consider converting this driver to spimem? Looks like the
protocol used to communicate with the memory resembles the one used on
SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.

By doing this conversion you'd allow people to connect an AT25 EEPROM
to an advanced SPI controller that does not support regular SPI
transfers and you wouldn't have to forge SPI messages manually.

Here is a patch (only compile tested) doing that. The diffstat is not in
favor of this conversion, but I find the resulting code cleaner and more
future proof.

Regards,

Boris

drivers/misc/eeprom/at25.c | 302 ++++++++++++++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 198 insertions(+), 104 deletions(-)

--->8---
diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 99de6939cd5a..b9697a27e69b 100644
--- a/drivers/misc/eeprom/at25.c
+++ b/drivers/misc/eeprom/at25.c
@@ -17,7 +17,7 @@
 #include <linux/sched.h>
 
 #include <linux/nvmem-provider.h>
-#include <linux/spi/spi.h>
+#include <linux/spi/spi-mem.h>
 #include <linux/spi/eeprom.h>
 #include <linux/property.h>
 
@@ -29,12 +29,17 @@
  */
 
 struct at25_data {
-	struct spi_device	*spi;
+	struct spi_mem		*spimem;
 	struct mutex		lock;
 	struct spi_eeprom	chip;
 	unsigned		addrlen;
 	struct nvmem_config	nvmem_config;
 	struct nvmem_device	*nvmem;
+	void			*scratchbuf;
+	struct {
+		struct spi_mem_dirmap_desc *rdesc[2];
+		struct spi_mem_dirmap_desc *wdesc[2];
+	} dirmap;
 };
 
 #define	AT25_WREN	0x06		/* latch the write enable */
@@ -63,17 +68,117 @@ struct at25_data {
 
 #define	io_limit	PAGE_SIZE	/* bytes */
 
+static void at25_destroy_dirmaps(struct at25_data *at25)
+{
+	if (at25->dirmap.rdesc[0])
+		spi_mem_dirmap_destroy(at25->dirmap.rdesc[0]);
+	if (at25->dirmap.rdesc[1])
+		spi_mem_dirmap_destroy(at25->dirmap.rdesc[1]);
+	if (at25->dirmap.wdesc[0])
+		spi_mem_dirmap_destroy(at25->dirmap.wdesc[0]);
+	if (at25->dirmap.wdesc[1])
+		spi_mem_dirmap_destroy(at25->dirmap.wdesc[1]);
+}
+
+static int at25_create_dirmaps(struct at25_data *at25)
+{
+	struct spi_mem_dirmap_info info = {
+		.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_READ, 1),
+				      SPI_MEM_OP_ADDR(at25->addrlen, 0, 1),
+				      SPI_MEM_OP_NO_DUMMY,
+				      SPI_MEM_OP_DATA_IN(0, NULL, 1)),
+		.offset = 0,
+		.length = at25->chip.byte_len,
+	};
+	struct spi_mem_dirmap_desc *desc;
+	int ret;
+
+	if (info.length > 1 << at25->addrlen)
+		info.length = at25->chip.byte_len;
+
+	desc = spi_mem_dirmap_create(at25->spimem, &info);
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+
+	at25->dirmap.rdesc[0] = desc;
+
+	info.op_tmpl.cmd.opcode = AT25_WRITE;
+	info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
+
+	desc = spi_mem_dirmap_create(at25->spimem, &info);
+	if (IS_ERR(desc)) {
+		ret = PTR_ERR(desc);
+		goto err_destroy_dirmaps;
+	}
+
+	at25->dirmap.wdesc[0] = desc;
+
+	if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR))
+		return 0;
+
+	info.offset = 256;
+	info.length = at25->chip.byte_len - 256;
+	info.op_tmpl.cmd.opcode = AT25_READ | AT25_INSTR_BIT3;
+	info.op_tmpl.data.dir = SPI_MEM_DATA_IN;
+
+	desc = spi_mem_dirmap_create(at25->spimem, &info);
+	if (IS_ERR(desc)) {
+		ret = PTR_ERR(desc);
+		goto err_destroy_dirmaps;
+	}
+
+	at25->dirmap.rdesc[1] = desc;
+
+	info.op_tmpl.cmd.opcode = AT25_WRITE | AT25_INSTR_BIT3;
+	info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
+
+	desc = spi_mem_dirmap_create(at25->spimem, &info);
+	if (IS_ERR(desc)) {
+		ret = PTR_ERR(desc);
+		goto err_destroy_dirmaps;
+	}
+
+	at25->dirmap.wdesc[1] = desc;
+
+	return 0;
+
+err_destroy_dirmaps:
+	at25_destroy_dirmaps(at25);
+
+	return ret;
+}
+
+static int at25_rdsr(struct at25_data *at25)
+{
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_RDSR, 1),
+					  SPI_MEM_OP_NO_ADDR,
+					  SPI_MEM_OP_NO_DUMMY,
+					  SPI_MEM_OP_DATA_IN(1, at25->scratchbuf, 1));
+	int ret;
+
+	ret = spi_mem_exec_op(at25->spimem, &op);
+	if (ret)
+		return ret;
+
+	return *((u8 *)at25->scratchbuf);
+}
+
+static int at25_wren(struct at25_data *at25)
+{
+	struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_WREN, 1),
+					  SPI_MEM_OP_NO_ADDR,
+					  SPI_MEM_OP_NO_DUMMY,
+					  SPI_MEM_OP_NO_DATA);
+
+	return spi_mem_exec_op(at25->spimem, &op);
+}
+
 static int at25_ee_read(void *priv, unsigned int offset,
 			void *val, size_t count)
 {
+	struct spi_mem_dirmap_desc *desc;
 	struct at25_data *at25 = priv;
-	char *buf = val;
-	u8			command[EE_MAXADDRLEN + 1];
-	u8			*cp;
 	ssize_t			status;
-	struct spi_transfer	t[2];
-	struct spi_message	m;
-	u8			instr;
 
 	if (unlikely(offset >= at25->chip.byte_len))
 		return -EINVAL;
@@ -82,37 +187,10 @@ static int at25_ee_read(void *priv, unsigned int offset,
 	if (unlikely(!count))
 		return -EINVAL;
 
-	cp = command;
-
-	instr = AT25_READ;
-	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
-		if (offset >= (1U << (at25->addrlen * 8)))
-			instr |= AT25_INSTR_BIT3;
-	*cp++ = instr;
-
-	/* 8/16/24-bit address is written MSB first */
-	switch (at25->addrlen) {
-	default:	/* case 3 */
-		*cp++ = offset >> 16;
-		/* fall through */
-	case 2:
-		*cp++ = offset >> 8;
-		/* fall through */
-	case 1:
-	case 0:	/* can't happen: for better codegen */
-		*cp++ = offset >> 0;
-	}
-
-	spi_message_init(&m);
-	memset(t, 0, sizeof(t));
-
-	t[0].tx_buf = command;
-	t[0].len = at25->addrlen + 1;
-	spi_message_add_tail(&t[0], &m);
-
-	t[1].rx_buf = buf;
-	t[1].len = count;
-	spi_message_add_tail(&t[1], &m);
+	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255)
+		desc = at25->dirmap.rdesc[1];
+	else
+		desc = at25->dirmap.rdesc[0];
 
 	mutex_lock(&at25->lock);
 
@@ -122,8 +200,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
 	 * other devices on the bus need to be accessed regularly or
 	 * this chip is clocked very slowly
 	 */
-	status = spi_sync(at25->spi, &m);
-	dev_dbg(&at25->spi->dev, "read %zu bytes at %d --> %zd\n",
+	status = spi_mem_dirmap_read(desc, offset, count, val);
+	dev_dbg(&at25->spimem->spi->dev, "read %zu bytes at %d --> %zd\n",
 		count, offset, status);
 
 	mutex_unlock(&at25->lock);
@@ -149,7 +227,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 	buf_size = at25->chip.page_size;
 	if (buf_size > io_limit)
 		buf_size = io_limit;
-	bounce = kmalloc(buf_size + at25->addrlen + 1, GFP_KERNEL);
+	bounce = kmalloc(buf_size, GFP_KERNEL);
 	if (!bounce)
 		return -ENOMEM;
 
@@ -158,47 +236,32 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 	 */
 	mutex_lock(&at25->lock);
 	do {
+		struct spi_mem_dirmap_desc *desc;
 		unsigned long	timeout, retries;
 		unsigned	segment;
 		unsigned	offset = (unsigned) off;
-		u8		*cp = bounce;
 		int		sr;
-		u8		instr;
 
-		*cp = AT25_WREN;
-		status = spi_write(at25->spi, cp, 1);
+		status = at25_wren(at25);
 		if (status < 0) {
-			dev_dbg(&at25->spi->dev, "WREN --> %d\n", status);
+			dev_dbg(&at25->spimem->spi->dev, "WREN --> %d\n",
+				status);
 			break;
 		}
 
-		instr = AT25_WRITE;
-		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
-			if (offset >= (1U << (at25->addrlen * 8)))
-				instr |= AT25_INSTR_BIT3;
-		*cp++ = instr;
-
-		/* 8/16/24-bit address is written MSB first */
-		switch (at25->addrlen) {
-		default:	/* case 3 */
-			*cp++ = offset >> 16;
-			/* fall through */
-		case 2:
-			*cp++ = offset >> 8;
-			/* fall through */
-		case 1:
-		case 0:	/* can't happen: for better codegen */
-			*cp++ = offset >> 0;
-		}
+		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255)
+			desc = at25->dirmap.wdesc[1];
+		else
+			desc = at25->dirmap.wdesc[0];
 
 		/* Write as much of a page as we can */
 		segment = buf_size - (offset % buf_size);
 		if (segment > count)
 			segment = count;
-		memcpy(cp, buf, segment);
-		status = spi_write(at25->spi, bounce,
-				segment + at25->addrlen + 1);
-		dev_dbg(&at25->spi->dev, "write %u bytes at %u --> %d\n",
+		memcpy(bounce, buf, segment);
+		status = spi_mem_dirmap_write(desc, offset, segment, bounce);
+		dev_dbg(&at25->spimem->spi->dev,
+			"write %u bytes at %u --> %d\n",
 			segment, offset, status);
 		if (status < 0)
 			break;
@@ -211,10 +274,9 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 		timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
 		retries = 0;
 		do {
-
-			sr = spi_w8r8(at25->spi, AT25_RDSR);
+			sr = at25_rdsr(at25);
 			if (sr < 0 || (sr & AT25_SR_nRDY)) {
-				dev_dbg(&at25->spi->dev,
+				dev_dbg(&at25->spimem->spi->dev,
 					"rdsr --> %d (%02x)\n", sr, sr);
 				/* at HZ=100, this is sloooow */
 				msleep(1);
@@ -225,7 +287,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
 		} while (retries++ < 3 || time_before_eq(jiffies, timeout));
 
 		if ((sr < 0) || (sr & AT25_SR_nRDY)) {
-			dev_err(&at25->spi->dev,
+			dev_err(&at25->spimem->spi->dev,
 				"write %u bytes offset %u, timeout after %u msecs\n",
 				segment, offset,
 				jiffies_to_msecs(jiffies -
@@ -304,7 +366,7 @@ static int at25_fw_to_chip(struct device *dev, struct spi_eeprom *chip)
 	return 0;
 }
 
-static int at25_probe(struct spi_device *spi)
+static int at25_probe(struct spi_mem *spimem)
 {
 	struct at25_data	*at25 = NULL;
 	struct spi_eeprom	chip;
@@ -313,12 +375,12 @@ static int at25_probe(struct spi_device *spi)
 	int			addrlen;
 
 	/* Chip description */
-	if (!spi->dev.platform_data) {
-		err = at25_fw_to_chip(&spi->dev, &chip);
+	if (!spimem->spi->dev.platform_data) {
+		err = at25_fw_to_chip(&spimem->spi->dev, &chip);
 		if (err)
 			return err;
 	} else
-		chip = *(struct spi_eeprom *)spi->dev.platform_data;
+		chip = *(struct spi_eeprom *)spimem->spi->dev.platform_data;
 
 	/* For now we only support 8/16/24 bit addressing */
 	if (chip.flags & EE_ADDR1)
@@ -328,37 +390,48 @@ static int at25_probe(struct spi_device *spi)
 	else if (chip.flags & EE_ADDR3)
 		addrlen = 3;
 	else {
-		dev_dbg(&spi->dev, "unsupported address type\n");
+		dev_dbg(&spimem->spi->dev, "unsupported address type\n");
 		return -EINVAL;
 	}
 
-	/* Ping the chip ... the status register is pretty portable,
-	 * unlike probing manufacturer IDs.  We do expect that system
-	 * firmware didn't write it in the past few milliseconds!
-	 */
-	sr = spi_w8r8(spi, AT25_RDSR);
-	if (sr < 0 || sr & AT25_SR_nRDY) {
-		dev_dbg(&spi->dev, "rdsr --> %d (%02x)\n", sr, sr);
-		return -ENXIO;
-	}
-
-	at25 = devm_kzalloc(&spi->dev, sizeof(struct at25_data), GFP_KERNEL);
+	at25 = devm_kzalloc(&spimem->spi->dev, sizeof(struct at25_data), GFP_KERNEL);
 	if (!at25)
 		return -ENOMEM;
 
 	mutex_init(&at25->lock);
 	at25->chip = chip;
-	at25->spi = spi;
-	spi_set_drvdata(spi, at25);
+	at25->spimem = spimem;
+	spi_mem_set_drvdata(spimem, at25);
 	at25->addrlen = addrlen;
 
-	at25->nvmem_config.name = dev_name(&spi->dev);
-	at25->nvmem_config.dev = &spi->dev;
+	/*
+	 * Can't be allocated with devm_kmalloc() because we need a DMA-safe
+	 * buffer.
+	 */
+	at25->scratchbuf = kmalloc(1, GFP_KERNEL);
+
+	/* Ping the chip ... the status register is pretty portable,
+	 * unlike probing manufacturer IDs.  We do expect that system
+	 * firmware didn't write it in the past few milliseconds!
+	 */
+	sr = at25_rdsr(at25);
+	if (sr < 0 || sr & AT25_SR_nRDY) {
+		dev_dbg(&spimem->spi->dev, "rdsr --> %d (%02x)\n", sr, sr);
+		err = -ENXIO;
+		goto err_free_scratchbuf;
+	}
+
+	err = at25_create_dirmaps(at25);
+	if (err)
+		goto err_free_scratchbuf;
+
+	at25->nvmem_config.name = dev_name(&spimem->spi->dev);
+	at25->nvmem_config.dev = &spimem->spi->dev;
 	at25->nvmem_config.read_only = chip.flags & EE_READONLY;
 	at25->nvmem_config.root_only = true;
 	at25->nvmem_config.owner = THIS_MODULE;
 	at25->nvmem_config.compat = true;
-	at25->nvmem_config.base_dev = &spi->dev;
+	at25->nvmem_config.base_dev = &spimem->spi->dev;
 	at25->nvmem_config.reg_read = at25_ee_read;
 	at25->nvmem_config.reg_write = at25_ee_write;
 	at25->nvmem_config.priv = at25;
@@ -366,17 +439,38 @@ static int at25_probe(struct spi_device *spi)
 	at25->nvmem_config.word_size = 1;
 	at25->nvmem_config.size = chip.byte_len;
 
-	at25->nvmem = devm_nvmem_register(&spi->dev, &at25->nvmem_config);
-	if (IS_ERR(at25->nvmem))
-		return PTR_ERR(at25->nvmem);
+	at25->nvmem = devm_nvmem_register(&spimem->spi->dev,
+					  &at25->nvmem_config);
+	if (IS_ERR(at25->nvmem)) {
+		err = PTR_ERR(at25->nvmem);
+		goto err_destroy_dirmaps;
+	}
 
-	dev_info(&spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
+	dev_info(&spimem->spi->dev, "%d %s %s eeprom%s, pagesize %u\n",
 		(chip.byte_len < 1024) ? chip.byte_len : (chip.byte_len / 1024),
 		(chip.byte_len < 1024) ? "Byte" : "KByte",
 		at25->chip.name,
 		(chip.flags & EE_READONLY) ? " (readonly)" : "",
 		at25->chip.page_size);
 	return 0;
+
+err_destroy_dirmaps:
+	at25_destroy_dirmaps(at25);
+
+err_free_scratchbuf:
+	kfree(at25->scratchbuf);
+
+	return err;
+}
+
+static int at25_remove(struct spi_mem *spimem)
+{
+	struct at25_data *at25 = spi_mem_get_drvdata(spimem);
+
+	at25_destroy_dirmaps(at25);
+	kfree(at25->scratchbuf);
+
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -387,15 +481,15 @@ static const struct of_device_id at25_of_match[] = {
 };
 MODULE_DEVICE_TABLE(of, at25_of_match);
 
-static struct spi_driver at25_driver = {
-	.driver = {
-		.name		= "at25",
+static struct spi_mem_driver at25_driver = {
+	.spidrv.driver = {
+		.name = "at25",
 		.of_match_table = at25_of_match,
 	},
-	.probe		= at25_probe,
+	.probe = at25_probe,
+	.remove = at25_remove,
 };
-
-module_spi_driver(at25_driver);
+module_spi_mem_driver(at25_driver);
 
 MODULE_DESCRIPTION("Driver for most SPI EEPROMs");
 MODULE_AUTHOR("David Brownell");

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Geert Uytterhoeven]

Hi Boris,

On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> On Fri, 18 Jan 2019 15:05:23 +0100
> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> > This patch series contains two improvements for the AT25 SPI EEPROM
> > driver, related to SPI transfers.
> >
> > Changes compared to v1:
> >   - Merge "off" and "offset" into a single variable instead of just
> >     killing the cast, as suggested by Arnd,
> >   - Add Acked-by,
> >   - Dropped "[PATCH 3/3] eeprom: at25: Split writes in two SPI transfers
> >     to optimize DMA", as this is better implemented in the SPI
> >     controller driver (cfr. e.g. "[v2 PATCH 2/2] spi: sh-msiof: Use DMA
> >     if possible",
> >     https://lore.kernel.org/linux-renesas-soc/1547803771-9564-2-git-send-email-na-hoan@jinso.co.jp/)
> >
> > Tested on a Renesas Ebisu development board with R-Car E3 using MSIOF
> > and a 25LC040 EEPROM.
>
> Did you consider converting this driver to spimem? Looks like the
> protocol used to communicate with the memory resembles the one used on
> SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.

No, I hadn't considered doing that ;-)

> By doing this conversion you'd allow people to connect an AT25 EEPROM
> to an advanced SPI controller that does not support regular SPI
> transfers and you wouldn't have to forge SPI messages manually.
>
> Here is a patch (only compile tested) doing that. The diffstat is not in
> favor of this conversion, but I find the resulting code cleaner and more
> future proof.

Thanks, will give it a try, eventually...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 1/2] eeprom: at25: Merge "off" and "offset" in at25_ee_write()

[Boris Brezillon]

On Fri, 18 Jan 2019 15:05:24 +0100
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> Since commit 01973a01f9ec34b7 ("eeprom: at25: remove nvmem regmap
> dependency") changed the type of "off" from "loff_t" to "unsigned int",
> "off" and "offset" are now the same type, and can be merged into a
> single variable.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
> v2:
>   - Merge "off" and "offset" into a single variable instead of just
>     killing the cast, as suggested by Arnd.
> ---
>  drivers/misc/eeprom/at25.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 99de6939cd5ac2e3..2d8e5388e574710d 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -130,7 +130,8 @@ static int at25_ee_read(void *priv, unsigned int offset,
>  	return status;
>  }
>  
> -static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
> +static int at25_ee_write(void *priv, unsigned int offset, void *val,
> +			 size_t count)
>  {
>  	struct at25_data *at25 = priv;
>  	const char *buf = val;
> @@ -138,10 +139,10 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>  	unsigned		buf_size;
>  	u8			*bounce;
>  
> -	if (unlikely(off >= at25->chip.byte_len))
> +	if (unlikely(offset >= at25->chip.byte_len))
>  		return -EFBIG;
> -	if ((off + count) > at25->chip.byte_len)
> -		count = at25->chip.byte_len - off;
> +	if ((offset + count) > at25->chip.byte_len)
> +		count = at25->chip.byte_len - offset;
>  	if (unlikely(!count))
>  		return -EINVAL;
>  
> @@ -160,7 +161,6 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>  	do {
>  		unsigned long	timeout, retries;
>  		unsigned	segment;
> -		unsigned	offset = (unsigned) off;
>  		u8		*cp = bounce;
>  		int		sr;
>  		u8		instr;
> @@ -234,7 +234,7 @@ static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
>  			break;
>  		}
>  
> -		off += segment;
> +		offset += segment;
>  		buf += segment;
>  		count -= segment;
>  

Re: [PATCH v2 2/2] eeprom: at25: Use spi_message_init_with_transfers() instead of open coding

[Boris Brezillon]

On Fri, 18 Jan 2019 15:05:25 +0100
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> Reduce code duplication in at25_ee_read() by using the
> spi_message_init_with_transfers() helper.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Acked-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Boris Brezillon <bbrezillon@kernel.org>

> ---
> v2:
>   - Add Acked-by.
> ---
>  drivers/misc/eeprom/at25.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
> index 2d8e5388e574710d..18a093e14517ab45 100644
> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c
> @@ -103,16 +103,15 @@ static int at25_ee_read(void *priv, unsigned int offset,
>  		*cp++ = offset >> 0;
>  	}
>  
> -	spi_message_init(&m);
>  	memset(t, 0, sizeof(t));
>  
>  	t[0].tx_buf = command;
>  	t[0].len = at25->addrlen + 1;
> -	spi_message_add_tail(&t[0], &m);
>  
>  	t[1].rx_buf = buf;
>  	t[1].len = count;
> -	spi_message_add_tail(&t[1], &m);
> +
> +	spi_message_init_with_transfers(&m, t, ARRAY_SIZE(t));
>  
>  	mutex_lock(&at25->lock);
>  

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Geert Uytterhoeven]

Hi Boris,

On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> Did you consider converting this driver to spimem? Looks like the
> protocol used to communicate with the memory resembles the one used on
> SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.
>
> By doing this conversion you'd allow people to connect an AT25 EEPROM
> to an advanced SPI controller that does not support regular SPI
> transfers and you wouldn't have to forge SPI messages manually.
>
> Here is a patch (only compile tested) doing that. The diffstat is not in
> favor of this conversion, but I find the resulting code cleaner and more
> future proof.

Thanks, seems to work fine, with the 512-byte 25LC040 I have!

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

I did notice that the first two-byte transfer (command+offset) of each
message is now split in two one-byte transfers, though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Greg Kroah-Hartman]

On Tue, Jan 29, 2019 at 08:02:37PM +0100, Geert Uytterhoeven wrote:
> Hi Boris,
> 
> On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> > Did you consider converting this driver to spimem? Looks like the
> > protocol used to communicate with the memory resembles the one used on
> > SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.
> >
> > By doing this conversion you'd allow people to connect an AT25 EEPROM
> > to an advanced SPI controller that does not support regular SPI
> > transfers and you wouldn't have to forge SPI messages manually.
> >
> > Here is a patch (only compile tested) doing that. The diffstat is not in
> > favor of this conversion, but I find the resulting code cleaner and more
> > future proof.
> 
> Thanks, seems to work fine, with the 512-byte 25LC040 I have!
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> I did notice that the first two-byte transfer (command+offset) of each
> message is now split in two one-byte transfers, though.

Ok, I'll drop this patch series and wait for the updated version to be
sent out :)

thanks,

greg k-h

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Boris Brezillon]

On Tue, 29 Jan 2019 20:02:37 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Hi Boris,
> 
> On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:
> > Did you consider converting this driver to spimem? Looks like the
> > protocol used to communicate with the memory resembles the one used on
> > SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.
> >
> > By doing this conversion you'd allow people to connect an AT25 EEPROM
> > to an advanced SPI controller that does not support regular SPI
> > transfers and you wouldn't have to forge SPI messages manually.
> >
> > Here is a patch (only compile tested) doing that. The diffstat is not in
> > favor of this conversion, but I find the resulting code cleaner and more
> > future proof.  
> 
> Thanks, seems to work fine, with the 512-byte 25LC040 I have!
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> I did notice that the first two-byte transfer (command+offset) of each
> message is now split in two one-byte transfers, though.

That's something we can optimize in drivers/spi/spi-mem.c if you think
it makes a difference.

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Boris Brezillon]

Hi Greg,

On Wed, 30 Jan 2019 15:50:54 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:

> On Tue, Jan 29, 2019 at 08:02:37PM +0100, Geert Uytterhoeven wrote:
> > Hi Boris,
> > 
> > On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:  
> > > Did you consider converting this driver to spimem? Looks like the
> > > protocol used to communicate with the memory resembles the one used on
> > > SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.
> > >
> > > By doing this conversion you'd allow people to connect an AT25 EEPROM
> > > to an advanced SPI controller that does not support regular SPI
> > > transfers and you wouldn't have to forge SPI messages manually.
> > >
> > > Here is a patch (only compile tested) doing that. The diffstat is not in
> > > favor of this conversion, but I find the resulting code cleaner and more
> > > future proof.  
> > 
> > Thanks, seems to work fine, with the 512-byte 25LC040 I have!
> > 
> > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > 
> > I did notice that the first two-byte transfer (command+offset) of each
> > message is now split in two one-byte transfers, though.  
> 
> Ok, I'll drop this patch series and wait for the updated version to be
> sent out :)

I'd like to further simplify the patch by using the recently introduced
devm_spi_mem_dirmap_create() function (queued to Mark's spi/for-5.1
branch), which means I'll have to wait v5.1-rc1 before sending a new
version. I'll let you decide if it's worth applying Geert's patches in
the meantime.

Regards,

Boris

Re: [PATCH v2 0/2] eeprom: at25: SPI transfer improvements

[Greg Kroah-Hartman]

On Wed, Jan 30, 2019 at 04:08:39PM +0100, Boris Brezillon wrote:
> Hi Greg,
> 
> On Wed, 30 Jan 2019 15:50:54 +0100
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> 
> > On Tue, Jan 29, 2019 at 08:02:37PM +0100, Geert Uytterhoeven wrote:
> > > Hi Boris,
> > > 
> > > On Fri, Jan 18, 2019 at 11:07 PM Boris Brezillon <bbrezillon@kernel.org> wrote:  
> > > > Did you consider converting this driver to spimem? Looks like the
> > > > protocol used to communicate with the memory resembles the one used on
> > > > SPI NANDs/NORs and fits pretty well in the spi_mem_op representation.
> > > >
> > > > By doing this conversion you'd allow people to connect an AT25 EEPROM
> > > > to an advanced SPI controller that does not support regular SPI
> > > > transfers and you wouldn't have to forge SPI messages manually.
> > > >
> > > > Here is a patch (only compile tested) doing that. The diffstat is not in
> > > > favor of this conversion, but I find the resulting code cleaner and more
> > > > future proof.  
> > > 
> > > Thanks, seems to work fine, with the 512-byte 25LC040 I have!
> > > 
> > > Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > 
> > > I did notice that the first two-byte transfer (command+offset) of each
> > > message is now split in two one-byte transfers, though.  
> > 
> > Ok, I'll drop this patch series and wait for the updated version to be
> > sent out :)
> 
> I'd like to further simplify the patch by using the recently introduced
> devm_spi_mem_dirmap_create() function (queued to Mark's spi/for-5.1
> branch), which means I'll have to wait v5.1-rc1 before sending a new
> version. I'll let you decide if it's worth applying Geert's patches in
> the meantime.

If you all want Geert's patches merged now, someone has to resend them
please :)

thanks,

greg k-h