Attention is currently required from: Máté Kukri (mkukri), Michael Niewöhner, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55250 )
Change subject: [WIP] src/superio/smsc: Add support for the SCH5555 ......................................................................
Patch Set 1:
(5 comments)
File src/superio/smsc/sch5555/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/55250/comment/b7b28c4c_f79a2334 PS1, Line 3: sch5555_ I'd drop the `sch5555_` prefix for .c files, they're in a folder named `sch5555`.
https://review.coreboot.org/c/coreboot/+/55250/comment/28aa89c5_710321ed PS1, Line 4: sch5555_ramstage.c
does not exist -> remove
Even better: add the file and uncomment the entry.
File src/superio/smsc/sch5555/sch5555.h:
https://review.coreboot.org/c/coreboot/+/55250/comment/d007ccca_d0e1b0e4 PS1, Line 82: uint8_t uart_no I would use the LDN or PNP device here, like other Super I/Os (ITE/Nuvoton/Winbond) do
File src/superio/smsc/sch5555/sch5555_bootblock.c:
https://review.coreboot.org/c/coreboot/+/55250/comment/a6d51a23_e1c9201c PS1, Line 2:
- no need to repeat the name in the filename […]
early_init.c is usually for code compiled in both bootblock and romstage.
https://review.coreboot.org/c/coreboot/+/55250/comment/a4848e36_b8f3f72c PS1, Line 15: smsc_write8(LDN_SELECT, LDN_GLOBAL);
I'd make a helper function for this: […]
See Michael's comment
-
Angel Pons (Code Review)