Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34337 )
Change subject: soc/intel/common: add PAD_CFG_NF_BUF_TRIG macro ......................................................................
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 4:
Patch Set 3:
I personally find the GPIO macros very cryptic, and would prefer something more like sandybridge and ivybridge's gpio.c if possible.
Did you mean to use separate structures instead of macros? As it is done in https://github.com/coreboot/coreboot/blob/master/src/mainboard/asrock/b75pro...
Honestly, macros look better to me ) But that's my personal opinion
One of the advantages of using structures like the one you linked to is that you have to assign values to named members. If you forget one of the elements in an array, the rest of the values would get shifted downwards.
https://github.com/coreboot/coreboot/blob/master/src/soc/intel/common/block/... In the case of macros, I get the gpio structure from the pad value.
If I have not configured a pad, then it will be defined as NC. GPIO logic is disconnected from the pad output. For example coffeelake_rvp: https://github.com/coreboot/coreboot/blob/master/src/mainboard/intel/coffeel... Please correct me if I'm mistaken.
And that can be potentially fatal to the GPIO pins: if a pin used as input gets configured as an output, short-circuits can happen!
But, if I miss the .gpio16 = GPIO_DIR_INPUT element, it will be configured as an output. Any mistake in both cases will be fatal.
I was thinking of Baytrail, which is using a different kind of macros. I know the Baytrail macros are position-dependent, but am not sure about the ones you linked (I have no board which uses that code). My observations on Baytrail may not apply to these macros, but it would be nice to check.
At least on Baytrail, GPIO muxes default to function 0, which may or may not be a GPIO. However, GPIOs seem to default to inputs. In addition, GPIO_NC on Baytrail is defined as GPIO_OUT_HIGH :) It makes sense if you consider unconnected pins set as inputs will float, though. Floating inputs can act as antennas and activate randomly, which is not desired.
If you miss '.gpio16 = GPIO_DIR_INPUT', only GPIO16 will be affected. However, with the Baytrail GPIO array, GPIO16 would end up with the settings for GPIO17, GPIO17 would end up with the settings for GPIO18... That could cause a lot of havoc.
In any case, this is not a blocker for this change.