Nico Huber has posted comments on this change. ( https://review.coreboot.org/20224 )
Change subject: serial: support custom baud rates on linux ......................................................................
Patch Set 1:
(10 comments)
With a generic set_custom_baudrate() and guarding done in the header file, the code could be free of guards. See inline comments.
https://review.coreboot.org/#/c/20224/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@7 PS1, Line 7: support Upper-case "Support" please.
https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@9 PS1, Line 9: the Upper-case...
https://review.coreboot.org/#/c/20224/1//COMMIT_MSG@9 PS1, Line 9: because : broken because *of* broken?
https://review.coreboot.org/#/c/20224/1/linux_baud.h File linux_baud.h:
PS1: Missing include guards and license header.
https://review.coreboot.org/#/c/20224/1/linux_baud.h@1 PS1, Line 1: int linux_set_custom_baudrate(int fd, int speed); Why not call it generically `set_custom_baudrate`? You could provide a stub then in case `#if IS_LINUX != 1` and wouldn't have to bother with the guard in the code.
https://review.coreboot.org/#/c/20224/1/linux_baud.c File linux_baud.c:
https://review.coreboot.org/#/c/20224/1/linux_baud.c@35 PS1, Line 35: * for more info. */ Urgh, who invented this comment style? Not your fault, I know this style is common in flashrom. Though, I'd prefer the style like used above for the license header, with the dangling /* and */ on separate lines.
Also, I wouldn't place URLs in the code. They tend to break. Rather put them into the commit message?
https://review.coreboot.org/#/c/20224/1/serial.c File serial.c:
https://review.coreboot.org/#/c/20224/1/serial.c@44 PS1, Line 44: #if IS_LINUX Guarding a #include shouldn't be necessary.
https://review.coreboot.org/#/c/20224/1/serial.c@126 PS1, Line 126: * so setting a custom rate can be attempted. */ How about a use_custom_baud() function instead, that returns true in case the requested baud rate isn't present in the sp_baudtable? That is_custom_baud() could just always return false `#if IS_LINUX != 1`.
https://review.coreboot.org/#/c/20224/1/serial.c@210 PS1, Line 210: if (baud >= 0) { With the mentioned use_custom_baud() and an always present set_custom_baudrate() the code flow could be free of guards and look as follows:
if (use_custom_baud()) { ... if (set_custom_baudrate()) ... ... } else { const struct baudentry *entry = round_baud(baud); if (cfsetispeed...) { ... } }
https://review.coreboot.org/#/c/20224/1/serial.c@257 PS1, Line 257: Was this line break added intentionally?