[GitHub] [nuttx] gustavonihei commented on a diff in pull request #8202: arch/xtensa/esp32: Add support for RTC IRQs

[GitBox <gi...@apache.org>]

gustavonihei commented on code in PR #8202:
URL: https://github.com/apache/nuttx/pull/8202#discussion_r1082631904


##########
arch/xtensa/src/esp32/esp32_gpio.c:
##########
@@ -222,258 +220,152 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
   func  = 0;
   cntrl = 0;
 
-  if ((attr & FUNCTION_MASK) == FUNCTION_RTC) /* RTCIO */
+  if ((attr & INPUT) != 0)
     {
-      if (rtc_gpio_is_valid_gpio(pin) == 0)
-        {
-          gpioerr("Pin %d is not a valid RTC pin!\n", pin);
-          return -1;
-        }
-
-      rtc_gpio_idx = g_rtc_io_num_map[pin];
-      rtc_reg_desc = g_rtc_io_desc[rtc_gpio_idx];
-      g_pin_rtc_controlled[rtc_gpio_idx] = true;
-
-      setbits(rtc_reg_desc.reg, rtc_reg_desc.mux);
-
-      modifyreg32(rtc_reg_desc.reg,
-        ((RTC_IO_TOUCH_PAD1_FUN_SEL_V) << (rtc_reg_desc.func)),
-        (((RTCIO_PIN_FUNC) & RTC_IO_TOUCH_PAD1_FUN_SEL_V) <<
-        (rtc_reg_desc.func)));
-
-      if (rtc_reg_desc.pulldown)
-        {
-          resetbits(rtc_reg_desc.reg, rtc_reg_desc.pulldown);
-        }
-
-      if (rtc_reg_desc.pullup)
-        {
-          resetbits(rtc_reg_desc.reg, rtc_reg_desc.pullup);
-        }
-
-      if ((attr & PULLUP) != 0)
+      if (pin < 32)
         {
-          if (rtc_reg_desc.pullup)
-            {
-              setbits(rtc_reg_desc.reg, rtc_reg_desc.pullup);
-            }
+          putreg32((UINT32_C(1) << pin), GPIO_ENABLE_W1TC_REG);
         }
-      else if ((attr & PULLDOWN) != 0)
+      else
         {
-          if (rtc_reg_desc.pulldown)
-            {
-              setbits(rtc_reg_desc.reg, rtc_reg_desc.pulldown);
-            }
+          putreg32((UINT32_C(1) << (pin - 32)), GPIO_ENABLE1_W1TC_REG);
         }
 
-      if ((attr & INPUT) != 0)
-        {
-          /* Enable Input */
-
-          setbits(rtc_reg_desc.reg, rtc_reg_desc.ie);
-
-          /* Disable Output */
-
-          putreg32((UINT32_C(1) << pin), RTC_GPIO_ENABLE_W1TC_REG);
-        }
-      else if ((attr & OUTPUT) != 0)
-        {
-          /* Disable Input */
+      /* Input enable */
 
-          resetbits(rtc_reg_desc.reg, rtc_reg_desc.ie);
+      func |= FUN_IE;
 
-          /* Enable Output */
+      /* Some pins only support Pull-Up and Pull-Down resistor on RTC GPIO */
 
-          putreg32((UINT32_C(1) << pin), RTC_GPIO_ENABLE_W1TS_REG);
-        }
-      else
+      if (gpio_is_valid_rtc_gpio(pin))
         {
-          resetbits(rtc_reg_desc.reg, rtc_reg_desc.ie);
-          putreg32((UINT32_C(1) << pin), RTC_GPIO_ENABLE_W1TC_REG);
-        }
+          uint32_t rtc_gpio_idx = g_gpio_to_rtcio_map[pin];
+          uint32_t regval;
+          uint32_t rtc_gpio_pin;
+          bool en_pu = false;
+          bool en_pd = false;
 
-      if ((attr & DRIVE_MASK) != 0)
-        {
-          if (rtc_reg_desc.drv_v)
+          if ((attr & PULLUP) != 0)
             {
-              uint32_t val = ((attr & DRIVE_MASK) >> DRIVE_SHIFT) - 1;
-              modifyreg32(rtc_reg_desc.reg,
-                ((rtc_reg_desc.drv_v) << (rtc_reg_desc.drv_s)),
-                (((val) & rtc_reg_desc.drv_v) << (rtc_reg_desc.drv_s)));
-            }
-        }
-
-      if ((attr & OPEN_DRAIN) != 0)
-        {
-          /* All RTC GPIOs have the same position for the drive bits.
-           * We can use any RTC_GPIO_PINn_PAD_DRIVER.
-           */
-
-          REG_SET_FIELD(rtc_gpio_to_addr[rtc_gpio_idx],
-                        RTC_GPIO_PIN0_PAD_DRIVER,
-                        true);
-        }
-      else
-        {
-          REG_SET_FIELD(rtc_gpio_to_addr[rtc_gpio_idx],
-                        RTC_GPIO_PIN0_PAD_DRIVER,
-                        false);
-        }
+              if (!rtc_gpio_is_pull_supported(rtc_gpio_idx))
+                {
+                  gpioerr("Pins 34-39 don't support PullUp/PullDown\n");
+                  PANIC();
+                }

Review Comment:
   A suggestion for a cleaner code:
   The error message should be moved to where the check is made:
   ```c
   static bool rtc_gpio_is_pull_supported(uint32_t rtcio_num)
   {
     /* Pins 34 thorugh 39 use RTC channels 0 to 5 and don't support PU/PD */
   
     bool is_pull_supported = rtcio_num > 5;
   
     gpioerr("Pins 34-39 don't support PullUp/PullDown\n");
   
     return is_pull_supported;
   }
   ```
   Then we may just use an assertion in every other place.
   ```suggestion
                 ASSERT(rtc_gpio_is_pull_supported(rtc_gpio_idx));
   ```
   
   The `inline` keyword I believe may be omitted, the compiler most probably will inline it when GPIO logs are deactivated.



-- 
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