serial: stm32: Implement configure and config_get api calls #12366
Conversation
psidhu
force-pushed
the
pr-stm32-uart-update-api
branch
from
a748c2a
to
e6cbd7e
Compare
Codecov Report
@@ Coverage Diff @@
## master #12366 +/- ##
=======================================
Coverage 48.27% 48.27%
=======================================
Files 295 295
Lines 44283 44283
Branches 10601 10601
=======================================
Hits 21377 21377
Misses 18637 18637
Partials 4269 4269Continue to review full report at Codecov.
|
galak
added
platform: STM32
drivers/serial/uart_stm32.c
Outdated
| }; \ | ||
| \ | ||
| static struct uart_stm32_data uart_stm32_data_##name = { \ | ||
| .conf = { \ | ||
| .baudrate = DT_UART_STM32_##name##_BAUD_RATE, \ | ||
| .parity = UART_CFG_PARITY_NONE, \ |
There was a problem hiding this comment.
Do we actually need these fields?
Why wouldn't we be able to use registers for storage and save some RAM?
There was a problem hiding this comment.
Agreed. I'll push something new shortly.
psidhu
force-pushed
the
pr-stm32-uart-update-api
branch
3 times, most recently
from
466b88b
to
5ba966d
Compare
|
@erwango recheck. |
drivers/serial/uart_stm32.c
Outdated
|
|
||
| static int uart_stm32_config_get(struct device *dev, struct uart_config *cfg) | ||
| { | ||
| cfg->baudrate = uart_stm32_get_baudrate(dev); |
There was a problem hiding this comment.
I had a discussion with @erwango long days ago, here is the link: #8306
(don't know why can't copy the link now, so I copied them)
Not sure we need to use data fields to store information, since we can use device registers.
No, you should not do that. We must using an variable to store the baud_rate setted before.
the baud_rate we read using LL_USART_GetBaudRate maybe not equal the baud_rate setted using LL_USART_SetBaudRate because the BRR is inaccurate for some specify baud rate.
eg: I had set uart's baud rate to 115200, but the baudrate readed is 115107.
332 baud_rate = LL_USART_GetBaudRate(UartInstance, clock_rate, LL_USART_OVERSAMPLING_16);
(gdb) p baud_rate
$1 = 115107
So in my options, we should use a variable to save the baud rate.
There was a problem hiding this comment.
Right, my comment was for other fields (parity and so on), I guess you agree for those ?
There was a problem hiding this comment.
I didn't realize that could occur. I've pushed new code, please recheck.
Implement these two new api calls. Allows on-the-fly configuration adjustment of uarts. Signed-off-by: Pushpal Sidhu <[email protected]>
psidhu
force-pushed
the
pr-stm32-uart-update-api
branch
from
5ba966d
to
200775f
Compare
| { | ||
| struct uart_stm32_data *data = DEV_DATA(dev); | ||
| USART_TypeDef *UartInstance = UART_STRUCT(dev); | ||
| const u32_t parity = uart_stm32_cfg2ll_parity(cfg->parity); |
There was a problem hiding this comment.
Can you check if we can save some code footprint by getting uart_stm32_cfg2ll_xxx functions to return and error code if requested valued is not supported and remove the checks below?
There was a problem hiding this comment.
I did a quick and dirty test with hello world on the nucleo_l4r5zi:
;; as is code
Memory region Used Size Region Size %age Used
FLASH: 13304 B 2 MB 0.63%
SRAM: 4372 B 640 KB 0.67%
IDT_LIST: 120 B 2 KB 5.86%
;; With cfg2ll functions returning error codes
Memory region Used Size Region Size %age Used
FLASH: 13320 B 2 MB 0.64%
SRAM: 4372 B 640 KB 0.67%
IDT_LIST: 120 B 2 KB 5.86%
I think the increase in codesize is due to error checks still needing to occur (to check the error of the cfg2ll functions), but now having more return codes out of those functions.
Implement these two new api calls. Allows on-the-fly configuration
adjustment of uarts.
Signed-off-by: Pushpal Sidhu [email protected]