[PATCH 0/3] spi: bitbang fixes and passive serial driver

[PATCH 0/3] spi: bitbang fixes and passive serial driver

[Michael Grzeschik]

This series includes a bugfix and adds LSB mode for the bitbang driver.
It also includes a new character device driver to passively clock a
firmware into an FPGA.

Michael Grzeschik (3):
  spi: bitbang: fix shift for getmosi
  spi: bitbang: add lsb first support
  spi: psdev: add passive serial driver

 drivers/spi/Kconfig            |   9 +
 drivers/spi/Makefile           |   1 +
 drivers/spi/spi-bitbang-txrx.h |  98 +++++---
 drivers/spi/spi-bitbang.c      |   3 +-
 drivers/spi/spipsdev.c         | 520 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 601 insertions(+), 30 deletions(-)
 create mode 100644 drivers/spi/spipsdev.c

-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] spi: bitbang: fix shift for getmosi

[Michael Grzeschik]

The driver needs to shift the word bit after reading the mosi bit.
Otherwise the return word will have an Off-by-one bit value.

Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/spi/spi-bitbang-txrx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..b6e348d 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on leading edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
 		setsck(spi, cpol);
+		word <<= 1;
 	}
 	return word;
 }
@@ -89,9 +89,9 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on trailing edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
+		word <<= 1;
 	}
 	return word;
 }
-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: bitbang: fix shift for getmosi

[Gerhard Sittig]

On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:
> 
> The driver needs to shift the word bit after reading the mosi bit.
> Otherwise the return word will have an Off-by-one bit value.

The MISO gets read (is incoming), MOSI is outgoing.

> 
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

Is the Cc: to stable@ appropriate without Fixes: or a version
that's supposed to be affected?  Has a bug been verified and are
you certain that the fix is correct?

> ---
>  drivers/spi/spi-bitbang-txrx.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> index c616e41..b6e348d 100644
> --- a/drivers/spi/spi-bitbang-txrx.h
> +++ b/drivers/spi/spi-bitbang-txrx.h
> @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  		spidelay(nsecs);
>  
>  		/* sample MSB (from slave) on leading edge */
> -		word <<= 1;
>  		if ((flags & SPI_MASTER_NO_RX) == 0)
>  			word |= getmiso(spi);
>  		setsck(spi, cpol);
> +		word <<= 1;
>  	}
>  	return word;
>  }

This seems wrong.  You might have observed an off by one error.
But this code change is suspicious.

Shifting first and ORing then makes the sample end up in the
lowest bit.  That's expected.

ORing the sample and shifting afterwards actually would be
creating an off by one error. :-O


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] spi: bitbang: fix shift for getmosi

[Mark Brown]

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

On Wed, Mar 12, 2014 at 10:23:34PM +0100, Gerhard Sittig wrote:
> On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:

> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

> Is the Cc: to stable@ appropriate without Fixes: or a version
> that's supposed to be affected?  Has a bug been verified and are
> you certain that the fix is correct?

Like I said in my reply I'm somewhat dubious that this is actually an
issue but if it is then it's been present since at least 2.6.35 (when
the header file was split out of the driver).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/3] spi: bitbang: add lsb first support

[Michael Grzeschik]

The bitbang spi driver currently only supports the MSB mode. This patch
adds the possibility to clock the data in LSB mode.

Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/spi/spi-bitbang-txrx.h | 98 +++++++++++++++++++++++++++++-------------
 drivers/spi/spi-bitbang.c      |  3 +-
 2 files changed, 71 insertions(+), 30 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index b6e348d..7f9c020 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -49,22 +49,42 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
 
-	/* clock starts at inactive polarity */
-	for (word <<= (32 - bits); likely(bits); bits--) {
-
-		/* setup MSB (to slave) on trailing edge */
-		if ((flags & SPI_MASTER_NO_TX) == 0)
-			setmosi(spi, word & (1 << 31));
-		spidelay(nsecs);	/* T(setup) */
-
-		setsck(spi, !cpol);
-		spidelay(nsecs);
-
-		/* sample MSB (from slave) on leading edge */
-		if ((flags & SPI_MASTER_NO_RX) == 0)
-			word |= getmiso(spi);
-		setsck(spi, cpol);
-		word <<= 1;
+	if (spi->mode & SPI_LSB_FIRST) {
+		/* clock starts at inactive polarity */
+		for (; likely(bits); bits--) {
+
+			/* setup MSB (to slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & 1);
+			spidelay(nsecs);	/* T(setup) */
+
+			setsck(spi, !cpol);
+			spidelay(nsecs);
+
+			/* sample LSB (from slave) on leading edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			setsck(spi, cpol);
+			word >>= 1;
+		}
+	} else {
+		/* clock starts at inactive polarity */
+		for (word <<= (32 - bits); likely(bits); bits--) {
+
+			/* setup MSB (to slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & (1 << 31));
+			spidelay(nsecs);        /* T(setup) */
+
+			setsck(spi, !cpol);
+			spidelay(nsecs);
+
+			/* sample MSB (from slave) on leading edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			setsck(spi, cpol);
+			word <<= 1;
+		}
 	}
 	return word;
 }
@@ -76,22 +96,42 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 {
 	/* if (cpol == 0) this is SPI_MODE_1; else this is SPI_MODE_3 */
 
-	/* clock starts at inactive polarity */
-	for (word <<= (32 - bits); likely(bits); bits--) {
+	if (spi->mode & SPI_LSB_FIRST) {
+		/* clock starts at inactive polarity */
+		for (; likely(bits); bits--) {
+
+			/* setup MSB (to slave) on leading edge */
+			setsck(spi, !cpol);
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & 1);
+			spidelay(nsecs); /* T(setup) */
+
+			setsck(spi, cpol);
+			spidelay(nsecs);
+
+			/* sample MSB (from slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			word >>= 1;
+		}
+	} else {
+		/* clock starts at inactive polarity */
+		for (word <<= (32 - bits); likely(bits); bits--) {
 
-		/* setup MSB (to slave) on leading edge */
-		setsck(spi, !cpol);
-		if ((flags & SPI_MASTER_NO_TX) == 0)
-			setmosi(spi, word & (1 << 31));
-		spidelay(nsecs); /* T(setup) */
+			/* setup MSB (to slave) on leading edge */
+			setsck(spi, !cpol);
+			if ((flags & SPI_MASTER_NO_TX) == 0)
+				setmosi(spi, word & (1 << 31));
+			spidelay(nsecs); /* T(setup) */
 
-		setsck(spi, cpol);
-		spidelay(nsecs);
+			setsck(spi, cpol);
+			spidelay(nsecs);
 
-		/* sample MSB (from slave) on trailing edge */
-		if ((flags & SPI_MASTER_NO_RX) == 0)
-			word |= getmiso(spi);
-		word <<= 1;
+			/* sample MSB (from slave) on trailing edge */
+			if ((flags & SPI_MASTER_NO_RX) == 0)
+				word |= getmiso(spi);
+			word <<= 1;
+		}
 	}
 	return word;
 }
diff --git a/drivers/spi/spi-bitbang.c b/drivers/spi/spi-bitbang.c
index bd222f6..3624f96 100644
--- a/drivers/spi/spi-bitbang.c
+++ b/drivers/spi/spi-bitbang.c
@@ -432,7 +432,8 @@ int spi_bitbang_start(struct spi_bitbang *bitbang)
 	spin_lock_init(&bitbang->lock);
 
 	if (!master->mode_bits)
-		master->mode_bits = SPI_CPOL | SPI_CPHA | bitbang->flags;
+		master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_LSB_FIRST
+			| bitbang->flags;
 
 	if (master->transfer || master->transfer_one_message)
 		return -EINVAL;
-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: bitbang: add lsb first support

[Gerhard Sittig]

On Wed, Mar 12, 2014 at 16:20 +0100, Michael Grzeschik wrote:
> 
> The bitbang spi driver currently only supports the MSB mode. This patch
> adds the possibility to clock the data in LSB mode.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
>  drivers/spi/spi-bitbang-txrx.h | 98 +++++++++++++++++++++++++++++-------------
>  drivers/spi/spi-bitbang.c      |  3 +-
>  2 files changed, 71 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
> index b6e348d..7f9c020 100644
> --- a/drivers/spi/spi-bitbang-txrx.h
> +++ b/drivers/spi/spi-bitbang-txrx.h
> @@ -49,22 +49,42 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  {
>  	/* if (cpol == 0) this is SPI_MODE_0; else this is SPI_MODE_2 */
>  
> -	/* clock starts at inactive polarity */
> -	for (word <<= (32 - bits); likely(bits); bits--) {
> -
> -		/* setup MSB (to slave) on trailing edge */
> -		if ((flags & SPI_MASTER_NO_TX) == 0)
> -			setmosi(spi, word & (1 << 31));
> -		spidelay(nsecs);	/* T(setup) */
> -
> -		setsck(spi, !cpol);
> -		spidelay(nsecs);
> -
> -		/* sample MSB (from slave) on leading edge */
> -		if ((flags & SPI_MASTER_NO_RX) == 0)
> -			word |= getmiso(spi);
> -		setsck(spi, cpol);
> -		word <<= 1;
> +	if (spi->mode & SPI_LSB_FIRST) {
> +		/* clock starts at inactive polarity */
> +		for (; likely(bits); bits--) {
> +
> +			/* setup MSB (to slave) on trailing edge */
> +			if ((flags & SPI_MASTER_NO_TX) == 0)
> +				setmosi(spi, word & 1);
> +			spidelay(nsecs);	/* T(setup) */
> +
> +			setsck(spi, !cpol);
> +			spidelay(nsecs);
> +
> +			/* sample LSB (from slave) on leading edge */
> +			if ((flags & SPI_MASTER_NO_RX) == 0)
> +				word |= getmiso(spi);
> +			setsck(spi, cpol);
> +			word >>= 1;
> +		}
> +	} else {
> +		/* clock starts at inactive polarity */
> +		for (word <<= (32 - bits); likely(bits); bits--) {
> +
> +			/* setup MSB (to slave) on trailing edge */
> +			if ((flags & SPI_MASTER_NO_TX) == 0)
> +				setmosi(spi, word & (1 << 31));
> +			spidelay(nsecs);        /* T(setup) */
> +
> +			setsck(spi, !cpol);
> +			spidelay(nsecs);
> +
> +			/* sample MSB (from slave) on leading edge */
> +			if ((flags & SPI_MASTER_NO_RX) == 0)
> +				word |= getmiso(spi);
> +			setsck(spi, cpol);
> +			word <<= 1;
> +		}
>  	}
>  	return word;
>  }

Would it be useful to not duplicate the transmission logic, but
instead to optionally "bit swap" the TX and RX data before and
after the common transmission logic?  Just a though whether this
might help maintenance.  There might even be existing and proven
code to bit swap integers?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: bitbang: add lsb first support

[Mark Brown]

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

On Wed, Mar 12, 2014 at 10:29:41PM +0100, Gerhard Sittig wrote:

> Would it be useful to not duplicate the transmission logic, but
> instead to optionally "bit swap" the TX and RX data before and
> after the common transmission logic?  Just a though whether this
> might help maintenance.  There might even be existing and proven
> code to bit swap integers?

I was wondering myself, but on the other hand that's an extra operation
and this is about as CPU intensive as one can get.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] spi: bitbang: add lsb first support

[Gerhard Sittig]

On Wed, Mar 12, 2014 at 21:35 +0000, Mark Brown wrote:
> 
> On Wed, Mar 12, 2014 at 10:29:41PM +0100, Gerhard Sittig wrote:
> 
> > Would it be useful to not duplicate the transmission logic, but
> > instead to optionally "bit swap" the TX and RX data before and
> > after the common transmission logic?  Just a though whether this
> > might help maintenance.  There might even be existing and proven
> > code to bit swap integers?
> 
> I was wondering myself, but on the other hand that's an extra operation
> and this is about as CPU intensive as one can get.

Actually I thought that bitbanging pins including the timing
would be the most expensive part, and that swapping bits might
not be negligable but could be acceptable in comparison.  But
this was just a guess, no research was done.


http://graphics.stanford.edu/~seander/bithacks.html#BitReverseObvious
and the following sections suggest that code might get less
expensive when you accept to no longer see at first glance what's
happening.  Given a good identifier and an appropriate comment,
http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel
might be acceptable for mainline.

Naively reading the code, you would expect some 15 assembly
instructions and only two registers in use (for ARM).  A quick
experiment shows that the 32bit constants add some more work, yet
the complete swapping is done in 88 bytes of code (a sequence of
19 instructions and three constants).  Other architectures weight
in at some 120 bytes of code.

How do some 50 additional CPU instructions sound (calling the bit
swapper two times), in comparison to the whole SPI bitbanging
loop, for the optional case of LSB first transfers and 32bit
data?  In contrast to duplicating the core bitbanging routine's
body (two times for differing SPI modes).


BTW are these bitbang routines referenced from several sites, not
only spi-gpio.c.  They encode "be" in their name which might
become misleading then they operate in LSB mode as well.

And spidelay() appears to sometimes be a NOP -- could this be the
reason for unreliable data transmission and bit errors in certain
environments (optimized code, inlined GPIO access, fast CPU, slow
SPI slave), instead of the suspected receiver's bit shifting?


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] spi: bitbang: add lsb first support

[Mark Brown]

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

On Thu, Mar 13, 2014 at 07:11:36PM +0100, Gerhard Sittig wrote:
> On Wed, Mar 12, 2014 at 21:35 +0000, Mark Brown wrote:

> > I was wondering myself, but on the other hand that's an extra operation
> > and this is about as CPU intensive as one can get.

> Actually I thought that bitbanging pins including the timing
> would be the most expensive part, and that swapping bits might
> not be negligable but could be acceptable in comparison.  But
> this was just a guess, no research was done.

I've not done any research here either really.  My thinking here is
based on the fact that everything other than the delays is essentially
overhead on those delays that stretches the time taken to perform the
overall operation.

> BTW are these bitbang routines referenced from several sites, not
> only spi-gpio.c.  They encode "be" in their name which might
> become misleading then they operate in LSB mode as well.

Indeed.

> And spidelay() appears to sometimes be a NOP -- could this be the
> reason for unreliable data transmission and bit errors in certain
> environments (optimized code, inlined GPIO access, fast CPU, slow
> SPI slave), instead of the suspected receiver's bit shifting?

That's a very interesting observation - I hadn't noticed that this was
the case.  That code is pretty old, modern processors are a lot faster
and it does seem entirely possible that we're able to bash data out
quickly enough to cause problems for some systems, especially if the
timing is uneven which I'd not be surprised by.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] spi: bitbang: add lsb first support

[Geert Uytterhoeven]

Hi Gerhard,

On Thu, Mar 13, 2014 at 7:11 PM, Gerhard Sittig <gsi-ynQEQJNshbs@public.gmane.org> wrote:
> http://graphics.stanford.edu/~seander/bithacks.html#BitReverseObvious
> and the following sections suggest that code might get less
> expensive when you accept to no longer see at first glance what's
> happening.  Given a good identifier and an appropriate comment,
> http://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel
> might be acceptable for mainline.

I had faint memories of seeing this algorithm in the Linux kernel sources,
but I can't seem to find it anymore.

We do have a different implementation in lib/bitrev.c.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] spi: psdev: add passive serial driver

[Michael Grzeschik]

This patch introduces the psdev driver. It is used to communicate to an
Altera passive serial connected FPGA by writing into an character
device.

It simply initates the protocol on open and clocks the data into the
FPGA by using the underlying SPI bus on write. The used SPI bus can also
be an bitbang SPI bus. On close it will check for the response of the
FPGA and tells if the write process was successful.

Signed-off-by: Michael Grzeschik <m.grzeschik-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---
 drivers/spi/Kconfig    |   9 +
 drivers/spi/Makefile   |   1 +
 drivers/spi/spipsdev.c | 520 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 530 insertions(+)
 create mode 100644 drivers/spi/spipsdev.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 581ee2a..ab6d438 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -208,6 +208,15 @@ config SPI_GPIO
 	  GPIO operations, you should be able to leverage that for better
 	  speed with a custom version of this driver; see the source code.
 
+config SPI_PASSIVE_SERIAL
+	tristate "GPIO-based bitbanging SPI Altera Passive Serial"
+	depends on GPIOLIB
+	select SPI_BITBANG
+	help
+	  This driver uses GPIOs to communicate by alteras passive serial
+	  protocol. It creates an character device, that can be used to
+	  simply cat RBF Firmware images into the FPGA.
+
 config SPI_IMX
 	tristate "Freescale i.MX SPI controllers"
 	depends on ARCH_MXC || COMPILE_TEST
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 95af48d..296b1dd 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -8,6 +8,7 @@ ccflags-$(CONFIG_SPI_DEBUG) := -DDEBUG
 # config declarations into driver model code
 obj-$(CONFIG_SPI_MASTER)		+= spi.o
 obj-$(CONFIG_SPI_SPIDEV)		+= spidev.o
+obj-$(CONFIG_SPI_PASSIVE_SERIAL)	+= spipsdev.o
 
 # SPI master controller drivers (bus)
 obj-$(CONFIG_SPI_ALTERA)		+= spi-altera.o
diff --git a/drivers/spi/spipsdev.c b/drivers/spi/spipsdev.c
new file mode 100644
index 0000000..bda89b2
--- /dev/null
+++ b/drivers/spi/spipsdev.c
@@ -0,0 +1,520 @@
+/*
+ * Passive Serial Programming Driver
+ *
+ * Copyright (C) 2014 Michael Grzeschik <mgr-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/ioctl.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/compat.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_device.h>
+
+#include <linux/spi/spi.h>
+
+#include <linux/uaccess.h>
+
+#define N_PS_MINORS		32
+
+unsigned int major;
+static DECLARE_BITMAP(minors, N_PS_MINORS);
+
+struct psdev_data {
+	dev_t			devt;
+	spinlock_t		spi_lock;
+	struct spi_device	*spi;
+	struct list_head	device_entry;
+	bool			open;
+
+	struct mutex		buf_lock;
+	u8			*buffer;
+
+	/* gpios for ps protocol */
+	unsigned		nconfig_gpio;
+	unsigned		confd_gpio;
+	unsigned		nstat_gpio;
+};
+
+static LIST_HEAD(device_list);
+static DEFINE_MUTEX(minor_lock);
+
+static unsigned bufsiz = 4096;
+module_param(bufsiz, uint, S_IRUGO);
+MODULE_PARM_DESC(bufsiz, "data bytes in biggest supported SPI message");
+
+/*
+ * We can't use the standard synchronous wrappers for file I/O; we
+ * need to protect against async removal of the underlying spi_device.
+ */
+static void psdev_complete(void *arg)
+{
+	if (arg)
+		complete(arg);
+}
+
+static ssize_t psdev_sync(struct psdev_data *psdev, struct spi_message *message)
+{
+	DECLARE_COMPLETION_ONSTACK(done);
+	int status;
+
+	message->complete = psdev_complete;
+	message->context = &done;
+
+	spin_lock_irq(&psdev->spi_lock);
+	if (psdev->spi == NULL)
+		status = -ESHUTDOWN;
+	else
+		status = spi_async(psdev->spi, message);
+	spin_unlock_irq(&psdev->spi_lock);
+
+	if (status == 0) {
+		unsigned long time_left = msecs_to_jiffies(100);
+
+		time_left = wait_for_completion_timeout(&done, time_left);
+		if (!time_left) {
+			message->context = NULL;
+			return -ETIMEDOUT;
+		}
+		status = message->status;
+		if (status == 0)
+			status = message->actual_length;
+	}
+	return status;
+}
+
+static inline ssize_t psdev_sync_write(struct psdev_data *psdev, size_t len)
+{
+	struct spi_transfer t = {
+		.tx_buf		= psdev->buffer,
+		.len		= len,
+	};
+	struct spi_message m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	return psdev_sync(psdev, &m);
+}
+
+/* Write-only message with current device setup */
+static ssize_t psdev_write(struct file *filp, const char __user *buf,
+		size_t count, loff_t *f_pos)
+{
+	struct psdev_data *psdev;
+	ssize_t	status = 0;
+	unsigned long missing;
+
+	if (count > bufsiz)
+		return -EMSGSIZE;
+
+	psdev = filp->private_data;
+
+	mutex_lock(&psdev->buf_lock);
+	missing = copy_from_user(psdev->buffer, buf, count);
+	if (missing == 0)
+		status = psdev_sync_write(psdev, count);
+	else
+		status = -EFAULT;
+	mutex_unlock(&psdev->buf_lock);
+
+	return status;
+}
+
+static int psdev_start(struct psdev_data *psdev)
+{
+	struct device *dev = &psdev->spi->dev;
+	unsigned long timeout;
+	int ret = -ENXIO;
+
+	dev_dbg(dev, "Initiating programming\n");
+
+	/* initiate an FPGA programming */
+	gpio_set_value(psdev->nconfig_gpio, 0);
+
+	/*
+	 * after about 2 µs the FPGA must acknowledge with
+	 * STATUS and CONFIG DONE lines at low level
+	 */
+	timeout = jiffies + usecs_to_jiffies(2);
+	do {
+		if ((gpio_get_value(psdev->nstat_gpio) == 0) &&
+		    (gpio_get_value(psdev->confd_gpio) == 0)) {
+			ret = 0;
+			break;
+		}
+		ndelay(100);
+
+	} while (time_before(jiffies, timeout));
+
+	if (ret != 0) {
+		dev_err(dev, "FPGA does not acknowledge the programming initiation\n");
+		if (gpio_get_value(psdev->nstat_gpio))
+			dev_err(dev, "STATUS is still high!\n");
+		if (gpio_get_value(psdev->confd_gpio))
+			dev_err(dev, "CONFIG DONE is still high!\n");
+
+		return ret;
+	}
+
+	/* arm the FPGA to await its new firmware */
+	gpio_set_value(psdev->nconfig_gpio, 1);
+
+	ret = -ENXIO;
+	timeout = jiffies + usecs_to_jiffies(1600);
+	do {
+		if (gpio_get_value(psdev->nstat_gpio) == 0) {
+			ret = 0;
+			break;
+		}
+		udelay(10);
+
+	} while (time_before(jiffies, timeout));
+
+	if (ret != 0) {
+		dev_err(dev, "FPGA does not acknowledge the programming start\n");
+		return ret;
+	}
+
+	dev_dbg(dev, "Initiating passed\n");
+
+	/* at the end, wait at least 2 µs prior beginning writing data */
+	udelay(2);
+
+	return 0;
+}
+
+static int psdev_open(struct inode *inode, struct file *filp)
+{
+	struct psdev_data *psdev, *tmp;
+	struct device *dev;
+	int status = -ENXIO;
+
+	list_for_each_entry_safe(psdev, tmp, &device_list, device_entry) {
+		if (psdev->devt == inode->i_rdev) {
+			status = 0;
+			break;
+		}
+	}
+
+	if (psdev->open) {
+		status = -EBUSY;
+		goto out;
+	}
+
+	dev = &psdev->spi->dev;
+
+	if (status == 0) {
+		if (!psdev->buffer) {
+			psdev->buffer = kmalloc(bufsiz, GFP_KERNEL);
+			if (!psdev->buffer) {
+				dev_dbg(dev, "open/ENOMEM\n");
+				status = -ENOMEM;
+			}
+		}
+		if (status == 0) {
+			filp->private_data = psdev;
+			nonseekable_open(inode, filp);
+		}
+	} else
+		pr_debug("psdev: nothing for minor %d\n", iminor(inode));
+
+	psdev_start(psdev);
+
+	psdev->open = 1;
+
+ out:
+
+	return status;
+}
+
+static int psdev_release(struct inode *inode, struct file *filp)
+{
+	struct psdev_data *psdev;
+	unsigned long timeout;
+	struct device *dev;
+	int status = -ENXIO;
+
+	psdev = filp->private_data;
+	filp->private_data = NULL;
+
+	dev = &psdev->spi->dev;
+
+	/*
+	 * when programming was successfully,
+	 * both status lines should be at high level
+	 */
+	timeout = jiffies + usecs_to_jiffies(1000);
+	do {
+		if ((gpio_get_value(psdev->nstat_gpio)) &&
+		    (gpio_get_value(psdev->confd_gpio))) {
+			status = 0;
+			break;
+		}
+		udelay(10);
+
+	} while (time_before(jiffies, timeout));
+
+	if (status != 0) {
+		dev_err(dev, "FPGA does not acknowledge the programming initiation\n");
+		if (gpio_get_value(psdev->nstat_gpio) == 0)
+			dev_err(dev, "STATUS is still low!\n");
+		if (gpio_get_value(psdev->confd_gpio) == 0)
+			dev_err(dev, "CONFIG DONE is still low!\n");
+	}
+
+	kfree(psdev->buffer);
+	psdev->buffer = NULL;
+
+	psdev->open = 0;
+
+	return status;
+}
+
+static const struct file_operations psdev_fops = {
+	.owner =	THIS_MODULE,
+	.write =	psdev_write,
+	.open =		psdev_open,
+	.release =	psdev_release,
+	.llseek =	no_llseek,
+};
+
+static struct class *psdev_class;
+
+/*-------------------------------------------------------------------------*/
+#ifdef CONFIG_OF
+
+static const struct of_device_id psdev_dt_ids[] = {
+	{ .compatible = "altr,passive-serial" },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, psdev_dt_ids);
+
+static int psdev_probe_dt(struct psdev_data *psdev)
+{
+	int ret = 0;
+	struct device *dev = &psdev->spi->dev;
+	struct device_node *np = dev->of_node;
+	const struct of_device_id *of_id =
+			of_match_device(psdev_dt_ids, &psdev->spi->dev);
+
+	if (!of_id)
+		return -EINVAL;
+
+	ret = of_get_named_gpio(np, "gpio-nstatus", 0);
+	if (ret < 0) {
+		dev_err(dev, "gpio-nstatus property not found\n");
+		goto error;
+	}
+	psdev->nstat_gpio = ret;
+
+	ret = of_get_named_gpio(np, "gpio-confdone", 0);
+	if (ret < 0) {
+		dev_err(dev, "gpio-confdone property not found\n");
+		goto error;
+	}
+	psdev->confd_gpio = ret;
+
+	ret = of_get_named_gpio(np, "gpio-nconfig", 0);
+	if (ret < 0) {
+		dev_err(dev, "gpio-nconfig property not found\n");
+		goto error;
+	}
+	psdev->nconfig_gpio = ret;
+
+	ret = devm_gpio_request_one(dev, psdev->nconfig_gpio,
+			GPIOF_OUT_INIT_HIGH, "nconfig");
+	if (ret) {
+		dev_err(dev, "unable to get nconfig gpio\n");
+		goto error;
+	}
+
+	ret = devm_gpio_request_one(dev, psdev->nstat_gpio,
+			GPIOF_DIR_IN,  "nstat");
+	if (ret) {
+		dev_err(dev, "unable to get nstat gpio\n");
+		goto error;
+	}
+
+	ret = devm_gpio_request_one(dev, psdev->confd_gpio,
+			GPIOF_DIR_IN, "confd");
+	if (ret) {
+		dev_err(dev, "unable to get confd gpio\n");
+		goto error;
+	}
+
+ error:
+	return ret;
+}
+#else
+static inline int spi_gpio_probe_dt(struct platform_device *pdev)
+{
+	return -EINVAL;
+}
+#endif
+
+static int psdev_probe(struct spi_device *spi)
+{
+	struct psdev_data *psdev;
+	unsigned long minor;
+	int status = 0;
+
+	/* Allocate driver data */
+	psdev = devm_kzalloc(&spi->dev, sizeof(*psdev), GFP_KERNEL);
+	if (!psdev)
+		return -ENOMEM;
+
+	/* Initialize the driver data */
+	psdev->spi = spi;
+	spin_lock_init(&psdev->spi_lock);
+	mutex_init(&psdev->buf_lock);
+
+	INIT_LIST_HEAD(&psdev->device_entry);
+	/* If we can allocate a minor number, hook up this device.
+	 * Reusing minors is fine so long as udev or mdev is working.
+	 */
+	mutex_lock(&minor_lock);
+	minor = find_first_zero_bit(minors, N_PS_MINORS);
+	if (minor < N_PS_MINORS) {
+		struct device *dev;
+
+		psdev->devt = MKDEV(major, minor);
+		dev = device_create(psdev_class, &spi->dev, psdev->devt,
+				    psdev, "psdev%lu", minor);
+		status = PTR_ERR_OR_ZERO(dev);
+		if (status) {
+			mutex_unlock(&minor_lock);
+			return -ENODEV;
+		}
+	} else {
+		mutex_unlock(&minor_lock);
+		dev_dbg(&spi->dev, "no minor number available!\n");
+		return -ENODEV;
+	}
+	set_bit(minor, minors);
+	mutex_unlock(&minor_lock);
+
+	list_add(&psdev->device_entry, &device_list);
+
+	spi->mode |= SPI_LSB_FIRST | SPI_NO_CS;
+	spi_setup(spi);
+
+	status = psdev_probe_dt(psdev);
+	if (status)
+		goto error;
+
+	spi_set_drvdata(spi, psdev);
+
+	return status;
+
+ error:
+	mutex_lock(&minor_lock);
+	clear_bit(MINOR(psdev->devt), minors);
+	mutex_unlock(&minor_lock);
+
+	list_del(&psdev->device_entry);
+	device_destroy(psdev_class, psdev->devt);
+
+	return status;
+}
+
+static int psdev_remove(struct spi_device *spi)
+{
+	struct psdev_data *psdev = spi_get_drvdata(spi);
+
+	/* make sure ops on existing fds can abort cleanly */
+	spin_lock_irq(&psdev->spi_lock);
+	psdev->spi = NULL;
+	spin_unlock_irq(&psdev->spi_lock);
+
+	/* prevent new opens */
+	list_del(&psdev->device_entry);
+	device_destroy(psdev_class, psdev->devt);
+	mutex_lock(&minor_lock);
+	clear_bit(MINOR(psdev->devt), minors);
+	mutex_unlock(&minor_lock);
+
+	return 0;
+}
+
+static struct spi_driver psdev_spi_driver = {
+	.driver = {
+		.name =		"psdev",
+		.owner =	THIS_MODULE,
+		.of_match_table = of_match_ptr(psdev_dt_ids),
+	},
+	.probe =	psdev_probe,
+	.remove =	psdev_remove,
+
+	/* NOTE:  suspend/resume methods are not necessary here.
+	 * We don't do anything except pass the requests to/from
+	 * the underlying controller.  The refrigerator handles
+	 * most issues; the controller driver handles the rest.
+	 */
+};
+
+/*-------------------------------------------------------------------------*/
+
+static int __init psdev_init(void)
+{
+	int status;
+
+	/* Claim our 256 reserved device numbers.  Then register a class
+	 * that will key udev/mdev to add/remove /dev nodes.  Last, register
+	 * the driver which manages those device numbers.
+	 */
+	BUILD_BUG_ON(N_PS_MINORS > 256);
+	status = register_chrdev(0, "psdev", &psdev_fops);
+	if (status < 0)
+		return status;
+
+	major = status;
+
+	psdev_class = class_create(THIS_MODULE, "psdev");
+	if (IS_ERR(psdev_class)) {
+		unregister_chrdev(major, psdev_spi_driver.driver.name);
+		return PTR_ERR(psdev_class);
+	}
+
+	status = spi_register_driver(&psdev_spi_driver);
+	if (status < 0) {
+		class_destroy(psdev_class);
+		unregister_chrdev(major, psdev_spi_driver.driver.name);
+	}
+	return status;
+}
+module_init(psdev_init);
+
+static void __exit psdev_exit(void)
+{
+	spi_unregister_driver(&psdev_spi_driver);
+	class_destroy(psdev_class);
+	unregister_chrdev(major, psdev_spi_driver.driver.name);
+}
+module_exit(psdev_exit);
+
+MODULE_AUTHOR("Michael Grzeschik, <mgrzeschik-hi6Y0CQ0nG0@public.gmane.org>");
+MODULE_DESCRIPTION("User mode SPI Passive Serial interface");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("spi:psdev");
-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/3] spi: bitbang fixes and passive serial driver

[Michael Grzeschik]

On Wed, Mar 12, 2014 at 04:20:33PM +0100, Michael Grzeschik wrote:
> This series includes a bugfix and adds LSB mode for the bitbang driver.
> It also includes a new character device driver to passively clock a
> firmware into an FPGA.
> 
> Michael Grzeschik (3):
>   spi: bitbang: fix shift for getmosi
>   spi: bitbang: add lsb first support
>   spi: psdev: add passive serial driver
> 
>  drivers/spi/Kconfig            |   9 +
>  drivers/spi/Makefile           |   1 +
>  drivers/spi/spi-bitbang-txrx.h |  98 +++++---
>  drivers/spi/spi-bitbang.c      |   3 +-
>  drivers/spi/spipsdev.c         | 520 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 601 insertions(+), 30 deletions(-)
>  create mode 100644 drivers/spi/spipsdev.c
> 
> -- 
> 1.9.0

Got the linux-arm-kernel list wrong. Will resend.

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/3] spi: bitbang fixes and passive serial driver

[Michael Grzeschik]

This series includes a bugfix and adds LSB mode for the bitbang driver.
It also includes a new character device driver to passively clock a
firmware into an FPGA.

It is an resend after the cc to linux-arm-kernel list got fixed.

Michael Grzeschik (3):
  spi: bitbang: fix shift for getmosi
  spi: bitbang: add lsb first support
  spi: psdev: add passive serial driver

 drivers/spi/Kconfig            |   9 +
 drivers/spi/Makefile           |   1 +
 drivers/spi/spi-bitbang-txrx.h |  98 +++++---
 drivers/spi/spi-bitbang.c      |   3 +-
 drivers/spi/spipsdev.c         | 520 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 601 insertions(+), 30 deletions(-)
 create mode 100644 drivers/spi/spipsdev.c

-- 
1.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] spi: bitbang: fix shift for getmosi

[Michael Grzeschik]

The driver needs to shift the word bit after reading the mosi bit.
Otherwise the return word will have an Off-by-one bit value.

Cc: <stable@vger.kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/spi/spi-bitbang-txrx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..b6e348d 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on leading edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
 		setsck(spi, cpol);
+		word <<= 1;
 	}
 	return word;
 }
@@ -89,9 +89,9 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on trailing edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
+		word <<= 1;
 	}
 	return word;
 }
-- 
1.9.0

[PATCH 1/3] spi: bitbang: fix shift for getmosi

[Michael Grzeschik]

The driver needs to shift the word bit after reading the mosi bit.
Otherwise the return word will have an Off-by-one bit value.

Cc: <stable@vger.kernel.org>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
 drivers/spi/spi-bitbang-txrx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bitbang-txrx.h b/drivers/spi/spi-bitbang-txrx.h
index c616e41..b6e348d 100644
--- a/drivers/spi/spi-bitbang-txrx.h
+++ b/drivers/spi/spi-bitbang-txrx.h
@@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on leading edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
 		setsck(spi, cpol);
+		word <<= 1;
 	}
 	return word;
 }
@@ -89,9 +89,9 @@ bitbang_txrx_be_cpha1(struct spi_device *spi,
 		spidelay(nsecs);
 
 		/* sample MSB (from slave) on trailing edge */
-		word <<= 1;
 		if ((flags & SPI_MASTER_NO_RX) == 0)
 			word |= getmiso(spi);
+		word <<= 1;
 	}
 	return word;
 }
-- 
1.9.0

Re: [PATCH 1/3] spi: bitbang: fix shift for getmosi

[Mark Brown]

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

On Wed, Mar 12, 2014 at 04:53:35PM +0100, Michael Grzeschik wrote:
> The driver needs to shift the word bit after reading the mosi bit.
> Otherwise the return word will have an Off-by-one bit value.

This isn't exactly new code...  do we understand why nobody has noticed
this before?

> @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  		spidelay(nsecs);
>  
>  		/* sample MSB (from slave) on leading edge */
> -		word <<= 1;
>  		if ((flags & SPI_MASTER_NO_RX) == 0)
>  			word |= getmiso(spi);
>  		setsck(spi, cpol);
> +		word <<= 1;
>  	}

Just looking at the context here it's not obvious to me that this is
helping - it means that the last bit we read is going to be shifted
which seems wrong, we ought to be reading into LSB.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 1/3] spi: bitbang: fix shift for getmosi

[Mark Brown]

On Wed, Mar 12, 2014 at 04:53:35PM +0100, Michael Grzeschik wrote:
> The driver needs to shift the word bit after reading the mosi bit.
> Otherwise the return word will have an Off-by-one bit value.

This isn't exactly new code...  do we understand why nobody has noticed
this before?

> @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
>  		spidelay(nsecs);
>  
>  		/* sample MSB (from slave) on leading edge */
> -		word <<= 1;
>  		if ((flags & SPI_MASTER_NO_RX) == 0)
>  			word |= getmiso(spi);
>  		setsck(spi, cpol);
> +		word <<= 1;
>  	}

Just looking at the context here it's not obvious to me that this is
helping - it means that the last bit we read is going to be shifted
which seems wrong, we ought to be reading into LSB.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140312/7bbc3eaf/attachment.sig>

Re: [PATCH 1/3] spi: bitbang: fix shift for getmosi

[Gerhard Sittig]

On Wed, Mar 12, 2014 at 16:24 +0000, Mark Brown wrote:
> 
> On Wed, Mar 12, 2014 at 04:53:35PM +0100, Michael Grzeschik wrote:
> > The driver needs to shift the word bit after reading the mosi bit.
> > Otherwise the return word will have an Off-by-one bit value.
> 
> This isn't exactly new code...  do we understand why nobody has noticed
> this before?

I too suspect that the bug (if there is any) is elsewhere.

> > @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
> >  		spidelay(nsecs);
> >  
> >  		/* sample MSB (from slave) on leading edge */
> > -		word <<= 1;
> >  		if ((flags & SPI_MASTER_NO_RX) == 0)
> >  			word |= getmiso(spi);
> >  		setsck(spi, cpol);
> > +		word <<= 1;
> >  	}
> 
> Just looking at the context here it's not obvious to me that this is
> helping - it means that the last bit we read is going to be shifted
> which seems wrong, we ought to be reading into LSB.

It might be more robust to 

  if (!(flags & SPI_MASTER_NO_RX) && getmiso(spi))
    word |= 1;
    
instead.  This decouples the construction of the received bits
buffer from whatever the getmiso() implementation might look
like.  That's just a thought after the recent GPIO discussion
about whether 1/0 is given or should not be assumed (and I still
suspect that "!!x" need not result in exactly 1/0 values).

And I agree with Mark that the "late shift" is most probably
wrong.  Just noticed the resend too late and responded in the
other thread.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office-ynQEQJNshbs@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/3] spi: bitbang: fix shift for getmosi

[Gerhard Sittig]

On Wed, Mar 12, 2014 at 16:24 +0000, Mark Brown wrote:
> 
> On Wed, Mar 12, 2014 at 04:53:35PM +0100, Michael Grzeschik wrote:
> > The driver needs to shift the word bit after reading the mosi bit.
> > Otherwise the return word will have an Off-by-one bit value.
> 
> This isn't exactly new code...  do we understand why nobody has noticed
> this before?

I too suspect that the bug (if there is any) is elsewhere.

> > @@ -61,10 +61,10 @@ bitbang_txrx_be_cpha0(struct spi_device *spi,
> >  		spidelay(nsecs);
> >  
> >  		/* sample MSB (from slave) on leading edge */
> > -		word <<= 1;
> >  		if ((flags & SPI_MASTER_NO_RX) == 0)
> >  			word |= getmiso(spi);
> >  		setsck(spi, cpol);
> > +		word <<= 1;
> >  	}
> 
> Just looking at the context here it's not obvious to me that this is
> helping - it means that the last bit we read is going to be shifted
> which seems wrong, we ought to be reading into LSB.

It might be more robust to 

  if (!(flags & SPI_MASTER_NO_RX) && getmiso(spi))
    word |= 1;
    
instead.  This decouples the construction of the received bits
buffer from whatever the getmiso() implementation might look
like.  That's just a thought after the recent GPIO discussion
about whether 1/0 is given or should not be assumed (and I still
suspect that "!!x" need not result in exactly 1/0 values).

And I agree with Mark that the "late shift" is most probably
wrong.  Just noticed the resend too late and responded in the
other thread.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de