[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 66:

(5 comments)

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

https://review.coreboot.org/c/coreboot/+/83789/comment/b94d7299_985f199c?usp... :
PS49, Line 436: Name (JTAG, Package (0x02)
this is mostly for FW debugging. why would kernel gpio controller driver would like to expose via ACPI. IMO, we should only expose GPIO PINs those are meaningful for kernel driver bindings and configuration. There is absolute no need to expose JTAG native PADs for kernel.

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

https://review.coreboot.org/c/coreboot/+/83789/comment/29916c31_338ffba1?usp... :
PS55, Line 135: GPPV
> Acknowledged

I don't see a comment section as request and you have marked resolved?

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

https://review.coreboot.org/c/coreboot/+/83789/comment/8bd01501_553ced98?usp... :
PS65, Line 770: GPIO
`VGP0` aka virtual GPIO

https://review.coreboot.org/c/coreboot/+/83789/comment/eceddf51_b80f98e8?usp... :
PS65, Line 805: GADD
can you please move these ACPI methods before exposing the `GPIO` device ?

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

https://review.coreboot.org/c/coreboot/+/83789/comment/aef4bac8_72c23638?usp... :
PS49, Line 31: 1
> We don't have COMM2, so GPP_COMM3 cannot use this macro, though.

best to my understanding `GPIO community 2` is always `GPD` and we don't need to expose those as it seems not required to configure the PADs. There is no harm of defining

```
#define COMM_0			0
#define COMM_1			INC(COMM_0)
#define COMM_3			INC(COMM_1)
#define COMM_4			INC(COMM_3)
#define COMM_5			INC(COMM_4)
```

The following code relies on the above macro definition. As long as you pass the correct PID for GPIO Community 3 onward, there's no problem maintaining the INC macro here.

https://review.coreboot.org/c/coreboot/+/83789/55..65/src/soc/intel/pantherl...

Additionally, there is also no harm to keep `COMM_2` in between because this GPIO community exists but not exposed for FW/SW configuration.

same logic is true for `GPP_COMM3_ID` as well. you just need a unique number for `_UID`