Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35080 )
Change subject: Documentation/acpi: Add new document on adding ACPI devices to devicetree ......................................................................
Patch Set 3:
(9 comments)
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... File Documentation/acpi/devicetree.md:
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 12: Devicetree and overridetree (if applicab This should really be a separate topic which you can link to eventually, but since nothing else exists yet...
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 20: AML : (ACPI Machine Language) code yourself I think you mean ASL (ACPI Source Language) here? that would be the other way to add a device (in this case into the DSDT).
Unless you mean having the mainboard use acpigen*() calls directly, which would technically be possible but I'm not sure I've seen it done that way.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 42: AML I think you mean ASL here (the disassembled AML bytecode is the only thing that is really readable)
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 59: 0x00061A80 I would put this as 400000 so it is easier to see that it is just representing 400khz.
Either is valid to compile, but the disassembled code always uses hex.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 70: 0x15, : 0x03 You cover it below but perhaps add a quick comment describing what these mean here as well.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 87: chip drivers/i2c/generic This quickly gets out of hand if you follow everything that needs to be done, but it might be worth mentioning that the associated driver (i.e. CONFIG_DRIVERS_I2C_GENERIC here but typically found in the Kconfig file in the driver directory) needs to be enabled in the mainboard Kconfig as well or it is very confusing to debug why the code is not getting generated at boot.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 127: GPP_A21_IRQ Also may be worth noting that this is an SOC-specific definition which may vary depending on the SOC.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 127: ACPI_IRQ_WAKE_EDGE_LOW Perhaps point to arch/x86/include/arch/acpi_device.h for where to find these definitions.
https://review.coreboot.org/c/coreboot/+/35080/3/Documentation/acpi/devicetr... PS3, Line 149: device i2c 15 on end Might also mention that this ties into the PCI device definition to indicate which I2C bus this is present on.
i.e. "device pci 15.0 on" resolves to "Scope (_SB.PCI0.I2C0)" where this I2C slave address is located.