On Tue, 13 May 2008, Haavard Skinnemoen wrote:
On Tue, 13 May 2008 13:20:22 +0200 (CEST) Guennadi Liakhovetski lg@denx.de wrote:
- /* 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.
Appropriate or not from the esthetic PoV, I don't see another chance to make it useful - either make it run-time configurable either via command parameters, or environment varables, ot at least compile-time, so platforms could specify something meaningful there. BTW, same holds for the flags. So, I'd do
#ifndef CONFIG_MXC_SPI_IFACE #define CONFIG_MXC_SPI_IFACE 0 #endif #ifndef CONFIG_MXC_SPI_MODE #define CONFIG_MXC_SPI_MODE SPI_MODE_0 #endif
slave = spi_setup_slave(CONFIG_MXC_SPI_IFACE, device, 1000000, CONFIG_MXC_SPI_MODE);
Without these sspi is 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.
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.
Ok, sorry, you're right.
-/*
- 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?
Well, I just don't like different things being called with the same name:-)
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.
and it works too - rtc works, sspi works with above modifications and setting
#define CONFIG_MXC_SPI_MODE (SPI_MODE_2 | SPI_CS_HIGH) /* Default SPI mode */
in mx31ads.h
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