-
Notifications
You must be signed in to change notification settings - Fork 8.2k
[TOPIC-GPIO] drivers: ieee802154_rf2xx: convert to the new GPIO API #22214
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
[TOPIC-GPIO] drivers: ieee802154_rf2xx: convert to the new GPIO API #22214
Conversation
Use new GPIO configuration API, set pin active level in devicetree source. Signed-off-by: Piotr Mienkowski <[email protected]>
|
@nandojve please review. |
| reset-gpios = <&portb 15 0>; | ||
| slptr-gpios = <&porta 20 0>; | ||
| dig2-gpios = <&portb 17 0>; | ||
| irq-gpios = <&portb 0 (GPIO_ACTIVE_HIGH | GPIO_PULL_DOWN)>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The RF2XX data sheet doesn't mention anything about not driving output pins, like IRQ, in sleep state. It seems like there is no need to configure pin pull downs. Nevertheless, I preserved the existing configuration since I don't have a hardware to test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, is not clear for sure. The section 1.3 Digital Pins we got about DIGx pins. For IRQ pin, since entering on the TRX_OFF state from P_ON, SLEEP, DEEP_SLEEP or RESET state is indicated by interrupt IRQ_4 (AWAKE_END), if enabled, we can assume this pin have some level at sleep. The major problem with IRQ is user configurable and can be level High or level Low. Today I'm setting the default (level High). The MCLK is low if disabled.
The most important are RESET and SLPTR. This pins MUST NOT float any time otherwise radio will be at unstable state.
Today radio don't have any Power Manage functions implemented and all this can be revised in future. For the moment, I would keep same values.
nandojve
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make sure to keep original sequence otherwise radio will be at RESET state.
| gpio_pin_set(ctx->reset_gpio, conf->reset.pin, 1); | ||
| k_busy_wait(10); | ||
| gpio_pin_write(ctx->reset_gpio, conf->reset.pin, 1); | ||
| gpio_pin_set(ctx->reset_gpio, conf->reset.pin, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, make sure to keep original sequence otherwise radio will be at RESET state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nandojve I believe this is correct. Because the reset pin is marked active low and gpio_pin_set() operates on the active level, the command before the wait enables reset, and the one after disables it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, but reset is active low see 1.1 Pin Descriptions on the datasheet. If this is on code need to be fixed anyway. I will have access to radios again next week and I can go in deep. How urgent is this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I mis-wrote: reset is active low in its devicetree definition, so passing 1 to gpio_pin_set() makes it low. This is different from gpio_pin_set_raw().
Urgency: This will be merged today or tomorrow to enable a major refactoring required to get this branch into 2.2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, please, move forward!
Use new GPIO configuration API, set pin active level in devicetree source.