From mboxrd@z Thu Jan  1 00:00:00 1970
From: Scott Wood <scottwood@freescale.com>
Date: Fri, 14 Sep 2012 14:24:48 -0500
Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
In-Reply-To: <201209142021.12033.marex@denx.de>
References: <1347643742-19966-1-git-send-email-jose.goncalves@inov.pt>
	<1347643742-19966-10-git-send-email-jose.goncalves@inov.pt>
	<201209142021.12033.marex@denx.de>
Message-ID: <20120914192448.GA5398@buserror.net>
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, 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@inov.pt>
> 
> [...]
> 
> > +#include <common.h>
> > +#include <nand.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/s3c24xx_cpu.h>
> > +#include <asm/errno.h>
> > +
> > +#define MAX_CHIPS	2
> > +static int nand_cs[MAX_CHIPS] = { 0, 1 };
> > +
> > +#ifdef CONFIG_SPL_BUILD
> > +#define printf(arg...) do {} while (0)
> 
> This doesn't seem quite right ...
> 
> 1) this should be in CPU directory
> 2) should be enabled only if CONFIG_SPL_SERIAL_SUPPORT is not set
> 3) should be inline function, not a macro

What specifically should be in the CPU directory?

> > +#endif
> > +
> > +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip)
> > +{
> > +	struct s3c24xx_nand *const nand = s3c24xx_get_base_nand();
> > +	u_long nfcont;
> > +
> > +	nfcont = readl(&nand->nfcont);
> > +
> > +	switch (chip) {
> > +	case -1:
> > +		nfcont |= NFCONT_NCE1 | NFCONT_NCE0;
> > +		break;
> > +	case 0:
> > +		nfcont &= ~NFCONT_NCE0;
> > +		break;
> > +	case 1:
> > +		nfcont &= ~NFCONT_NCE1;
> > +		break;
> > +	default:
> > +		return;
> > +	}
> > +
> > +	writel(nfcont, &nand->nfcont);
> > +}
> > +
> > +/*
> > + * 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.

-Scott