[coreboot-gerrit] [XL] Change in coreboot[main]: soc/intel/ptl: Add GPIOs for Panther Lake SOC

[Subrata Banik (Code Review)]

Attention is currently required from: Anil Kumar K, Bora Guvendik, Cliff Huang, Elyes Haouas, Jamie Ryu, Jérémy Compostella, Kapil Porwal, Paul Menzel, Pranava Y N, Ravishankar Sarawadi, Saurabh Mishra, Wonkyu Kim.

Subrata Banik has posted comments on this change by Cliff Huang. ( https://review.coreboot.org/c/coreboot/+/83789?usp=email )

Change subject: soc/intel/ptl: Add GPIOs for Panther Lake SOC
......................................................................

Patch Set 55:

(11 comments)

File src/soc/intel/pantherlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/83789/comment/faa6851c_d3b6a6cc?usp... :
PS17, Line 55: 
             : 	Name (_DSD, Package (0x04)  // _DSD: Device-Specific Data
> Subrata,  […]

Acknowledged

File src/soc/intel/pantherlake/acpi/gpio.asl:

https://review.coreboot.org/c/coreboot/+/83789/comment/a6296c6b_5a379ed9?usp... :
PS55, Line 18: {
one space

https://review.coreboot.org/c/coreboot/+/83789/comment/9be5a82c_12828168?usp... :
PS55, Line 20: // Interrupt IRQ_EN
remove this comment

https://review.coreboot.org/c/coreboot/+/83789/comment/e44a6de2_c839a5de?usp... :
PS55, Line 28:  // 
use `/* ... */` comment style

https://review.coreboot.org/c/coreboot/+/83789/comment/73e8a168_03cf9ed5?usp... :
PS55, Line 58: //
remove this comment

https://review.coreboot.org/c/coreboot/+/83789/comment/afcd4425_c0e055f1?usp... :
PS55, Line 60: /* Device Properties for _DSD */,
don't need this. if you would like to explain what is special about this GUID, then please use the comment section to specify here.

Commenting for one GPIO comm is enough IMO

https://review.coreboot.org/c/coreboot/+/83789/comment/e457806b_ffb08404?usp... :
PS55, Line 84: 0x10
can you please help me to understand what this field actually implies to ?

https://review.coreboot.org/c/coreboot/+/83789/comment/9f9012f1_c77546e2?usp... :
PS55, Line 135: GPPV
may  be worth adding a comment saying, this section explains each GPIO bank, for example: GPIO_0 COMM has two GPIO bank like GPP_V and GPP_C.

File src/soc/intel/pantherlake/include/soc/gpio_defs.h:

https://review.coreboot.org/c/coreboot/+/83789/comment/98686cb9_d222aec7?usp... :
PS55, Line 256: 
no need to have an empty line

https://review.coreboot.org/c/coreboot/+/83789/comment/47873f07_bab4e366?usp... :
PS55, Line 257: GPP_V
can you use `_start_` in between to make it clear?

for example:

`GPP_V_START_OFFSET`

File src/soc/intel/pantherlake/include/soc/gpio_soc_defs.h:

https://review.coreboot.org/c/coreboot/+/83789/comment/c251470b_e67cc33e?usp... :
PS55, Line 36: Community0
is there any specific syntax to follow here?

if not then can you please use `Community 0`