Attention is currently required from: Sean Rhodes, Andy Pont, Paul Menzel. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58428 )
Change subject: mb/starlabs/labtop: Add LabTop Mk IV ......................................................................
Patch Set 95: Code-Review+1
(8 comments)
File src/mainboard/starlabs/labtop/Kconfig:
https://review.coreboot.org/c/coreboot/+/58428/comment/37f853a2_e2afd964 PS95, Line 19: EC_STARLABS_FAN Already selected through `BOARD_STARLABS_LABTOP_SERIES`, looks like TGL also has this redundancy. Not sure if it's best to keep the select in the `_SERIES` Kconfig or specify it per-board.
File src/mainboard/starlabs/labtop/variants/cml/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/58428/comment/cb9f0f21_a260e33c PS95, Line 2: # CPU nit: missing a tab
https://review.coreboot.org/c/coreboot/+/58428/comment/1b66ec85_4cec5d2e PS95, Line 79: # Internal Webcam : register "usb2_ports[6]" = "USB2_PORT_MID(OC0)" : # Internal Bluetooth : register "usb2_ports[9]" = "USB2_PORT_MID(OC0)" Internal USB ports shouldn't be assigned any overcurrent pins
File src/mainboard/starlabs/labtop/variants/cml/gpio.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/a1261d57_1e157ba3 PS95, Line 19: PAD_CFG_GPI(GPP_H7, NONE, PLTRST), You probably want to configure the pads for UART #2 here as well
File src/mainboard/starlabs/labtop/variants/cml/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/f85cbddf_1efeef12 PS95, Line 14: 0x10EC1200 nit: drop value from comment, it's in the code already
File src/mainboard/starlabs/labtop/variants/cml/romstage.c:
https://review.coreboot.org/c/coreboot/+/58428/comment/1aed28b4_b10bc4c8 PS95, Line 11: u8 Would `unsigned int` work? I'd like to drop the cast in the returned value.
https://review.coreboot.org/c/coreboot/+/58428/comment/600248bf_1e32de48 PS95, Line 18: ID3 nit: missing opening parenthesis
https://review.coreboot.org/c/coreboot/+/58428/comment/4508448e_b8a05e15 PS95, Line 23: * 1 1 1 Samsung 4G single channel : * 1 1 0 Samsung 8G dual channel : * 1 0 1 Micron 4G single channel : * 1 0 0 Micron 8G dual channel : * 0 1 1 Hynix 4G single channel : * 0 1 0 Hynix 8G dual channel : * 0 0 1 Micron 16G dual channel : * 0 0 0 Hynix 16G dual channel nit: Could you please invert the order of the rows? It makes more sense to start with `0 0 0`
-
Angel Pons (Code Review)