Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34497 )
Change subject: mb/google/drallion: Add new mainboard ......................................................................
Patch Set 9:
(4 comments)
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... File src/mainboard/google/drallion/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... PS9, Line 32: CONFIG_EC_GOOGLE_WILCO Nit: I think these could just be changed to '-y' I don't think the board would build without wilco ec being selected. If I'm wrong, please feel free to ignore this.
Or ignore it even if I'm right. It's just a nit.
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... File src/mainboard/google/drallion/dsdt.asl:
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... PS9, Line 62: #if CONFIG(EC_GOOGLE_WILCO) Nit: You're selecting EC_GOOGLE_WILCO in the kconfig. You have calls to various wilco ec functions all over the mainboard. I doubt it would build without the EC_GOOGLE_WILCO enabled, so this seems like a useless #if.
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... File src/mainboard/google/drallion/ramstage.c:
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... PS9, Line 25: #if CONFIG(GENERATE_SMBIOS_TABLES) Is this guard needed? Wouldn't they just not get called?
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... File src/mainboard/google/drallion/variants/drallion/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/34497/9/src/mainboard/google/dralli... PS9, Line 210: "MISCCFG_GPSIDEDPCGEN | MISCCFG_GPRTCDLCGEN | MISCCFG_GSXSLCGEN | MISCCFG_GPDPCGEN | MISCCFG_GPDLCGEN" Nit: Maybe wrap this line?