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
