Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31619 )
Change subject: soc/intel/cannonlake: Add ASL functions to manipulate RX/TX buffers ......................................................................
Patch Set 2: Code-Review+1
(3 comments)
Header files need some more work to fix Jenkins' issue.
https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... File src/soc/intel/cannonlake/acpi/gpio_op.asl:
https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/acpi/gpio_o... PS2, Line 115: , Arg1 This is storing the intermediate result, right? Seems unnecessary.
The masking, too. We know it is a single bit, so what is there to mask?
https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... File src/soc/intel/cannonlake/include/soc/gpio_defs.h:
https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... PS2, Line 253: #define GPIORXSTATE_MASK 0x1 : #define GPIORXSTATE_SHIFT 1 NB. this seems odd
https://review.coreboot.org/#/c/31619/2/src/soc/intel/cannonlake/include/soc... PS2, Line 261: #define GPIORXBUFDIS_SHIFT 9 The register bits should apply to both PCH-H and PCH-LP, don't they? Maybe `gpio.h` is a better place.