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

[Mike Frysinger <vapier@gentoo.org>]

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 ?

> --- /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 "&&" ?

> +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 ?

> +	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)

> +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

> +	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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111103/23601ddc/attachment.pgp