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/11/02 10:48:56 UTC
[GitHub] [incubator-nuttx] jarivanewijk opened a new pull request #4774: GPIO driver: Use generic /dev/gpioN
jarivanewijk opened a new pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774
## Summary
This PR changes the names under which GPIO pins are registered as generic /dev/gpioN instead of the specific names for different pintypes (/dev/gpinN, /dev/gpintN, /dev/gpoutN). In my opinion it does not make much sense to distinguish between pintypes in the name, especially because these pintypes can be changed AFTER the driver is registered with the setpintype command. I think these changes would also allow some board-specific low-level GPIO drivers to be simplified (but that's not included in this PR).
## Impact
All GPIO pins need to be registered with a unique minor number. Previously there could be a /dev/gpin1 and a /dev/gpout1 without issues, but we cannot map both to /dev/gpio1. Most board-level code already used unique numbers, except for some ESP32 boards. I already included the required changes in this PR. All board-specific low-level GPIO drivers should remain functional.
Applications that rely on GPIO pins being registered as /dev/gpinN, /dev/gpintN and /dev/gpoutN need to be adapted to now refer to /dev/gpioN. For incubator-nuttx-apps I think this is only the case for /examples/timer-gpout.
## Testing
Tested on an S32K144EVB (with the newly implemented GPIO driver). It would be much appreciated if anybody could verify this on another board as well, especially one of the ESP32 boards which I had to adapt.
--
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] jarivanewijk commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-989972732
I finally was able to get back to this. I have addressed all of your comments. I have also added additional debugasserts to ensure everything is implemented as intended. I have also taken out the commit with the lower half GPIO driver for the S32K144EVB board, because I think that fits better in a separate PR. I think this is "done" for now. Please have another look and let me know if there are any remaining comments.
I will open some additional PRs to incubator-nuttx-apps to make sure the timer-gpout example application is not affected by these changes. I have also modified the gpio example to allow pintypes to be changed, which is very convenient for testing.
--
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] jarivanewijk commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961338049
@xiaoxiang781216, not yet, sorry. I need to find some time to finish this, but that might not be until the end of next week.
I still want to have a look at some of the error checking and handling. I also need to prepare a PR for incubator-nuttx-apps to make sure we don't leave some apps in a broken state.
--
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] jarivanewijk commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961338049
@xiaoxiang781216, not yet, sorry. I need to find some time to finish this, but that might not be until the end of next week.
I still want to have a look at some of the error checking and handling. I also need to prepare a PR for incubator-nuttx-apps to make sure we don't leave some apps in a broken state.
--
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] xiaoxiang781216 commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-990141407
LGTM.
--
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] xiaoxiang781216 commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766028160
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -347,8 +349,9 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR bool *ptr = (FAR bool *)((uintptr_t)arg);
DEBUGASSERT(ptr != NULL);
+ DEBUGASSERT(dev->gp_ops->go_read != NULL);
ret = dev->gp_ops->go_read(dev, ptr);
- DEBUGASSERT(ret < 0 || *ptr == 0 || *ptr == 1);
Review comment:
why remove ret < 0?
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -562,13 +619,11 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
{
return ret;
}
-
- fmt = "/dev/gpint%u";
}
break;
}
- snprintf(devname, 16, fmt, (unsigned int)minor);
+ snprintf(devname, sizeof(devname), "/dev/gpio%u", (unsigned int) minor);
Review comment:
remove the space before minor
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -578,50 +633,19 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
* Name: gpio_pin_unregister
*
* Description:
- * Unregister GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
- *
+ * Unregister GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number.
*
****************************************************************************/
-void gpio_pin_unregister(FAR struct gpio_dev_s *dev, int minor)
+int gpio_pin_unregister(FAR struct gpio_dev_s *dev, int minor)
{
- FAR const char *fmt;
- char devname[16];
-
- switch (dev->gp_pintype)
- {
- case GPIO_INPUT_PIN:
- case GPIO_INPUT_PIN_PULLUP:
- case GPIO_INPUT_PIN_PULLDOWN:
- {
- fmt = "/dev/gpin%u";
- }
- break;
-
- case GPIO_OUTPUT_PIN:
- case GPIO_OUTPUT_PIN_OPENDRAIN:
- {
- fmt = "/dev/gpout%u";
- }
- break;
-
- default:
- {
- fmt = "/dev/gpint%u";
- }
- break;
- }
+ char devname[32];
- snprintf(devname, sizeof(devname), fmt, (unsigned int)minor);
+ snprintf(devname, sizeof(devname), "/dev/gpio%u", (unsigned int) minor);
Review comment:
remove space before minor too
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -489,7 +496,65 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
case GPIOC_SETPINTYPE:
{
- ret = dev->gp_ops->go_setpintype(dev, arg);
+ FAR enum gpio_pintype_e pintype = (FAR enum gpio_pintype_e) arg;
Review comment:
remove FAR
--
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] xiaoxiang781216 commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766049681
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -489,7 +496,65 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
case GPIOC_SETPINTYPE:
{
- ret = dev->gp_ops->go_setpintype(dev, arg);
+ FAR enum gpio_pintype_e pintype = (FAR enum gpio_pintype_e) arg;
Review comment:
FAR is only used for pointer, not the value type like enum.
--
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] xiaoxiang781216 commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746865886
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
Review comment:
Here is the PR: https://github.com/apache/incubator-nuttx/pull/4697. Sorry, forget to update the comment.
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766050340
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -562,13 +619,11 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
{
return ret;
}
-
- fmt = "/dev/gpint%u";
}
break;
}
- snprintf(devname, 16, fmt, (unsigned int)minor);
+ snprintf(devname, sizeof(devname), "/dev/gpio%u", (unsigned int) minor);
Review comment:
Changed.
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -578,50 +633,19 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
* Name: gpio_pin_unregister
*
* Description:
- * Unregister GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
- *
+ * Unregister GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number.
*
****************************************************************************/
-void gpio_pin_unregister(FAR struct gpio_dev_s *dev, int minor)
+int gpio_pin_unregister(FAR struct gpio_dev_s *dev, int minor)
{
- FAR const char *fmt;
- char devname[16];
-
- switch (dev->gp_pintype)
- {
- case GPIO_INPUT_PIN:
- case GPIO_INPUT_PIN_PULLUP:
- case GPIO_INPUT_PIN_PULLDOWN:
- {
- fmt = "/dev/gpin%u";
- }
- break;
-
- case GPIO_OUTPUT_PIN:
- case GPIO_OUTPUT_PIN_OPENDRAIN:
- {
- fmt = "/dev/gpout%u";
- }
- break;
-
- default:
- {
- fmt = "/dev/gpint%u";
- }
- break;
- }
+ char devname[32];
- snprintf(devname, sizeof(devname), fmt, (unsigned int)minor);
+ snprintf(devname, sizeof(devname), "/dev/gpio%u", (unsigned int) minor);
Review comment:
Changed.
--
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] xiaoxiang781216 commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766048429
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -347,8 +349,9 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR bool *ptr = (FAR bool *)((uintptr_t)arg);
DEBUGASSERT(ptr != NULL);
+ DEBUGASSERT(dev->gp_ops->go_read != NULL);
ret = dev->gp_ops->go_read(dev, ptr);
- DEBUGASSERT(ret < 0 || *ptr == 0 || *ptr == 1);
Review comment:
if ret < 0, we should skip check *ptr equal 0 or 1 since the check is meaningless in case of fail.
--
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] acassis commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746633945
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
Review comment:
You are right, 99 is not the limit anymore
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766075385
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -347,8 +349,9 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR bool *ptr = (FAR bool *)((uintptr_t)arg);
DEBUGASSERT(ptr != NULL);
+ DEBUGASSERT(dev->gp_ops->go_read != NULL);
ret = dev->gp_ops->go_read(dev, ptr);
- DEBUGASSERT(ret < 0 || *ptr == 0 || *ptr == 1);
Review comment:
Ah, I think I get it now! Thanks. I have changed it back.
--
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] acassis commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-957684030
Sure @jarivanewijk I will test it on ESP32/C3
--
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] juniskane commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
juniskane commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746620057
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
Review comment:
Didn't some previous commit remove the limit of 99? Comment still valid?
--
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 #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746971209
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
*
****************************************************************************/
int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
{
- FAR const char *fmt;
- char devname[32];
+ char devname[16];
Review comment:
```suggestion
char devname[32];
```
If we consider the max value for `int minor`, it requires more than 16 characters. This change should be reverted.
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766041514
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -347,8 +349,9 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR bool *ptr = (FAR bool *)((uintptr_t)arg);
DEBUGASSERT(ptr != NULL);
+ DEBUGASSERT(dev->gp_ops->go_read != NULL);
ret = dev->gp_ops->go_read(dev, ptr);
- DEBUGASSERT(ret < 0 || *ptr == 0 || *ptr == 1);
Review comment:
Please correct me if my thought process is wrong, but I believe ret should always be zero when things are going right. ret is smaller than zero when go_read returned an error code. So this debugassert will always trigger unless something goes actually wrong. So it was wrong in the first place.
--
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] xiaoxiang781216 commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961311106
@jarivanewijk is it ready to remove the draft from PR?
--
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] xiaoxiang781216 commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961311106
@jarivanewijk is it ready to remove the draft from PR?
--
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] acassis commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-957684030
Sure @jarivanewijk I will test it on ESP32/C3
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766043072
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -489,7 +496,65 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
case GPIOC_SETPINTYPE:
{
- ret = dev->gp_ops->go_setpintype(dev, arg);
+ FAR enum gpio_pintype_e pintype = (FAR enum gpio_pintype_e) arg;
Review comment:
Why? It's used in a similar fashion all over this driver? I don't understand why it's not applicable 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] jarivanewijk commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961338049
@xiaoxiang781216, not yet, sorry. I need to find some time to finish this, but that might not be until the end of next week.
I still want to have a look at some of the error checking and handling. I also need to prepare a PR for incubator-nuttx-apps to make sure we don't leave some apps in a broken state.
--
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] xiaoxiang781216 commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-961311106
@jarivanewijk is it ready to remove the draft from PR?
--
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 #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
gustavonihei commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746972143
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -578,47 +568,16 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
* Name: gpio_pin_unregister
*
* Description:
- * Unregister GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
- *
+ * Unregister GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
*
****************************************************************************/
void gpio_pin_unregister(FAR struct gpio_dev_s *dev, int minor)
{
- FAR const char *fmt;
char devname[16];
Review comment:
```suggestion
char devname[32];
```
If we consider the max value for int minor, it requires more than 16 characters.
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -562,13 +554,11 @@ int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
{
return ret;
}
-
- fmt = "/dev/gpint%u";
}
break;
}
- snprintf(devname, 16, fmt, (unsigned int)minor);
+ snprintf(devname, 16, "/dev/gpio%u", (unsigned int) minor);
Review comment:
```suggestion
snprintf(devname, sizeof(devname), "/dev/gpio%u", (unsigned int) minor);
```
Using `sizeof` to make it less error prone to future changes.
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746637626
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
Review comment:
I was not aware. I will update the comments. So there's no limit anymore? (except for the maximum number that the int type can hold, of course).
--
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] acassis commented on pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
acassis commented on pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#issuecomment-957684030
Sure @jarivanewijk I will test it on ESP32/C3
--
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] acassis commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
acassis commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r746978316
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -511,20 +511,14 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
* Name: gpio_pin_register
*
* Description:
- * Register GPIO pin device driver.
- *
- * - Input pin types will be registered at /dev/gpinN
- * - Output pin types will be registered at /dev/gpoutN
- * - Interrupt pin types will be registered at /dev/gpintN
- *
- * Where N is the provided minor number in the range of 0-99.
+ * Register GPIO pin device driver at /dev/gpioN, where N is the provided
+ * minor number in the range of 0-99.
*
****************************************************************************/
int gpio_pin_register(FAR struct gpio_dev_s *dev, int minor)
{
- FAR const char *fmt;
- char devname[32];
+ char devname[16];
Review comment:
I think the right size (including \0) is 20
--
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] xiaoxiang781216 commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766048429
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -347,8 +349,9 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
FAR bool *ptr = (FAR bool *)((uintptr_t)arg);
DEBUGASSERT(ptr != NULL);
+ DEBUGASSERT(dev->gp_ops->go_read != NULL);
ret = dev->gp_ops->go_read(dev, ptr);
- DEBUGASSERT(ret < 0 || *ptr == 0 || *ptr == 1);
Review comment:
if ret < 0, we should skip check *ptr equal 0 or 1 since the check is meaningless in case of fail,: the driver writer mayn't touch ptr in fail path, and then left ptr with a random value.
--
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] jarivanewijk commented on a change in pull request #4774: GPIO driver: Use generic /dev/gpioN
Posted by GitBox <gi...@apache.org>.
jarivanewijk commented on a change in pull request #4774:
URL: https://github.com/apache/incubator-nuttx/pull/4774#discussion_r766075475
##########
File path: drivers/ioexpander/gpio.c
##########
@@ -489,7 +496,65 @@ static int gpio_ioctl(FAR struct file *filep, int cmd, unsigned long arg)
case GPIOC_SETPINTYPE:
{
- ret = dev->gp_ops->go_setpintype(dev, arg);
+ FAR enum gpio_pintype_e pintype = (FAR enum gpio_pintype_e) arg;
Review comment:
That's... an excellent point. I somehow had in mind that this was a pointer. Sorry, my bad! Changed.
--
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