Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
> Date: Sat, 27 Jan 2007 21:19:29 +0100 > From: Hans-Peter Nilsson <[EMAIL PROTECTED]> > Add SPI_LSB_FIRST (still supported by spi_bitbang, I presume :) Never mind, I just noticed that I can't read. brgds, H-P - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
> From: David Brownell <[EMAIL PROTECTED]> > Date: Fri, 26 Jan 2007 20:21:54 -0800 > In fact, how about this one instead? It uses a bit in spi->mode > since that's pretty easy to test. And while it doesn't get rid > of the multiple conditionals when the tx buffer is null, it does > let the other values be constants (0, ~0) that compilers like. I'm fine, except the typo. > (I can update your patch #4, etc, to match.) Or I could, and test it to boot. :) > +#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_TX_1) Add SPI_LSB_FIRST (still supported by spi_bitbang, I presume :) brgds, H-P - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Friday 26 January 2007 3:21 pm, David Brownell wrote:
> > FWIW, I defined it as a single bit in that patch, because that's
> > what my HW can do when the transmitter is disabled - and because
> > MOSI *is* a single-valued signal. ;-)
>
> I wouldn't mind a single bit flag either, saying whether to shift
> out all ones or all zeroes. The controller driver would morph it
> to something appropriate ... 0xff, 0x, etc.
In fact, how about this one instead? It uses a bit in spi->mode
since that's pretty easy to test. And while it doesn't get rid
of the multiple conditionals when the tx buffer is null, it does
let the other values be constants (0, ~0) that compilers like.
(I can update your patch #4, etc, to match.)
- Dave
Index: g26/include/linux/spi/spi.h
===
--- g26.orig/include/linux/spi/spi.h2007-01-26 15:16:26.0 -0800
+++ g26/include/linux/spi/spi.h 2007-01-26 17:07:30.0 -0800
@@ -71,6 +71,7 @@ struct spi_device {
#defineSPI_MODE_3 (SPI_CPOL|SPI_CPHA)
#defineSPI_CS_HIGH 0x04/* chipselect active
high? */
#defineSPI_LSB_FIRST 0x08/* per-word
bits-on-wire */
+#defineSPI_TX_10x10/* shift out ones on
rx-only */
u8 bits_per_word;
int irq;
void*controller_state;
@@ -296,8 +297,9 @@ extern struct spi_master *spi_busnum_to_
* the data being transferred; that may reduce overhead, when the
* underlying driver uses dma.
*
- * If the transmit buffer is null, zeroes will be shifted out
- * while filling rx_buf. If the receive buffer is null, the data
+ * If the transmit buffer is null, zeroes will be shifted out while
+ * filling rx_buf, unless SPI_TX_1 is set in spi->mode (in which case
+ * ones will be shifted out). If the receive buffer is null, the data
* shifted in will be discarded. Only "len" bytes shift out (or in).
* It's an error to try to shift out a partial word. (For example, by
* shifting out three bytes with word size of sixteen or twenty bits;
Index: g26/drivers/spi/spi_bitbang.c
===
--- g26.orig/drivers/spi/spi_bitbang.c 2007-01-26 17:29:55.0 -0800
+++ g26/drivers/spi/spi_bitbang.c 2007-01-26 17:36:17.0 -0800
@@ -73,10 +73,14 @@ static unsigned bitbang_txrx_8(
u8 *rx = t->rx_buf;
while (likely(count > 0)) {
- u8 word = 0;
+ u8 word;
if (tx)
word = *tx++;
+ else if (spi->mode & SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -99,10 +103,14 @@ static unsigned bitbang_txrx_16(
u16 *rx = t->rx_buf;
while (likely(count > 1)) {
- u16 word = 0;
+ u16 word;
if (tx)
word = *tx++;
+ else if (spi->mode & SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -125,10 +133,14 @@ static unsigned bitbang_txrx_32(
u32 *rx = t->rx_buf;
while (likely(count > 3)) {
- u32 word = 0;
+ u32 word;
if (tx)
word = *tx++;
+ else if (spi->mode & SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -176,6 +188,8 @@ int spi_bitbang_setup_transfer(struct sp
}
EXPORT_SYMBOL_GPL(spi_bitbang_setup_transfer);
+#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_TX_1)
+
/**
* spi_bitbang_setup - default setup for per-word I/O loops
*/
@@ -192,8 +206,11 @@ int spi_bitbang_setup(struct spi_device
* just bitbang_txrx_le_cphaX() routines shifting the other way, and
* some hardware controllers also have this support.
*/
- if ((spi->mode & SPI_LSB_FIRST) != 0)
+ if (spi->mode & ~MODEBITS) {
+ dev_dbg(>dev, "unsupported SPI mode bits %04x\n",
+ spi->mode & ~MODEBITS);
return -EINVAL;
+ }
if (!cs) {
cs = kzalloc(sizeof *cs, GFP_KERNEL);
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Saturday 27 January 2007 2:11 am, Hans-Peter Nilsson wrote: > > From: David Brownell <[EMAIL PROTECTED]> > > Date: Fri, 26 Jan 2007 15:21:15 -0800 > > > > > That flag could work in conjunction with a byte > > > > > > Or why not a 32-bit word! > > > > If a driver wants a repeating 32-bit pattern, they should just > > provide a properly initialized tx buffer. > > I'm suggesting the default_tx_word be able to fill out "common" > values of bits_per_word when > 8, else it'd be useless as a > 1-filler for larger sizes than byte. Making it a single bit > then wouldn't cater to your debug-with-scope use-case. That wasn't my idea, it was borrowed from someone else's debug arsenal. ;) In any case, I think that sort of debug assist doesn't need to stay in production system. See the patch I posted previously... - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
> From: David Brownell <[EMAIL PROTECTED]> > Date: Fri, 26 Jan 2007 15:21:15 -0800 > > > That flag could work in conjunction with a byte > > > > Or why not a 32-bit word! > > If a driver wants a repeating 32-bit pattern, they should just > provide a properly initialized tx buffer. I'm suggesting the default_tx_word be able to fill out "common" values of bits_per_word when > 8, else it'd be useless as a 1-filler for larger sizes than byte. Making it a single bit then wouldn't cater to your debug-with-scope use-case. brgds, H-P - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
From: David Brownell [EMAIL PROTECTED] Date: Fri, 26 Jan 2007 15:21:15 -0800 That flag could work in conjunction with a byte Or why not a 32-bit word! If a driver wants a repeating 32-bit pattern, they should just provide a properly initialized tx buffer. I'm suggesting the default_tx_word be able to fill out common values of bits_per_word when 8, else it'd be useless as a 1-filler for larger sizes than byte. Making it a single bit then wouldn't cater to your debug-with-scope use-case. brgds, H-P - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Friday 26 January 2007 3:21 pm, David Brownell wrote:
FWIW, I defined it as a single bit in that patch, because that's
what my HW can do when the transmitter is disabled - and because
MOSI *is* a single-valued signal. ;-)
I wouldn't mind a single bit flag either, saying whether to shift
out all ones or all zeroes. The controller driver would morph it
to something appropriate ... 0xff, 0x, etc.
In fact, how about this one instead? It uses a bit in spi-mode
since that's pretty easy to test. And while it doesn't get rid
of the multiple conditionals when the tx buffer is null, it does
let the other values be constants (0, ~0) that compilers like.
(I can update your patch #4, etc, to match.)
- Dave
Index: g26/include/linux/spi/spi.h
===
--- g26.orig/include/linux/spi/spi.h2007-01-26 15:16:26.0 -0800
+++ g26/include/linux/spi/spi.h 2007-01-26 17:07:30.0 -0800
@@ -71,6 +71,7 @@ struct spi_device {
#defineSPI_MODE_3 (SPI_CPOL|SPI_CPHA)
#defineSPI_CS_HIGH 0x04/* chipselect active
high? */
#defineSPI_LSB_FIRST 0x08/* per-word
bits-on-wire */
+#defineSPI_TX_10x10/* shift out ones on
rx-only */
u8 bits_per_word;
int irq;
void*controller_state;
@@ -296,8 +297,9 @@ extern struct spi_master *spi_busnum_to_
* the data being transferred; that may reduce overhead, when the
* underlying driver uses dma.
*
- * If the transmit buffer is null, zeroes will be shifted out
- * while filling rx_buf. If the receive buffer is null, the data
+ * If the transmit buffer is null, zeroes will be shifted out while
+ * filling rx_buf, unless SPI_TX_1 is set in spi-mode (in which case
+ * ones will be shifted out). If the receive buffer is null, the data
* shifted in will be discarded. Only len bytes shift out (or in).
* It's an error to try to shift out a partial word. (For example, by
* shifting out three bytes with word size of sixteen or twenty bits;
Index: g26/drivers/spi/spi_bitbang.c
===
--- g26.orig/drivers/spi/spi_bitbang.c 2007-01-26 17:29:55.0 -0800
+++ g26/drivers/spi/spi_bitbang.c 2007-01-26 17:36:17.0 -0800
@@ -73,10 +73,14 @@ static unsigned bitbang_txrx_8(
u8 *rx = t-rx_buf;
while (likely(count 0)) {
- u8 word = 0;
+ u8 word;
if (tx)
word = *tx++;
+ else if (spi-mode SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -99,10 +103,14 @@ static unsigned bitbang_txrx_16(
u16 *rx = t-rx_buf;
while (likely(count 1)) {
- u16 word = 0;
+ u16 word;
if (tx)
word = *tx++;
+ else if (spi-mode SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -125,10 +133,14 @@ static unsigned bitbang_txrx_32(
u32 *rx = t-rx_buf;
while (likely(count 3)) {
- u32 word = 0;
+ u32 word;
if (tx)
word = *tx++;
+ else if (spi-mode SPI_TX_1)
+ word = ~0;
+ else
+ word = 0;
word = txrx_word(spi, ns, word, bits);
if (rx)
*rx++ = word;
@@ -176,6 +188,8 @@ int spi_bitbang_setup_transfer(struct sp
}
EXPORT_SYMBOL_GPL(spi_bitbang_setup_transfer);
+#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_TX_1)
+
/**
* spi_bitbang_setup - default setup for per-word I/O loops
*/
@@ -192,8 +206,11 @@ int spi_bitbang_setup(struct spi_device
* just bitbang_txrx_le_cphaX() routines shifting the other way, and
* some hardware controllers also have this support.
*/
- if ((spi-mode SPI_LSB_FIRST) != 0)
+ if (spi-mode ~MODEBITS) {
+ dev_dbg(spi-dev, unsupported SPI mode bits %04x\n,
+ spi-mode ~MODEBITS);
return -EINVAL;
+ }
if (!cs) {
cs = kzalloc(sizeof *cs, GFP_KERNEL);
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Saturday 27 January 2007 2:11 am, Hans-Peter Nilsson wrote: From: David Brownell [EMAIL PROTECTED] Date: Fri, 26 Jan 2007 15:21:15 -0800 That flag could work in conjunction with a byte Or why not a 32-bit word! If a driver wants a repeating 32-bit pattern, they should just provide a properly initialized tx buffer. I'm suggesting the default_tx_word be able to fill out common values of bits_per_word when 8, else it'd be useless as a 1-filler for larger sizes than byte. Making it a single bit then wouldn't cater to your debug-with-scope use-case. That wasn't my idea, it was borrowed from someone else's debug arsenal. ;) In any case, I think that sort of debug assist doesn't need to stay in production system. See the patch I posted previously... - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
From: David Brownell [EMAIL PROTECTED] Date: Fri, 26 Jan 2007 20:21:54 -0800 In fact, how about this one instead? It uses a bit in spi-mode since that's pretty easy to test. And while it doesn't get rid of the multiple conditionals when the tx buffer is null, it does let the other values be constants (0, ~0) that compilers like. I'm fine, except the typo. (I can update your patch #4, etc, to match.) Or I could, and test it to boot. :) +#define MODEBITS (SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_TX_1) Add SPI_LSB_FIRST (still supported by spi_bitbang, I presume :) brgds, H-P - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
Date: Sat, 27 Jan 2007 21:19:29 +0100 From: Hans-Peter Nilsson [EMAIL PROTECTED] Add SPI_LSB_FIRST (still supported by spi_bitbang, I presume :) Never mind, I just noticed that I can't read. brgds, H-P - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Friday 26 January 2007 2:46 am, Hans-Peter Nilsson wrote:
> > From: David Brownell <[EMAIL PROTECTED]>
> > Date: Thu, 25 Jan 2007 05:02:56 -0800
>
> > On Wednesday 24 January 2007 8:52 pm, Hans-Peter Nilsson wrote:
> > > (Please CC me on replies, I'm not subscribed to LKML or the SPI list.
> > > Thanks.)
> > >
> > > The SD/MMC SPI-based protocol isn't really duplex. In the
> > > normal case there's either information transmitted or received,
> > > not both simultaneously. The unused data is always 0xff; ones.
> > > Unfortunately, the SPI framework leaves outgoing data for a
> > > left-out tx_buf just as "undefined"
> >
> > In current kernels that's actually changed. The value to be shifted
> > out is now specified ... as all-zeroes, which is what various chips
> > need to receive, and various (half-duplex/Microwire) controllers seem
> > to be doing in any case.
> >
> > If that's an issue -- and MMC-over-SPI needs to specify some other
> > value --
>
> Well, it *is* specified as ones (0xff).
Fair enough; I don't recall that I actually saw a spec saying that.
> > I think a better way to package this would be to define a new
> > flag for spi->mode, since controller drivers are already supposed
> > to be checking that to make sure they handle all the options which
> > have been specified.
>
> But it's unspecified what the controller drivers should do with
> flags don't recognize (should they refuse? warn? ignore? - I see
> they all ignore)
They should refuse.
> if (spi->mode & ~(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST))
> return -EINVAL;
>
> (I think I'll do exactly that with the drivers I wrote.)
Something like that, yes. It might be nice to have a KERN_DEBUG message
saying which flags were troublesome; production systems will of course
never trigger those failures, so there's no point in having a message
that's not automagically compiled-out.
> > That flag could work in conjunction with a byte
>
> Or why not a 32-bit word!
If a driver wants a repeating 32-bit pattern, they should just
provide a properly initialized tx buffer.
> FWIW, I defined it as a single bit in that patch, because that's
> what my HW can do when the transmitter is disabled - and because
> MOSI *is* a single-valued signal. ;-)
I wouldn't mind a single bit flag either, saying whether to shift
out all ones or all zeroes. The controller driver would morph it
to something appropriate ... 0xff, 0x, etc.
> While I was looking, I noticed that the memory-layout of
> non-8-bit words for SPI is a bit unclear.
The SPI_LSB_FIRST flag describes bit order on the wire.
In-memory bit ordering should be native/CPU bit order
(which may be slightly unclear). So the confusion is
not for 8, 16, or 32 bit words ... but 9, 12, 20, and
other sample sizes.
> The endianness of
> data shifted out doesn't really define endianness in memory or
> whether 24-bit words bytes are in order {hi, med, low, pad} or
> indeed {pad, low, med, hi or whatever combination. For a LE
> architecture, storing data as LE in memory makes sense, and both
> with and without padding makes sense too. Perhaps best to just
> document that it's undefined?
No, the point is to alow portable drivers. So that must be
specified. What I'm writing up is that it must be right
justified, so its the MSBs that are undefined (for RX) or
ignored (TX).
> > then please re-issue this patch against 2.6.20, including
> > the update to the bitbang driver ("reference implementation").
>
> I used patch-2.6.20-rc6; I see not too many SPI things changed
> besides defining the previously undefined as 0. I didn't find a
> reason to introduce a mode-flag; it should always be enough to
> look at the word. A flag is redundant with default_tx_word != 0
> (in 2.6.20 semantics). So, I just added that word and adjusted
> the bitbang-driver. As a courtesy, I also added simple bail-out
> code for the other drivers (not compiled and submitted
> separately).
OK.
> Here's an updated patch for 2.6.20-rc6.
Thanks, I'll eyeball it and merge some version.
- Dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/2] take 2: (was-kind-of: 3/5 SPI tx_default) 2.6.20-rc6
On Friday 26 January 2007 2:46 am, Hans-Peter Nilsson wrote:
From: David Brownell [EMAIL PROTECTED]
Date: Thu, 25 Jan 2007 05:02:56 -0800
On Wednesday 24 January 2007 8:52 pm, Hans-Peter Nilsson wrote:
(Please CC me on replies, I'm not subscribed to LKML or the SPI list.
Thanks.)
The SD/MMC SPI-based protocol isn't really duplex. In the
normal case there's either information transmitted or received,
not both simultaneously. The unused data is always 0xff; ones.
Unfortunately, the SPI framework leaves outgoing data for a
left-out tx_buf just as undefined
In current kernels that's actually changed. The value to be shifted
out is now specified ... as all-zeroes, which is what various chips
need to receive, and various (half-duplex/Microwire) controllers seem
to be doing in any case.
If that's an issue -- and MMC-over-SPI needs to specify some other
value --
Well, it *is* specified as ones (0xff).
Fair enough; I don't recall that I actually saw a spec saying that.
I think a better way to package this would be to define a new
flag for spi-mode, since controller drivers are already supposed
to be checking that to make sure they handle all the options which
have been specified.
But it's unspecified what the controller drivers should do with
flags don't recognize (should they refuse? warn? ignore? - I see
they all ignore)
They should refuse.
if (spi-mode ~(SPI_CPHA | SPI_CPOL | SPI_CS_HIGH | SPI_LSB_FIRST))
return -EINVAL;
(I think I'll do exactly that with the drivers I wrote.)
Something like that, yes. It might be nice to have a KERN_DEBUG message
saying which flags were troublesome; production systems will of course
never trigger those failures, so there's no point in having a message
that's not automagically compiled-out.
That flag could work in conjunction with a byte
Or why not a 32-bit word!
If a driver wants a repeating 32-bit pattern, they should just
provide a properly initialized tx buffer.
FWIW, I defined it as a single bit in that patch, because that's
what my HW can do when the transmitter is disabled - and because
MOSI *is* a single-valued signal. ;-)
I wouldn't mind a single bit flag either, saying whether to shift
out all ones or all zeroes. The controller driver would morph it
to something appropriate ... 0xff, 0x, etc.
While I was looking, I noticed that the memory-layout of
non-8-bit words for SPI is a bit unclear.
The SPI_LSB_FIRST flag describes bit order on the wire.
In-memory bit ordering should be native/CPU bit order
(which may be slightly unclear). So the confusion is
not for 8, 16, or 32 bit words ... but 9, 12, 20, and
other sample sizes.
The endianness of
data shifted out doesn't really define endianness in memory or
whether 24-bit words bytes are in order {hi, med, low, pad} or
indeed {pad, low, med, hi or whatever combination. For a LE
architecture, storing data as LE in memory makes sense, and both
with and without padding makes sense too. Perhaps best to just
document that it's undefined?
No, the point is to alow portable drivers. So that must be
specified. What I'm writing up is that it must be right
justified, so its the MSBs that are undefined (for RX) or
ignored (TX).
then please re-issue this patch against 2.6.20, including
the update to the bitbang driver (reference implementation).
I used patch-2.6.20-rc6; I see not too many SPI things changed
besides defining the previously undefined as 0. I didn't find a
reason to introduce a mode-flag; it should always be enough to
look at the word. A flag is redundant with default_tx_word != 0
(in 2.6.20 semantics). So, I just added that word and adjusted
the bitbang-driver. As a courtesy, I also added simple bail-out
code for the other drivers (not compiled and submitted
separately).
OK.
Here's an updated patch for 2.6.20-rc6.
Thanks, I'll eyeball it and merge some version.
- Dave
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
