[U-Boot-Users] [PATCH v2 1/3] New i.MX31 SPI driver

[Haavard Skinnemoen]

Guennadi Liakhovetski <lg at denx.de> wrote:
> On Thu, 8 May 2008, Haavard Skinnemoen wrote:
> > Therefore, I'm going to remove it in the next version of my patchset.
> > If you can tell me how it's supposed to work, I can try to minimize the
> > breakage.
> 
> Would be better if we could avoid any breakage completely, please.

That's certainly what I'm aiming for, but I need someone to help me
test it.

> I added this function, because 1) the current spi_xfer() doesn't support 
> multiple SPI busses, and 2) is not particularly friendly when the SPI 
> controller itself controls chipselects. As far as I can see your new 
> proposed API doesn't solve the former of these problems either. One could 
> get around this problem by numbering all chipselects on all busses 
> through, but that would be too ugly.

I don't think global chip select numbering would be _that_ ugly, but
yeah, maybe we should add a bus parameter as well.

> So, the spi_select does just that - 
> selects a bus and a device to talk to. Of course this is racy, but as long 
> as there's no multitasking, it should be ok.

Yeah, multitasking isn't an issue.

> As you certainly noticed while working on your API improvements, the 
> current API is very unflexible. But as I didn't have resources to rework 
> it completely and change multiple existing drivers, I chose the lesser 
> evil - added an auxiliary function.

Lesser evil compared to what exactly? Than participating in the
discussion about the new API?

> Wouldn't an API like
> 
> struct spi_slave *spi_attach(int bus, int cs); /* also does init */
> int spi_xfer(struct spi_slave *slave, bitlen, dout, din);
> void spi_detach(struct spi_slave *slave);
> 
> (approximately) be better?

I did propose something similar a while ago:

/* Set slave-specific parameters and enable SPI */
int spi_claim_bus(int cs, unsigned int max_hz, unsigned int mode);

/* Disable SPI */
void spi_release_bus(int cs);

But I have to say I like the idea about passing a spi_slave
"handle" around...

How about something like this?

/*
 * Set up communication parameters for a SPI slave. This doesn't
 * necessarily enable the controller.
 */
struct spi_slave *spi_create_slave(int bus, int cs, unsigned int
		max_hz, unsigned int mode);

/*
 * Get exclusive access to the SPI bus for communicating with the given
 * slave. Returns a negative value if the bus is busy (drivers may omit
 * such checks if they don't want the extra code/data overhead.)
 */
int spi_claim_bus(struct spi_slave *slave);

/*
 * Release the SPI bus. This may disable the controller and/or put it
 * in low-power mode
 */
void spi_release_bus(struct spi_slave *slave);

/*
 * Transfer data on the SPI bus.
 *
 * flags can be a combination of any of the following bits:
 *   SPI_XFER_BEGIN: Assert CS before starting the transfer
 *   SPI_XFER_END: Deassert CS after the transfer
 */
int spi_xfer(struct spi_slave *slave, int bitlen, const void *dout,
		void *din, unsigned long flags);

What do you think?

Haavard