From mboxrd@z Thu Jan  1 00:00:00 1970
From: Vipin KUMAR <vipin.kumar@st.com>
Date: Thu, 22 Apr 2010 09:58:59 +0530
Subject: [U-Boot] [PATCH 10/17] SPEAr : FSMC driver support added
In-Reply-To: <20100421170208.GA27045@schlenkerla.am.freescale.net>
References: <1271836483-15978-2-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-3-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-4-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-5-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-6-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-7-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-8-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-9-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-10-git-send-email-vipin.kumar@st.com>
	<1271836483-15978-11-git-send-email-vipin.kumar@st.com>
	<20100421170208.GA27045@schlenkerla.am.freescale.net>
Message-ID: <4BCFD08B.5020008@st.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 4/21/2010 10:32 PM, Scott Wood wrote:
> On Wed, Apr 21, 2010 at 01:24:36PM +0530, Vipin KUMAR wrote:
>> +#if defined(CONFIG_BOARD_NAND_LP)
> 
> CONFIG_SYS_FSMC_NAND_LP, CONFIG_SYS_FSMC_NAND_16BIT, etc.

Incomplete comment :)
Are these deprecated

>> +			/*
>> +			 * length is intentionally kept a higher multiple of 2
>> +			 * to read at least 13 bytes even in case of 16 bit NAND
>> +			 * devices
>> +			 */
>> +			len = ((len + 1) >> 1) << 1;
> 
> len = roundup(len, 2);
> 

OK, I would change that.
Please find the changes in patch-set v2

>> +	fsmc_version = (peripid2 >> FSMC_REVISION_SHFT) & \
>> +		       FSMC_REVISION_MSK;
> 
> Unnecessary backslash.
> 
Would be removed

>> +#ifndef __FSMC_NAND_H__
>> +#define __FSMC_NAND_H__
>> +
>> +struct fsmc_regs {
>> +	u8 reserved_1[0x40];
>> +	u32 genmemctrl_pc;		/* 0x40 */
>> +	u32 genmemctrl_sts;		/* 0x44 */
>> +	u32 genmemctrl_comm;		/* 0x48 */
>> +	u32 genmemctrl_attrib;		/* 0x4c */
>> +	u32 genmemctrl_ioata;		/* 0x50 */
>> +	u32 genmemctrl_ecc1;		/* 0x54 */
>> +	u32 genmemctrl_ecc2;		/* 0x58 */
>> +	u32 genmemctrl_ecc3;		/* 0x5c */
>> +	u8 reserved_2[0xfe0 - 0x60];
>> +	u32 genmemctrl_peripid0;	/* 0xfe0 */
>> +	u32 genmemctrl_peripid1;	/* 0xfe4 */
>> +	u32 genmemctrl_peripid2;	/* 0xfe8 */
>> +	u32 genmemctrl_peripid3;	/* 0xfec */
>> +	u32 genmemctrl_pcellid0;	/* 0xff0 */
>> +	u32 genmemctrl_pcellid1;	/* 0xff4 */
>> +	u32 genmemctrl_pcellid2;	/* 0xff8 */
>> +	u32 genmemctrl_pcellid3;	/* 0xffc */
>> +};
> 
> Is the genmemctrl_ prefix really needed?
> 

The peripheral's registers are named like these so I kept it.

>> +extern int spear_nand_init(struct nand_chip *nand);
> 
> fsmc_nand_init?
> 
Yes, I would change this
Thanks

> -Scott
>