On Fri, 2015-07-17 at 22:24 +0000, LEMIEUX, SYLVAIN wrote:
Hi Albert,
Thanks for the feedback.
From: U-Boot [mailto:u-boot-bounces@lists.denx.de] On Behalf Of Albert ARIBAUD Sent: 17-Jul-15 5:20 PM
Hello Sylvain,
On Fri, 17 Jul 2015 16:48:52 -0400, slemieux.tyco@gmail.com slemieux.tyco@gmail.com wrote:
- Fixed checkpatch script output in legacy code. A single warning remaining.
The following warning from the legacy code is still present: lpc32xx_nand.c:195: WARNING: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt
+static u_char lpc32xx_read_byte(struct mtd_info *mtd) +{
- struct nand_chip *this = mtd->priv;
- unsigned long *preg = (unsigned long *)this->IO_ADDR_R;
- volatile unsigned long tmp32;
- tmp32 = *preg;
- return (u_char)tmp32;
+}
The volatile above has no reason to exist; the warning is justified here as we have accessors that guarantee that the access will not be optimized away or reordered, juste like the 'volatile' above tries to do (and yes, these accessors *use* 'volatile'. All the more a reason not to use it again here).
Besides, the code is quite verbose and not precise enough. Yes, 'unsigned long' is 32-bit-ish, but in U-Boot, when something is 32-bit, that is explicit.
All in all, the whole function could be expressed as:
static u_char lpc32xx_read_byte(struct mtd_info *mtd) { struct nand_chip *this = mtd->priv; return (u_char)readl(this->IO_ADDR_R); }BTW, isn't IO_ADDR_R pointing to the data register, and isn't the data register 16-bits? And if so, then why the 32-bits read?
The register is 16 bits; this implementation is the porting of the initial code. I will wait for feedback and see how we want to approach this (add DMA and HW ECC to the NAND SLC driver sent by Vladimir or update the driver as part of the porting effort).
If the register is 16-bit, then you should use readw(), not readl().
Why are there two different versions of this driver being submitted in parallel?
-Scott