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?
Also, I did not find where the IO_ADDR_R field is assigned. Did I miss it?
This is just one case, but I suspect in many places, the code is unnecessarily complex. I understand it was ported, not written from scratch, but I think porting code should not prevent us from making it smaller, more efficient and more maintainable.
Amicalement,