[U-Boot] [PATCH 1/3] nand: lpc32xx: add SLC NAND driver

[Albert ARIBAUD]

Hello Sylvain,

On Fri, 17 Jul 2015 16:48:52 -0400, slemieux.tyco at gmail.com
<slemieux.tyco at gmail.com> wrote:

> 1) 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,
-- 
Albert.