Skip to content

Improve configuration checking for GPIO.#58

Open
robertlipe wants to merge 18 commits into
lupyuen:masterfrom
robertlipe:gpio_check
Open

Improve configuration checking for GPIO.#58
robertlipe wants to merge 18 commits into
lupyuen:masterfrom
robertlipe:gpio_check

Conversation

@robertlipe

Copy link
Copy Markdown

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

lupyuen commented Apr 13, 2022

Copy link
Copy Markdown
Owner

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