Haavard Skinnemoen wrote:
> On Tue, 5 Feb 2008 23:34:46 -0500
> Mike Frysinger <[EMAIL PROTECTED]> 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
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
U-Boot-Users mailing list
U-Boot-Users@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/u-boot-users
