Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 8: Code-Review-1
(5 comments)
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/51nb.c File src/ec/51nb/51nb.c:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/51nb.c@23 PS8, Line 23: /* Enable function 5 (PS/2 AUX) */ looks like superio stuff and should be moved to src/superio/51nb/ Is it a common superio, like ITE?
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Kconfig File src/ec/51nb/Kconfig:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Kconfig@43 PS8, Line 43: 0x00200000 is that the offset of the IFD ec region?
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Makefile.inc File src/ec/51nb/Makefile.inc:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/Makefile.inc@25 PS8, Line 25: $(51NB_EC_INSERT) $(obj)/coreboot.pre \ is the IFD ec region in use?
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/ec.h File src/ec/51nb/ec.h:
https://review.coreboot.org/#/c/32531/8/src/ec/51nb/ec.h@20 PS8, Line 20: #define SETUP_COMMAND 0x4e probably mainboard specific
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/hda_verb.c File src/mainboard/51nb/x210/hda_verb.c:
https://review.coreboot.org/#/c/32531/8/src/mainboard/51nb/x210/hda_verb.c@2... PS8, Line 25: no need for this file. use soc/intel/common/block/hda/hda.c instead