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

drivers: serial: nrf: Use flow control from device tree #25999

Merged

Conversation

nordic-krch
Copy link
Contributor

@nordic-krch nordic-krch commented Jun 5, 2020

Removed flow control configuration from Kconfig and started to use device tree. Added option to have single direction flow control with only rts or cts pin.

@zephyrbot
Copy link
Collaborator

zephyrbot commented Jun 5, 2020

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@@ -1,5 +1,4 @@
CONFIG_GPIO=y
CONFIG_UART_0_NRF_FLOW_CONTROL=y
Copy link
Member

@anangl anangl Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name of this file is wrong. It should be nrf5340pdk_nrf5340_cpuapp.conf. Could you correct it on the occasion?

@@ -1452,8 +1464,8 @@ static int uarte_nrfx_pm_control(struct device *dev, u32_t ctrl_command,
(UARTE_PROP(idx, pin_prop)), \
(NRF_UARTE_PSEL_DISCONNECTED))

#define UARTE_RTS_CTS_PINS_SET(idx) \
(UARTE_HAS_PROP(idx, rts_pin) && UARTE_HAS_PROP(idx, cts_pin))
#define UARTE_RTS_OR_CTS_PINS_SET(idx) \
Copy link
Member

@anangl anangl Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about naming it HW_FLOW_CONTROL_AVAILABLE?

#define HWFC_CONFIG_CHECK(idx) \
BUILD_ASSERT( \
(UARTE_PROP(idx, hw_flow_control) && \
(UARTE_HAS_PROP(idx, rts_pin) || UARTE_HAS_PROP(idx, cts_pin)))\
Copy link
Member

@anangl anangl Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use UARTE_RTS_OR_CTS_PINS_SET(idx) here, for better readability. This line should be also indented more.

@@ -140,7 +140,7 @@
};

&uart0 {
compatible = "nordic,nrf-uart";
compatible = "nordic,nrf-uarte";
Copy link
Member

@anangl anangl Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was that intentional? If so, it should be mentioned in the commit message, or perhaps better done in a separate commit.

Copy link
Contributor Author

@nordic-krch nordic-krch Jun 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed, it was accidentally added.

Copy link
Contributor

@joerchan joerchan left a comment

HCI uart sample has not enabled HW FC for bbc_microbit board.

@nordic-krch
Copy link
Contributor Author

nordic-krch commented Jun 8, 2020

HCI uart sample has not enabled HW FC for bbc_microbit board.

what do you mean? DTS for bbc-micro has HWFC off for uart0 and sample does not modify it so it will have hwfc off.

@nordic-krch
Copy link
Contributor Author

nordic-krch commented Jun 8, 2020

@anangl can you re-review?

anangl
anangl approved these changes Jun 8, 2020
@joerchan
Copy link
Contributor

joerchan commented Jun 8, 2020

HCI uart sample has not enabled HW FC for bbc_microbit board.

what do you mean? DTS for bbc-micro has HWFC off for uart0 and sample does not modify it so it will have hwfc off.

Right. HWFC is required for hci-uart though. I'm thinking we should actually remove the BBC microbit board configuration then.
Can take that outside of this PR though.

@ioannisg
Copy link
Member

ioannisg commented Jun 8, 2020

@nordic-krch pls, rebase for conflict resolution

nordic-krch added 4 commits Jun 9, 2020
Cleaned up flow control configuration. Added support for using only
cts or only rts.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Cleaned up flow control configuration. Added support for using only
cts or only rts.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Removed flow control configuration from Kconfig and updated samples
to use device tree for that.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
Conf file was not renamed after board renaming.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
@carlescufi carlescufi merged commit fe7ffc0 into zephyrproject-rtos:master Jun 9, 2020
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants