[U-Boot-Users] [PATCH v2] spi: Kill spi_chipsel table and introduce spi_setup()

[Ben Warren]

Haavard Skinnemoen wrote:
> On Tue, 5 Feb 2008 23:34:46 -0500
> Mike Frysinger <vapier at 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