Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38920 )
Change subject: [WIP] mb/gigabyte: Add Gigabyte Z170X-Gaming 7 ......................................................................
Patch Set 2:
(13 comments)
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/Kconfig:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 14: SUPERIO_ITE_IT8528E You're better off selecting IT8728E, really. IT8528E is an EC for laptops and such.
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 32: config DEVICETREE : string : default "devicetree.cb" That's the default
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 190: [4] = 0, \ : [5] = 0, \ Enable these
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 193: # SATA4 and SATA5 are located in the lower right corner : # of the board, but there is no connector for this Smells like copypasta
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 211: # Set params for PEG 0:1:0 Check schematics for PCIe port routing
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 330: chip superio/nuvoton/nct6791d Not at all!
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 34: // CPU Please drop, it does not provide any value.
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/gma-mainboard.ads:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 26: ports : constant Port_List := The rest of Ada code is indented with three spaces
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/gpio.h:
PS2: This should be a gpio.c instead
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 25: static const struct pad_config gpio_table[] = { Are these correct?
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 37: /* Intel, SkylakeHDMI */ Add a blank line between codecs for clarity
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/include/gpio.h:
PS2: why two gpio.h ?
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/mainboard.c:
PS2: This should not be necessary at all for SKL.