Evgeny Zinoviev 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 24:
(23 comments)
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/Kconfig:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 31: default "pci8086,0166.rom"
Is the VBIOS tested?
Not on this machine yet
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/acpi_tables.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 20: /* Disable USB ports in S3 by default */ : gnvs->s3u0 = 0; : gnvs->s3u1 = 0; : : /* Disable USB ports in S5 by default */ : gnvs->s5u0 = 0; : gnvs->s5u1 = 0;
These can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 28: // the lid is open by default.
Please use C-style comments for consistency
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 2: register "gfx.did" = "{ 0x80000410, 0x80000320, 0x80000410, 0x80000410, 0x00000005 }"
This can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 19: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 27: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 32: 0x0
0
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 44: register "docking_supported" = "0"
is zero, can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 50: register "pcie_hotplug_map" = "{ 0, 0, 0, 0, 0, 0, 0, 0 }"
is zero, can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 66: High Definition Audio Audio
Stereo audio again :^)
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 12: : : #define BRIGHTNESS_UP _SB.PCI0.GFX0.INCB : #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB : #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
Are these used?
No
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 28: /* Some generic macros */
Please remove this comment. I killed it on a recent change.
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 30: : #include <southbridge/intel/bd82x6x/acpi/platform.asl> : /* global NVS and variables. */ : #include <southbridge/intel/bd82x6x/acpi/globalnvs.asl> : #include <southbridge/intel/bd82x6x/acpi/sleepstates.asl>
Some of these are now under common
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 30: Analog
This one is likely to be unused
Done
https://review.coreboot.org/c/coreboot/+/32604/1/src/mainboard/apple/macbook... File src/mainboard/apple/macbookair5_2/gnvs.c:
PS1:
See current patchset
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... File src/mainboard/apple/macbookair5_2/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 21: Cirrus
Cirrus CS4206
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 24: 0x0000000b
Use decimal here
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 25: /* NID 0x01: Subsystem ID. */
All of the "NID" comments are of no use, they can be dropped
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 26: 0x0
Use decimal here as well
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 57: 0x80862806, /* Codec Vendor / Device ID: Intel */
Add a space between the two codecs
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 60: 0x00000004
Also decimal
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 62: 0x3
Also decimal
Done
https://review.coreboot.org/c/coreboot/+/32604/23/src/mainboard/apple/macboo... PS23, Line 75: 0x00270500, /* Set PS to D0 on DAC1 */
Please correct this
Done