Attention is currently required from: Dinesh Gehlot, Eric Lai, Kapil Porwal, Nick Vaccaro, Subrata Banik.
Varshit Pandya has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81626?usp=email )
Change subject: mb/google/brya: Add new baseboard trulo ......................................................................
Patch Set 2:
(6 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/81626/comment/2522e74d_eac3fc4d : PS2, Line 11: would be good if you could add the "TEST" info
File src/mainboard/google/brya/Kconfig:
https://review.coreboot.org/c/coreboot/+/81626/comment/c812be61_8a8faf30 : PS2, Line 128: config BOARD_GOOGLE_BASEBOARD_TRULO : def_bool n : select BOARD_GOOGLE_BRYA_COMMON : select CHROMEOS_DRAM_PART_NUMBER_IN_CBI if CHROMEOS : select SOC_INTEL_ALDERLAKE_PCH_N : select SYSTEM_TYPE_LAPTOP all the baseboard related config are in alphabetical order, may be move this after the "BOARD_GOOGLE_BASEBOARD_NISSA" ? (start from line 102)
File src/mainboard/google/brya/variants/baseboard/trulo/gpio.c:
https://review.coreboot.org/c/coreboot/+/81626/comment/62e80ecb_d2bf28bf : PS2, Line 1: empty line after the line containing license information, just like how you did for ec.h file.
https://review.coreboot.org/c/coreboot/+/81626/comment/2e728f0b_c0bbd5d1 : PS2, Line 5: #include <types.h> : #include <soc/gpio.h> sort them in alphabetical order ?
https://review.coreboot.org/c/coreboot/+/81626/comment/4d0d9940_ceb51a89 : PS2, Line 8: /* Pad configuration in ramstage */ I would put an empty line after include file list ending (line 8 should be an empty line)
File src/mainboard/google/brya/variants/baseboard/trulo/include/baseboard/ec.h:
https://review.coreboot.org/c/coreboot/+/81626/comment/c45b893b_4a60115e : PS2, Line 6: #include <ec/ec.h> : #include <ec/google/chromeec/ec_commands.h> : #include <baseboard/gpio.h> sort them alphabetically ..