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

[Haavard Skinnemoen]

On Tue, 13 May 2008 17:06:35 +0200 (CEST)
Guennadi Liakhovetski <lg at 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