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 20:08:56 UTC

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

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