On Saturday 05 November 2011 10:36:30 Simon Glass wrote:
On Thu, Nov 3, 2011 at 6:36 PM, Mike Frysinger wrote:
On Thursday 03 November 2011 18:41:34 Simon Glass wrote:
--- /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;elsereturn 1;+}
shouldn't that be "||" and not "&&" ?
This function should be removed as it doesn't print enough errors.
this func is part of the SPI API. you can't remove it ;).
+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.
i think this is the wrong direction ... other SPI buses are not warning about invalid bus/cs combos and that seems to be fine. i don't think the tegra spi bus needs to be uniquely verbose.
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 ...
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));
what you have now is fine if you prefer it that way -mike