[U-Boot-Users] [RFC/PATCH] SPI API improvements
Guennadi Liakhovetski
lg at denx.de
Tue May 13 13:20:22 CEST 2008
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 *)®, (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 at denx.de
More information about the U-Boot
mailing list