Nico Huber has posted comments on this change. ( https://review.coreboot.org/20224 )
Change subject: serial: Support custom baud rates on linux ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/#/c/20224/2/custom_baud.c File custom_baud.c:
https://review.coreboot.org/#/c/20224/2/custom_baud.c@56 PS2, Line 56: (void)baud; Do we have warnings for unused parameters enabled? I'm really curious, usually I have to do with compilers that warn about statements with no effect. ;)
https://review.coreboot.org/#/c/20224/2/custom_baud.c@57 PS2, Line 57: return 0; `return -1;`? Just in case somebody calls it by accident? I wouldn't expect it, just an idea to set a good pattern.
https://review.coreboot.org/#/c/20224/2/serial.c File serial.c:
https://review.coreboot.org/#/c/20224/2/serial.c@145 PS2, Line 145: Superfluous space.
(yeah, I know you just copied it)
https://review.coreboot.org/#/c/20224/2/serial.c@155 PS2, Line 155: #endif You might have noticed that I'm not a fan of guards in code files... I would accept it as is, but here's another idea: Make that baudtable a parameter and move the whole function into custom_baud.c where we already have the Linux/non-Linux separation.
Also, grouping the stubs together, would make it more obvious why the set_custom_baudrate() stub is never called.
https://review.coreboot.org/#/c/20224/2/serial.c@218 PS2, Line 218: msg_perr_strerror("Could not set custom baudrate: "); We should bail out or fallback?
-
Nico Huber (Code Review)