[U-Boot] [PATCH v2 3/8] tegra2: spi: Add SPI driver for Tegra2 SOC

[Simon Glass]

Hi Mike,

On Thu, Nov 3, 2011 at 6:36 PM, Mike Frysinger <vapier at gentoo.org> wrote:
> On Thursday 03 November 2011 18:41:34 Simon Glass wrote:
>> --- a/arch/arm/include/asm/arch-tegra2/tegra2.h
>> +++ b/arch/arm/include/asm/arch-tegra2/tegra2.h
>>
>>  #define NV_PA_APB_UARTC_BASE (NV_PA_APB_MISC_BASE + 0x6200)
>>  #define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300)
>>  #define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400)
>> +#define TEGRA2_SPI_BASE              (NV_PA_APB_MISC_BASE + 0xC380)
>>  #define NV_PA_PMC_BASE               0x7000E400
>>  #define NV_PA_CSITE_BASE     0x70040000
>
> shouldn't it use the same naming convention ?  NV_xxxx_SPI_BASE ?

Actually we are moving away from this - the prefixes just obfuscate
the meaning. A later patch will tidy this up a little.

>
>> --- /dev/null
>> +++ b/drivers/spi/tegra2_spi.c
>>
>> +int spi_cs_is_valid(unsigned int bus, unsigned int cs)
>> +{
>> +     /* Tegra2 SPI-Flash - only 1 device ('bus/cs') */
>> +     if (bus > 0 && cs != 0)
>> +             return 0;
>> +     else
>> +             return 1;
>> +}
>
> shouldn't that be "||" and not "&&" ?

This function should be removed as it doesn't print enough errors.

>
>> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
>> +             unsigned int max_hz, unsigned int mode)
>> +{
>> +     struct tegra_spi_slave *spi;
>> +
>> +     if (!spi_cs_is_valid(bus, cs))
>> +             return NULL;
>> +
>> +     if (bus != 0) {
>> +             printf("SPI error: unsupported bus %d\n", bus);
>> +             return NULL;
>> +     }
>> +     if (cs != 0) {
>> +             printf("SPI error: unsupported chip select %d on bus %d\n",
>> +                    cs, bus);
>> +             return NULL;
>> +     }
>
> doesn't spi_cs_is_valid() make these two later checks redundant ?

Yes - have removed the function.

>
>> +     if (mode > SPI_MODE_3) {
>> +             printf("SPI error: unsupported SPI mode %i\n", mode);
>> +             return NULL;
>> +     }
>
> this is weird ... i'd just drop it as this isn't something that should be in
> spi drivers, but rather the common layer (if we choose to do so)

OK

>
>> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
>> +             const void *data_out, void *data_in, unsigned long flags)
>> +{
>> ...
>> +     if (bitlen & 7)
>> +             return -1;
>
> i'd use (bitlen % 8) as that is what all the other drivers are doing

OK

>
>> +     reg = readl(&regs->status);
>> +     writel(reg, &regs->status);     /* Clear all SPI events via R/W */
>
> are these R1C or W1C bits ?  if the latter, you could just write -1 and avoid
> the read altogether ...
> -mike
>

The next line is:

	debug("spi_xfer entry: STATUS = %08x\n", reg);

and I didn't want to remove that, so I need to keep the read
unfortunately. It could perhaps be this if you are keen:

writel(-1, &regs->status);     /* Clear all SPI events via R/W */
debug("spi_xfer entry: STATUS = %08x\n", readl(&reg->status));

Regards,
Simon