[U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver

[Tom Rini]

On Fri, Sep 14, 2012 at 02:24:48PM -0500, Scott Wood wrote:
> On Fri, Sep 14, 2012 at 08:21:11PM +0200, Marek Vasut wrote:
> > Dear Jos? Miguel Gon?alves,
> > 
> > > NAND Flash driver with HW ECC for the S3C24XX SoCs.
> > > Currently it only supports SLC NAND chips.
> > > 
> > > Signed-off-by: Jos? Miguel Gon?alves <jose.goncalves at inov.pt>
[snip]
> > > +/*
> > > + * Hardware specific access to control-lines function
> > > + */
> > > +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int
> > > ctrl) +{
> > > +	struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
> > > +	struct nand_chip *this = mtd->priv;
> > > +
> > > +	if (ctrl & NAND_CTRL_CHANGE) {
> > > +		if (ctrl & NAND_CLE)
> > > +			this->IO_ADDR_W = &nand->nfcmmd;
> > > +		else if (ctrl & NAND_ALE)
> > > +			this->IO_ADDR_W = &nand->nfaddr;
> > > +		else
> > > +			this->IO_ADDR_W = &nand->nfdata;
> > 
> > Why don't you use local variable here?
> 
> Because then you'll access the wrong data when the function is called
> again without NAND_CTRL_CHANGE.  This is a common way of writing the
> hwcontrol function, though the way ndfc.c does it is simpler (you can use
> a local variable if you ignore NAND_CTRL_CHANGE altogether).
> 
> > > +		if (ctrl & NAND_NCE)
> > > +			s3c_nand_select_chip(mtd, *(int *)this->priv);
> > 
> > Uh, how's this *(int *) supposed to work?
> 
> Usually driver-private data is a struct; apparently in this driver it's
> an int.

Shall we take both of these comments as requests to do things
differently (struct like everyone else does, simpiler code like ndfc.c
does) unless there's good reason to not change?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120914/a48c29f8/attachment.pgp>