Attention is currently required from: Tarun Tuli, David Wu, Kangheui Won, Ren Kuo, Tyler Wang, Nick Vaccaro, Eric Lai.
Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/69693 )
Change subject: mb/google/brya/var/craask: Modify GPIOs for craaskneto/craaskino ......................................................................
Patch Set 6:
(4 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/69693/comment/50ba9ce2_6426fa98 PS6, Line 7: brya nit: nissa
File src/mainboard/google/brya/variants/craask/fw_config.c:
https://review.coreboot.org/c/coreboot/+/69693/comment/f60ca872_f5d46d70 PS6, Line 20: _extern `_extend` to be consistent with gpio.c?
https://review.coreboot.org/c/coreboot/+/69693/comment/88d63256_0a9c1d91 PS6, Line 68: if (id < 0x20) { Since the LTE pads are the only ones that differ, can you put the if statement around the LTE block only? See nivviks for an example.
https://review.coreboot.org/c/coreboot/+/69693/comment/5ed0af26_45413370 PS6, Line 103: gpio_padbased_override(padbased_table, wfc_disable_pads,
Since you're disabling MIPI WFC in both cases ((id >= 0x20) || (id < 0x20)), why not just set the pa […]
For (id < 0x20), they're only disabled if the fw_config is not set, so I think we still need it in fw_config.c.
But I don't think we need to handle the (id < 0x20) and (id >= 0x20) cases differently. If the WFC is not present on (id >= 0x20), the fw_config should not be set. So we can move this outside the `if (id < 0x20)` statement, as I suggested above.