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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
