Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37483 )
Change subject: mb/gigabyte/ga-h61m-s2pv: Add ga-h61m-ds2v as a variant ......................................................................
Patch Set 1: Code-Review+1
(10 comments)
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/Kconfig:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 4: ## Copyright (C) 2018-2019 Angel Pons th3fanbus@gmail.com Did you add new, creative work, this year?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 34: select NO_UART_ON_SUPERIO if !BOARD_GIGABYTE_GA_H61M_S2PV Would be much easier to read without the inversion.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/variants/ga-h61m-ds2v/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 17: register "gfx.did" = "{ 0x80000100, 0x80000240, 0x80000410 }" : register "gfx.ndid" = "3" Is this useful?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 22: register "c1_battery" = "1" Will the *battery values ever be used?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 70: irq 0xf1 = 0x80 Does this work? I see no entry for this in the respective `superio.c`, but there could be.
Please test after removing all power including the CMOS battery if there is any doubt.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 75: irq 0xf0 = 0x08 Does this work? I see no entry for this in the respective `superio.c`, but there could be.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 79: irq 0x25 = 0x40 : irq 0x26 = 0xf7 : irq 0x27 = 0x10 : irq 0x2c = 0x80 Does this work?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 87: irq 0xc1 = 0x37 : irq 0xcb = 0x00 Does this work?
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 89: irq 0xf0 = 0x10 : irq 0xf1 = 0x42 : irq 0xf6 = 0x1c Does this work? I see no entries for these in the respective `superio.c`, but there could be.
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... File src/mainboard/gigabyte/ga-h61m-s2pv/variants/ga-h61m-ds2v/hda_verb.c:
https://review.coreboot.org/c/coreboot/+/37483/1/src/mainboard/gigabyte/ga-h... PS1, Line 22: 0x0000000f, /* Number of 4 dword sets */ Why write the number in hex? makes review harder.