Re: [PATCH v2 1/3] mtd: spinand: Add #define-s for page-read ops with three-byte addresses

[Miquel Raynal]

Hi Jeff,

Jeff Kletsky <lede@xxxxxxxxxxxx> wrote on Sun, 19 May 2019 13:27:58
-0700:

> On 5/14/19 11:49 PM, Schrempf Frieder wrote:
> 
> > On 15.05.19 08:17, Marek Vasut wrote:  
> >> On 5/14/19 11:53 PM, Jeff Kletsky wrote:  
> >>> From: Jeff Kletsky <git-commits@xxxxxxxxxxxx>  
> >> That #define in $subject is called a macro.
> >>
> >> Seems this patch adds a lot of almost duplicate code, can it be somehow
> >> de-duplicated ?  
> > We could add another parameter naddr or addrlen to the
> > SPINAND_PAGE_READ_FROM_CACHE_XX_OPs and pass the value 2 for all
> > existing chips except for GD5F1GQ4UFxxG which needs 3 bytes address length.
> >
> > This would cause one more argument to each of the macro calls in all
> > chip drivers. As long as there are only two flavors (2 and 3 bytes) I'm
> > not sure if this really would make things easier and also this is "only"
> > preprocessor code.
> >
> > So anyways, I would be fine with both approaches, Jeff's current one or
> > one with another parameter for the address length.
> >
> > By the way: Jeff, you didn't carry my Reviewed-by tag to v2. So I will
> > just reply again to add the tags.
> >  
> >>> The GigaDevice GD5F1GQ4UFxxG SPI NAND utilizes three-byte addresses
> >>> for its page-read ops.
> >>>
> >>> http://www.gigadevice.com/datasheet/gd5f1gq4xfxxg/
> >>>
> >>> Signed-off-by: Jeff Kletsky <git-commits@xxxxxxxxxxxx>
> >>> ---
> >>>    include/linux/mtd/spinand.h | 30 ++++++++++++++++++++++++++++++
> >>>    1 file changed, 30 insertions(+)
> >>>
> >>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> >>> index b92e2aa955b6..05fe98eebe27 100644
> >>> --- a/include/linux/mtd/spinand.h
> >>> +++ b/include/linux/mtd/spinand.h
> >>> @@ -68,30 +68,60 @@
> >>>    		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
> >>>    		   SPI_MEM_OP_DATA_IN(len, buf, 1))  
> >>>    >>> +#define SPINAND_PAGE_READ_FROM_CACHE_OP_3A(fast, addr, ndummy, buf, len) \  
> >>> +	SPI_MEM_OP(SPI_MEM_OP_CMD(fast ? 0x0b : 0x03, 1),		\
> >>> +		   SPI_MEM_OP_ADDR(3, addr, 1),				\
> >>> +		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
> >>> +		   SPI_MEM_OP_DATA_IN(len, buf, 1))
> >>> +
> >>>    #define SPINAND_PAGE_READ_FROM_CACHE_X2_OP(addr, ndummy, buf, len)	\
> >>>    	SPI_MEM_OP(SPI_MEM_OP_CMD(0x3b, 1),				\
> >>>    		   SPI_MEM_OP_ADDR(2, addr, 1),				\
> >>>    		   SPI_MEM_OP_DUMMY(ndummy, 1),				\
> >>>    		   SPI_MEM_OP_DATA_IN(len, buf, 2))  
> >>>    >>> [ _3A addition repeated three more times for similar ops ... ]  
> 
> It's easy enough to change the wording, and will do so on the next revision.
> 
> However, it's not clear to me that there is consensus on if the present
> set of macros is acceptable/preferred over definition of a set of ones
> that accept an additional parameter.
> 
> At least from my perspective and as Schrempf Frieder has hinted at,
> these macros are syntactic sugar and all result in equivalent C code.
> 
> Either should compile to the same run-time size and performance (assuming
> reasonably that a construct like `true ? 0x0b : 0x03` is optimized out).
> 
> Adding an additional parameter, at least for me, wouldn't improve readability
> of the code and is offset by the need to refactor four other files. Even
> though it should be a simple/trivial refactor, I do not have any examples
> of the four other manufacturers' chips to be able to confirm proper operation.
> 
> I'll prepare a reworded set of patches with the present macro structure.
> 
> If there is strong feeling for refactoring the macro set, please let me know.

On my side I would rather not add this extra argument, I know it is not
very conventional to add so much macros but once you've read one you
read all of them and I think it improves the readability of the code
using it.


Thanks,
MiquÃl