[U-Boot] [PATCH v2] spi: add altera spi controller support

[Mike Frysinger]

On Monday 22 March 2010 23:36:19 Thomas Chou wrote:
> +#include <common.h>
> +#include <malloc.h>
> +#include <spi.h>
> +#include <asm/io.h>
> +#include <nios2-io.h>

side note, but am i the only one who thinks nios headers in include/ is bad 
mojo ?

> +static nios_spi_t *nios_spi = (nios_spi_t *)CONFIG_SYS_SPI_BASE;

shouldnt this be based on the bus # so that you can support multiple ones at 
runtime ?  i.e. you add a regs member to the spi slave struct and operate off 
that at runtime.

> +void spi_init (void)

i think the spaces should be consumed before the paren per LKML style

> +{
> +	/* empty read buffer */
> +	if (readl (&nios_spi->status) & NIOS_SPI_RRDY)
> +		readl (&nios_spi->rxdata);
> +	return;
> +}

useless return ...

> +struct spi_slave *spi_setup_slave (unsigned int bus, unsigned int cs,
> +				  unsigned int max_hz, unsigned int mode)
> +{
> +	struct spi_slave *slave;
> +
> +	slave = malloc (sizeof(struct spi_slave));

sizeof(*slave)

> +	if (!slave)
> +		return NULL;
> +
> +	slave->bus = bus;
> +	slave->cs = cs;
> +
> +	return slave;
> +}

shouldnt you be validating the bus/cs values here ?  presumably you only have 
1 bus atm, so bus != 0 is an error.  and the cs is being used to write to an 
"unsigned slaveselect", so the range of values is at most 0...31 inclusive 
(assuming your hardware can actually support 32 CS's).

> +int spi_claim_bus (struct spi_slave *slave)
> +{
> +	return 0;
> +}
> +
> +void spi_release_bus (struct spi_slave *slave)
> +{
> +	return;
> +}

i'm pretty sure you should be programming either nios_spi->slaveselect or 
nios_spi->control here rather than in the spi_xfer func ...

> +int spi_xfer (struct spi_slave *slave, unsigned int bitlen, const void
> *dout, +	     void *din, unsigned long flags)
> +{
> +	int i, iter = bitlen >> 3;
> +	const uchar *txp = dout;
> +	uchar *rxp = din;
> +	uchar d;
> +
> +	if (flags & SPI_XFER_BEGIN) {
> +		writel (1 << slave->cs, &nios_spi->slaveselect);
> +		writel (NIOS_SPI_SSO, &nios_spi->control);
> +	}
> +
> +	for (i = 0; i < iter; i++) {
> +		writel (txp ? txp[i] : 0, &nios_spi->txdata);
> +		while (!(readl (&nios_spi->status) & NIOS_SPI_RRDY))
> +			;
> +		d = readl (&nios_spi->rxdata);
> +		if (rxp)
> +			rxp[i] = d;
> +	}
> +	if (flags & SPI_XFER_END)
> +		writel (0, &nios_spi->control);
> +
> +	return 0;
> +}

since you only support multiples of 8 bytes here, your code should return an 
error when (bitlen & 0x7)

your XFER_END should force the CS to go low ... is that what the control does, 
or is it the slaveselect ?
-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/20100323/c7acb0fe/attachment.pgp