On Fri, 16 May 2008 12:41:04 +0200 (CEST) Guennadi Liakhovetski lg@denx.de wrote:
On Fri, 16 May 2008, Haavard Skinnemoen wrote:
From: Haavard Skinnemoen hskinnemoen@atmel.com
This patch gets rid of the spi_chipsel table and adds a handful of new functions that makes the SPI layer cleaner and more flexible.
Ok, this looks good to me now. And it works too. Just one question:
Great!
do { reg = 0x2c000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day1);
err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day1,SPI_XFER_BEGIN | SPI_XFER_END);if (err) return err;
reg = 0x28000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&time);
err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&time,SPI_XFER_BEGIN | SPI_XFER_END);if (err) return err;
reg = 0x2c000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day2);
err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day2,SPI_XFER_BEGIN | SPI_XFER_END);Here... We perform 3 transfers on SPI one after another, and every time we do "SPI_XFER_BEGIN | SPI_XFER_EN"... Doesn't this defeat the whole purpose of these flags? Would it be bad, if we did
Well, no, I wouldn't say it defeats the purpose of these flags...
reg = 0x2c000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day1);
err = spi_xfer(slave, 32, (uchar *)®, (uchar *)&day1,SPI_XFER_BEGIN);
if (err)return err;reg = 0x28000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&time);
err |= spi_xfer(slave, 32, (uchar *)®, (uchar *)&time, 0);
if (err)return err;reg = 0x2c000000;
err = spi_xfer(0, 32, (uchar *)®, (uchar *)&day2);
err |= spi_xfer(slave, 32, (uchar *)®, (uchar *)&day2,SPI_XFER_END);if (err) return err;
? The worst that can happen with this, is that the first or the second transfer return an error, and we go on with one or two more transfers instead of aborting immediately. Can this have any negative effects?
While I don't know the RTC in question well enough to say for sure, there's a very real possibility that your suggestion simply won't work.
The purpose of the spi_xfer flags parameter isn't to gang together multiple transfers -- it's to allow more precise chip select control so that you can split a transfer into multiple spi_xfer() calls. This is useful for devices where you typically start with a command, possibly with a few bytes of parameters, then transfer data without releasing the chip select in between.
The SPI flash driver posted later in this thread does this -- it first sends a command along with any address bytes, and then starts the actual data transfer with the buffer provided by the caller. The chip select must stay active during the whole sequence, so without this tweak, it would have to copy everything into a temporary buffer first.
If you try to combine multiple transfers into one like you did above, the chip select will stay active all the time. And many devices expect the chip select to go inactive after each command, and may simply ignore all but the first or last if it stays active.
It could be that your solution works, but I wanted to mimic the existing behaviour as closely as possible. And I definitely don't want to "optimize" chip select control without knowing the device in question very well.
reg = 0x2c000000 | day | 0x80000000;
- spi_xfer(0, 32, (uchar *)®, (uchar *)&day);
spi_xfer(slave, 32, (uchar *)®, (uchar *)&day,
SPI_XFER_BEGIN | SPI_XFER_END);reg = 0x28000000 | time | 0x80000000;
- spi_xfer(0, 32, (uchar *)®, (uchar *)&time);
- spi_xfer(slave, 32, (uchar *)®, (uchar *)&time,
SPI_XFER_BEGIN | SPI_XFER_END);- spi_release_bus(slave);
}
void rtc_reset(void)
Here error is not checked at all... So, it should be no problem doing only SPI_XFER_BEGIN in the first xfer and only SPI_XFER_END in the second one.
No, I don't think that will work, for the reasons mentioned above. There's no error checking because the existing code doesn't check for errors...and there's no way to return an error status from the function anyway.
Btw, there are a few spi_xfer() semantics that I want to be a bit more clearly defined, but I forgot to document it before sending the patch. These are: * If spi_xfer() fails, it automatically terminates the transfer as if the SPI_XFER_END flag was set. * If bitlen == 0, spi_xfer() must deactivate CS if the SPI_XFER_END flag is set. * If dout == NULL, spi_xfer() will transmit unspecified data. Alternatively, we could specify that it must send zeroes. * If din == NULL, spi_xfer() will ignore any received data.
If people agree that these semantics make sense, we should probably go through the drivers and make sure it's handled appropriately.
Oh, and I want to do something about the "bitlen" parameter, but I don't wanna open that particular can of worms in this patchset :-)
Haavard