Attention is currently required from: Raul Rangel, Jason Nien, Martin Roth, Tim Van Patten, Karthik Ramasubramanian, Mark Hasemeyer.
Jon Murphy 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/15599dc3_ca0bb96c PS2, Line 10: SYS_RST_ODL
I prefer using SoC GPIO names over net names in the GPIO table for multiple reasons: […]
1. If you have the net name, the desired operation for what should happen to the GPIO becomes more apparent. 2. If the net name is different, you can update the mapping and override the init. We already do that with the GPIO_NN macros. You can see where I have done this in src/mainboard/google/skyrim/variants/frostflow/include/variant/gpio.h/c 3. Same as above. if we move the net name, we'll want to initialize the GPIO in a similar/the same way. We change the GPIO mapping in baseboard/gpio.h or variant/gpio.h and verify that the init is correct. This provides a single source of truth for the GPIO allocation, makes updates to GPIO numbers all occur in a single place, and drastically improves readability.
The catalyst for this change was CB:74512, but something along these lines has come up multiple times. Code review with GPIO_NN is cumbersome and lacks some meaning. Having random GPIO_NN designations throughout the code without the context of the net name is error prone and can lead to mistakes not only in selecting the correct GPIO, but in updating all of the places it's referenced in the code.