Christoph Pomaska 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 4:
(6 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 1: if BOARD_GIGABYTE_GA_Z170X_GAMING_7
It doesn't match with the board name defined in your Kconfig.name.
Done
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.
Done
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
Done
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... File src/mainboard/gigabyte/ga-z170x-gaming7/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/38920/2/src/mainboard/gigabyte/ga-z... PS2, Line 2: bool "GA-Z170X-Gaming 7"
I think it should match with your Kconfig's MAINBOARD_PART_NUMBER's default value? Like only "Z170X- […]
Done
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
Done
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
Done