Re: [PATCH v3 01/17] spi: spi-mem: Add DTR templates for cmd, address, dummy and data phase

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Apurva Nandan <a-nandan@ti.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>,
	Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Mark Brown <broonie@kernel.org>,
	Patrice Chotard <patrice.chotard@foss.st.com>,
	Christophe Kerello <christophe.kerello@foss.st.com>,
	Daniel Palmer <daniel@0x0f.com>,
	Alexander Lobakin <alobakin@pm.me>,
	<linux-mtd@lists.infradead.org>, <linux-kernel@vger.kernel.org>,
	<linux-spi@vger.kernel.org>, <p.yadav@ti.com>
Subject: Re: [PATCH v3 01/17] spi: spi-mem: Add DTR templates for cmd, address, dummy and data phase
Date: Tue, 4 Jan 2022 16:31:00 +0100	[thread overview]
Message-ID: <20220104163100.56850d0b@collabora.com> (raw)
In-Reply-To: <20220101074250.14443-2-a-nandan@ti.com>

On Sat, 1 Jan 2022 13:12:34 +0530
Apurva Nandan <a-nandan@ti.com> wrote:

> Setting dtr field of spi_mem_op is useful when creating templates
> for DTR ops in spinand.h. Also, 2 bytes cmd phases are required when
> operating in Octal DTR SPI mode.
> 
> Create new templates for dtr mode cmd, address, dummy and data phase
> in spi_mem_op, which set the dtr field to 1 and also allow passing
> the nbytes for the cmd phase.
> 
> Signed-off-by: Apurva Nandan <a-nandan@ti.com>
> ---
>  include/linux/spi/spi-mem.h | 41 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
> 
> diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> index 85e2ff7b840d..682378a9c600 100644
> --- a/include/linux/spi/spi-mem.h
> +++ b/include/linux/spi/spi-mem.h
> @@ -20,6 +20,14 @@
>  		.nbytes = 1,					\
>  	}
>  
> +#define SPI_MEM_OP_CMD_DTR(__nbytes, __opcode, __buswidth)	\
> +	{							\
> +		.nbytes = __nbytes,				\
> +		.opcode = __opcode,				\
> +		.buswidth = __buswidth,				\
> +		.dtr = 1,					\
> +	}
> +
>  #define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)		\
>  	{							\
>  		.nbytes = __nbytes,				\
> @@ -27,6 +35,14 @@
>  		.buswidth = __buswidth,				\
>  	}
>  
> +#define SPI_MEM_OP_ADDR_DTR(__nbytes, __val, __buswidth)	\
> +	{							\
> +		.nbytes = __nbytes,				\
> +		.val = __val,					\
> +		.buswidth = __buswidth,				\
> +		.dtr = 1,					\
> +	}
> +
>  #define SPI_MEM_OP_NO_ADDR	{ }
>  
>  #define SPI_MEM_OP_DUMMY(__nbytes, __buswidth)			\
> @@ -35,6 +51,13 @@
>  		.buswidth = __buswidth,				\
>  	}
>  
> +#define SPI_MEM_OP_DUMMY_DTR(__nbytes, __buswidth)		\
> +	{							\
> +		.nbytes = __nbytes,				\
> +		.buswidth = __buswidth,				\
> +		.dtr = 1,					\
> +	}
> +
>  #define SPI_MEM_OP_NO_DUMMY	{ }
>  
>  #define SPI_MEM_OP_DATA_IN(__nbytes, __buf, __buswidth)		\
> @@ -45,6 +68,15 @@
>  		.buswidth = __buswidth,				\
>  	}
>  
> +#define SPI_MEM_OP_DATA_IN_DTR(__nbytes, __buf, __buswidth)	\
> +	{							\
> +		.dir = SPI_MEM_DATA_IN,				\
> +		.nbytes = __nbytes,				\
> +		.buf.in = __buf,				\
> +		.buswidth = __buswidth,				\
> +		.dtr = 1,					\
> +	}
> +
>  #define SPI_MEM_OP_DATA_OUT(__nbytes, __buf, __buswidth)	\
>  	{							\
>  		.dir = SPI_MEM_DATA_OUT,			\
> @@ -53,6 +85,15 @@
>  		.buswidth = __buswidth,				\
>  	}
>  
> +#define SPI_MEM_OP_DATA_OUT_DTR(__nbytes, __buf, __buswidth)	\
> +	{							\
> +		.dir = SPI_MEM_DATA_OUT,			\
> +		.nbytes = __nbytes,				\
> +		.buf.out = __buf,				\
> +		.buswidth = __buswidth,				\
> +		.dtr = 1,					\
> +	}
> +
>  #define SPI_MEM_OP_NO_DATA	{ }
>  
>  /**

How about:

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 85e2ff7b840d..9a8d42803026 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -13,44 +13,59 @@
 
 #include <linux/spi/spi.h>
 
-#define SPI_MEM_OP_CMD(__opcode, __buswidth)                   \
+#define SPI_MEM_OP_DTR .dtr = 1
+
+#define SPI_MEM_OP_CMD(__opcode, __buswidth, ...)              \
        {                                                       \
                .buswidth = __buswidth,                         \
                .opcode = __opcode,                             \
                .nbytes = 1,                                    \
+               __VA_ARGS__                                     \
        }
 
-#define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth)           \
+#define SPI_MEM_OP_EXT_CMD(__nbytes, __opcode, __buswidth, ...)        \
+       {                                                       \
+               .buswidth = __buswidth,                         \
+               .opcode = __opcode,                             \
+               .nbytes = __nbytes,                             \
+               __VA_ARGS__                                     \
+       }
+
+#define SPI_MEM_OP_ADDR(__nbytes, __val, __buswidth, ...)      \
        {                                                       \
                .nbytes = __nbytes,                             \
                .val = __val,                                   \
                .buswidth = __buswidth,                         \
+               __VA_ARGS__                                     \
        }
 
 #define SPI_MEM_OP_NO_ADDR     { }
 
-#define SPI_MEM_OP_DUMMY(__nbytes, __buswidth)                 \
+#define SPI_MEM_OP_DUMMY(__nbytes, __buswidth, ...)            \
        {                                                       \
                .nbytes = __nbytes,                             \
                .buswidth = __buswidth,                         \
+               __VA_ARGS__                                     \
        }
 
 #define SPI_MEM_OP_NO_DUMMY    { }
 
-#define SPI_MEM_OP_DATA_IN(__nbytes, __buf, __buswidth)                \
+#define SPI_MEM_OP_DATA_IN(__nbytes, __buf, __buswidth, ...)   \
        {                                                       \
                .dir = SPI_MEM_DATA_IN,                         \
                .nbytes = __nbytes,                             \
                .buf.in = __buf,                                \
                .buswidth = __buswidth,                         \
+               __VA_ARGS__                                     \
        }
 
-#define SPI_MEM_OP_DATA_OUT(__nbytes, __buf, __buswidth)       \
+#define SPI_MEM_OP_DATA_OUT(__nbytes, __buf, __buswidth, ...)  \
        {                                                       \
                .dir = SPI_MEM_DATA_OUT,                        \
                .nbytes = __nbytes,                             \
                .buf.out = __buf,                               \
                .buswidth = __buswidth,                         \
+               __VA_ARGS__                                     \
        }
 
 #define SPI_MEM_OP_NO_DATA     { }

and you get to define a DTR op like that:

	struct spi_mem_op op =
		SPI_MEM_OP(SPI_MEM_OP_EXT_CMD(2, 0x1234, 8, SPI_MEM_OP_DTR),
			   SPI_MEM_OP_ADDR(4, 0xdeadbeef, 8, SPI_MEM_OP_DTR),
			   SPI_MEM_OP_DATA_OUT(128, buf, 8, SPI_MEM_OP_DTR));

This also means we can extend the struct without having to define new macros.
