Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31634 )
Change subject: util/inteltool: Improve GPIO specification ......................................................................
Patch Set 10:
(4 comments)
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h File util/inteltool/gpio.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@34 PS10, Line 34: * Modes: : * -2 = GP-Out : * -1 = GP-In : * 0 = Undefined : * >0 = Native functions this should be an `enum`.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio.h@44 PS10, Line 44: uint32_t config0; : uint32_t config1; So these would be used to cache the register content for later processing? or for default values?
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h File util/inteltool/gpio_test.h:
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@24 PS10, Line 24: .name What do we gain from repeating the field identifiers in each row? IMO, it only wastes space.
https://review.coreboot.org/#/c/31634/10/util/inteltool/gpio_test.h@26 PS10, Line 26: FUNC(3, "ESPI_IO0") } }, Personally, I prefer more condensed tables where a row fits a single line. It feels easier to review. But that's just my opinion. Let's hear others, too.