[PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[Boris Brezillon]

The AT25 protocol fits pretty well in the spi-mem model. Convert the
at25 spi driver to a spi-mem driver and use the dirmap API instead of
forging SPI messages manually.
This makes the driver compatible with spi-mem-only controllers
(controllers implementing only the spi_mem ops).

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/misc/eeprom/at25.c | 282 +++++++++++++++++++++++--------------
 1 file changed, 176 insertions(+), 106 deletions(-)

diff --git a/drivers/misc/eeprom/at25.c b/drivers/misc/eeprom/at25.c
index 99de6939cd5a..818853babbd0 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,89 @@ struct at25_data {
 
 #define	io_limit	PAGE_SIZE	/* bytes */
 
+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 device *dev = &at25->spimem->spi->dev;
+
+	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
+		info.length = 256;
+
+	at25->dirmap.rdesc[0] = devm_spi_mem_dirmap_create(dev, at25->spimem,
+							   &info);
+	if (IS_ERR(at25->dirmap.rdesc[0]))
+		return PTR_ERR(at25->dirmap.rdesc[0]);
+
+	info.op_tmpl.cmd.opcode = AT25_WRITE;
+	info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
+
+	at25->dirmap.wdesc[0] = devm_spi_mem_dirmap_create(dev, at25->spimem,
+							   &info);
+	if (IS_ERR(at25->dirmap.wdesc[0]))
+		return PTR_ERR(at25->dirmap.wdesc[0]);
+
+	if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR))
+		return 0;
+
+	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;
+
+	at25->dirmap.rdesc[1] = devm_spi_mem_dirmap_create(dev, at25->spimem,
+							   &info);
+	if (IS_ERR(at25->dirmap.rdesc[1]))
+		return PTR_ERR(at25->dirmap.rdesc[1]);
+
+	info.op_tmpl.cmd.opcode = AT25_WRITE | AT25_INSTR_BIT3;
+	info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
+
+	at25->dirmap.wdesc[1] = devm_spi_mem_dirmap_create(dev, at25->spimem,
+							   &info);
+	if (IS_ERR(at25->dirmap.wdesc[1]))
+		return PTR_ERR(at25->dirmap.wdesc[1]);
+
+	return 0;
+}
+
+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;
+	unsigned int dirmap_offset;
 	ssize_t			status;
-	struct spi_transfer	t[2];
-	struct spi_message	m;
-	u8			instr;
 
 	if (unlikely(offset >= at25->chip.byte_len))
 		return -EINVAL;
@@ -82,38 +159,14 @@ 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;
+	if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) {
+		desc = at25->dirmap.rdesc[1];
+		dirmap_offset = offset - 256;
+	} else {
+		desc = at25->dirmap.rdesc[0];
+		dirmap_offset = offset;
 	}
 
-	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);
-
 	mutex_lock(&at25->lock);
 
 	/* Read it all at once.
@@ -122,8 +175,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, dirmap_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 +202,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,48 +211,37 @@ 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;
+		unsigned int	dirmap_offset;
 		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 && off > 255) {
+			desc = at25->dirmap.wdesc[1];
+			dirmap_offset = off -  256;
+		} else {
+			desc = at25->dirmap.wdesc[0];
+			dirmap_offset = off;
 		}
 
 		/* Write as much of a page as we can */
-		segment = buf_size - (offset % buf_size);
+		segment = buf_size - (dirmap_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",
-			segment, offset, status);
+		memcpy(bounce, buf, segment);
+		status = spi_mem_dirmap_write(desc, dirmap_offset, segment,
+					      bounce);
+		dev_dbg(&at25->spimem->spi->dev,
+			"write %u bytes at %u --> %d\n",
+			segment, off, status);
 		if (status < 0)
 			break;
 
@@ -211,10 +253,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,9 +266,9 @@ 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,
+				segment, off,
 				jiffies_to_msecs(jiffies -
 					(timeout - EE_TIMEOUT)));
 			status = -ETIMEDOUT;
@@ -304,7 +345,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 +354,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 +369,49 @@ 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 +419,34 @@ 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_free_scratchbuf;
+	}
 
-	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_free_scratchbuf:
+	kfree(at25->scratchbuf);
+
+	return err;
+}
+
+static int at25_remove(struct spi_mem *spimem)
+{
+	struct at25_data *at25 = spi_mem_get_drvdata(spimem);
+
+	kfree(at25->scratchbuf);
+
+	return 0;
 }
 
 /*-------------------------------------------------------------------------*/
@@ -387,15 +457,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");
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[Geert Uytterhoeven]

Hi Boris,

On Sat, Mar 30, 2019 at 3:16 PM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
> The AT25 protocol fits pretty well in the spi-mem model. Convert the
> at25 spi driver to a spi-mem driver and use the dirmap API instead of
> forging SPI messages manually.
> This makes the driver compatible with spi-mem-only controllers
> (controllers implementing only the spi_mem ops).

Thanks for your patch!

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

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

> --- a/drivers/misc/eeprom/at25.c
> +++ b/drivers/misc/eeprom/at25.c

> @@ -63,17 +68,89 @@ struct at25_data {
>
>  #define        io_limit        PAGE_SIZE       /* bytes */
>
> +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 device *dev = &at25->spimem->spi->dev;
> +
> +       if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
> +               info.length = 256;

Note that by hardcoding 256 you do loose the ability to support chips
where EE_INSTR_BIT3_IS_ADDR is set, _and_ more than one address byte is
used (i.e. extra bit is A16 or A24).
While the code parsing "address-width" doesn't handle that, the comment
for EE_INSTR_BIT3_IS_ADDR suggests such chips do exist (I doubt it,
though, but you may know better), and the flag can be enabled through
the "at25,addr-mode" property, or through platform_data.

> @@ -82,38 +159,14 @@ 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;
> +       if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) {

Loss of support for address bit A16/A24.

> +               desc = at25->dirmap.rdesc[1];
> +               dirmap_offset = offset - 256;
> +       } else {
> +               desc = at25->dirmap.rdesc[0];
> +               dirmap_offset = offset;
>         }

> @@ -158,48 +211,37 @@ 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;
> +               unsigned int    dirmap_offset;
>                 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 && off > 255) {

Loss of support for address bit A16/A24.

> +                       desc = at25->dirmap.wdesc[1];
> +                       dirmap_offset = off -  256;

Two spaces after minus sign.

> +               } else {
> +                       desc = at25->dirmap.wdesc[0];
> +                       dirmap_offset = off;
>                 }

> @@ -211,10 +253,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);

You might want to move the debug print inside the new function
at25_rdsr(), as there's another call plus debug print in at25_probe().

> @@ -328,37 +369,49 @@ 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.
> +        */

That is no longer true as of commit a66d972465d15b1d ("devres: Align
data[] to ARCH_KMALLOC_MINALIGN") in v4.20-rc5, right?

> +       at25->scratchbuf = kmalloc(1, GFP_KERNEL);

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[kbuild test robot]

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

Hi Boris,

I love your patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v5.1-rc3 next-20190401]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Boris-Brezillon/eeprom-at25-Convert-the-driver-to-the-spi-mem-interface/20190401-160450
config: i386-randconfig-m2-201913 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

>> ERROR: "spi_mem_driver_unregister" [drivers/misc/eeprom/at25.ko] undefined!
>> ERROR: "spi_mem_driver_register_with_owner" [drivers/misc/eeprom/at25.ko] undefined!
>> ERROR: "devm_spi_mem_dirmap_create" [drivers/misc/eeprom/at25.ko] undefined!
>> ERROR: "spi_mem_dirmap_write" [drivers/misc/eeprom/at25.ko] undefined!
>> ERROR: "spi_mem_dirmap_read" [drivers/misc/eeprom/at25.ko] undefined!
>> ERROR: "spi_mem_exec_op" [drivers/misc/eeprom/at25.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[kbuild test robot]

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

Hi Boris,

I love your patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on v5.1-rc3 next-20190401]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Boris-Brezillon/eeprom-at25-Convert-the-driver-to-the-spi-mem-interface/20190401-160450
config: x86_64-randconfig-m3-201913 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/misc/eeprom/at25.o: In function `at25_rdsr':
>> drivers/misc/eeprom/at25.c:130: undefined reference to `spi_mem_exec_op'
   drivers/misc/eeprom/at25.o: In function `at25_ee_read':
>> drivers/misc/eeprom/at25.c:178: undefined reference to `spi_mem_dirmap_read'
   drivers/misc/eeprom/at25.o: In function `at25_create_dirmaps':
>> drivers/misc/eeprom/at25.c:86: undefined reference to `devm_spi_mem_dirmap_create'
   drivers/misc/eeprom/at25.c:94: undefined reference to `devm_spi_mem_dirmap_create'
   drivers/misc/eeprom/at25.c:106: undefined reference to `devm_spi_mem_dirmap_create'
   drivers/misc/eeprom/at25.c:114: undefined reference to `devm_spi_mem_dirmap_create'
   drivers/misc/eeprom/at25.o: In function `at25_wren':
   drivers/misc/eeprom/at25.c:144: undefined reference to `spi_mem_exec_op'
   drivers/misc/eeprom/at25.o: In function `at25_ee_write':
>> drivers/misc/eeprom/at25.c:240: undefined reference to `spi_mem_dirmap_write'
   drivers/misc/eeprom/at25.o: In function `at25_driver_init':
>> drivers/misc/eeprom/at25.c:468: undefined reference to `spi_mem_driver_register_with_owner'
   drivers/misc/eeprom/at25.o: In function `at25_driver_exit':
>> drivers/misc/eeprom/at25.c:468: undefined reference to `spi_mem_driver_unregister'

vim +130 drivers/misc/eeprom/at25.c

    70	
    71	static int at25_create_dirmaps(struct at25_data *at25)
    72	{
    73		struct spi_mem_dirmap_info info = {
    74			.op_tmpl = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_READ, 1),
    75					      SPI_MEM_OP_ADDR(at25->addrlen, 0, 1),
    76					      SPI_MEM_OP_NO_DUMMY,
    77					      SPI_MEM_OP_DATA_IN(0, NULL, 1)),
    78			.offset = 0,
    79			.length = at25->chip.byte_len,
    80		};
    81		struct device *dev = &at25->spimem->spi->dev;
    82	
    83		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR)
    84			info.length = 256;
    85	
  > 86		at25->dirmap.rdesc[0] = devm_spi_mem_dirmap_create(dev, at25->spimem,
    87								   &info);
    88		if (IS_ERR(at25->dirmap.rdesc[0]))
    89			return PTR_ERR(at25->dirmap.rdesc[0]);
    90	
    91		info.op_tmpl.cmd.opcode = AT25_WRITE;
    92		info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
    93	
    94		at25->dirmap.wdesc[0] = devm_spi_mem_dirmap_create(dev, at25->spimem,
    95								   &info);
    96		if (IS_ERR(at25->dirmap.wdesc[0]))
    97			return PTR_ERR(at25->dirmap.wdesc[0]);
    98	
    99		if (!(at25->chip.flags & EE_INSTR_BIT3_IS_ADDR))
   100			return 0;
   101	
   102		info.length = at25->chip.byte_len - 256;
   103		info.op_tmpl.cmd.opcode = AT25_READ | AT25_INSTR_BIT3;
   104		info.op_tmpl.data.dir = SPI_MEM_DATA_IN;
   105	
 > 106		at25->dirmap.rdesc[1] = devm_spi_mem_dirmap_create(dev, at25->spimem,
   107								   &info);
   108		if (IS_ERR(at25->dirmap.rdesc[1]))
   109			return PTR_ERR(at25->dirmap.rdesc[1]);
   110	
   111		info.op_tmpl.cmd.opcode = AT25_WRITE | AT25_INSTR_BIT3;
   112		info.op_tmpl.data.dir = SPI_MEM_DATA_OUT;
   113	
   114		at25->dirmap.wdesc[1] = devm_spi_mem_dirmap_create(dev, at25->spimem,
   115								   &info);
   116		if (IS_ERR(at25->dirmap.wdesc[1]))
   117			return PTR_ERR(at25->dirmap.wdesc[1]);
   118	
   119		return 0;
   120	}
   121	
   122	static int at25_rdsr(struct at25_data *at25)
   123	{
   124		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_RDSR, 1),
   125						  SPI_MEM_OP_NO_ADDR,
   126						  SPI_MEM_OP_NO_DUMMY,
   127						  SPI_MEM_OP_DATA_IN(1, at25->scratchbuf, 1));
   128		int ret;
   129	
 > 130		ret = spi_mem_exec_op(at25->spimem, &op);
   131		if (ret)
   132			return ret;
   133	
   134		return *((u8 *)at25->scratchbuf);
   135	}
   136	
   137	static int at25_wren(struct at25_data *at25)
   138	{
   139		struct spi_mem_op op = SPI_MEM_OP(SPI_MEM_OP_CMD(AT25_WREN, 1),
   140						  SPI_MEM_OP_NO_ADDR,
   141						  SPI_MEM_OP_NO_DUMMY,
   142						  SPI_MEM_OP_NO_DATA);
   143	
   144		return spi_mem_exec_op(at25->spimem, &op);
   145	}
   146	
   147	static int at25_ee_read(void *priv, unsigned int offset,
   148				void *val, size_t count)
   149	{
   150		struct spi_mem_dirmap_desc *desc;
   151		struct at25_data *at25 = priv;
   152		unsigned int dirmap_offset;
   153		ssize_t			status;
   154	
   155		if (unlikely(offset >= at25->chip.byte_len))
   156			return -EINVAL;
   157		if ((offset + count) > at25->chip.byte_len)
   158			count = at25->chip.byte_len - offset;
   159		if (unlikely(!count))
   160			return -EINVAL;
   161	
   162		if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && offset > 255) {
   163			desc = at25->dirmap.rdesc[1];
   164			dirmap_offset = offset - 256;
   165		} else {
   166			desc = at25->dirmap.rdesc[0];
   167			dirmap_offset = offset;
   168		}
   169	
   170		mutex_lock(&at25->lock);
   171	
   172		/* Read it all at once.
   173		 *
   174		 * REVISIT that's potentially a problem with large chips, if
   175		 * other devices on the bus need to be accessed regularly or
   176		 * this chip is clocked very slowly
   177		 */
 > 178		status = spi_mem_dirmap_read(desc, dirmap_offset, count, val);
   179		dev_dbg(&at25->spimem->spi->dev, "read %zu bytes at %d --> %zd\n",
   180			count, offset, status);
   181	
   182		mutex_unlock(&at25->lock);
   183		return status;
   184	}
   185	
   186	static int at25_ee_write(void *priv, unsigned int off, void *val, size_t count)
   187	{
   188		struct at25_data *at25 = priv;
   189		const char *buf = val;
   190		int			status = 0;
   191		unsigned		buf_size;
   192		u8			*bounce;
   193	
   194		if (unlikely(off >= at25->chip.byte_len))
   195			return -EFBIG;
   196		if ((off + count) > at25->chip.byte_len)
   197			count = at25->chip.byte_len - off;
   198		if (unlikely(!count))
   199			return -EINVAL;
   200	
   201		/* Temp buffer starts with command and address */
   202		buf_size = at25->chip.page_size;
   203		if (buf_size > io_limit)
   204			buf_size = io_limit;
   205		bounce = kmalloc(buf_size, GFP_KERNEL);
   206		if (!bounce)
   207			return -ENOMEM;
   208	
   209		/* For write, rollover is within the page ... so we write at
   210		 * most one page, then manually roll over to the next page.
   211		 */
   212		mutex_lock(&at25->lock);
   213		do {
   214			struct spi_mem_dirmap_desc *desc;
   215			unsigned long	timeout, retries;
   216			unsigned	segment;
   217			unsigned int	dirmap_offset;
   218			int		sr;
   219	
   220			status = at25_wren(at25);
   221			if (status < 0) {
   222				dev_dbg(&at25->spimem->spi->dev, "WREN --> %d\n",
   223					status);
   224				break;
   225			}
   226	
   227			if (at25->chip.flags & EE_INSTR_BIT3_IS_ADDR && off > 255) {
   228				desc = at25->dirmap.wdesc[1];
   229				dirmap_offset = off -  256;
   230			} else {
   231				desc = at25->dirmap.wdesc[0];
   232				dirmap_offset = off;
   233			}
   234	
   235			/* Write as much of a page as we can */
   236			segment = buf_size - (dirmap_offset % buf_size);
   237			if (segment > count)
   238				segment = count;
   239			memcpy(bounce, buf, segment);
 > 240			status = spi_mem_dirmap_write(desc, dirmap_offset, segment,
   241						      bounce);
   242			dev_dbg(&at25->spimem->spi->dev,
   243				"write %u bytes at %u --> %d\n",
   244				segment, off, status);
   245			if (status < 0)
   246				break;
   247	
   248			/* REVISIT this should detect (or prevent) failed writes
   249			 * to readonly sections of the EEPROM...
   250			 */
   251	
   252			/* Wait for non-busy status */
   253			timeout = jiffies + msecs_to_jiffies(EE_TIMEOUT);
   254			retries = 0;
   255			do {
   256				sr = at25_rdsr(at25);
   257				if (sr < 0 || (sr & AT25_SR_nRDY)) {
   258					dev_dbg(&at25->spimem->spi->dev,
   259						"rdsr --> %d (%02x)\n", sr, sr);
   260					/* at HZ=100, this is sloooow */
   261					msleep(1);
   262					continue;
   263				}
   264				if (!(sr & AT25_SR_nRDY))
   265					break;
   266			} while (retries++ < 3 || time_before_eq(jiffies, timeout));
   267	
   268			if ((sr < 0) || (sr & AT25_SR_nRDY)) {
   269				dev_err(&at25->spimem->spi->dev,
   270					"write %u bytes offset %u, timeout after %u msecs\n",
   271					segment, off,
   272					jiffies_to_msecs(jiffies -
   273						(timeout - EE_TIMEOUT)));
   274				status = -ETIMEDOUT;
   275				break;
   276			}
   277	
   278			off += segment;
   279			buf += segment;
   280			count -= segment;
   281	
   282		} while (count > 0);
   283	
   284		mutex_unlock(&at25->lock);
   285	
   286		kfree(bounce);
   287		return status;
   288	}
   289	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

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

[-- Attachment #3: Type: text/plain, Size: 144 bytes --]

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[Greg Kroah-Hartman]

On Sat, Mar 30, 2019 at 03:16:37PM +0100, Boris Brezillon wrote:
> The AT25 protocol fits pretty well in the spi-mem model. Convert the
> at25 spi driver to a spi-mem driver and use the dirmap API instead of
> forging SPI messages manually.
> This makes the driver compatible with spi-mem-only controllers
> (controllers implementing only the spi_mem ops).
> 
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/misc/eeprom/at25.c | 282 +++++++++++++++++++++++--------------
>  1 file changed, 176 insertions(+), 106 deletions(-)

Will there be a new version of this to fix up the problems that 0-day
found in it?

thanks,

greg k-h

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[Geert Uytterhoeven]

On Thu, Apr 25, 2019 at 9:44 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sat, Mar 30, 2019 at 03:16:37PM +0100, Boris Brezillon wrote:
> > The AT25 protocol fits pretty well in the spi-mem model. Convert the
> > at25 spi driver to a spi-mem driver and use the dirmap API instead of
> > forging SPI messages manually.
> > This makes the driver compatible with spi-mem-only controllers
> > (controllers implementing only the spi_mem ops).
> >
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/misc/eeprom/at25.c | 282 +++++++++++++++++++++++--------------
> >  1 file changed, 176 insertions(+), 106 deletions(-)
>
> Will there be a new version of this to fix up the problems that 0-day
> found in it?

gmail-whitespace-damaged fix:

diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
index f2abe27010eff133..98145d7d43d0c728 100644
--- a/drivers/misc/eeprom/Kconfig
+++ b/drivers/misc/eeprom/Kconfig
@@ -36,6 +36,7 @@ config EEPROM_AT25
        depends on SPI && SYSFS
        select NVMEM
        select NVMEM_SYSFS
+       select SPI_MEM
        help
          Enable this driver to get read/write support to most SPI EEPROMs,
          after you configure the board init code to know about each eeprom

Boris: what's the plan?

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] eeprom: at25: Convert the driver to the spi-mem interface

[Boris Brezillon]

Hi Geert,

On Tue, 3 Sep 2019 17:06:52 +0200
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Thu, Apr 25, 2019 at 9:44 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sat, Mar 30, 2019 at 03:16:37PM +0100, Boris Brezillon wrote:  
> > > The AT25 protocol fits pretty well in the spi-mem model. Convert the
> > > at25 spi driver to a spi-mem driver and use the dirmap API instead of
> > > forging SPI messages manually.
> > > This makes the driver compatible with spi-mem-only controllers
> > > (controllers implementing only the spi_mem ops).
> > >
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/misc/eeprom/at25.c | 282 +++++++++++++++++++++++--------------
> > >  1 file changed, 176 insertions(+), 106 deletions(-)  
> >
> > Will there be a new version of this to fix up the problems that 0-day
> > found in it?  
> 
> gmail-whitespace-damaged fix:
> 
> diff --git a/drivers/misc/eeprom/Kconfig b/drivers/misc/eeprom/Kconfig
> index f2abe27010eff133..98145d7d43d0c728 100644
> --- a/drivers/misc/eeprom/Kconfig
> +++ b/drivers/misc/eeprom/Kconfig
> @@ -36,6 +36,7 @@ config EEPROM_AT25
>         depends on SPI && SYSFS
>         select NVMEM
>         select NVMEM_SYSFS
> +       select SPI_MEM
>         help
>           Enable this driver to get read/write support to most SPI EEPROMs,
>           after you configure the board init code to know about each eeprom
> 
> Boris: what's the plan?

Sorry, I don't have time to respin this patch. Feel free to send a new
version if you need it.

Regards,

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/