[U-Boot] [PATCH] Adds driver for Xilinx' xps_spi SPI controller.

[Graeme Smecher]

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