drivers: serial: rpi_pico: added functions for runtime uart configuration #49883
Conversation
drivers/serial/uart_rpi_pico.c
Outdated
| uart_inst_t * const uart_inst = config->uart_dev; | ||
| struct uart_rpi_data *data = dev->data; | ||
|
|
||
| int baudrate = uart_set_baudrate(uart_inst, cfg->baudrate);\ |
drivers/serial/uart_rpi_pico.c
Outdated
| uart_inst_t * const uart_inst = config->uart_dev; | ||
| struct uart_rpi_data *data = dev->data; | ||
|
|
||
| int baudrate = uart_set_baudrate(uart_inst, cfg->baudrate);\ |
There was a problem hiding this comment.
This should also validate all parameters before making any configuration changes, if one setting is not valid, no UART configuration change should occur
|
@WebmasterTD Thanks. Please fix according to the compliance checks, and rebase to squash your commits. |
WebmasterTD
changed the title
WebmasterTD
changed the title
drivers/serial/uart_rpi_pico.c
Outdated
| return -EINVAL; | ||
| } | ||
|
|
||
| uint stop_bits = 0; |
There was a problem hiding this comment.
Does this need to be a 32-bit variable?
drivers/serial/uart_rpi_pico.c
Outdated
| return -EINVAL; | ||
| } | ||
|
|
||
| int baudrate = uart_set_baudrate(uart_inst, cfg->baudrate); |
There was a problem hiding this comment.
Does this need to be signed?
There was a problem hiding this comment.
The function returns uint, so the assigned type should also be unsigned. This also means that the assertion below should be ==, not <=
drivers/serial/uart_rpi_pico.c
Outdated
| return -EINVAL; | ||
| } | ||
|
|
||
| int baudrate = uart_set_baudrate(uart_inst, cfg->baudrate); |
There was a problem hiding this comment.
The function returns uint, so the assigned type should also be unsigned. This also means that the assertion below should be ==, not <=
|
changed baudrate to uint |
drivers/serial/uart_rpi_pico.c
Outdated
| */ | ||
| data->uart_config = (struct uart_config){ | ||
| .baudrate = baudrate, | ||
| .data_bits = UART_CFG_DATA_BITS_5, |
There was a problem hiding this comment.
5 data bits? Might be the default for the silicon but that would be very non-standard
There was a problem hiding this comment.
It does seem non standard to me as well.
However according the documentation:
UARTLCR_H
| Bits | Name | Description | Type | Reset |
|---|---|---|---|---|
| 6:5 | WLEN | Word length. These bits indicate the number of data bits transmitted or received in a frame as follows: b11 = 8 bits b10 = 7 bits b01 = 6 bits b00 = 5 bits. | RW | 0x0 |
https://datasheets.raspberrypi.com/rp2040/rp2040-datasheet.pdf#tab-registerlist_uart page:431
There was a problem hiding this comment.
Hm... I think we should set this to 8-bit as default. UARTs are used for console/log output (most of the time). So setting to 5-bit is not going to work.
There was a problem hiding this comment.
Changed the default to 8 bits
There was a problem hiding this comment.
Looking better, added one nitpick. Also, please remove the commit messages of the commits making up your final commits, such as "fixed typo", "fixed compliance checks" etc.
Please note that Zephyr entered a feature freeze yesterday, as we are approaching the v3.2.0 release, meaning that new features can't be merged. I recommend that you finish this now anyway, because sometimes exceptions are made (although this changes existing code, so I doubt it) and because it's fresh in your, and the reviewers' memory. Once 3.2 is released, this can then be merged.
|
@WebmasterTD please rebase on a fresh |
|
@WebmasterTD any updates? |
|
Sorry for the inactivity. I just updated the default to 8 bits as discussed above and synchronised the branch to the current |
| .flow_ctrl = UART_CFG_FLOW_CTRL_NONE, | ||
| .parity = UART_CFG_PARITY_NONE, | ||
| .stop_bits = UART_CFG_STOP_BITS_1 | ||
| }; |
There was a problem hiding this comment.
Do you need to call uart_set_format() here, since the hardware defaults to 5-bit at reset? Also might be a good idea to document that the data bit setting is different than hardware reset.
There was a problem hiding this comment.
Added an extra comment the deviation from hw default.
Added a call for uart_set_format()
|
@WebmasterTD Could you rebase and address all issues? |
|
Seems like a CI job might need to be re-run: https://github.com/zephyrproject-rtos/zephyr/actions/runs/3456815237/jobs/5770430170 |
|
Please fix the compliance check errors. |
…tion Added functions for runtime uart configuartion for raspberry pico Signed-off-by: Akos Melczer <[email protected]>
Added functions to raspberry pico uart to enable runtime configuration. Necessary when using Modbus RTU.
Signed-off-by: Akos Melczer [email protected]