Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 37: Code-Review+1
(11 comments)
https://review.coreboot.org/c/coreboot/+/32531/37/Documentation/mainboard/51... File Documentation/mainboard/51nb/x210.jpg:
PS37: Please put this image into https://tinyjpg.com/ for space savings of 26%
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ac... File src/mainboard/51nb/x210/acpi/battery.asl:
PS37: Is this file using spaces? The other ACPI files seem affected as well
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/bo... File src/mainboard/51nb/x210/board.fmd:
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/bo... PS37, Line 6: FLASH@0xff800000 0x800000 FLASH 8M
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/de... File src/mainboard/51nb/x210/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/de... PS37, Line 59: 3 SaGv_Enabled
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/de... PS37, Line 98: 0x0 Just 0 (apply everywhere)
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/de... PS37, Line 229: nit: align the "on" keywords?
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ds... File src/mainboard/51nb/x210/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ds... PS37, Line 17: Should be making use of arch/acpi.h
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ds... PS37, Line 30: global Either drop the comment, or capitalize `Global`
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ds... PS37, Line 33: // CPU This comment provides little value
https://review.coreboot.org/c/coreboot/+/32531/37/src/mainboard/51nb/x210/ds... PS37, Line 36: Scope (_SB) { : Device (PCI0) Device (_SB.PCI0)
This also solves the inconsistency of the opening braces
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... File src/mainboard/51nb/x210/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32531/33/src/mainboard/51nb/x210/hd... PS33, Line 39: /* Pin Complex (NID 0x19) */
ideally someone would add which pin is what, as with other boards
Ideally, I've yet to see that happen, though... One could also use higher-level macros as well.
In any case, this can be done later, so I would remove the comments for now.