Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/37459 )
Change subject: hatch: Fix FPMCU pwr/rst gpio handling ......................................................................
Patch Set 2:
(6 comments)
https://review.coreboot.org/c/coreboot/+/37459/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/37459/2//COMMIT_MSG@20 PS2, Line 20: BUG=? BRANCH=hatch TEST=
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 414: 1 Can you do a git blame on this to see why it was set to 1 in the first place? I am suspecting if there was some power leakage that was observed in S5?
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 424: *num = ARRAY_SIZE(default_sleep_gpio_table); : return default_sleep_gpio_table; This can just be: *num = 0; return NULL;
since default_sleep_gpio_table is just empty.
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/helios/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 161: if (slp_typ == ACPI_S5) {
are you fixing s5 or s3? I thought that you were fixing entering/exiting s3.
I believe Craig wants to disable the pads only when entering S5 and leave as is when going to S3.
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 166: *num = ARRAY_SIZE(sleep_gpio_table); : return sleep_gpio_table; Same comment as baseboard.
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... File src/mainboard/google/hatch/variants/kohaku/gpio.c:
https://review.coreboot.org/c/coreboot/+/37459/2/src/mainboard/google/hatch/... PS2, Line 154: *num = ARRAY_SIZE(sleep_gpio_table); : return sleep_gpio_table; same comment as baseboard.