Hi Simon,
-----"Simon Glass" <s...@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.wall...@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" <s...@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 <s...@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
