Attention is currently required from: Evgeny Zinoviev, Felix Held.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32604 )
Change subject: mb/apple: Add MacBook Air 5,2 (A1466) support ......................................................................
Patch Set 41: Code-Review+1
(6 comments)
File src/mainboard/apple/macbookair5_2/Kconfig:
https://review.coreboot.org/c/coreboot/+/32604/comment/84a824e6_994e5c69 PS41, Line 31: config MAX_CPUS : int : default 8 Shouldn't be necessary
https://review.coreboot.org/c/coreboot/+/32604/comment/9bd02d81_9e20c568 PS41, Line 40: default "Apple Inc." Sounds like `MAINBOARD_VENDOR` could be changed so that this isn't needed. But not in this patch.
File src/mainboard/apple/macbookair5_2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32604/comment/06467776_d7fc7e3a PS41, Line 33: register "pcie_port_coalesce" = "1" Is this actually needed?
File src/mainboard/apple/macbookair5_2/early_init.c:
https://review.coreboot.org/c/coreboot/+/32604/comment/9895a251_f58c93dc PS41, Line 43: int const unsigned int
https://review.coreboot.org/c/coreboot/+/32604/comment/e86be0d4_00e44a47 PS41, Line 57: switch (ramcfg) { : case 0: : case 1: : spd_index = 0; : break; : case 2: : spd_index = 1; : break; : } : : if (spd_index == -1) : die("Unsupported memory, RAMCFG=%d\n", ramcfg); To avoid a signed-unsigned comparison in `get_spd_data()` between `int spd_index` and `spd_file_len`, the former variable can be made unsigned by refactoring the switch block as follows:
``` switch (ramcfg) { case 0: case 1: spd_index = 0; break; case 2: spd_index = 1; break; default: die("Unsupported memory, RAMCFG=%u\n", ramcfg); } ```
File src/mainboard/apple/macbookair5_2/spd/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/32604/comment/715493f6_47709bf3 PS41, Line 4: = Shouldn't this be `+=` instead?