Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45972 )
Change subject: soc/intel/alderlake/acpi: Add SoC ACPI directory for ADL ......................................................................
Patch Set 2:
(9 comments)
I see lots of potential for common code.
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/camera_clock_ctl.asl:
PS2: I think this is going to be useful for other SoCs as well (e.g. TGL)
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 20: Method (OFST, 0x1, NotSerialized) IMHO, this method is unnecessary. The calculation could be done directly in RAOW.
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 26: OR Or
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 27: source and destination Clock source select
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 31: 0x3 This is the number of arguments. It should be written in decimal.
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 44: * Arg0: Clock source select(0: IMGCLKOUT_0, 1: IMGCLKOUT_1, 2: IMGCLKOUT_2, 3: IMGCLKOUT_3, : * 4: IMGCLKOUT_4, 5: IMGCLKOUT_5) I'd rather specify the range of valid values:
* Arg0: Clock source select (0 .. 5 => IMGCLKOUT_0 .. IMGCLKOUT_5)
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... PS2, Line 46: Select 24MHz / 19.2 MHz Frequency select
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/dptf.asl:
PS2: This just gave me an idea: Why not have each SoC define its ACPI device IDs in a file? e.g. include/soc/acpi_def.h
I think _ADR defines using the PCI_DEV macro should work too
https://review.coreboot.org/c/coreboot/+/45972/2/src/soc/intel/alderlake/acp... File src/soc/intel/alderlake/acpi/ipu.asl:
PS2: These and other files seem to be great candidates for common ACPI.