From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Fri, 17 Jul 2015 18:01:45 -0500
Subject: [U-Boot] [PATCH 1/3] nand: lpc32xx: add SLC NAND driver
In-Reply-To: <4F172219764C784B84C2C1FF44E7DFB102FF1F78@003FCH1MPN2-042.003f.mgd2.msft.net>
References: <1437166134-21204-2-git-send-email-slemieux.tyco@gmail.com>
 <20150717232019.06c1db16@lilith>
 <4F172219764C784B84C2C1FF44E7DFB102FF1F78@003FCH1MPN2-042.003f.mgd2.msft.net>
Message-ID: <1437174105.2993.199.camel@freescale.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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