Hi Simon,
-----"Simon Glass" sjg@chromium.org schrieb: -----
Betreff: Re: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
Hi Wolfgang,
On Thu, 2 Jul 2020 at 02:11, Wolfgang Wallner wolfgang.wallner@br-automation.com wrote:
Hi Simon,
I dont know NHLT well enough to actually review the code, but I did compare the files in this patch to the version in coreboot. Most of the changes are obvious (coding style, spelling, ...), but some things are not, see the remarks below.
-----"Simon Glass" sjg@chromium.org schrieb: -----
Betreff: [PATCH v1 22/43] x86: Add support for building up an NHLT structure
The Intel Non-High-Definition-Audio Link Table (NHLT) table describes the audio codecs and connections in a system. Various devices can contribute information to produce the table.
Add functions to allow adding to the structure that is eventually written to the ACPI tables. Also add the device-tree bindings.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v1:
- Add a new patch to support building up an NHLT structure
arch/x86/include/asm/acpi_nhlt.h | 314 ++++++++++++++++++++ arch/x86/lib/Makefile | 1 + arch/x86/lib/acpi_nhlt.c | 482 +++++++++++++++++++++++++++++++ 3 files changed, 797 insertions(+) create mode 100644 arch/x86/include/asm/acpi_nhlt.h create mode 100644 arch/x86/lib/acpi_nhlt.c
diff --git a/arch/x86/include/asm/acpi_nhlt.h b/arch/x86/include/asm/acpi_nhlt.h new file mode 100644 index 0000000000..4d2573d5ff --- /dev/null +++ b/arch/x86/include/asm/acpi_nhlt.h
[snip]
+/*
- Serialize NHLT object to ACPI table. Take in the beginning address of where
- the table will reside and return the address of the next ACPI table. On
- error 0 will be returned. The NHLT object is no longer valid after this
- function is called.
- */
+uintptr_t nhlt_serialise(struct nhlt *nhlt, uintptr_t acpi_addr);
The implementation for this functions seems to be missing in this patch. In coreboot it looks like this:
uintptr_t nhlt_serialize(struct nhlt *nhlt, uintptr_t acpi_addr) { return nhlt_serialize_oem_overrides(nhlt, acpi_addr, NULL, NULL, 0); }
Yes we don't need this at present. If we do we would likely put this in the device tree. Since coreboot doesn't have a proper device tree it uses code to call override functions to get the data, which IMO makes it quite hard to work out the config for a particular board
If we don't need the implementation of nhlt_serialise(), shouldn't we then also drop its declaration in the header file?
[..]
regards, Wolfgang