[PATCH 2.6.17-rc3-omap] omap_uwire byteswap bugfix

[PATCH 2.6.17-rc3-omap] omap_uwire byteswap bugfix

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

I mentioned a while back that my original omap_uwire code had some bugs
that came from a bogus optimization ... here's a patch fixing them.
(Please merge...)

The consequence of that bogus optimization is that some SPI protocol
drivers (layered on top of this driver) would get data in strange byte
orders.  Hence for example an ads7846 issue that Juha and Imre noticed,
but were trying to fix at the wrong level.  (So in a moment I'll post
a separate patch for that driver.)

Other drivers layered on top of omap_uwire may also need to revisit
how they've handled byte order.

- Dave


[-- Attachment #2: omap-uwire.patch --]
[-- Type: text/x-diff, Size: 2442 bytes --]

Get rid of broken optimization in MicroWire driver:  don't try to morph
consecutive single-byte operations into one (faster) two-byte operation.
This resolves some byteswap problems.  (And consequently allows fixing
some bugs in at least the OMAP version of the ads7846 driver...)

Also, reject LSB-first device modes; this controller doesn't support them.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Index: osk/drivers/spi/omap_uwire.c
===================================================================
--- osk.orig/drivers/spi/omap_uwire.c	2006-04-22 14:41:22.000000000 -0700
+++ osk/drivers/spi/omap_uwire.c	2006-05-02 18:37:24.000000000 -0700
@@ -196,6 +196,9 @@ static int uwire_txrx(struct spi_device 
 	if (t->tx_buf && t->rx_buf)
 		return -EPERM;
 
+	if (!bits)
+		bits = 8;
+
 	w = spi->chip_select << 10;
 	w |= CS_CMD;
 
@@ -206,18 +209,15 @@ static int uwire_txrx(struct spi_device 
 
 		/* write one or two bytes at a time */
 		while (len >= 1) {
-			/* tx is msb-aligned */
+			/* tx bit 15 is first sent; we byteswap multibyte words
+			 * (msb-first) on the way out from memory.
+			 */
 			val = *buf++;
-			if (len > 1 && (!bits || bits > 8)) {
-				if (!bits)
-					bits = 16;
+			if (bits > 8) {
 				bytes = 2;
 				val |= *buf++ << 8;
-			} else {
-				if (!bits || bits > 8)
-					bits = 8;
+			} else
 				bytes = 1;
-			}
 			val <<= 16 - bits;
 
 #ifdef	VERBOSE
@@ -253,15 +253,10 @@ static int uwire_txrx(struct spi_device 
 
 		/* read one or two bytes at a time */
 		while (len) {
-			if (len > 1 && (!bits || bits > 8)) {
-				if (!bits)
-					bits = 16;
+			if (bits > 8) {
 				bytes = 2;
-			} else {
-				if (!bits || bits > 8)
-					bits = 8;
+			} else
 				bytes = 1;
-			}
 
 			/* start read */
 			val = START | w | (bits << 0);
@@ -275,7 +270,9 @@ static int uwire_txrx(struct spi_device 
 						RDRB, 0))
 				goto eio;
 
-			/* rx is lsb-aligned */
+			/* rx bit 0 is last received; multibyte words will
+			 * be properly byteswapped on the way to memory.
+			 */
 			val = uwire_read_reg(UWIRE_RDR);
 			val &= (1 << bits) - 1;
 			*buf++ = (u8) val;
@@ -320,6 +317,12 @@ static int uwire_setup(struct spi_device
 		goto done;
 	}
 
+	if (spi->mode & SPI_LSB_FIRST) {
+		pr_debug("%s: lsb first?\n", spi->dev.bus_id);
+		status = -EINVAL;
+		goto done;
+	}
+
 	/* mode 0..3, clock inverted separately;
 	 * standard nCS signaling;
 	 * don't treat DI=high as "not ready"

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 2.6.17-rc3-omap] omap_uwire byteswap bugfix

[Paul Mundt]

On Tue, May 02, 2006 at 10:15:48PM -0700, David Brownell wrote:
> Also, reject LSB-first device modes; this controller doesn't support them.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> 
> Index: osk/drivers/spi/omap_uwire.c
> ===================================================================
> --- osk.orig/drivers/spi/omap_uwire.c	2006-04-22 14:41:22.000000000 -0700
> +++ osk/drivers/spi/omap_uwire.c	2006-05-02 18:37:24.000000000 -0700
> @@ -320,6 +317,12 @@ static int uwire_setup(struct spi_device
>  		goto done;
>  	}
>  
> +	if (spi->mode & SPI_LSB_FIRST) {
> +		pr_debug("%s: lsb first?\n", spi->dev.bus_id);
> +		status = -EINVAL;
> +		goto done;
> +	}
> +
>  	/* mode 0..3, clock inverted separately;
>  	 * standard nCS signaling;
>  	 * don't treat DI=high as "not ready"

This breaks the build, SPI_LSB_FIRST isn't defined anywhere..

drivers/spi/omap_uwire.c: In function `uwire_setup':
drivers/spi/omap_uwire.c:320: error: `SPI_LSB_FIRST' undeclared (first use in this function)
drivers/spi/omap_uwire.c:320: error: (Each undeclared identifier is reported only once
drivers/spi/omap_uwire.c:320: error: for each function it appears in.)
make[2]: *** [drivers/spi/omap_uwire.o] Error 1
make[1]: *** [drivers/spi] Error 2
make: *** [drivers] Error 2

Looks like this is missing..

	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/spi-devices-can-require-lsb-first-encodings.patch

which isn't in Linus's tree either yet however.

Re: [PATCH 2.6.17-rc3-omap] omap_uwire byteswap bugfix

[David Brownell]

On Thursday 04 May 2006 7:53 am, Paul Mundt wrote:

> > +	if (spi->mode & SPI_LSB_FIRST) {
> > +		pr_debug("%s: lsb first?\n", spi->dev.bus_id);
> > +		status = -EINVAL;
> > +		goto done;
> > +	}
> > +
> >  	/* mode 0..3, clock inverted separately;
> >  	 * standard nCS signaling;
> >  	 * don't treat DI=high as "not ready"
> 
> This breaks the build, SPI_LSB_FIRST isn't defined anywhere..
> ...
> Looks like this is missing..
> 
> 	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/spi-devices-can-require-lsb-first-encodings.patch
> 
> which isn't in Linus's tree either yet however.

Whoops, yes.  My bad, sorry; it was in _my_ tree and I overlooked how it got there.
I thought maybe Juha had merged it.

Simplest fix would be to merge that patch from Greg's tree.

- Dave

Re: [PATCH 2.6.17-rc3-omap] omap_uwire byteswap bugfix

[David Brownell]

[-- Attachment #1: Type: text/plain, Size: 783 bytes --]

On Thursday 04 May 2006 8:18 am, David Brownell wrote:
> On Thursday 04 May 2006 7:53 am, Paul Mundt wrote:
> 
> > This breaks the build, SPI_LSB_FIRST isn't defined anywhere..
> > ...
> > Looks like this is missing..
> > 
> > 	http://www.kernel.org/pub/linux/kernel/people/gregkh/gregkh-2.6/gregkh-01-driver/spi-devices-can-require-lsb-first-encodings.patch
> > 
> > which isn't in Linus's tree either yet however.
> 
> Whoops, yes.  My bad, sorry; it was in _my_ tree and I overlooked how it got there.
> I thought maybe Juha had merged it.
> 
> Simplest fix would be to merge that patch from Greg's tree.

I take that back.  Simplest fix is to remove that code fragment, since
an equivalent one -- in the spi_bitbang infrastructure -- is found in
that nyet-merged patch.

- Dave


[-- Attachment #2: omap-uwire-tweak.patch --]
[-- Type: text/x-diff, Size: 821 bytes --]

Build fix ... the LSB_FIRST option isn't yet merged, and in any
case this driver doesn't need it since the LSB_FIRST patch handles
it in the spi_bitbang infrastructure used by omap_uwire.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Index: osk/drivers/spi/omap_uwire.c
===================================================================
--- osk.orig/drivers/spi/omap_uwire.c	2006-05-03 08:03:40.000000000 -0700
+++ osk/drivers/spi/omap_uwire.c	2006-05-04 10:05:14.000000000 -0700
@@ -317,12 +317,6 @@ static int uwire_setup(struct spi_device
 		goto done;
 	}
 
-	if (spi->mode & SPI_LSB_FIRST) {
-		pr_debug("%s: lsb first?\n", spi->dev.bus_id);
-		status = -EINVAL;
-		goto done;
-	}
-
 	/* mode 0..3, clock inverted separately;
 	 * standard nCS signaling;
 	 * don't treat DI=high as "not ready"

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]