From ae60ceb8bcbb490581cffaa12141362deb65db9e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?D=C3=A1niel=20Buga?= <bugadani@gmail.com>
Date: Thu, 3 Apr 2025 12:41:27 +0200
Subject: [PATCH] Remove restrictive three-wire check (#3325)

---
 esp-hal/CHANGELOG.md                   |  1 +
 esp-hal/src/spi/master.rs              | 16 +-------
 hil-test/tests/spi_half_duplex_read.rs | 54 ++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 14 deletions(-)

diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md
index 0af5d6578..56680f8f1 100644
--- a/esp-hal/CHANGELOG.md
+++ b/esp-hal/CHANGELOG.md
@@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ESP32-C6: Keep ADC enabled to improve radio signal strength (#3249)
 - Fix off-by-one in the allowed range of the spi clock calculations (#3266)
 - Fixed an issue where inverting a pin via the interconnect matrix was ineffective (#3312)
+- The half-duplex SPI APIs should accept more valid line width combinations (#3325)
 
 ### Removed
 
diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs
index 1b6deebd5..b53cc167f 100644
--- a/esp-hal/src/spi/master.rs
+++ b/esp-hal/src/spi/master.rs
@@ -3505,19 +3505,6 @@ impl Driver {
         no_mosi_miso: bool,
         data_mode: DataMode,
     ) -> Result<(), Error> {
-        let three_wire = cmd.mode() == DataMode::Single
-            || address.mode() == DataMode::Single
-            || data_mode == DataMode::Single;
-
-        if three_wire
-            && ((cmd != Command::None && cmd.mode() != DataMode::Single)
-                || (address != Address::None && address.mode() != DataMode::Single)
-                || data_mode != DataMode::Single)
-        {
-            error!("Three-wire mode is only supported for single data line mode");
-            return Err(Error::Unsupported);
-        }
-
         self.init_spi_data_mode(cmd.mode(), address.mode(), data_mode)?;
 
         #[cfg(esp32)]
@@ -3553,7 +3540,8 @@ impl Driver {
         reg_block.user().modify(|_, w| {
             w.usr_miso_highpart().clear_bit();
             w.usr_mosi_highpart().clear_bit();
-            w.sio().bit(three_wire);
+            // This bit tells the hardware whether we use Single or SingleTwoDataLines
+            w.sio().bit(data_mode == DataMode::Single);
             w.doutdin().clear_bit();
             w.usr_miso().bit(!is_write && !no_mosi_miso);
             w.usr_mosi().bit(is_write && !no_mosi_miso);
diff --git a/hil-test/tests/spi_half_duplex_read.rs b/hil-test/tests/spi_half_duplex_read.rs
index 00847254e..e6e85e898 100644
--- a/hil-test/tests/spi_half_duplex_read.rs
+++ b/hil-test/tests/spi_half_duplex_read.rs
@@ -148,4 +148,58 @@ mod tests {
 
         assert_eq!(buffer.as_slice(), &[0xFF; DMA_BUFFER_SIZE]);
     }
+
+    #[test]
+    fn data_mode_combinations_are_not_rejected(ctx: Context) {
+        const DMA_BUFFER_SIZE: usize = 4;
+
+        let (buffer, descriptors, tx, txd) = dma_buffers!(DMA_BUFFER_SIZE, DMA_BUFFER_SIZE);
+        let dma_rx_buf = DmaRxBuf::new(descriptors, buffer).unwrap();
+        let dma_tx_buf = DmaTxBuf::new(txd, tx).unwrap();
+
+        let mut buffer = [0xAA; DMA_BUFFER_SIZE];
+        let mut spi = ctx.spi.with_buffers(dma_rx_buf, dma_tx_buf);
+
+        let modes = [
+            // 4-wire half-duplex mode
+            (Command::None, Address::None, DataMode::SingleTwoDataLines),
+            // Simple 3-wire half-duplex mode
+            (Command::None, Address::None, DataMode::Single),
+            // Simple DSPI/QSPI modes
+            (Command::None, Address::None, DataMode::Dual),
+            (Command::None, Address::None, DataMode::Quad),
+            // Half-duplex modes with command and/or address phases
+            (
+                Command::_8Bit(0x32, DataMode::Single),
+                Address::_24Bit(0x2C << 8, DataMode::Single),
+                DataMode::Single,
+            ),
+            (
+                Command::_8Bit(0x32, DataMode::Single),
+                Address::_24Bit(0x2C << 8, DataMode::Single),
+                DataMode::Dual,
+            ),
+            (
+                Command::_8Bit(0x32, DataMode::Single),
+                Address::_24Bit(0x2C << 8, DataMode::Single),
+                DataMode::Quad,
+            ),
+            // SingleTwoDataLines is not meaningful for command/address phases but supporting it
+            // shouldn't be an issue.
+            (
+                Command::_8Bit(0x32, DataMode::SingleTwoDataLines),
+                Address::_24Bit(0x2C << 8, DataMode::SingleTwoDataLines),
+                DataMode::Quad,
+            ),
+        ];
+
+        for (command, address, data) in modes {
+            if let Err(e) = spi.half_duplex_read(data, command, address, 0, &mut buffer) {
+                panic!(
+                    "Failed to read with command {:?}, address {:?}, data mode {:?}: {:?}",
+                    command, address, data, e
+                );
+            }
+        }
+    }
 }