Hi Mike, Thanks again for reviewing! Comments in-line; an updated patch will follow.
On 03/08/10 10:59 AM, Mike Frysinger wrote: > On Tuesday, August 03, 2010 11:47:42 Graeme Smecher wrote: > >> --- a/drivers/spi/Makefile >> +++ b/drivers/spi/Makefile >> >> COBJS-$(CONFIG_ALTERA_SPI) += altera_spi.o >> +COBJS-$(CONFIG_XILINX_SPI) += xilinx_spi.o >> COBJS-$(CONFIG_ATMEL_DATAFLASH_SPI) += atmel_dataflash_spi.o >> > this is a sorted list. please keep it that way > Whoops -- OK. >> --- /dev/null >> +++ b/drivers/spi/xilinx_spi.c >> >> +static ulong xilinx_spi_base_list[] = { >> +#ifdef XPAR_SPI_0_BASEADDR >> + XPAR_SPI_0_BASEADDR, >> +#endif >> +#ifdef XPAR_SPI_1_BASEADDR >> + XPAR_SPI_1_BASEADDR, >> +#endif >> +#ifdef XPAR_SPI_2_BASEADDR >> + XPAR_SPI_2_BASEADDR, >> +#endif >> +#ifdef XPAR_SPI_3_BASEADDR >> + XPAR_SPI_3_BASEADDR, >> +#endif >> +}; >> > if this is only ever read, you might want to declare it const so it ends up in > the rodata section. > Done. > what if someone defines just XPAR_SPI_3_BASEADDR ? do you want to be > consistent in bus id 3 always corresponds to XPAR_SPI_3_BASEADDR ? or are you > OK with the numbers always being relative ? doesnt matter to me, just > highlighting some things that might have been missed. > These numbers come from autogenerated headers and are (as far as I know) always numbered from 0. Offhand, the above code seems like the least confusing approach. I don't have a particularly strong opinion either. cheers, Graeme _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot