From mboxrd@z Thu Jan  1 00:00:00 1970
From: Bo Shen <voice.shen@atmel.com>
Date: Tue, 05 Mar 2013 10:03:57 +0800
Subject: [U-Boot] [PATCH 4/4] ARM: atmel: add sama5d3xek support
In-Reply-To: <20130304151459.GA25797@bill-the-cat>
References: <1362034848-6371-1-git-send-email-voice.shen@atmel.com>
	<1362034848-6371-5-git-send-email-voice.shen@atmel.com>
	<20130304151459.GA25797@bill-the-cat>
Message-ID: <5135528D.7050800@atmel.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

Hi Tom,

On 3/4/2013 23:14, Tom Rini wrote:
> On Thu, Feb 28, 2013 at 03:00:47PM +0800, Bo Shen wrote:
>
>> Add sama5d3xek support with following feature
>>    - boot from NAND flash, PMECC support, 4bit ECC @ 512 bytes sector
>>    - boot from SPI flash support
>>    - boot from SD card support
>>    - LCD support
>>    - EMAC support
>>    - USB support
>>
>> Signed-off-by: Bo Shen <voice.shen@atmel.com>
>
> Some minor comments:
>
> [snip]
>> +	if (cpu_is_sama5d3())
>> +		switch (extension_id) {
>> +		case ARCH_EXID_SAMA5D31:
>> +			return CONFIG_SYS_AT91_D31_CPU_NAME;
>> +		case ARCH_EXID_SAMA5D33:
>> +			return CONFIG_SYS_AT91_D33_CPU_NAME;
>> +		case ARCH_EXID_SAMA5D34:
>> +			return CONFIG_SYS_AT91_D34_CPU_NAME;
>> +		case ARCH_EXID_SAMA5D35:
>> +			return CONFIG_SYS_AT91_D35_CPU_NAME;
>> +		default:
>> +			return CONFIG_SYS_AT91_UNKNOWN_CPU;
>
> These aren't configurable, and are used once.  Just put the strings
> here.

OK, I will use strings here directly here in next version.

>
>> @@ -0,0 +1,268 @@
>> +/*
>> + * Configuation settings for the SAMA5D3xEK board.
> [snip]
>> +#undef CONFIG_USE_IRQ			/* we don't need IRQ/FIQ stuff	*/
>> +
>> +#undef CONFIG_CMDLINE_TAG		/* enable passing of ATAGs	*/
>> +#undef CONFIG_SETUP_MEMORY_TAGS
>> +#undef CONFIG_INITRD_TAG
>
> Just leave these, and the other #undef's out.

You mean I need not to #undef these, because these are not defined, am I 
right?

>> +/*
>> + * Command line configuration.
>> + */
>> +#include <config_cmd_default.h>
>> +#undef CONFIG_CMD_FPGA
>> +#undef CONFIG_CMD_IMI
>> +#undef CONFIG_CMD_IMLS
>> +#undef CONFIG_CMD_AUTOSCRIPT
>> +#undef CONFIG_CMD_LOADS
>
> These are fine to leave in 'tho.

These are no useful for us.
I will consider remove unneeded #undef

>> +#ifdef CONFIG_USE_IRQ
>> +#error CONFIG_USE_IRQ not supported
>> +#endif
>
> Just drop that part.

Ok, I will drop this in next version.

> And please check things with checkpatch.pl, I
> thought I saw a '#define<tab>FOO' in there.  Thanks!

I have checked this patch with checkpatch.pl, and get no errors and no 
warnings.

Best Regards,
Bo Shen