Refactored I2S GPIO to use GpioPinSpec

Devices/lilygo-tdeck/lilygo,tdeck.dts

@@ -36,9 +36,9 @@
 	i2s0 {
 		compatible = "espressif,esp32-i2s";
 		port = <I2S_NUM_0>;
-		pin-bclk = <7>;
+		pin-bclk = <&gpio0 7 GPIO_FLAG_NONE>;
-		pin-ws = <5>;
+		pin-ws = <&gpio0 5 GPIO_FLAG_NONE>;
-		pin-data-out = <6>;
+		pin-data-out = <&gpio0 6 GPIO_FLAG_NONE>;
 	};
 
 	spi0 {

Platforms/PlatformEsp32/Bindings/espressif,esp32-i2s.yaml

@@ -12,23 +12,22 @@ properties:
       The port number, defined by i2s_port_t.
       Depending on the hardware, these values are available: I2S_NUM_0, I2S_NUM_1
   pin-bclk:
-    type: int
+    type: phandle-array
     required: true
-    description: BCK pin
+    description: Bit clock pin
   pin-ws:
-    type: int
+    type: phandle-array
     required: true
-    description: WS pin
+    description: Word (slot) select pin
   pin-data-out:
-    type: int
+    type: phandle-array
-    default: GPIO_PIN_NONE
+    default: GPIO_PIN_SPEC_NONE
-    description: DATA OUT pin
+    description: Data output pin
   pin-data-in:
-    type: int
+    type: phandle-array
-    default: GPIO_PIN_NONE
+    default: GPIO_PIN_SPEC_NONE
-    description: DATA IN pin
+    description: Data input pin
   pin-mclk:
-    type: int
+    type: phandle-array
-    required: false
+    default: GPIO_PIN_SPEC_NONE
-    default: GPIO_PIN_NONE
+    description: Master clock pin
-    description: MCLK pin

Platforms/PlatformEsp32/Include/tactility/bindings/esp32_i2s.h

@@ -3,6 +3,7 @@
 
 #include <tactility/bindings/bindings.h>
 #include <tactility/drivers/esp32_i2s.h>
+#include <tactility/drivers/esp32_gpio.h>
 #include <driver/i2s_common.h>
 
 #ifdef __cplusplus

Platforms/PlatformEsp32/Include/tactility/drivers/esp32_i2s.h

@@ -11,11 +11,11 @@ extern "C" {
 
 struct Esp32I2sConfig {
     i2s_port_t port;
-    gpio_pin_t pin_bclk;
+    struct GpioPinSpec pin_bclk;
-    gpio_pin_t pin_ws;
+    struct GpioPinSpec pin_ws;
-    gpio_pin_t pin_data_out;
+    struct GpioPinSpec pin_data_out;
-    gpio_pin_t pin_data_in;
+    struct GpioPinSpec pin_data_in;
-    gpio_pin_t pin_mclk;
+    struct GpioPinSpec pin_mclk;
 };
 
 #ifdef __cplusplus

Platforms/PlatformEsp32/Source/drivers/esp32_gpio.cpp

@@ -92,7 +92,7 @@ static error_t get_flags(GpioDescriptor* descriptor, gpio_flags_t* flags) {
 }
 
 static error_t get_native_pin_number(GpioDescriptor* descriptor, void* pin_number) {
-    auto* esp_pin_number = static_cast<gpio_num_t*>(pin_number);
+    auto* esp_pin_number = reinterpret_cast<gpio_num_t*>(pin_number);
     *esp_pin_number = static_cast<gpio_num_t>(descriptor->pin);
     return ERROR_NONE;
 }

Platforms/PlatformEsp32/Source/drivers/esp32_i2s.cpp

@@ -3,6 +3,8 @@
 #include <driver/i2s_common.h>
 
 #include <tactility/driver.h>
+#include <tactility/drivers/gpio_controller.h>
+#include <tactility/drivers/gpio_descriptor.h>
 #include <tactility/drivers/i2s_controller.h>
 #include <tactility/log.h>
 
@@ -14,23 +16,106 @@
 
 #define TAG "esp32_i2s"
 
+static void release_pin(GpioDescriptor** gpio_descriptor) {
+    if (*gpio_descriptor == nullptr) return;
+    check(gpio_descriptor_release(*gpio_descriptor) == ERROR_NONE);
+    *gpio_descriptor = nullptr;
+}
+
+static bool acquire_pin_or_set_null(const GpioPinSpec& pin_spec, GpioDescriptor** gpio_descriptor) {
+    if (pin_spec.gpio_controller == nullptr) {
+        *gpio_descriptor = nullptr;
+        return true;
+    }
+    *gpio_descriptor = gpio_descriptor_acquire(pin_spec.gpio_controller, pin_spec.pin, GPIO_OWNER_GPIO);
+    if (*gpio_descriptor == nullptr) {
+        LOG_E(TAG, "Failed to acquire pin %u", pin_spec.pin);
+    }
+
+    return *gpio_descriptor != nullptr;
+}
+
+/**
+ * Safely acquire the native pin avalue.
+ * Set to GPIO_NUM_NC if the descriptor is null.
+ * @param[in] descriptor Pin descriptor to acquire
+ * @return Native pin number
+ */
+static gpio_num_t get_native_pin(GpioDescriptor* descriptor) {
+    if (descriptor != nullptr) {
+        gpio_num_t pin;
+        check(gpio_descriptor_get_native_pin_number(descriptor, &pin) == ERROR_NONE);
+        return pin;
+    } else {
+        return GPIO_NUM_NC;
+    }
+}
+
+/**
+ * Returns true if the given pin is inverted
+ * @param[in] descriptor Pin descriptor to check, nullable
+ */
+static bool is_pin_inverted(GpioDescriptor* descriptor) {
+    if (!descriptor) return false;
+    gpio_flags_t flags;
+    check(gpio_descriptor_get_flags(descriptor, &flags) == ERROR_NONE);
+    return (flags & GPIO_FLAG_ACTIVE_LOW) != 0;
+}
+
 struct Esp32I2sInternal {
     Mutex mutex {};
-    i2s_chan_handle_t tx_handle = nullptr;
-    i2s_chan_handle_t rx_handle = nullptr;
     I2sConfig config {};
     bool config_set = false;
+    GpioDescriptor* bclk_descriptor = nullptr;
+    GpioDescriptor* ws_descriptor = nullptr;
+    GpioDescriptor* data_out_descriptor = nullptr;
+    GpioDescriptor* data_in_descriptor = nullptr;
+    GpioDescriptor* mclk_descriptor = nullptr;
+    i2s_chan_handle_t tx_handle = nullptr;
+    i2s_chan_handle_t rx_handle = nullptr;
 
     Esp32I2sInternal() {
         mutex_construct(&mutex);
     }
 
     ~Esp32I2sInternal() {
+        cleanup_pins();
         mutex_destruct(&mutex);
     }
+
+    void cleanup_pins() {
+        release_pin(&bclk_descriptor);
+        release_pin(&ws_descriptor);
+        release_pin(&data_out_descriptor);
+        release_pin(&data_in_descriptor);
+        release_pin(&mclk_descriptor);
+    }
+
+    bool init_pins(Esp32I2sConfig* dts_config) {
+        check (!ws_descriptor && !bclk_descriptor && !data_out_descriptor && !data_in_descriptor && !mclk_descriptor);
+        auto& ws_spec = dts_config->pin_ws;
+        auto& bclk_spec = dts_config->pin_bclk;
+        auto& data_in_spec = dts_config->pin_data_in;
+        auto& data_out_spec = dts_config->pin_data_out;
+        auto& mclk_spec = dts_config->pin_mclk;
+
+        bool success = acquire_pin_or_set_null(ws_spec, &ws_descriptor) &&
+            acquire_pin_or_set_null(bclk_spec, &bclk_descriptor) &&
+            acquire_pin_or_set_null(data_in_spec, &data_in_descriptor) &&
+            acquire_pin_or_set_null(data_out_spec, &data_out_descriptor) &&
+            acquire_pin_or_set_null(mclk_spec, &mclk_descriptor);
+
+        if (!success) {
+            cleanup_pins();
+            LOG_E(TAG, "Failed to acquire all pins");
+            return false;
+        }
+
+        return true;
+    }
 };
 
-#define GET_CONFIG(device) ((Esp32I2sConfig*)device->config)
+#define GET_CONFIG(device) ((Esp32I2sConfig*)(device)->config)
 #define GET_DATA(device) ((Esp32I2sInternal*)device_get_driver_data(device))
 
 #define lock(data) mutex_lock(&data->mutex);
@@ -63,7 +148,7 @@ static i2s_data_bit_width_t to_esp32_bits_per_sample(uint8_t bits) {
     }
 }
 
-static void get_esp32_std_config(const I2sConfig* config, const Esp32I2sConfig* dts_config, i2s_std_config_t* std_cfg) {
+static void get_esp32_std_config(Esp32I2sInternal* internal, const I2sConfig* config, i2s_std_config_t* std_cfg) {
     std_cfg->clk_cfg = I2S_STD_CLK_DEFAULT_CONFIG(config->sample_rate);
     std_cfg->slot_cfg = I2S_STD_PHILIPS_SLOT_DEFAULT_CONFIG(to_esp32_bits_per_sample(config->bits_per_sample), I2S_SLOT_MODE_STEREO);
 
@@ -81,16 +166,28 @@ static void get_esp32_std_config(const I2sConfig* config, const Esp32I2sConfig*
         std_cfg->slot_cfg.slot_mask = I2S_STD_SLOT_BOTH;
     }
 
+    gpio_num_t mclk_pin = get_native_pin(internal->mclk_descriptor);
+    gpio_num_t bclk_pin = get_native_pin(internal->bclk_descriptor);
+    gpio_num_t ws_pin = get_native_pin(internal->ws_descriptor);
+    gpio_num_t data_out_pin = get_native_pin(internal->data_out_descriptor);
+    gpio_num_t data_in_pin = get_native_pin(internal->data_in_descriptor);
+    LOG_I(TAG, "Configuring I2S pins: MCLK=%d, BCLK=%d, WS=%d, DATA_OUT=%d, DATA_IN=%d", mclk_pin, bclk_pin, ws_pin, data_out_pin, data_in_pin);
+
+    bool mclk_inverted = is_pin_inverted(internal->mclk_descriptor);
+    bool bclk_inverted = is_pin_inverted(internal->bclk_descriptor);
+    bool ws_inverted = is_pin_inverted(internal->ws_descriptor);
+    LOG_I(TAG, "Inverted pins: MCLK=%u, BCLK=%u, WS=%u", mclk_inverted, bclk_inverted, ws_inverted);
+
     std_cfg->gpio_cfg = {
-        .mclk = (gpio_num_t)dts_config->pin_mclk,
+        .mclk = mclk_pin,
-        .bclk = (gpio_num_t)dts_config->pin_bclk,
+        .bclk = bclk_pin,
-        .ws = (gpio_num_t)dts_config->pin_ws,
+        .ws = ws_pin,
-        .dout = (gpio_num_t)dts_config->pin_data_out,
+        .dout = data_out_pin,
-        .din = (gpio_num_t)dts_config->pin_data_in,
+        .din = data_in_pin,
         .invert_flags = {
-            .mclk_inv = false,
+            .mclk_inv = mclk_inverted,
-            .bclk_inv = false,
+            .bclk_inv = bclk_inverted,
-            .ws_inv = false
+            .ws_inv = ws_inverted
         }
     };
 }
@@ -129,46 +226,46 @@ static error_t set_config(Device* device, const struct I2sConfig* config) {
         return ERROR_INVALID_ARGUMENT;
     }
 
-    auto* driver_data = GET_DATA(device);
+    auto* internal = GET_DATA(device);
     auto* dts_config = GET_CONFIG(device);
-    lock(driver_data);
+    lock(internal);
 
-    cleanup_channel_handles(driver_data);
+    cleanup_channel_handles(internal);
-    driver_data->config_set = false;
+    internal->config_set = false;
 
     // Create new channel handles
     i2s_chan_config_t chan_cfg = I2S_CHANNEL_DEFAULT_CONFIG(dts_config->port, I2S_ROLE_MASTER);
-    esp_err_t esp_error = i2s_new_channel(&chan_cfg, &driver_data->tx_handle, &driver_data->rx_handle);
+    esp_err_t esp_error = i2s_new_channel(&chan_cfg, &internal->tx_handle, &internal->rx_handle);
     if (esp_error != ESP_OK) {
         LOG_E(TAG, "Failed to create I2S channels: %s", esp_err_to_name(esp_error));
-        unlock(driver_data);
+        unlock(internal);
         return ERROR_RESOURCE;
     }
 
-    i2s_std_config_t std_cfg;
+    i2s_std_config_t std_cfg = {};
-    get_esp32_std_config(config, dts_config, &std_cfg);
+    get_esp32_std_config(internal, config, &std_cfg);
 
-    if (driver_data->tx_handle) {
+    if (internal->tx_handle) {
-        esp_error = i2s_channel_init_std_mode(driver_data->tx_handle, &std_cfg);
+        esp_error = i2s_channel_init_std_mode(internal->tx_handle, &std_cfg);
-        if (esp_error == ESP_OK) esp_error = i2s_channel_enable(driver_data->tx_handle);
+        if (esp_error == ESP_OK) esp_error = i2s_channel_enable(internal->tx_handle);
     }
-    if (esp_error == ESP_OK && driver_data->rx_handle) {
+    if (esp_error == ESP_OK && internal->rx_handle) {
-        esp_error = i2s_channel_init_std_mode(driver_data->rx_handle, &std_cfg);
+        esp_error = i2s_channel_init_std_mode(internal->rx_handle, &std_cfg);
-        if (esp_error == ESP_OK) esp_error = i2s_channel_enable(driver_data->rx_handle);
+        if (esp_error == ESP_OK) esp_error = i2s_channel_enable(internal->rx_handle);
     }
 
     if (esp_error != ESP_OK) {
         LOG_E(TAG, "Failed to initialize/enable I2S channels: %s", esp_err_to_name(esp_error));
-        cleanup_channel_handles(driver_data);
+        cleanup_channel_handles(internal);
-        unlock(driver_data);
+        unlock(internal);
         return esp_err_to_error(esp_error);
     }
 
     // Update runtime config to reflect current state
-    memcpy(&driver_data->config, config, sizeof(I2sConfig));
+    memcpy(&internal->config, config, sizeof(I2sConfig));
-    driver_data->config_set = true;
+    internal->config_set = true;
 
-    unlock(driver_data);
+    unlock(internal);
     return ERROR_NONE;
 }
 
@@ -188,9 +285,17 @@ static error_t get_config(Device* device, struct I2sConfig* config) {
 
 static error_t start(Device* device) {
     ESP_LOGI(TAG, "start %s", device->name);
+
+    auto* dts_config = GET_CONFIG(device);
+
     auto* data = new(std::nothrow) Esp32I2sInternal();
     if (!data) return ERROR_OUT_OF_MEMORY;
 
+    if (!data->init_pins(dts_config)) {
+        LOG_E(TAG, "Failed to init one or more pins");
+        return ERROR_RESOURCE;
+    }
+
     device_set_driver_data(device, data);
 
     return ERROR_NONE;

TactilityKernel/Include/tactility/drivers/gpio.h

@@ -12,23 +12,22 @@ extern "C" {
 
 #define GPIO_FLAGS_MASK 0x1f
 
+#define GPIO_PIN_NONE -1
+
+#define GPIO_PIN_SPEC_NONE ((struct GpioPinSpec) { NULL, 0, GPIO_FLAG_NONE })
+
 #define GPIO_FLAG_NONE 0
 #define GPIO_FLAG_ACTIVE_HIGH (0 << 0)
 #define GPIO_FLAG_ACTIVE_LOW (1 << 0)
-
 #define GPIO_FLAG_DIRECTION_INPUT (1 << 1)
 #define GPIO_FLAG_DIRECTION_OUTPUT (1 << 2)
 #define GPIO_FLAG_DIRECTION_INPUT_OUTPUT (GPIO_FLAG_DIRECTION_INPUT | GPIO_FLAG_DIRECTION_OUTPUT)
-
 #define GPIO_FLAG_PULL_UP (0 << 3)
 #define GPIO_FLAG_PULL_DOWN (1 << 4)
-
 #define GPIO_FLAG_INTERRUPT_BITMASK (0b111 << 5) // 3 bits to hold the values [0, 5]
 #define GPIO_FLAG_INTERRUPT_FROM_OPTIONS(options) (gpio_int_type_t)((options & GPIO_FLAG_INTERRUPT_BITMASK) >> 5)
 #define GPIO_FLAG_INTERRUPT_TO_OPTIONS(options, interrupt) (options | (interrupt << 5))
 
-#define GPIO_PIN_NONE -1
-
 typedef enum {
     GPIO_INTERRUPT_DISABLE = 0,
     GPIO_INTERRUPT_POS_EDGE = 1,