Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41390 )
Change subject: mainboard/lenovo/x230: Add ThinkPad x230s as a variant ......................................................................
Patch Set 16: Code-Review+1
(20 comments)
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@12 PS16, Line 12: pci-e PCIe
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@13 PS16, Line 13: n Capitalize all elements of the list?
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@14 PS16, Line 14: tpm TPM
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@18 PS16, Line 18: DIMM 8GiB 8 GiB SO-DIMM
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@20 PS16, Line 20: usb2 USB2
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@20 PS16, Line 20: pci-e PCIe
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@21 PS16, Line 21: sata SATA
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@21 PS16, Line 21: usb2 USB2
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@21 PS16, Line 21: two spaces
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@21 PS16, Line 21: wwan WWAN
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@22 PS16, Line 22: pci-e PCIe
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@24 PS16, Line 24: graphic I'd use plural here, "graphics init"
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@27 PS16, Line 27: Thinkpad ThinkPad
https://review.coreboot.org/c/coreboot/+/41390/16//COMMIT_MSG@30 PS16, Line 30: Seabios SeaBIOS
And it fits on the previous line
https://review.coreboot.org/c/coreboot/+/41390/16/Documentation/mainboard/le... File Documentation/mainboard/lenovo/x230s.md:
https://review.coreboot.org/c/coreboot/+/41390/16/Documentation/mainboard/le... PS16, Line 1: missing "ThinkPad"
https://review.coreboot.org/c/coreboot/+/41390/16/Documentation/mainboard/le... PS16, Line 11: Its I assume the "Its" possessive pronoun refers to "the ThinkPad X230s", but it is not clear at all.
Instead, I would use "The".
https://review.coreboot.org/c/coreboot/+/41390/16/Documentation/mainboard/le... PS16, Line 13: mine not sure if documentation should refer to the author. It's not directly visible in doc.cb.o who wrote what part of it.
https://review.coreboot.org/c/coreboot/+/41390/16/src/mainboard/lenovo/x230/... File src/mainboard/lenovo/x230/Kconfig:
https://review.coreboot.org/c/coreboot/+/41390/16/src/mainboard/lenovo/x230/... PS16, Line 22: !BOARD_LENOVO_X230S That there are mixed `!BOARD_LENOVO_X230S` and `BOARD_LENOVO_X230 || BOARD_LENOVO_X230T` is odd.
https://review.coreboot.org/c/coreboot/+/41390/16/src/mainboard/lenovo/x230/... File src/mainboard/lenovo/x230/variants/x230s/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/41390/16/src/mainboard/lenovo/x230/... PS16, Line 17: 0x0065 0x0065 = 101, this is redundant
https://review.coreboot.org/c/coreboot/+/41390/16/src/mainboard/lenovo/x230/... PS16, Line 23: # Huh? An empty comment?