Guennadi Liakhovetski lg@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