Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve configuration checking for GPIO. #58

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

robertlipe
Copy link

Improve configuration checking for GPIO.

Assert that the number of entries in the {in, out, intr} tables is appropriate
for the BOARD_NGPIO{in, out, intr} #defines. This helps catch configuration
errors such as raising the number of config entries and not knowing you have to
edit the g_gpio[] tables themselves.

Summary

Impact

Testing

@robertlipe
Copy link
Author

When reviewing this, please be sure to choose "Changes from last commit" as the web interface wants to show changes that were not in my commit. I changed only one file.

15e49b6

@lupyuen
Copy link
Owner

lupyuen commented Apr 13, 2022

Thanks for the PR :-) Sadly I found more issues with BL602 EVB GPIO while testing the PineDio Stack touch panel (see the warnings below)...

https://github.com/lupyuen/cst816s-nuttx/blob/main/cst816s.c#L1259-L1379

Also this call is incorrect: bl602evb/bl602_gpio.c

bl602_gpio_set_intmod(g_gpiointinputs[i], 1, GLB_GPIO_INT_TRIG_NEG_PULSE);

Because bl602_gpio_set_intmod accepts a gpio_pin (uint8_t), not a pinset (uint32_t).

I might have to drop bl602evb/bl602_gpio.c altogether and revamp it as BL602 GPIO Expander...

https://github.com/lupyuen/bl602_expander

Otherwise we would have too many GPIO fixes to patch in NuttX Mainline 🤔

UPDATE: More details here: apache#5810 (comment)

@robertlipe
Copy link
Author

How messy.
bl602_gpio_set_intmod does take an 8-bit gpio_pin, but it probably shouldn't. That's just indicative of more breakage.

As we learn more about the limits of (the implementation of) bl602_gpio.c moving to GPIO expander totally has a lot of merit. It'd be a breaking change for any existing code, but I think that's OK because it moves it to a more portable/more POSIX-y style. We do need a way for other code to be able to tweak a few bits as GPIOs here and there as interrupts are configured, LCD needs backlight, SPI needs chip selects, and so on. If we can do that by talking to bottom half, so be it.

Also, if this symbol lives on, please consider making bl602_device_table[] an actual table of structs; reading the manual struct generation with bar + FOO_COL is needlessly exhausting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants