Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39111 )
Change subject: soc/intel/tigerlake: Add Jasper lake GPIO support ......................................................................
Patch Set 15:
(6 comments)
https://review.coreboot.org/c/coreboot/+/39111/12/src/soc/intel/common/acpi/... File src/soc/intel/common/acpi/gpio_op.asl:
https://review.coreboot.org/c/coreboot/+/39111/12/src/soc/intel/common/acpi/... PS12, Line 4: Copyright (C) 2020 Intel Corporation.
Done […]
Ack
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/ac... File src/soc/intel/tigerlake/acpi/gpio_op.asl:
PS15:
I think this file should be converted to ASL 2.0 syntax as well. […]
Ack
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio.h:
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 24: #define CROS_GPIO_DEVICE_NAME "GCM0"
This is not correct. There are multiple GPIO devices for TGL.
Yes, this is not required since we are naming devices as GCM0 and kernel driver should be able to bind it from HID and UID
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 41: /* Common macro definition for gpio_op.asl file */
Aren't these macros already defined in some header file?
No Furquan, these marcros are not defined in another header file. Since these macros gets changed over soc, we have kept it inside soc folder.
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/gpio_soc_defs.h:
PS15:
Shouldn't this file be renamed as gpio_soc_defs_tgl. […]
Ack and Done
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... File src/soc/intel/tigerlake/include/soc/pmc.h:
https://review.coreboot.org/c/coreboot/+/39111/15/src/soc/intel/tigerlake/in... PS15, Line 135: #define PMC_GPP_B 0x1 : #define PMC_GPP_A 0x0 : #define PMC_GPP_R 0x4 : #define PMC_GPP_S 0x6 : #define PMC_GPD 0x3 : #define PMC_GPP_H 0xA : #define PMC_GPP_D 0x7 : #define PMC_GPP_F 0x2 : #define PMC_GPP_C 0x8 : #define PMC_GPP_E 0xF
Shouldn't these be ordered by number?
Ack
-
Maulik V Vaghela (Code Review)