[U-Boot] [PATCHv3 3/3] 83xx, kmeter1: added NAND support

[Heiko Schocher]

Hello Scott

Scott Wood wrote:
> On Mon, Jul 13, 2009 at 12:15:12PM +0200, Heiko Schocher wrote:
>> +#define read_mode()	in_8((volatile unsigned char __iomem *) \
>> +				CONFIG_NAND_MODE_REG)
>> +#define write_mode(val)	out_8((volatile unsigned char __iomem *) \
>> +				CONFIG_NAND_MODE_REG, val)
>> +#define read_data()	in_8((volatile unsigned char __iomem *) \
>> +				CONFIG_NAND_DATA_REG)
>> +#define write_data(val)	out_8((volatile unsigned char __iomem *) \
>> +				CONFIG_NAND_DATA_REG, val)
> 
> No need for volatile when using accessors.  If you kept a pointer around
> instead of casting here, you could reasonably use the accessors directly
> without needing wrappers...
> 
> If this is purely for U-Boot and not shared with Linux we can drop the
> __iomem.

OK, I fix it.

>> +static void kpn_nand_hwcontrol(struct mtd_info *mtd, int cmd, unsigned int ctrl)
>> +{
>> +	u8	reg_val = read_mode();
> 
> No tab after "u8".
> 
>> +	if (ctrl & NAND_CTRL_CHANGE) {
>> +		if ( ctrl & NAND_NCE)
> 
> No space after "(".

Yep, you are right. Fixed.

>> +			reg_val = reg_val & ~KPN_CE1N;
>> +		else
>> +			reg_val = reg_val | KPN_CE1N;
>> +		write_mode(reg_val);
>> +	}
>> +	if (cmd == NAND_CMD_NONE)
>> +		return;
>> +
>> +	reg_val = reg_val & ~(KPN_ALE + KPN_CLE);
>> +	if (ctrl & NAND_CLE)
>> +		reg_val = reg_val | KPN_CLE;
>> +	if (ctrl & NAND_ALE)
>> +		reg_val = reg_val | KPN_ALE;
> 
> If ALE/CLE is sticky in the hardware mode register, you could probably
> move this under NAND_CTRL_CHANGE and simplify things a little.

I try this out.

>> diff --git a/include/configs/kmeter1.h b/include/configs/kmeter1.h
>> index 811ba88..4de0dfc 100644
>> --- a/include/configs/kmeter1.h
>> +++ b/include/configs/kmeter1.h
>> @@ -324,6 +324,12 @@
>>  #define CONFIG_SYS_DTT_HYSTERESIS	3
>>  #define CONFIG_SYS_DTT_BUS_NUM		(CONFIG_SYS_MAX_I2C_BUS)
>>
>> +#if defined(CONFIG_CMD_NAND)
>> +#define	CONFIG_NAND_KMETER1
> 
> No tab after #define.

fixed.

>> +#define CONFIG_SYS_MAX_NAND_DEVICE	1
>> +#define CONFIG_SYS_NAND_BASE		CONFIG_SYS_PIGGY_BASE
>> +#endif
>> +
>>  #if defined(CONFIG_PCI)
>>  #define CONFIG_CMD_PCI
>>  #endif
> 
> This file looks a little different in the current tree (2 rather than
> CONFIG_SYS_MAX_I2C_BUS), so it wouldn't apply cleanly.

Hmm... this is the third patch of a patchset, so it apply cleanly, if
the other 2 patches are first applied ... or should I base this patch
against current nand-flash tree, because this patch goes through your
tree?

thanks for reviewing
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany