On Tue, 13 May 2008 17:06:35 +0200 (CEST) Guennadi Liakhovetski lg@denx.de wrote:
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.
Ok, so how about we introduce
CONFIG_DEFAULT_SPI_IFACE CONFIG_DEFAULT_SPI_MODE
so that other platforms can do the same thing too?
As for run-time configurability...here's one suggestion: * Allow optional prefix "<bus>:" before the chip select number. Use board-specific default if not specified * Add optional parameter specifying the mode at the end. This can be parsed as a 32-bit hexadecimal number so you can specify pretty much anything, but normally you'd just specify 0, 1, 2 or 3, indicating one of the standard SPI modes.
Or perhaps we should use environment variables instead...?
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:-)
We already have things like spi_xfer() which is implemented differently depending on the SPI driver being used.
However, I think we might want a common spi_slave struct for a different reason as well: so that we can pass it to the board hooks -- spi_activate_cs(), spi_deactivate_cs() and spi_cs_is_valid(). We probably need to add a "bus" parameter to the former two anyway, and by passing them a struct, we can easily add new parameters later without having to change prototypes all over the tree...
So I guess I'm in favour of your second suggestion -- define a common struct spi_slave which can be embedded into a controller-specific struct.
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 */
Great. I have a few other fixes that I had to make in order for other boards to compile. I've also been doing some testing with DataFlash, and it seems to work fine on my hardware too, at least so far...
Haavard