[U-Boot] [PATCH] andes_spi: add andes_spi interface

[Mike Frysinger]

On Fri, Apr 22, 2011 at 3:20 AM, Macpaul Lin wrote:
> +#ifdef DEBUG
> +#define debug(fmt, args...)    printf(fmt, ##args)
> +#else
> +#define debug(fmt, args...)
> +#endif

common.h already provides this for you

> +void andes_spi_spit_en(struct andes_spi_slave *ds)
> +{
> +       debug("%s: dcr: %x, write value: %x\n",
> +                       __func__, readl(&ds->regs->dcr),
> +                       readl(&ds->regs->dcr) | ANDES_SPI_DCR_SPIT);
> +       writel((readl(&ds->regs->dcr) | ANDES_SPI_DCR_SPIT), &ds->regs->dcr);
> +}

putting io accessors into debug() is generally a bad idea.  i'd
suggest you store this in a local variable and then pass that to
everything else.  it'd also make the code cleaner.
unsigned long dcr = readl(&ds->regs->dcr);

> +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,
> +                       unsigned int max_hz, unsigned int mode)
> +{
> +       struct andes_spi_slave  *ds;
> +
> +       if (!spi_cs_is_valid(bus, cs))
> +               return NULL;
> +
> +       ds = malloc(sizeof(*ds));
> +       if (!ds)
> +               return NULL;
> +
> +       ds->slave.bus = bus;
> +       ds->slave.cs = cs;
> +       ds->regs = (struct andes_spi_regs *)CONFIG_SYS_SPI_BASE;
> +       ds->freq = max_hz;

your spi controller has no frequency limit ?  your spi_claim_bus
indicates that there is ...

> +static int andes_spi_read(struct spi_slave *slave, unsigned int len,
> +                           u8 *rxp, unsigned long flags)
> +{
> ....
> +       while (len > 0) {
> +               debug(" ");
> +               if (len / 4 > 0)
> +                       left = 4;
> +               else
> +                       left = len;

seems like you want:
left = max(len, 4);

> +               data = readl(&ds->regs->data);
> +               for (i = 0; i < left; i++) {
> +                       debug("%02x ", data & 0xff);
> +                       *rxp++ = data;
> +                       data >>= 8;
> +                       len--;
> +               }
> +       }

looks to me like you write too much data ... if someone does a spi
read of 1 byte, you still write out 4 ?

> +int spi_xfer(struct spi_slave *slave, unsigned int bitlen,
> +            const void *dout, void *din, unsigned long flags)
> +{
> ...
> +       /*
> +        * It's not clear how non-8-bit-aligned transfers are supposed to be
> +        * represented as a stream of bytes...this is a limitation of
> +        * the current SPI interface - here we terminate on receiving such a
> +        * transfer request.
> +        */
> +
> +       if (bitlen % 8) {
> +               /* Errors always terminate an ongoing transfer */
> +               flags |= SPI_XFER_END;
> +               goto out;
> +       }

this is correct behavior

> --- /dev/null
> +++ b/drivers/spi/andes_spi.h

i cant think of any reason other code would want to include this ...
so please put this header in drivers/spi/
-mike