Hi Mike,
On Thu, Nov 3, 2011 at 6:36 PM, Mike Frysinger vapier@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