[PATCH 2/2] mtd: sunxi: nand: refactor ->read_page()/->write_page() code

Hi Brian,

On Wed, 30 Sep 2015 11:36:27 -0700
Brian Norris [off-list ref] wrote:

On Wed, Sep 16, 2015 at 09:46:37AM +0200, Boris Brezillon wrote:

quoted

Most of the logic to read/write pages with the HW ECC engine enabled
is common to the HW_ECC and NAND_ECC_HW_SYNDROME scheme.
Refactor the code to avoid code duplication.

Hmm, a benign commit description to describe a somewhat complicated
patch. This seems to do several different types of refactoring all at
once, and it makes it a bit hard to review. Can you perhaps refactor
this into 2 or more patches?

Fair enough, I'll try to split it in several pieces.

e.g., I think the NFC_USER_DATA_TO_BUF()
stuff can be orthogonal from the introduction of
sunxi_nfc_hw_ecc_read_chunk() and sunxi_nfc_hw_ecc_read_extra_oob().

Yes, it is, even if I doubt removing this small piece of code can make
the patch more readable ;-).

quoted

Signed-off-by: Boris Brezillon <redacted>
---
 drivers/mtd/nand/sunxi_nand.c | 404 ++++++++++++++++++++++--------------------
 1 file changed, 212 insertions(+), 192 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index dc44435..640b96c 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c

@@ -159,6 +159,13 @@
 /* NFC_USER_DATA helper macros */
 #define NFC_BUF_TO_USER_DATA(buf)	((buf)[0] | ((buf)[1] << 8) | \
 					((buf)[2] << 16) | ((buf)[3] << 24))
+#define NFC_USER_DATA_TO_BUF(buf, val)	\
+	{				\
+		(buf)[0] = val;		\
+		(buf)[1] = val >> 8;	\
+		(buf)[2] = val >> 16;	\
+		(buf)[3] = val >> 24;	\
+	}

Two things about this macro:

  1) you should probably wrap 'val' in parentheses

Yep ...

  2) the use of 'val' 4 times in this macro means it will be evaluated 4
  times; this *can* be OK, except the 'val' parameter as used in context
  is actually a register read (readl()). Is this intentional? Anyway,
  such a construct kinda hides the actual behavior, whether or not it's
  intentional.

... and no, it's not intentional.

To kill off all concerns, perhaps this should be a static inline
function instead. And we might do the same with NFC_BUF_TO_USER_DATA()
then, to match.

I like this idea (I'll use lower cases if I convert them to inline
functions).

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com