Hi Mike,
On Thu, Nov 3, 2011 at 6:36 PM, Mike Frysinger <vap...@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(®s->status);
>> + writel(reg, ®s->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, ®s->status); /* Clear all SPI events via R/W */
debug("spi_xfer entry: STATUS = %08x\n", readl(®->status));
Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
