Hi Rémy,
I do have a few comments, see below:
On Mon, Jun 01, 2009 at 08:21:42PM +0200, Remy Bohmer wrote:
> V2 reworked comments from Jean-Christophe PLAGNIOL-VILLARD
[...]
> +COBJS-$(CONFIG_DISPLAY_CPUINFO) +=cpuinfo.o
missing space ?
> + switch ((cidr & AT91_CIDR_EPROC) >> 5) {
> + /* 0x1 = ARM946ES, 0x2 = ARM7TDMI, 0x4 = ARM920T */
> + case 0x5: txt = "ARM926EJS"; break;
> + default: txt = "undefined"; break;
since there are only four choices (until now at least...), why not
put the 'txt' for all of them ?
and the 'undefined' string is not a good choice: it represents a
configuration not known to U-Boot, but its numeric value is well
defined. So I suggest you change this to 'unknown', or even better,
'unknown: 0xfoo'.
> + }
> + printf("Embedded Processor: %s\n", txt);
> +
> + switch ((cidr & AT91_CIDR_ARCH) >> 20) {
> + case 0x19: txt = "AT91SAM9xx"; break;
> + case 0x29: txt = "AT91SAM9XExx"; break;
> + case 0x39: txt = "CAP9"; break;
> + default: txt = "undefined"; break;
same comments as above for 'undefined'.
> + switch ((cidr & AT91_CIDR_NVPTYP) >> 28) {
> + case 0x0: txt = "ROM"; break;
> + case 0x1: txt = "ROMless or onchip flash"; break;
> + case 0x2: txt = "Embedded flash memory"; break;
> + case 0x3: txt = "ROM and Embedded flash memory"; break;
> + case 0x4: txt = "SRAM emulating ROM"; break;
> + default: txt = "undefined"; break;
ditto.
> + switch (cidr & AT91_CIDR_SRAMSIZ) {
> + case AT91_CIDR_SRAMSIZ_1K: txt = "1K"; break;
> + case AT91_CIDR_SRAMSIZ_2K: txt = "2K"; break;
> + case AT91_CIDR_SRAMSIZ_112K: txt = "112K"; break;
> + case AT91_CIDR_SRAMSIZ_4K: txt = "4K"; break;
> + case AT91_CIDR_SRAMSIZ_80K: txt = "80K"; break;
> + case AT91_CIDR_SRAMSIZ_160K: txt = "160K"; break;
> + case AT91_CIDR_SRAMSIZ_8K: txt = "8K"; break;
> + case AT91_CIDR_SRAMSIZ_16K: txt = "16K"; break;
> + case AT91_CIDR_SRAMSIZ_32K: txt = "32K"; break;
> + case AT91_CIDR_SRAMSIZ_64K: txt = "64K"; break;
> + case AT91_CIDR_SRAMSIZ_128K: txt = "128K"; break;
> + case AT91_CIDR_SRAMSIZ_256K: txt = "256K"; break;
> + case AT91_CIDR_SRAMSIZ_96K: txt = "96K"; break;
> + case AT91_CIDR_SRAMSIZ_512K: txt = "512K"; break;
> + default: txt = "undefined"; break;
bad alignment here (you did align the 'txt =' properly in all other
switch/case statements, but not here).
ditto for 'undefined'.
Stelian.
--
Stelian Pop <stel...@popies.net>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
