Hannah Williams has posted comments on this change. ( https://review.coreboot.org/18892 )
Change subject: soc/intel/common/block:[WIP] Create header for GPIO controller config ......................................................................
Patch Set 8:
(6 comments)
https://review.coreboot.org/#/c/18892/8/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
Line 21: #include <types.h>
In looking through existing released SoCs and forthcoming ones, what is dif
What I found was that newer ones were only trying to use the reserved bits from before. Hence backwards compatibility is not an issue. We can have PAD_CONFIG0_MASK and PAD_CONFIG1_MASK in soc code to leave out the reserved bits which would be different for different SOCs. However I do not know about forthcoming ones. I only compared EDS of SKL, APL and GLK
Line 50: GPIO_D = 1, // GPIO driver
Not everything in this file is tab indented.
will fix
Line 86: #define M(mode) (mode)
What is this for? Needing a define from soc/gpio.h?
the number of bits used for mode is different in different SOCs
Line 250: #define GLK_GPIO_PAD1_CONF(ios_state, term_H_L, ios_term) \
So this is gemini lake only?
I am missing INTSEL - on adding that it should be common and not just GLK
Line 266: const char *pad_name;
This is a huge structure to maintain per pad config. 24 bytes per pad?
I can put pad_name in only for debug coreboot
Line 281: typedef void(*config_write)(int pad, int reg_offset, uint32_t value);
Why the need for typedefs?
I was going to remove the direct access to iosf_read\write and let the SOC layer decide the read\write access function. Not that it is anything other than iosf read\write today but what if a different access type is needed in the future