Bora Guvendik 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 14:
(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.
Removed overridetree.cb for deltaur, it shouldn't be there.
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?
Done
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?
Nope, done.
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. […]
Done
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?
Done
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 overr […]
You are right, changed that for deltan variant and removed deltaur/gpio.c since not needed.
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?
Done