Matthew Garrett has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32531 )
Change subject: Add support for the 51nb X210 ......................................................................
Patch Set 8:
(5 comments)
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/ […]
I'm not sure? There's no superio on the board, just the EC, and the EC also handles PS/2. It's using the same LDN functions as the Nuvoton SuperIOs for this purpose, but I can't see any other similarities.
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?
No
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?
No, vendor firmware doesn't use the IFD ec region.
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
No, this is (as far as I can tell) specific to the EC part.
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. […]
Will do.