Messages in this thread | | | From | David Brownell <> | Subject | Re: [patch 2.6.28-rc9] spi: spi_write_then_read() regression fix | Date | Tue, 23 Dec 2008 10:37:53 -0800 |
| |
On Monday 22 December 2008, Ingo Oeser wrote: > Hi David, > > On Monday 22 December 2008, you wrote: > > The write_then_read() helper is > > simplifying a common half-duplex idiom for short operations, > > but the hardware still does full duplex. Buffer layout is: > > > > Before: WWWWW0000000 > > After: xxxxxRRRRRRR > > > > That is, for every bit shifted out (W, 0) another one gets > > shifted in (x, R). The I/O primitive essentially swaps > > contents of a one-word shift register between master and > > slave; or, sequences of such words. Words don't need to > > be byte-size, though that's a common option. > > ... > > Wow, what interesting hardware logic and a nice explanation.
It's standard SPI ... yes, it's kind of interesting, and I've not come across anything else that synchronizes RX and TX in quite the same way.
I suspect that hardware designers find the ability to get quick transfers (tens of megabits/second) from just a shift register is fairly attractive. Less work than high speed I2C, for one example.
> Could you put that into a comment somewhere close to those helpers?
Fair enough. That file does lack any mention of the I/O model for SPI ... and it's certainly a bit of a surprise to anyone who's not had to look at it before!
> You can safely assume, that any code which Linus doesn't understand > is non-trivial and needs a comment :-)
I'm not sure I'd agree with that -- while he's many things, "expert in everything" is not one of them! -- but I do agree that comment would be worth adding.
However, with regards to $PATCH ... considering that the '28 release is just around the corner, I changed my mind and would suggest just reverting f9b90e39cbc5c4d6ef60022fd1f25d541df0aad1 much though I very much like that fix in general.
Reason being: it needs a bit more work, I forgot that it will break for example drivers using drivers/spi/omap_uwire.c since that is built on top of truly half-duplex hardware. (Used with OMAP1 SOCs.) The kerneldoc says it's for providing MicroWire style transactions, so it's kind of significant that it work on MicroWire hardware too. ;)
So an updated version would need to both zero the TX bits when using normal full-duplex SPI hardware ... and use more or less the current code when running on half-duplex variants.
- Dave
| |