Re: [U-Boot-Users] [RFC/PATCH] SPI API improvements

Hi Haavard,

On Fri, 9 May 2008, Haavard Skinnemoen wrote:

diff --git a/common/cmd_spi.c b/common/cmd_spi.c index 7604422..b0e7db1 100644 --- a/common/cmd_spi.c +++ b/common/cmd_spi.c @@ -38,19 +38,13 @@ #endif

/*

• • External table of chip select functions (see the appropriate board
• • support for the actual definition of the table).
• */

-extern spi_chipsel_type spi_chipsel[]; -extern int spi_chipsel_cnt;

-/*

• Values from last command.

*/ static int device; static int bitlen; static uchar dout[MAX_SPI_BYTES]; static uchar din[MAX_SPI_BYTES]; +static struct spi_slave *slave;

Don't think this is needed...

/*

• SPI read/write

@@ -65,6 +59,7 @@ static uchar din[MAX_SPI_BYTES];

int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) {

• struct spi_slave *slave;

...because it is only ever used in this function and is shadowed by this local variable.

@@ -101,19 +96,22 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) } }

• if ((device < 0) || (device >= spi_chipsel_cnt)) {

• printf("Invalid device %d, giving up.\n", device);
  

• return 1;
  

• } if ((bitlen < 0) || (bitlen > (MAX_SPI_BYTES * 8))) { printf("Invalid bitlen %d, giving up.\n", bitlen); return 1; }

• debug ("spi_chipsel[%d] = %08X\n",

• device, (uint)spi_chipsel[device]);
  

• /* FIXME: Make these parameters configurable */
• slave = spi_setup_slave(0, device, 1000000, SPI_MODE_0);

Until it is configurable (I think, you mean at runtime), please use the CONFIG_MXC_SPI_IFACE macro, otherwise it will be useless on mx31ads.

diff --git a/drivers/rtc/mc13783-rtc.c b/drivers/rtc/mc13783-rtc.c index 35b1b8b..5a1ef4f 100644 --- a/drivers/rtc/mc13783-rtc.c +++ b/drivers/rtc/mc13783-rtc.c @@ -24,13 +24,24 @@ #include <rtc.h> #include <spi.h>

+static struct spi_slave *slave;

In do_spi() you use a local variable, allocate a slave, claim the bus, xfer data, release the bus and free the slave on each call, which is also nice. Whereas, for example, in this driver you use a static variable, allocate a slave for it once, and, naturally, never free it. This is at least inconsistent, IMHO.

@@ -65,16 +78,29 @@ void rtc_set(struct rtc_time *rtc) { u32 time, day, reg;

• if (!slave) {

• /* FIXME: Verify the max SCK rate */
  

• slave = spi_setup_slave(1, 0, 1000000,
  

• 		SPI_MODE_2 | SPI_CS_HIGH);
  

• if (!slave)
  

• 	return;
  

• }

• time = mktime(rtc->tm_year, rtc->tm_mon, rtc->tm_mday, rtc->tm_hour, rtc->tm_min, rtc->tm_sec); day = time / 86400; time %= 86400;

• if (spi_claim_bus(slave))

• return;
  

• reg = 0x2c000000 | day | 0x80000000; spi_xfer(0, 32, (uchar *)&reg, (uchar *)&day);

You changed spi_xfer()'s prototype, but didn't change all calls.

diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index b2e3ab9..8279e3e 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c

[skip]

@@ -132,15 +125,15 @@ void spi_init(void) { }

-int spi_select(unsigned int bus, unsigned int dev, unsigned long mode) +struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs,

• 	unsigned int max_hz, unsigned int mode)
  

You changed the parameter name from dev to cs

{ unsigned int ctrl_reg;

• struct spi_slave *slave;

  if (bus >= sizeof(spi_bases) / sizeof(spi_bases[0]) || dev > 3)

but you didn't change it here

• return 1;
  

• spi_base = spi_bases[bus];

• return NULL;
  
  

  ctrl_reg = MXC_CSPICTRL_CHIPSELECT(dev) | MXC_CSPICTRL_BITCOUNT(31) |

and here.

diff --git a/include/spi.h b/include/spi.h index 3a55a68..b434402 100644 --- a/include/spi.h +++ b/include/spi.h @@ -31,22 +31,77 @@ #define SPI_MODE_1 (0|SPI_CPHA) #define SPI_MODE_2 (SPI_CPOL|0) #define SPI_MODE_3 (SPI_CPOL|SPI_CPHA) -#define SPI_CS_HIGH 0x04 /* chipselect active high? */ +#define SPI_CS_HIGH 0x04 /* CS active high */ #define SPI_LSB_FIRST 0x08 /* per-word bits-on-wire */ #define SPI_3WIRE 0x10 /* SI/SO signals shared */ #define SPI_LOOP 0x20 /* loopback mode */

-/*

• • The function call pointer type used to drive the chip select.
• */

-typedef void (*spi_chipsel_type)(int cs); +/* SPI transfer flags */ +#define SPI_XFER_BEGIN 0x01 /* Assert CS before transfer */ +#define SPI_XFER_END 0x02 /* Deassert CS after transfer */

+/* Driver-specific SPI slave data */ +struct spi_slave;

Well, I am a bit upset you decided to make struct spi_slave hardware-specific. I hoped it would be standard, and contain a void * to hardware-specific part. Or, better yet, be embeddable in hardware-specific object, so drivers then would use container_of to get to their data and wouldn't need to malloc 2 structs. But, as you say, it is not an operating system, so, let's see what others say.

After all above are fixed, and I can at least compile it again:-), I'll test it on hardware.

I only reviewed the parts I'd written or changed myself.

Thanks Guennadi --- Guennadi Liakhovetski, Ph.D.

DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de