Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38620 )
Change subject: mb/google/volteer: add volteer mainboard initial support ......................................................................
Patch Set 15: Code-Review+1
(16 comments)
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/baseboard/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 148: nit: tab is missing.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... File src/mainboard/google/volteer/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 27: 1 Note for future change: EN_PP3300_TOUCHSCREEN should be set to 0 here and the power to touchscreen should be enabled in ACPI
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 96: DN_20K This is not required.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 106: DN_20K Why is there a PD? Is that the default state of the pad? In general, I think for all PAD_NC we can keep the internal termination to NONE.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 160: UP_20K Why is an internal pull-up being configured?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 162: UP_20K same here?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 164: UP_20K same here?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 178: DN_20K This is not required.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 233: DN_20K Why? I think this can simply be PAD_CFG_GPO.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 237: DN_20K Why?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 265: /* F9 : Reserved ==> NC */ Why is there no GPP_F9 configuration here?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 278: /* PAD_CFG_GPI_SCI_LOW(GPP_F15, NONE, PLTRST, EDGE_SINGLE), */ Not needed?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 337: PLTRST Why PLTRST and not DEEP?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 354: DN_20K Not required?
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 425: DN_20K Not required.
https://review.coreboot.org/c/coreboot/+/38620/14/src/mainboard/google/volte... PS14, Line 440: RSMRST Why is this RSMRST?