Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32788 )
Change subject: soc/intel/common/block/gpio: Add gpio_pm_configure() function ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/gpio.h:
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 25: : union gpio_misc_pm { : uint8_t config; : struct { : uint8_t gdlcgen : 1; : uint8_t gpdpcgen : 1; : uint8_t gsxslcgen : 1; : uint8_t gprtcdlcgen : 1; : uint8_t gprcompcdlcgen : 1; : uint8_t gpsidedpcgen : 1; : uint8_t reserved : 2; : } bit; : }; : I don't see this used anywhere in the commit; what is this for?
https://review.coreboot.org/#/c/32788/6/src/soc/intel/common/block/include/i... PS6, Line 231: /* The function performs GPIO Power Management programming. */ : void gpio_pm_configure(uint8_t *misccfg_pm_mask, uint8_t *misccfg_pm_value); It is not obvious that this function takes arrays of data, expecting them to contain 'number of GPIO communities' elements in length. Can it take a length parameter too? Also if the data is not changing, const pointers would be preferred, to indicate the arrays are not modified.