Attention is currently required from: Raul Rangel, Jason Nien, Jon Murphy, Martin Roth, Tim Van Patten, Mark Hasemeyer.
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/74592 )
Change subject: mb/google/skyrim: Add named GPIO's ......................................................................
Patch Set 2:
(1 comment)
File src/mainboard/google/skyrim/variants/baseboard/gpio.c:
https://review.coreboot.org/c/coreboot/+/74592/comment/ffbe06d7_c66d67e9 PS2, Line 10: SYS_RST_ODL
One of the nice things about using the numbers is that we can quickly glance at the table to make su […]
I prefer using SoC GPIO names over net names in the GPIO table for multiple reasons: 1) It aligns well with the action/operation being performed i.e. configuring SoC GPIO is more intuitive than configuring mainboard netnames. 2) Net names may not exist on certain designs - eg. reference design may have WWAN net names but individual variants/OEM designs may not use WWAN at all. Similarly some net names might be used in OEM designs and are not used in reference designs eg. PRIVACY_SCREEN_PROTECT 3) We move net names around sometimes - eg. in Guybrush we moved GSC_SOC_INT_L because some other GPIO was chosen incorrectly.
I agree with human readability part. If I am not wrong, CB:74512 probably is a major factor behind this change where we could have caught the issue during code-review if it were netnames instead of GPIO_NN. If so, we can just pick the updates to variants/baseboard/include/baseboard/gpio.h from this change and use them where we are configuring a hardware block - eg. in devicetree, port descriptors etc.
-
Karthik Ramasubramanian (Code Review)