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

Scott Wood
2015-07-17T23:01:45Z

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 at 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 at gmail.com
> > <slemieux.tyco@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?
> >
>
> 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