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 2021/03/04 14:05:34 UTC
[GitHub] [incubator-nuttx] gustavonihei opened a new pull request #2977: ESP32: Fix GPIO pin function configuration
gustavonihei opened a new pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977
## Summary
This PR intends to address two issuea on the GPIO driver for ESP32:
- Allow a pin to be configured as Input and Output simultaneously
- Fix default GPIO function when no option is provided
This PR is porting the same fix from #2972
## Impact
Current implementation unconditionally enabled the INPUT function and this behavior has been changed.
In case any device was relying on this wrong behavior, it now must explicitly configure the GPIO as INPUT.
The affected I2C driver has been updated in this PR.
## Testing
`esp32-wrover-kit:bmp180`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587577888
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
}
if ((attr & OPEN_DRAIN) != 0)
{
- cntrl = (1 << GPIO_PIN_PAD_DRIVER_S);
+ cntrl |= (1 << GPIO_PIN_PAD_DRIVER_S);
Review comment:
The value here is only used one time and I don't think it will ever be changed from this function in the future. The rest of the fields are related to the pin's interrupt. But I understand if it's here for convenience.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#issuecomment-790751562
This PR has already passed all the checks. They got restarted with the accidental close/open. Let's merge it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587567629
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
Review comment:
For ESP32, Function 3 is the GPIO function for all GPIOs.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#issuecomment-790740144
> Please check the default GPIO function.
> (this is just to avoid a merge, since the PR is approved)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587612391
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
}
if ((attr & OPEN_DRAIN) != 0)
{
- cntrl = (1 << GPIO_PIN_PAD_DRIVER_S);
+ cntrl |= (1 << GPIO_PIN_PAD_DRIVER_S);
Review comment:
Yes, since `cntrl` will be used for bitfield manipulation, I think it is more appropriate to handle it with logical operations.
But I agree with you, probably it will not ever be changed in this function.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587615040
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
Review comment:
Ah I see! it's the shift that's not correct not the `2`. I thought you just changed the `2` to `0`. 2 is FUNCTION3 and 0 is FUNCTION1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei closed pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei closed pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#issuecomment-790740144
> Please check the default GPIO function.
> (this is just to avoid a merge, since the PR is approved)
See the answer above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei edited a comment on pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei edited a comment on pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#issuecomment-790740144
> Please check the default GPIO function.
> (this is just to avoid a merge, since the PR is approved)
Please, see the answer above.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587619610
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
Review comment:
Exactly, previously this line always ORed a 0 value because of the shift hehe
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587619610
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
Review comment:
Exactly, previously this line always OR'ed a 0 value because of the shift hehe
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] Ouss4 merged pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
Ouss4 merged pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] gustavonihei commented on a change in pull request #2977: ESP32: Fix GPIO pin function configuration
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #2977:
URL: https://github.com/apache/incubator-nuttx/pull/2977#discussion_r587604802
##########
File path: arch/xtensa/src/esp32/esp32_gpio.c
##########
@@ -222,12 +222,12 @@ int esp32_configgpio(int pin, gpio_pinattr_t attr)
}
else
{
- func |= (uint32_t)((2 >> FUNCTION_SHIFT) << MCU_SEL_S);
+ func |= (uint32_t)(PIN_FUNC_GPIO << MCU_SEL_S);
Review comment:
`FUNCTION_3` is just a definition of the ESP32 GPIO API, which configures the field `MCU_SEL` of the respective IO_MUX register with the value `2`.
The IO_MUX API defines `PIN_FUNC_GPIO` to ease the configuration of any pin as GPIO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org