Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34188 )
Change subject: mb/google/octopus/variants/garg: support LTE power sequence. ......................................................................
Patch Set 6:
(5 comments)
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/34188/6//COMMIT_MSG@8 PS6, Line 8: Can you please add some description about the power sequence requirements and timing here?
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 97: I think we should not remove the configuration for GPIO_67, GPIO_117 for the benefit of non-LTE skus. Otherwise atleast on Garg, these gpios will be left driving 1. Rather we should override the configuration in Garg to continue driving 1 for LTE skus.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/gpio.c:
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: ) Not sure if we need paranthesis i.e if we need this to look like a function.
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 26: , \ You don't need this in the last line of a macro. Also use the comma at the place where you are referring to the macro
https://review.coreboot.org/c/coreboot/+/34188/6/src/mainboard/google/octopu... PS6, Line 22: #define DEFAULT_OVERRIDE_GPIOS() PAD_NC(GPIO_104, UP_20K), \ : /* EN_PP3300_TOUCHSCREEN */ \ : PAD_CFG_GPO_IOSSTATE_IOSTERM(GPIO_146, 0, DEEP, NONE, Tx0RxDCRx0, \ : DISPUPD), \ : PAD_NC(GPIO_213, DN_20K), \ : Prefer introducing this macro as a separate CL and place this CL on top of that CL.