Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39502 )
Change subject: mb/google/deltaur: add deltaur mainboard initial support ......................................................................
Patch Set 12:
(7 comments)
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/Kconfig:
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 46: if !BOARD_GOOGLE_DELTAUR Why? I see an overridetree.cb for deltaur in this CL.
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/baseboard/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 11: smm-y += gpio.c Is this required?
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltan/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 10: romstage-y += gpio.c : verstage-y += gpio.c Are these required?
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltan/gpio.c:
PS11: Same comments as deltaur/gpio.c
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltaur/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 8: romstage-y += gpio.c : verstage-y += gpio.c Are these required?
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... File src/mainboard/google/deltaur/variants/deltaur/gpio.c:
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 16: variant_base_gpio_table This would be coming from the baseboard, right? And variant should really be providing only an override if required.
https://review.coreboot.org/c/coreboot/+/39502/11/src/mainboard/google/delta... PS11, Line 33: static const struct cros_gpio cros_gpios[] = { : }; : : const struct cros_gpio *variant_cros_gpios(size_t *num) : { : *num = ARRAY_SIZE(cros_gpios); : return cros_gpios; : } Do we need to add this here? I believe we can use whatever baseboard default is?
-
Furquan Shaikh (Code Review)