From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Fri, 14 Sep 2012 20:21:11 +0200
Subject: [U-Boot] [PATCH v2 09/11] S3C24XX: Add NAND Flash driver
In-Reply-To: <1347643742-19966-10-git-send-email-jose.goncalves@inov.pt>
References: <1347643742-19966-1-git-send-email-jose.goncalves@inov.pt>
	<1347643742-19966-10-git-send-email-jose.goncalves@inov.pt>
Message-ID: <201209142021.12033.marex@denx.de>
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

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

> +#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?

> +		if (ctrl & NAND_NCE)
> +			s3c_nand_select_chip(mtd, *(int *)this->priv);

Uh, how's this *(int *) supposed to work?
[...]