SPI: fix: avoid locks with 8/16 bit transfers

When SPI_HOLD_ON_CS is set, Zephyr holds CS down until the 'spi_release'
API is called, which is the behavior expected by Arduino users. However,
this requires SPI_LOCK_ON to also be set, and that uses the pointer to
'spi_config' structure to identify the lock holder.

Using two different structures for 8-bit and 16-bit transfers causes the
lock to not work as intended, causing the sketch to hang. Rework code to
only use a single 'spi_config' structure and update the 'operation'
field as needed.

Signed-off-by: Luca Burelli <[email protected]>

Author: pillo79

libraries/SPI/SPI.cpp

@@ -13,28 +13,26 @@ arduino::ZephyrSPI::ZephyrSPI(const struct device *spi) : spi_dev(spi) {
 
 uint8_t arduino::ZephyrSPI::transfer(uint8_t data) {
 	uint8_t rx = data;
-	if (transfer(&rx, sizeof(rx), &config) < 0) {
+	if (transfer(&rx, sizeof(rx), SPI_WORD_SET(8)) < 0) {
 		return 0;
 	}
 	return rx;
 }
 
 uint16_t arduino::ZephyrSPI::transfer16(uint16_t data) {
 	uint16_t rx = data;
-	if (transfer(&rx, sizeof(rx), &config16) < 0) {
+	if (transfer(&rx, sizeof(rx), SPI_WORD_SET(16)) < 0) {
 		return 0;
 	}
 	return rx;
 }
 
 void arduino::ZephyrSPI::transfer(void *buf, size_t count) {
-	int ret = transfer(buf, count, &config);
+	int ret = transfer(buf, count, SPI_WORD_SET(8));
 	(void)ret;
 }
 
-int arduino::ZephyrSPI::transfer(void *buf, size_t len, const struct spi_config *config) {
-	int ret;
-
+int arduino::ZephyrSPI::transfer(void *buf, size_t len, uint32_t flags) {
 	const struct spi_buf tx_buf = {.buf = buf, .len = len};
 	const struct spi_buf_set tx_buf_set = {
 		.buffers = &tx_buf,
@@ -47,7 +45,8 @@ int arduino::ZephyrSPI::transfer(void *buf, size_t len, const struct spi_config
 		.count = 1,
 	};
 
-	return spi_transceive(spi_dev, config, &tx_buf_set, &rx_buf_set);
+	config.operation = mode | flags;
+	return spi_transceive(spi_dev, &config, &tx_buf_set, &rx_buf_set);
 }
 
 void arduino::ZephyrSPI::usingInterrupt(int interruptNumber) {
@@ -57,7 +56,7 @@ void arduino::ZephyrSPI::notUsingInterrupt(int interruptNumber) {
 }
 
 void arduino::ZephyrSPI::beginTransaction(SPISettings settings) {
-	uint32_t mode = SPI_HOLD_ON_CS;
+	mode = SPI_HOLD_ON_CS;
 
 	// Set bus mode
 	switch (settings.getBusMode()) {
@@ -93,15 +92,9 @@ void arduino::ZephyrSPI::beginTransaction(SPISettings settings) {
 		break;
 	}
 
-	// Set SPI configuration structure for 8-bit transfers
+	// Set SPI configuration structure except for operation
 	memset(&config, 0, sizeof(struct spi_config));
-	config.operation = mode | SPI_WORD_SET(8);
 	config.frequency = max(SPI_MIN_CLOCK_FREQUENCY, settings.getClockFreq());
-
-	// Set SPI configuration structure for 16-bit transfers
-	memset(&config16, 0, sizeof(struct spi_config));
-	config16.operation = mode | SPI_WORD_SET(16);
-	config16.frequency = max(SPI_MIN_CLOCK_FREQUENCY, settings.getClockFreq());
 }
 
 void arduino::ZephyrSPI::endTransaction(void) {

libraries/SPI/SPI.h

@@ -59,12 +59,12 @@ class ZephyrSPI : public HardwareSPI {
 	virtual void end();
 
 private:
-	int transfer(void *buf, size_t len, const struct spi_config *config);
+	int transfer(void *buf, size_t len, uint32_t flags);
 
 protected:
 	const struct device *spi_dev;
 	struct spi_config config;
-	struct spi_config config16;
+	uint32_t mode;
 	int interrupt[INTERRUPT_COUNT];
 	size_t interrupt_pos = 0;
 };