Haavard Skinnemoen wrote:
On Tue, 5 Feb 2008 23:34:46 -0500 Mike Frysinger vapier@gentoo.org wrote:
so the new SPI interface has this API:
- void spi_init(void);
- int spi_setup(int cs, unsigned int max_hz, unsigned int mode);
- int spi_xfer(int cs, int bitlen, uchar *dout, uchar *din);
- int spi_cs_is_valid(int cs);
- void spi_cs_activate(int cs);
- void spi_cs_deactivate(int cs);
Yes, or at least that's the current API + my proposed patch.
there isnt a function to pair up with spi_setup() ? for example, the normal communication flow with a SPI flash:
- spi_setup - turn on SPI
- spi_cs_activate - assert CS
- spi_xfer -
- op code (read/write/erase)
- address
- actual block data
- spi_cs_deactivate - deassert CS
- ??? - turn off SPI
Right. I thought of spi_setup() more as a function that needs to be called one time per slave to set up communications parameters, not really for turning the SPI on as such.
But perhaps it would make sense to combine those two functions. How about we turn it into
/* 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);
The claim/release naming also makes it clear that the SPI device driver has exclusive access to the bus between those two calls.
If there really is a need to turn off the controller, or change the transfer rate on the fly, then this is good. OTOH, this is a bootloader, not an OS, and probably the vast majority of use cases would just be to initialize the controller to a speed that all devices can handle, transfer some data to/from one or more devices, then boot an OS. Maybe some people need to do more, I don't know.
you dont want to have the deactivate func to turn off SPI in case you need to toggle the CS during communication ... some SPI peripherals have undefined (floating) behavior with pins when it is actually turned off which is bad mojo ...
Sure, I didn't mean to suggest that spi_cs_deactivate() should turn off the whole SPI controller.
Btw, the master driver is currently controlling the chip selects from spi_xfer(). I suspect we need to give clients greater control of the chip selects eventually.
Decoupling chip select from spi_xfer() is a good idea, since spi_xfer() is all about sending and receiving streams of bits from the master point of view and is slave-agnostic. We may want to add a wrapper function so that the user doesn't have to remember too much. Something like:
int spi_send_receive(int cs, int bitlen, char *inbuf, char *outbuf) {
spi_cs_activate(cs);
spi_xfer(bitlen, inbuf, outbuf);
spi_cs_deactivate(cs);
}
yeah, yeah, should handle return codes too...
also, what's the deal with spi_xfer() taking a length in # of bits ? is it realistic to transmit anything less tan 8 bits ? the Linux kernel driver does not support this, so it cant be a big need ...
I don't know. That's unchanged from the original API. But I certainly wouldn't object if we turned it into a length in bytes.
I seem to remember working with a Broadcom device where some of the transfers were odd numbers of nibbles (e.g. 12 bits). Not necessarily a reason to keep bit granularity, but I don't see a reason to artificially limit things either.
nice work, Ben