From be6165760401e8679077411c8f094458abb6eda7 Mon Sep 17 00:00:00 2001
From: Marc-Antoine Ruel <maruel@chromium.org>
Date: Sun, 4 Nov 2018 15:44:39 -0500
Subject: [PATCH] sysfs: Fix TxPackets() to behave as expected.

spi: improve documentation accordingly.

A 'fix' was implemented in a851c85cbf4c but it was not good for TxPackets(). Fix
the implementation and fix the documentation.

The next step is to fix sysfs SPI implementation to stop refusing large
transactions, and instead transparently split them up. Filed as issue #310.

Fixes #222
(again)
---
 conn/spi/spi.go   | 21 +++++++++++----------
 host/sysfs/spi.go | 23 +++++++++++++++++------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/conn/spi/spi.go b/conn/spi/spi.go
index 822251af3..b1a2159db 100644
--- a/conn/spi/spi.go
+++ b/conn/spi/spi.go
@@ -101,14 +101,9 @@ type Packet struct {
 	// completed. This can be leveraged to create long transaction as multiple
 	// packets like to use 9 bits commands then 8 bits data.
 	//
-	// Casual observation on a Rasberry Pi 3 is that two packets with
-	// KeepCS:false, there is a few µs with CS asserted after the clock stops,
-	// then 11.2µs with CS not asserted, then CS is asserted for (roughly) one
-	// clock cycle before the clock starts again for the next packet. This seems
-	// to be independent of the port clock speed but this wasn't fully verified.
-	//
-	// It cannot be expected that the driver will correctly keep CS asserted even
-	// if KeepCS:true on the last packet.
+	// Normally during a spi.Conn.TxPackets() call, KeepCS should be set to true
+	// for all packets except the last one. If the last one is set to true, the
+	// CS line stays asserted, leaving the transaction hanging on the bus.
 	//
 	// KeepCS is ignored when NoCS was specified to Connect.
 	KeepCS bool
@@ -125,8 +120,14 @@ type Conn interface {
 	// The maximum number of bytes can be limited depending on the driver. Query
 	// conn.Limits.MaxTxSize() can be used to determine the limit.
 	//
-	// If the last packet has KeepCS:true, the behavior is undefined. The CS line
-	// will likely not stay asserted. This is a driver limitation.
+	// If the last packet has KeepCS:true, the CS line stays asserted. This
+	// enables doing SPI transaction over multiple calls.
+	//
+	// Conversely, if any packet beside the last one has KeepCS:false, the CS
+	// line will blip for a short amount of time to force a new transaction.
+	//
+	// It was observed on RPi3 hardware to have a one clock delay between each
+	// packet.
 	TxPackets(p []Packet) error
 }
 
diff --git a/host/sysfs/spi.go b/host/sysfs/spi.go
index 676f8eba0..096220d1b 100644
--- a/host/sysfs/spi.go
+++ b/host/sysfs/spi.go
@@ -183,7 +183,6 @@ func newSPI(busNumber, chipSelect int) (*SPI, error) {
 			f:          f,
 			busNumber:  busNumber,
 			chipSelect: chipSelect,
-			p:          [2]spi.Packet{{KeepCS: true}, {KeepCS: true}},
 		},
 	}, nil
 }
@@ -289,9 +288,13 @@ func (s *spiConn) Tx(w, r []byte) error {
 	if s.halfDuplex && len(w) != 0 && len(r) != 0 {
 		// Create two packets for HalfDuplex operation: one write then one read.
 		s.p[0].R = nil
+		s.p[0].KeepCS = true
 		s.p[1].W = nil
 		s.p[1].R = r
+		s.p[1].KeepCS = false
 		p = s.p[:2]
+	} else {
+		s.p[0].KeepCS = false
 	}
 	if err := s.txPackets(p); err != nil {
 		return fmt.Errorf("sysfs-spi: Tx() failed: %v", err)
@@ -396,10 +399,14 @@ func (s *spiConn) txPackets(p []spi.Packet) error {
 		if bits == 0 {
 			bits = s.bitsPerWord
 		}
-		m[i].reset(p[i].W, p[i].R, f, bits)
-		if !s.noCS && !p[i].KeepCS {
-			m[i].csChange = 1
+		csInvert := false
+		if !s.noCS {
+			// Invert CS behavior when a packet has KeepCS false, except for the last
+			// packet when KeepCS is true.
+			last := i == len(p)-1
+			csInvert = p[i].KeepCS == last
 		}
+		m[i].reset(p[i].W, p[i].R, f, bits, csInvert)
 	}
 	return s.f.Ioctl(spiIOCTx(len(m)), uintptr(unsafe.Pointer(&m[0])))
 }
@@ -517,7 +524,7 @@ type spiIOCTransfer struct {
 	pad         uint16
 }
 
-func (s *spiIOCTransfer) reset(w, r []byte, f physic.Frequency, bitsPerWord uint8) {
+func (s *spiIOCTransfer) reset(w, r []byte, f physic.Frequency, bitsPerWord uint8, csInvert bool) {
 	s.tx = 0
 	s.rx = 0
 	s.length = 0
@@ -533,7 +540,11 @@ func (s *spiIOCTransfer) reset(w, r []byte, f physic.Frequency, bitsPerWord uint
 	s.speedHz = uint32((f + 500*physic.MilliHertz) / physic.Hertz)
 	s.delayUsecs = 0
 	s.bitsPerWord = bitsPerWord
-	s.csChange = 0
+	if csInvert {
+		s.csChange = 1
+	} else {
+		s.csChange = 0
+	}
 	s.txNBits = 0
 	s.rxNBits = 0
 	s.pad = 0