Marco Chen 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 8:
(4 comments)
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:
Personal Opinion: Safe to put it back in the default state for non-LTE SKUs
One reviewer asked to evaluate whether all SKUs of garg can stay in the state of LTE SKUs and one reviewer suggest to play it safe.
My previous comment is also to play it safe if codes is manageable so I choose to follow this way.
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/baseboard/include/baseboard/gpio.h:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 36: /* LTE reset GPIO */ : #define GPIO_LTE_RESET GPIO_161 :
Not required anymore.
Done
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... File src/mainboard/google/octopus/variants/garg/variant.c:
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 37: if (slp_typ != ACPI_S5) : return;
nit: If you do this before calling get_board_sku(), one transaction to the EC can be saved in non-S5 […]
Yes and also add the comment to remind the reason in variant_smi_sleep().
https://review.coreboot.org/c/coreboot/+/34188/7/src/mainboard/google/octopu... PS7, Line 79: power_off_lte_module_if_exist
Fix the indentation issue. May need an additional tab to match alignment with return statement.
Done