drivers: gpio_rpi_pico: Extend the driver API #84548
Conversation
zephyrbot
added
area: GPIO
platform: Raspberry Pi Pico
ajf58
force-pushed
the
rpi-pico-add-gpio-get-config-api
branch
from
0b86b90 to
4416912
Compare
|
cc: @drensber |
tests/drivers/gpio/gpio_basic_api/boards/rpi_pico2_rp2350a_m33.conf
Outdated
Show resolved
Hide resolved
ajf58
changed the title
ajf58
force-pushed
the
rpi-pico-add-gpio-get-config-api
branch
from
099ef80 to
2bfaa02
Compare
| if (flags & GPIO_INPUT) { | ||
| gpio_set_dir(pin, GPIO_IN); | ||
| } else { | ||
| gpio_set_input_enabled(pin, false); |
There was a problem hiding this comment.
Given that the RP2xxx series is documented as supporting reading from input pins at any time, does it make sense to disable the pad here? It seems to me that the pad should only be disabled if the application explicitly requests. (Of course, that leaves the question of should there be a Zephyr way to explicitly disable an input ...)
There was a problem hiding this comment.
It's a a good question. Zephyr's unit tests for gpio_pin_get_config require that pin config is explicit, i.e. if gpio_pin_get_config set GPIO_INPUT on every pin, the test will fail.
Of course, that leaves the question of should there be a Zephyr way to explicitly disable an input
I think it is explicit already: if someone wants an output that's also an input, they should use GPIO_INPUT | GPIO_OUTPUT.
drivers/gpio/gpio_rpi_pico.c
Outdated
| if (gpio_get_dir(pin)) { | ||
| *flags |= gpio_get_out_level(pin) ? GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW; | ||
| if (data->single_ended_mask & BIT(pin)) { | ||
| *flags |= data->open_drain_mask & BIT(pin) ? GPIO_OPEN_DRAIN : GPIO_PUSH_PULL; |
There was a problem hiding this comment.
Don't you hate that? :-)
There was a problem hiding this comment.
That a tab is 8 characters wide and the column limit is 100? Yes on both fronts.
There was a problem hiding this comment.
Everyone who learned how to properly use a keyboard knows that the correct setting for tabs is 5 spaces, adjusted as needed to suit the document. :-)
Out of reset the pads are input enabled, output disabled. Disconnect the pad's input and output buffers, as well as any pullups. This can reduce input leakage current. Signed-off-by: Andrew Featherstone <[email protected]>
Reorder gpio_rpi_configure to disable input buffers when not in use. gpio_rpi_get_config can then determine whether a pin is configured as an input without requiring additional state variables, as well as reducing input leakage current. Signed-off-by: Andrew Featherstone <[email protected]>
Implement `gpio_get_pending_int`. Signed-off-by: Andrew Featherstone <[email protected]>
THe driver didn't implement this API, so add it. Signed-off-by: Andrew Featherstone <[email protected]>
Implement the `gpio_get_config` N.b. adding this API results in a new test failure in `test_gpio_config_trigger`. This suggests that there is some kind of dependency between this and the now-enabled `pin_get_config` test cases. Note that this adds a read-only API, it is unlikely to be the cause of the failure. Signed-off-by: Andrew Featherstone <[email protected]>
ajf58
force-pushed
the
rpi-pico-add-gpio-get-config-api
branch
from
2bfaa02 to
c39f6f3
Compare
ajf58
left a comment
ajf58
left a comment
There was a problem hiding this comment.
This push:
- Rebases onto a newer revision of
main. - Applies whitespaces requested in the review (although I would like to understand their rationale).
- Corrects compliance issues that came up under CI.
drivers/gpio/gpio_rpi_pico.c
Outdated
| if (gpio_get_dir(pin)) { | ||
| *flags |= gpio_get_out_level(pin) ? GPIO_OUTPUT_HIGH : GPIO_OUTPUT_LOW; | ||
| if (data->single_ended_mask & BIT(pin)) { | ||
| *flags |= data->open_drain_mask & BIT(pin) ? GPIO_OPEN_DRAIN : GPIO_PUSH_PULL; |
There was a problem hiding this comment.
That a tab is 8 characters wide and the column limit is 100? Yes on both fronts.
| if (flags & GPIO_INPUT) { | ||
| gpio_set_dir(pin, GPIO_IN); | ||
| } else { | ||
| gpio_set_input_enabled(pin, false); |
There was a problem hiding this comment.
It's a a good question. Zephyr's unit tests for gpio_pin_get_config require that pin config is explicit, i.e. if gpio_pin_get_config set GPIO_INPUT on every pin, the test will fail.
Of course, that leaves the question of should there be a Zephyr way to explicitly disable an input
I think it is explicit already: if someone wants an output that's also an input, they should use GPIO_INPUT | GPIO_OUTPUT.
|
At this point I think this PR it GTG. |
I believe there is no problem based on the following wording in the document you showed.
|
6 participants
Edit:
The original goal of this PR was to add just the
gpio_get_config. This PR has now widened to include a lot more functionality, extending the both the existing APIs to be more feature-complete, and adding new APIs that were missing.Tested locally using a physical test fixture supporting the GPIO loopback.