On Tue, 13 May 2008 13:20:22 +0200 (CEST) Guennadi Liakhovetski lg@denx.de wrote:
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...
Right.
- /* 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.
Do you really think platform-specific macros are appropriate here?
But yeah, I did mean at runtime. If we're going to support multiple busses, we need to expose that at the user interface level too.
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.
I don't think it's inconsistent...they're very different users. While this RTC driver will normally only ever see one slave, do_spi() needs to be able to communicate with whatever device the user tells it to, which may be different from one call to the next.
What I would really like here is a rtc_init() call which sets up the slave parameters once and for all. In the mean time, we'll have to either do the initialization on demand and keep it around, or do the initialization every time and take it down afterwards. I don't really have strong feelings one way or another, but I don't think consistency with the "spi" command is a very useful goal.
reg = 0x2c000000 | day | 0x80000000; spi_xfer(0, 32, (uchar *)®, (uchar *)&day);
You changed spi_xfer()'s prototype, but didn't change all calls.
Right, I'll fix it.
@@ -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.
Fixed.
-/*
- 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.
Instead of being upset, could you tell me what kind of information such a hardware-independent spi_slave struct should have, and why it might be useful outside the controller driver?
While I'm not fanatical about keeping the spi_slave layout hidden from its users, I do think this offers some potential optimizations in the controller driver (i.e. instead of storing a bus number, you can store a direct pointer to its register space.) I'm very concerned about keeping this as lightweight as possible, so unless defining a common spi_slave layout offers any actual advantages, I'd rather leave all of it up to the controller driver.
After all above are fixed, and I can at least compile it again:-), I'll test it on hardware.
With the below patch, it compiles on imx31_litekit at least.
Sorry about pushing this untested code on everyone, but I wanted to get it out early so I could get some feedback on the interface. I did _attempt_ to get it right so that you could see how it all fits together, but I did expect some breakage.
I'll do some more thorough testing during the next couple of days, but I'd like to know whether or not you all think this whole thing looks reasonable, given that the breakage gets sorted out.
Now, I guess I should start compile-testing powerpc...that's a bit problematic since practically no configurations build out of the box, and nobody seems to want the absolutely trivial fix I posted half a year ago...
I only reviewed the parts I'd written or changed myself.
Thanks for the feedback.
Haavard
diff --git a/common/cmd_spi.c b/common/cmd_spi.c index b0e7db1..15b2811 100644 --- a/common/cmd_spi.c +++ b/common/cmd_spi.c @@ -44,7 +44,6 @@ static int device; static int bitlen; static uchar dout[MAX_SPI_BYTES]; static uchar din[MAX_SPI_BYTES]; -static struct spi_slave *slave;
/* * SPI read/write @@ -111,7 +110,8 @@ int do_spi (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]) debug ("spi chipsel = %08X\n", device);
spi_claim_bus(slave); - if(spi_xfer(device, bitlen, dout, din) != 0) { + if(spi_xfer(slave, bitlen, dout, din, + SPI_XFER_BEGIN | SPI_XFER_END) != 0) { printf("Error with the SPI transaction.\n"); rcode = 1; } else { diff --git a/drivers/rtc/mc13783-rtc.c b/drivers/rtc/mc13783-rtc.c index 5a1ef4f..b6e1501 100644 --- a/drivers/rtc/mc13783-rtc.c +++ b/drivers/rtc/mc13783-rtc.c @@ -45,19 +45,22 @@ int rtc_get(struct rtc_time *rtc)
do { reg = 0x2c000000; - err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day1); + err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day1, + SPI_XFER_BEGIN | SPI_XFER_END);
if (err) return err;
reg = 0x28000000; - err = spi_xfer(0, 32, (uchar *)®, (uchar *)&time); + err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&time, + SPI_XFER_BEGIN | SPI_XFER_END);
if (err) return err;
reg = 0x2c000000; - err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day2); + err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day2, + SPI_XFER_BEGIN | SPI_XFER_END);
if (err) return err; @@ -95,10 +98,12 @@ void rtc_set(struct rtc_time *rtc) return;
reg = 0x2c000000 | day | 0x80000000; - spi_xfer(0, 32, (uchar *)®, (uchar *)&day); + spi_xfer(slave, 32, (uchar *)®, (uchar *)&day, + SPI_XFER_BEGIN | SPI_XFER_END);
reg = 0x28000000 | time | 0x80000000; - spi_xfer(0, 32, (uchar *)®, (uchar *)&time); + spi_xfer(slave, 32, (uchar *)®, (uchar *)&time, + SPI_XFER_BEGIN | SPI_XFER_END);
spi_release_bus(slave); } diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 8279e3e..435d64a 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -19,6 +19,7 @@ */
#include <common.h> +#include <malloc.h> #include <spi.h> #include <asm/io.h>
@@ -132,10 +133,10 @@ struct spi_slave *spi_setup_slave(unsigned int bus, unsigned int cs, struct spi_slave *slave;
if (bus >= sizeof(spi_bases) / sizeof(spi_bases[0]) || - dev > 3) + cs > 3) return NULL;
- ctrl_reg = MXC_CSPICTRL_CHIPSELECT(dev) | + ctrl_reg = MXC_CSPICTRL_CHIPSELECT(cs) | MXC_CSPICTRL_BITCOUNT(31) | MXC_CSPICTRL_DATARATE(7) | /* FIXME: calculate data rate */ MXC_CSPICTRL_EN |