Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30360 )
Change subject: src/mainboard/libretrend/lt1000: Initial commit ......................................................................
Patch Set 6:
(7 comments)
https://review.coreboot.org/#/c/30360/6/src/ec/libretrend/lt1000/Kconfig File src/ec/libretrend/lt1000/Kconfig:
https://review.coreboot.org/#/c/30360/6/src/ec/libretrend/lt1000/Kconfig@1 PS6, Line 1: EC_LIBRETREND_LT1000 this symbol is unused.
https://review.coreboot.org/#/c/30360/3/src/mainboard/libretrend/lt1000/Kcon... File src/mainboard/libretrend/lt1000/Kconfig:
https://review.coreboot.org/#/c/30360/3/src/mainboard/libretrend/lt1000/Kcon... PS3, Line 63: onfig CPU_MICROCODE_CBFS_LOC : hex : default 0xFFE115A0 : : config CBFS_SIZE : hex : default 0x5c0000 if you are not using FSP-T this is not needed.
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/Kcon... File src/mainboard/libretrend/lt1000/Kconfig:
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/Kcon... PS6, Line 10: select NO_FIXED_XIP_ROM_SIZE : select FSP_M_XIP This is already done in the soc Kconfig.
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/Kcon... PS6, Line 32: : config DEVICETREE : string : default "devicetree.cb" No need to redefine the default value.
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/Make... File src/mainboard/libretrend/lt1000/Makefile.inc:
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/Make... PS6, Line 16: $(CONFIG_SUPERIO_ITE_COMMON_ROMSTAGE) why is this conditional on this board?
https://review.coreboot.org/#/c/30360/3/src/mainboard/libretrend/lt1000/devi... File src/mainboard/libretrend/lt1000/devicetree.cb:
https://review.coreboot.org/#/c/30360/3/src/mainboard/libretrend/lt1000/devi... PS3, Line 283: io 0x60 = 0x3e8 : irq 0x70 = 7 : irq 0xf0 = 0x01 : irq 0xf1 = 0x53 do you need to set these resources if the device ends up being disabled?
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/hda_... File src/mainboard/libretrend/lt1000/hda_verb.h:
https://review.coreboot.org/#/c/30360/6/src/mainboard/libretrend/lt1000/hda_... PS6, Line 42: /* Pin Complex (NID 0x12) */ Those comments don't seem to add useful information.
-
Arthur Heymans (Code Review)