-
Notifications
You must be signed in to change notification settings - Fork 7.3k
Add Support for uart_line_ctrl_set() for UART RTS Line in NXP MCUX LPUART Driver #86710
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
Add Support for uart_line_ctrl_set() for UART RTS Line in NXP MCUX LPUART Driver #86710
Conversation
4abd6d8
to
1757607
Compare
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.
blocking over return value but please consider the other comments
drivers/serial/uart_mcux_lpuart.c
Outdated
#if CONFIG_UART_LINE_CTRL && \ | ||
defined(FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT) && FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT |
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.
nit you can make a macro for this expression since it is used in two places
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.
sure, thank you
drivers/serial/uart_mcux_lpuart.c
Outdated
case UART_LINE_CTRL_RTS: | ||
/* Disable Transmitter and Receiver */ | ||
config->base->CTRL &= ~(LPUART_CTRL_TE_MASK | LPUART_CTRL_RE_MASK); | ||
|
||
if (val >= 1U) { | ||
/* Reset TXRTS to set RXRTSE bit, this provides high-level on RTS line */ | ||
config->base->MODIR &= ~(LPUART_MODIR_TXRTSPOL_MASK | | ||
LPUART_MODIR_TXRTSE_MASK); | ||
config->base->MODIR |= LPUART_MODIR_RXRTSE_MASK; | ||
} else { | ||
/* Set TXRTSE to reset RXRTSE bit,this provide low-level on RTS line*/ | ||
config->base->MODIR &= ~(LPUART_MODIR_RXRTSE_MASK); | ||
config->base->MODIR |= (LPUART_MODIR_TXRTSPOL_MASK | | ||
LPUART_MODIR_TXRTSE_MASK); | ||
} | ||
break; |
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.
nit is to always make helper function if indent gets greater than 2 levels
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.
sure, I will update this. thank you.
drivers/serial/uart_mcux_lpuart.c
Outdated
break; | ||
|
||
default: | ||
ret = -ENODEV; |
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.
should be -ENOTSUP according to API if some control option is not enabled I think
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, I will update return value.
drivers/serial/uart_mcux_lpuart.c
Outdated
#if CONFIG_UART_LINE_CTRL && \ | ||
defined(FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT) && FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT |
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.
large expression used in multiple places should make macro for this
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, I will take a macro and use that. thank you.
1757607
to
969a378
Compare
Hi @decsny, I updated this PR as per your given comments. please review. thank you. |
969a378
to
dfd9018
Compare
drivers/serial/uart_mcux_lpuart.c
Outdated
#if defined(CONFIG_UART_LINE_CTRL) && \ | ||
defined(FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT) && \ | ||
(FSL_FEATURE_LPUART_HAS_MODEM_SUPPORT) | ||
#define CONFIG_UART_LINE_CTRL_ENABLE |
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.
I think the compliance check will fail because CONFIG_ is reserved for kconfigs
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.
Hi @decsny, yes it gets failed. I updated that macro and updated this PR. thank you.
- For MCUX LPUART driver, added support to control RTS line High/Low from other driver/app code. - This control is required to wakeup other device which is in sleep and configured its wakeup-source as UART-CTS line. Signed-off-by: Nirav Agrawal <[email protected]>
dfd9018
to
86dfa00
Compare
@dleach02, @DerekSnell, please review this PR. Thank you.
Regards,
Nirav