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/ab6c3a99_93fe6af7 PS2, Line 10: SYS_RST_ODL
- 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.
We moved netnames between build phases themselves - eg. GSC_SOC_INT moved from GPIO_X to GPIO_Y within guybrush phase1 and pahse2 itself. Not between guybrush and nipperkin. So phase specific overrides will have GPIO_NN still.
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
We are not configuring the GPIOs in random places in the code. They are all done in the GPIO table with comments above indicating their purpose.
Using GPIO_NN is prone to bugs when configuring the hardware blocks themselves. Net names definitely come in handy there. So I prefer to limit the use-case there before introducing another set of macros to configure the GPIOs themselves.
-
Karthik Ramasubramanian (Code Review)