You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/03/18 17:59:54 UTC

[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #5795: Feature/esp32s3 gpio

gustavonihei opened a new pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795


   ## Summary
   This PR intends to perform some clean ups to the **GPIO** driver interface and fix some small inconsistencies (e.g. arguments from different functions being referred to by different identifiers , `pin` vs `gpio`).
   
   Furthermore, it enables the UART pins to bypass the GPIO Matrix and go through the IOMUX directly. Does not bring any practical improvements, but makes the driver more correct.
   
   ## Impact
   Theoretically, UART pins are now able to achieve higher speeds when going through the IOMUX, but serial console will hardly benefit from this.
   
   ## Testing
   CI pass and `ostest`.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830359762



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);
+
+  DEBUGASSERT(is_valid_gpio(pin));
 
   if (inv)
     {
       regval |= GPIO_FUNC0_IN_INV_SEL;
     }
 
-  if (gpio != 0x3a)
+  if (pin != 0x3a)

Review comment:
       The **0x3c** and **0x38** values will be handled by the GPIO Matrix peripheral, so no need to perform extra verification here.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830322829



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +237,31 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
-    {
-      return;
-    }
+  DEBUGASSERT(is_valid_gpio(pin));
 
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
+  putreg32(1ul << pin, GPIO_ENABLE_W1TS_REG);

Review comment:
       Actually, there is actually another register for pins whose number are greater than 31.
   I need to double check it, but I think the current driver seems to not handle this case.
   Thanks for the heads up.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830363785



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       Thank you as well! Sometimes we take some information for granted, so I usually take this opportunity to do further research and learn a bit more hehehe




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830360673



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       Oah, sorry... I missed the line `uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;`. Again, was confused by `esp32s3_gpio_matrix_in` that has a swapped `regaddr` and `regval` calculation. Thank for educating me :)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830319158



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +237,31 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
-    {
-      return;
-    }
+  DEBUGASSERT(is_valid_gpio(pin));
 
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
+  putreg32(1ul << pin, GPIO_ENABLE_W1TS_REG);

Review comment:
       do we need to have something like 32 check? or `% 32`?

##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +237,31 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
-    {
-      return;
-    }
+  DEBUGASSERT(is_valid_gpio(pin));
 
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
+  putreg32(1ul << pin, GPIO_ENABLE_W1TS_REG);

Review comment:
       ```suggestion
     putreg32(UINT32_C(1) << pin, GPIO_ENABLE_W1TS_REG);
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830355439



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       Then should we add `DEBUGASSERT(signal_idx == 0x100);`? Or that is redundant?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830356944



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);

Review comment:
       Indeed, it seems catchy, but here the offset is based on `signal_idx`.
   Here the driver is configuring the GPIO Matrix to forward the input value from a pin (0-48) to a peripheral signal (0-256). So, for each of the 256 peripheral signals, there is a `GPIO_FUNCx_IN_SEL_CFG_REG` register.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830358201



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);

Review comment:
       Ok. NP. I just want to make sure that all is fine, so double checking the details.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830326986



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +237,31 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
-    {
-      return;
-    }
+  DEBUGASSERT(is_valid_gpio(pin));
 
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
+  putreg32(1ul << pin, GPIO_ENABLE_W1TS_REG);

Review comment:
       @pkarashchenko Indeed, the handling for that scenario was missing. I've also applied the fix you suggested. Thanks!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830343287



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);
+
+  DEBUGASSERT(is_valid_gpio(pin));
 
   if (inv)
     {
       regval |= GPIO_FUNC0_IN_INV_SEL;
     }
 
-  if (gpio != 0x3a)
+  if (pin != 0x3a)

Review comment:
       `pin` is the one to compared against **0x3a**, but the issue here is that `esp32s3_gpio_matrix_in` should not check if `pin` is within the valid range of GPIOs.
   
   Nice catch, thanks again!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830359324



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       > Then should we add DEBUGASSERT(signal_idx == 0x100);? Or that is redundant?
   
   The **0x100** may be simply written into the register and the GPIO Matrix peripheral will cancel the output.
   So no need for the `DEBUGASSERT` here.
   The expected value for `signal_idx` is one of those defined in the `esp32s3_gpio_sigmap.h` header. **0x100** matches `SIG_GPIO_OUT_IDX`.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830360918



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,35 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);

Review comment:
       ```suggestion
     uint32_t regval = pin << GPIO_FUNC0_IN_SEL_S;
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830337727



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);
+
+  DEBUGASSERT(is_valid_gpio(pin));
 
   if (inv)
     {
       regval |= GPIO_FUNC0_IN_INV_SEL;
     }
 
-  if (gpio != 0x3a)
+  if (pin != 0x3a)

Review comment:
       `0x3a == 58` and `ESP32S3_NPINS` is `49`, so this condition seems to be never hit. Should `signal_idx` be checked instead of `pin`?

##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);

Review comment:
       Sorry for dummy question, but maybe??
   ```suggestion
     uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (pin * 4);
   ```
   I'm just looking into `esp32s3_gpio_matrix_out` implementation so this question rises.

##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       The `if (signal_idx == 0x100)` check is missing.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830355795



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       I'm just looking into an API description:
   ```
    *   signal_idx    - Signal index.
    *                   - If signal_idx == 0x100, cancel output to the GPIO.
   ```
   so that is the point of my confusion




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830346622



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -171,24 +244,38 @@ void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
  * Name: esp32s3_gpio_matrix_out
  *
  * Description:
- *   Set signal output to gpio
- *   NOTE: one signal can output to several gpios
- *   If signal_idx == 0x100, cancel output put to the gpio
+ *   Set signal output to GPIO.
+ *   NOTE: one signal can output to several GPIOs.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *   signal_idx    - Signal index.
+ *                   - If signal_idx == 0x100, cancel output to the GPIO.
+ *   out_inv       - Flag indicating whether the signal output is inverted.
+ *   oen_inv       - Flag indicating whether the signal output enable is
+ *                   inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_out(uint32_t gpio, uint32_t signal_idx,
-                             bool out_inv, bool oen_inv)
+void esp32s3_gpio_matrix_out(uint32_t pin, uint32_t signal_idx, bool out_inv,
+                             bool oen_inv)
 {
-  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (gpio * 4);
+  uint32_t regaddr = GPIO_FUNC0_OUT_SEL_CFG_REG + (pin * 4);
   uint32_t regval = signal_idx << GPIO_FUNC0_OUT_SEL_S;
 
-  if (gpio >= ESP32S3_NGPIOS)
+  DEBUGASSERT(is_valid_gpio(pin));
+
+  if (pin < 32)
     {
-      return;
+      putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TS_REG);
+    }
+  else
+    {
+      putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TS_REG);
     }
-
-  putreg32(1ul << gpio, GPIO_ENABLE_W1TS_REG);
 
   if (out_inv)

Review comment:
       In fact the hardware register already expects that value and will behave accordingly, so there is no need for performing any validations on the driver level.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830356388



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,37 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);
+
+  DEBUGASSERT(is_valid_gpio(pin));
 
   if (inv)
     {
       regval |= GPIO_FUNC0_IN_INV_SEL;
     }
 
-  if (gpio != 0x3a)
+  if (pin != 0x3a)

Review comment:
       What about the other values?
   ```
    *   pin           - GPIO pin to be configured.
    *                   - If pin == 0x3c, cancel input to the signal, input 0
    *                     to signal.
    *                   - If pin == 0x3a, input nothing to signal.
    *                   - If pin == 0x38, cancel input to the signal, input 1
   ```
   should those be checked?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795#discussion_r830364309



##########
File path: arch/xtensa/src/esp32s3/esp32s3_gpio.c
##########
@@ -141,25 +202,35 @@ int esp32s3_configgpio(int pin, gpio_pinattr_t attr)
  * Name: esp32s3_gpio_matrix_in
  *
  * Description:
- *   Set gpio input to a signal
- *   NOTE: one gpio can input to several signals
- *   If gpio == 0x3c, cancel input to the signal, input 0 to signal.
- *   If gpio == 0x3a, input nothing to signal.
- *   If gpio == 0x38, cancel input to the signal, input 1 to signal.
+ *   Set GPIO input to a signal.
+ *   NOTE: one GPIO can input to several signals.
+ *
+ * Input Parameters:
+ *   pin           - GPIO pin to be configured.
+ *                   - If pin == 0x3c, cancel input to the signal, input 0
+ *                     to signal.
+ *                   - If pin == 0x3a, input nothing to signal.
+ *                   - If pin == 0x38, cancel input to the signal, input 1
+ *                     to signal.
+ *   signal_idx    - Signal index.
+ *   inv           - Flag indicating whether the signal is inverted.
+ *
+ * Returned Value:
+ *   None.
  *
  ****************************************************************************/
 
-void esp32s3_gpio_matrix_in(uint32_t gpio, uint32_t signal_idx, bool inv)
+void esp32s3_gpio_matrix_in(uint32_t pin, uint32_t signal_idx, bool inv)
 {
   uint32_t regaddr = GPIO_FUNC0_IN_SEL_CFG_REG + (signal_idx * 4);
-  uint32_t regval = (gpio << GPIO_FUNC0_IN_SEL_S);
+  uint32_t regval = (pin << GPIO_FUNC0_IN_SEL_S);

Review comment:
       Fixed.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [incubator-nuttx] pkarashchenko merged pull request #5795: ESP32-S3: Clean up and improve GPIO driver interface

Posted by GitBox <gi...@apache.org>.
pkarashchenko merged pull request #5795:
URL: https://github.com/apache/incubator-nuttx/pull/5795


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org