| Message ID | 20190719203752.11151-3-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA/HDA: abort probe when DMICs are detected | expand |
On 2019-07-19 22:37, Pierre-Louis Bossart wrote: > Move parts of the code outside of the Skylake driver to help detect > the presence of DMICs (which are not supported by the HDaudio legacy > driver). > > No functionality change (except for the removal of useless OR > operations), only indentation and checkpatch fixes, and making sure > that the code compiles without ACPI > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > include/sound/intel-nhlt.h | 41 ++++++++++++---- > sound/hda/Kconfig | 3 ++ > sound/hda/Makefile | 3 ++ > sound/hda/intel-nhlt.c | 98 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 137 insertions(+), 8 deletions(-) > create mode 100644 sound/hda/intel-nhlt.c > The relocation of nhlt is much appreciated - it shouldn't be _reserved_ for /skylake in the first place. Thanks Pierre for this update. > diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h > index f85fbf9c7ce4..857922f03931 100644 > --- a/include/sound/intel-nhlt.h > +++ b/include/sound/intel-nhlt.h > @@ -1,18 +1,17 @@ > /* SPDX-License-Identifier: GPL-2.0-only */ > /* > - * skl-nhlt.h - Intel HDA Platform NHLT header > + * intel-nhlt.h - Intel HDA Platform NHLT header > * > - * Copyright (C) 2015 Intel Corp > - * Author: Sanjiv Kumar <sanjiv.kumar@intel.com> > - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > - * > - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + * Copyright (c) 2015-2019 Intel Corporation > */ > -#ifndef __SKL_NHLT_H__ > -#define __SKL_NHLT_H__ > + > +#ifndef __INTEL_NHLT_H__ > +#define __INTEL_NHLT_H__ > > #include <linux/acpi.h> > > +#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) > + Is it really valid to have NHLT without ACPI? Correct me if I'm wrong, but I doubt it is. In such case, _INTEL_NHLT check alone should suffice. > struct wav_fmt { > u16 fmt_tag; > u16 channels; > @@ -116,4 +115,30 @@ enum { > NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, > }; > > +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); > + > +void intel_nhlt_free(struct nhlt_acpi_table *addr); > + > +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); > + > +#else > + > +struct nhlt_acpi_table; > + > +static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > +{ > + return NULL; > +} > + > +static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) > +{ > +} > + > +static inline int intel_nhlt_get_dmic_geo(struct device *dev, > + struct nhlt_acpi_table *nhlt) > +{ > + return 0; > +} > +#endif > + > #endif > diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig > index f6feced15f17..c20145552cc3 100644 > --- a/sound/hda/Kconfig > +++ b/sound/hda/Kconfig > @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE > > Note that the pre-allocation size can be changed dynamically > via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. > + > +config SND_INTEL_NHLT > + tristate If above is true, "depends on ACPI" would be expected. > diff --git a/sound/hda/Makefile b/sound/hda/Makefile > index 2160202e2dc1..8560f6ef1b19 100644 > --- a/sound/hda/Makefile > +++ b/sound/hda/Makefile > @@ -13,3 +13,6 @@ obj-$(CONFIG_SND_HDA_CORE) += snd-hda-core.o > > #extended hda > obj-$(CONFIG_SND_HDA_EXT_CORE) += ext/ > + > +snd-intel-nhlt-objs := intel-nhlt.o > +obj-$(CONFIG_SND_INTEL_NHLT) += snd-intel-nhlt.o > diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c > new file mode 100644 > index 000000000000..7ba871e470f2 > --- /dev/null > +++ b/sound/hda/intel-nhlt.c > @@ -0,0 +1,98 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2015-2019 Intel Corporation > + > +#include <linux/acpi.h> > +#include <sound/intel-nhlt.h> > + > +#define NHLT_ACPI_HEADER_SIG "NHLT" > + > +/* Unique identification for getting NHLT blobs */ > +static guid_t osc_guid = > + GUID_INIT(0xA69F886E, 0x6CEB, 0x4594, > + 0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53); > + > +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) > +{ > + acpi_handle handle; > + union acpi_object *obj; > + struct nhlt_resource_desc *nhlt_ptr = NULL; Superfluous space. In fact, its initialization is too. > + struct nhlt_acpi_table *nhlt_table = NULL; > + > + handle = ACPI_HANDLE(dev); > + if (!handle) { > + dev_err(dev, "Didn't find ACPI_HANDLE\n"); > + return NULL; > + } > + > + obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL); > + if (obj && obj->type == ACPI_TYPE_BUFFER) { Personally, I always favor code with lower indentation over the one with higher tabs - makes it easier for reader to well.. read. You could simply revert the behavior of if-statement and bail out immediately with below dev_dbg and return NULL. Afterward, entire block can be shifted left. > + nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer; > + if (nhlt_ptr->length) > + nhlt_table = (struct nhlt_acpi_table *) > + memremap(nhlt_ptr->min_addr, nhlt_ptr->length, > + MEMREMAP_WB); > + ACPI_FREE(obj); > + if (nhlt_table && > + (strncmp(nhlt_table->header.signature, > + NHLT_ACPI_HEADER_SIG, > + strlen(NHLT_ACPI_HEADER_SIG)) != 0)) { > + memunmap(nhlt_table); > + dev_err(dev, "NHLT ACPI header signature incorrect\n"); > + return NULL; > + } > + return nhlt_table; > + } > + > + dev_dbg(dev, "No NHLT table found\n"); > + return NULL; While at it, don't we leak obj here? Shouldn't we ACPI_FREE(obj) regardless of "obj->type == ACPI_TYPE_BUFFER" check's outcome? > +} > +EXPORT_SYMBOL_GPL(intel_nhlt_init); > + > +void intel_nhlt_free(struct nhlt_acpi_table *nhlt) > +{ > + memunmap((void *)nhlt); > +} > +EXPORT_SYMBOL_GPL(intel_nhlt_free); > + > +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) > +{ > + struct nhlt_endpoint *epnt; > + struct nhlt_dmic_array_config *cfg; > + unsigned int dmic_geo = 0; > + u8 j; > + > + if (!nhlt) > + return 0; Should this handler even assume possibility of nhlt param being null?
On Sat, 20 Jul 2019 23:06:46 +0200, Cezary Rojewski wrote: > > > --- a/sound/hda/Kconfig > > +++ b/sound/hda/Kconfig > > @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE > > Note that the pre-allocation size can be changed dynamically > > via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. > > + > > +config SND_INTEL_NHLT > > + tristate > > If above is true, "depends on ACPI" would be expected. This won't fix things in practice as the Kconfig reverse selection ignores the dependencies of the selected item. It'd be as a help for readers, though. Takashi
On 7/22/19 3:54 AM, Takashi Iwai wrote: > On Sat, 20 Jul 2019 23:06:46 +0200, > Cezary Rojewski wrote: >> >>> --- a/sound/hda/Kconfig >>> +++ b/sound/hda/Kconfig >>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE >>> Note that the pre-allocation size can be changed dynamically >>> via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. >>> + >>> +config SND_INTEL_NHLT >>> + tristate >> >> If above is true, "depends on ACPI" would be expected. > > This won't fix things in practice as the Kconfig reverse selection > ignores the dependencies of the selected item. It'd be as a help for > readers, though. There is a fallback if ACPI is not defined, so the code would always compile. Configurations which select SND_INTEL_NHLT already depend on ACPI.
On Mon, 22 Jul 2019 14:14:28 +0200, Pierre-Louis Bossart wrote: > > > > On 7/22/19 3:54 AM, Takashi Iwai wrote: > > On Sat, 20 Jul 2019 23:06:46 +0200, > > Cezary Rojewski wrote: > >> > >>> --- a/sound/hda/Kconfig > >>> +++ b/sound/hda/Kconfig > >>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE > >>> Note that the pre-allocation size can be changed dynamically > >>> via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. > >>> + > >>> +config SND_INTEL_NHLT > >>> + tristate > >> > >> If above is true, "depends on ACPI" would be expected. > > > > This won't fix things in practice as the Kconfig reverse selection > > ignores the dependencies of the selected item. It'd be as a help for > > readers, though. > > There is a fallback if ACPI is not defined, so the code would always > compile. Configurations which select SND_INTEL_NHLT already depend on > ACPI. IIUC, the question above came from the point: #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) .... #else .... #endif and here Cezary suggested to drop IS_ENABLED(CONFIG_ACPI) *iff* the dependency can be assured in Kconfig side. But for that assurance, putting "depends on ACPI" in config SND_INTEL_NHLT block won't suffice; that was my followup. So, as of the current code, we can drop IS_ENABLED(CONFIG_ACPI) from the ifdef above, yes. But the dependency is no rock solid at this point, so either some comments or keeping the extra ifdef like the above would be needed, IMO. thanks, Takashi
On 7/22/19 7:26 AM, Takashi Iwai wrote: > On Mon, 22 Jul 2019 14:14:28 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 7/22/19 3:54 AM, Takashi Iwai wrote: >>> On Sat, 20 Jul 2019 23:06:46 +0200, >>> Cezary Rojewski wrote: >>>> >>>>> --- a/sound/hda/Kconfig >>>>> +++ b/sound/hda/Kconfig >>>>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE >>>>> Note that the pre-allocation size can be changed dynamically >>>>> via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. >>>>> + >>>>> +config SND_INTEL_NHLT >>>>> + tristate >>>> >>>> If above is true, "depends on ACPI" would be expected. >>> >>> This won't fix things in practice as the Kconfig reverse selection >>> ignores the dependencies of the selected item. It'd be as a help for >>> readers, though. >> >> There is a fallback if ACPI is not defined, so the code would always >> compile. Configurations which select SND_INTEL_NHLT already depend on >> ACPI. > > IIUC, the question above came from the point: > > #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) > .... > #else > .... > #endif > > and here Cezary suggested to drop IS_ENABLED(CONFIG_ACPI) *iff* the > dependency can be assured in Kconfig side. But for that assurance, > putting "depends on ACPI" in config SND_INTEL_NHLT block won't > suffice; that was my followup. > > So, as of the current code, we can drop IS_ENABLED(CONFIG_ACPI) from > the ifdef above, yes. But the dependency is no rock solid at this > point, so either some comments or keeping the extra ifdef like the > above would be needed, IMO. this extra ifdef is a bit overkill, I added it to make sure that the fallbacks are used in nonsensical configurations w/ randconfig. In practice, all Intel drivers already depend on ACPI and for the legacy we already have: select SND_INTEL_NHLT if ACPI Not sure if we need to do anything more. > > > thanks, > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
On Mon, 22 Jul 2019 14:58:44 +0200, Pierre-Louis Bossart wrote: > > > > On 7/22/19 7:26 AM, Takashi Iwai wrote: > > On Mon, 22 Jul 2019 14:14:28 +0200, > > Pierre-Louis Bossart wrote: > >> > >> > >> > >> On 7/22/19 3:54 AM, Takashi Iwai wrote: > >>> On Sat, 20 Jul 2019 23:06:46 +0200, > >>> Cezary Rojewski wrote: > >>>> > >>>>> --- a/sound/hda/Kconfig > >>>>> +++ b/sound/hda/Kconfig > >>>>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE > >>>>> Note that the pre-allocation size can be changed dynamically > >>>>> via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. > >>>>> + > >>>>> +config SND_INTEL_NHLT > >>>>> + tristate > >>>> > >>>> If above is true, "depends on ACPI" would be expected. > >>> > >>> This won't fix things in practice as the Kconfig reverse selection > >>> ignores the dependencies of the selected item. It'd be as a help for > >>> readers, though. > >> > >> There is a fallback if ACPI is not defined, so the code would always > >> compile. Configurations which select SND_INTEL_NHLT already depend on > >> ACPI. > > > > IIUC, the question above came from the point: > > > > #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) > > .... > > #else > > .... > > #endif > > > > and here Cezary suggested to drop IS_ENABLED(CONFIG_ACPI) *iff* the > > dependency can be assured in Kconfig side. But for that assurance, > > putting "depends on ACPI" in config SND_INTEL_NHLT block won't > > suffice; that was my followup. > > > > So, as of the current code, we can drop IS_ENABLED(CONFIG_ACPI) from > > the ifdef above, yes. But the dependency is no rock solid at this > > point, so either some comments or keeping the extra ifdef like the > > above would be needed, IMO. > > this extra ifdef is a bit overkill, I added it to make sure that the > fallbacks are used in nonsensical configurations w/ randconfig. In > practice, all Intel drivers already depend on ACPI and for the legacy > we already have: > > select SND_INTEL_NHLT if ACPI > > Not sure if we need to do anything more. The missing piece is this implicit dependency information. You can just put some comments somewhere mentioning it. Takashi
On 7/22/19 8:01 AM, Takashi Iwai wrote: > On Mon, 22 Jul 2019 14:58:44 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 7/22/19 7:26 AM, Takashi Iwai wrote: >>> On Mon, 22 Jul 2019 14:14:28 +0200, >>> Pierre-Louis Bossart wrote: >>>> >>>> >>>> >>>> On 7/22/19 3:54 AM, Takashi Iwai wrote: >>>>> On Sat, 20 Jul 2019 23:06:46 +0200, >>>>> Cezary Rojewski wrote: >>>>>> >>>>>>> --- a/sound/hda/Kconfig >>>>>>> +++ b/sound/hda/Kconfig >>>>>>> @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE >>>>>>> Note that the pre-allocation size can be changed dynamically >>>>>>> via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. >>>>>>> + >>>>>>> +config SND_INTEL_NHLT >>>>>>> + tristate >>>>>> >>>>>> If above is true, "depends on ACPI" would be expected. >>>>> >>>>> This won't fix things in practice as the Kconfig reverse selection >>>>> ignores the dependencies of the selected item. It'd be as a help for >>>>> readers, though. >>>> >>>> There is a fallback if ACPI is not defined, so the code would always >>>> compile. Configurations which select SND_INTEL_NHLT already depend on >>>> ACPI. >>> >>> IIUC, the question above came from the point: >>> >>> #if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) >>> .... >>> #else >>> .... >>> #endif >>> >>> and here Cezary suggested to drop IS_ENABLED(CONFIG_ACPI) *iff* the >>> dependency can be assured in Kconfig side. But for that assurance, >>> putting "depends on ACPI" in config SND_INTEL_NHLT block won't >>> suffice; that was my followup. >>> >>> So, as of the current code, we can drop IS_ENABLED(CONFIG_ACPI) from >>> the ifdef above, yes. But the dependency is no rock solid at this >>> point, so either some comments or keeping the extra ifdef like the >>> above would be needed, IMO. >> >> this extra ifdef is a bit overkill, I added it to make sure that the >> fallbacks are used in nonsensical configurations w/ randconfig. In >> practice, all Intel drivers already depend on ACPI and for the legacy >> we already have: >> >> select SND_INTEL_NHLT if ACPI >> >> Not sure if we need to do anything more. > > The missing piece is this implicit dependency information. > You can just put some comments somewhere mentioning it. ok, will do. thanks for the guidance.
replying below to the feedback I missed earlier. Will send a v4 next week. >> diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c >> new file mode 100644 >> index 000000000000..7ba871e470f2 >> --- /dev/null >> +++ b/sound/hda/intel-nhlt.c >> @@ -0,0 +1,98 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2015-2019 Intel Corporation >> + >> +#include <linux/acpi.h> >> +#include <sound/intel-nhlt.h> >> + >> +#define NHLT_ACPI_HEADER_SIG "NHLT" >> + >> +/* Unique identification for getting NHLT blobs */ >> +static guid_t osc_guid = >> + GUID_INIT(0xA69F886E, 0x6CEB, 0x4594, >> + 0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53); >> + >> +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) >> +{ >> + acpi_handle handle; >> + union acpi_object *obj; >> + struct nhlt_resource_desc *nhlt_ptr = NULL; > > Superfluous space. In fact, its initialization is too. indeed, will remove and fix spacing. > >> + struct nhlt_acpi_table *nhlt_table = NULL; >> + >> + handle = ACPI_HANDLE(dev); >> + if (!handle) { >> + dev_err(dev, "Didn't find ACPI_HANDLE\n"); >> + return NULL; >> + } >> + >> + obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL); >> + if (obj && obj->type == ACPI_TYPE_BUFFER) { > > Personally, I always favor code with lower indentation over the one with > higher tabs - makes it easier for reader to well.. read. > You could simply revert the behavior of if-statement and bail out > immediately with below dev_dbg and return NULL. Afterward, entire block > can be shifted left. yes, makes sense. > >> + nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer; >> + if (nhlt_ptr->length) >> + nhlt_table = (struct nhlt_acpi_table *) >> + memremap(nhlt_ptr->min_addr, nhlt_ptr->length, >> + MEMREMAP_WB); >> + ACPI_FREE(obj); >> + if (nhlt_table && >> + (strncmp(nhlt_table->header.signature, >> + NHLT_ACPI_HEADER_SIG, >> + strlen(NHLT_ACPI_HEADER_SIG)) != 0)) { >> + memunmap(nhlt_table); >> + dev_err(dev, "NHLT ACPI header signature incorrect\n"); >> + return NULL; >> + } >> + return nhlt_table; >> + } >> + >> + dev_dbg(dev, "No NHLT table found\n"); >> + return NULL; > > While at it, don't we leak obj here? Shouldn't we ACPI_FREE(obj) > regardless of "obj->type == ACPI_TYPE_BUFFER" check's outcome? yes, that looks like a bug. This isn't new code I wrote, it's been that way for years... We may want to track this with a dedicated patch, rather than lumping fixes with indents. > >> +} >> +EXPORT_SYMBOL_GPL(intel_nhlt_init); >> + >> +void intel_nhlt_free(struct nhlt_acpi_table *nhlt) >> +{ >> + memunmap((void *)nhlt); >> +} >> +EXPORT_SYMBOL_GPL(intel_nhlt_free); >> + >> +int intel_nhlt_get_dmic_geo(struct device *dev, struct >> nhlt_acpi_table *nhlt) >> +{ >> + struct nhlt_endpoint *epnt; >> + struct nhlt_dmic_array_config *cfg; >> + unsigned int dmic_geo = 0; >> + u8 j; >> + >> + if (!nhlt) >> + return 0; > > Should this handler even assume possibility of nhlt param being null? yes, I added this when I allowed the driver to probe even when there is no NHLT on plain vanilla HDaudio solutions. The caller doesn't check for NHLT in the first place. see the probe sequence skl->nhlt = skl_nhlt_init(bus->dev); if (skl->nhlt == NULL) { #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) dev_err(bus->dev, "no nhlt info found\n"); err = -ENODEV; goto out_free; #else dev_warn(bus->dev, "no nhlt info found, continuing to try to enable HDaudio codec\n"); #endif } else { err = skl_nhlt_create_sysfs(skl); if (err < 0) { dev_err(bus->dev, "skl_nhlt_create_sysfs failed with err: %d\n", err); goto out_nhlt_free; } skl_nhlt_update_topology_bin(skl); /* create device for dsp clk */ err = skl_clock_device_register(skl); if (err < 0) { dev_err(bus->dev, "skl_clock_device_register failed with err: %d\n", err); goto out_clk_free; } } pci_set_drvdata(skl->pci, bus); err = skl_find_machine(skl, (void *)pci_id->driver_data); in which we directly call this helper, so skl->nhtl can be NULL if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; mach->mach_params.dmic_num = skl_get_dmic_geo(skl); } it's actually a feature: if there is no NHLT table or it's not valid, then there are no DMICs.
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index f85fbf9c7ce4..857922f03931 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -1,18 +1,17 @@ /* SPDX-License-Identifier: GPL-2.0-only */ /* - * skl-nhlt.h - Intel HDA Platform NHLT header + * intel-nhlt.h - Intel HDA Platform NHLT header * - * Copyright (C) 2015 Intel Corp - * Author: Sanjiv Kumar <sanjiv.kumar@intel.com> - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - * - * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + * Copyright (c) 2015-2019 Intel Corporation */ -#ifndef __SKL_NHLT_H__ -#define __SKL_NHLT_H__ + +#ifndef __INTEL_NHLT_H__ +#define __INTEL_NHLT_H__ #include <linux/acpi.h> +#if IS_ENABLED(CONFIG_ACPI) && IS_ENABLED(CONFIG_SND_INTEL_NHLT) + struct wav_fmt { u16 fmt_tag; u16 channels; @@ -116,4 +115,30 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, }; +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); + +void intel_nhlt_free(struct nhlt_acpi_table *addr); + +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); + +#else + +struct nhlt_acpi_table; + +static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{ + return NULL; +} + +static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) +{ +} + +static inline int intel_nhlt_get_dmic_geo(struct device *dev, + struct nhlt_acpi_table *nhlt) +{ + return 0; +} +#endif + #endif diff --git a/sound/hda/Kconfig b/sound/hda/Kconfig index f6feced15f17..c20145552cc3 100644 --- a/sound/hda/Kconfig +++ b/sound/hda/Kconfig @@ -29,3 +29,6 @@ config SND_HDA_PREALLOC_SIZE Note that the pre-allocation size can be changed dynamically via a proc file (/proc/asound/card*/pcm*/sub*/prealloc), too. + +config SND_INTEL_NHLT + tristate diff --git a/sound/hda/Makefile b/sound/hda/Makefile index 2160202e2dc1..8560f6ef1b19 100644 --- a/sound/hda/Makefile +++ b/sound/hda/Makefile @@ -13,3 +13,6 @@ obj-$(CONFIG_SND_HDA_CORE) += snd-hda-core.o #extended hda obj-$(CONFIG_SND_HDA_EXT_CORE) += ext/ + +snd-intel-nhlt-objs := intel-nhlt.o +obj-$(CONFIG_SND_INTEL_NHLT) += snd-intel-nhlt.o diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c new file mode 100644 index 000000000000..7ba871e470f2 --- /dev/null +++ b/sound/hda/intel-nhlt.c @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2015-2019 Intel Corporation + +#include <linux/acpi.h> +#include <sound/intel-nhlt.h> + +#define NHLT_ACPI_HEADER_SIG "NHLT" + +/* Unique identification for getting NHLT blobs */ +static guid_t osc_guid = + GUID_INIT(0xA69F886E, 0x6CEB, 0x4594, + 0xA4, 0x1F, 0x7B, 0x5D, 0xCE, 0x24, 0xC5, 0x53); + +struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +{ + acpi_handle handle; + union acpi_object *obj; + struct nhlt_resource_desc *nhlt_ptr = NULL; + struct nhlt_acpi_table *nhlt_table = NULL; + + handle = ACPI_HANDLE(dev); + if (!handle) { + dev_err(dev, "Didn't find ACPI_HANDLE\n"); + return NULL; + } + + obj = acpi_evaluate_dsm(handle, &osc_guid, 1, 1, NULL); + if (obj && obj->type == ACPI_TYPE_BUFFER) { + nhlt_ptr = (struct nhlt_resource_desc *)obj->buffer.pointer; + if (nhlt_ptr->length) + nhlt_table = (struct nhlt_acpi_table *) + memremap(nhlt_ptr->min_addr, nhlt_ptr->length, + MEMREMAP_WB); + ACPI_FREE(obj); + if (nhlt_table && + (strncmp(nhlt_table->header.signature, + NHLT_ACPI_HEADER_SIG, + strlen(NHLT_ACPI_HEADER_SIG)) != 0)) { + memunmap(nhlt_table); + dev_err(dev, "NHLT ACPI header signature incorrect\n"); + return NULL; + } + return nhlt_table; + } + + dev_dbg(dev, "No NHLT table found\n"); + return NULL; +} +EXPORT_SYMBOL_GPL(intel_nhlt_init); + +void intel_nhlt_free(struct nhlt_acpi_table *nhlt) +{ + memunmap((void *)nhlt); +} +EXPORT_SYMBOL_GPL(intel_nhlt_free); + +int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +{ + struct nhlt_endpoint *epnt; + struct nhlt_dmic_array_config *cfg; + unsigned int dmic_geo = 0; + u8 j; + + if (!nhlt) + return 0; + + epnt = (struct nhlt_endpoint *)nhlt->desc; + + for (j = 0; j < nhlt->endpoint_count; j++) { + if (epnt->linktype == NHLT_LINK_DMIC) { + cfg = (struct nhlt_dmic_array_config *) + (epnt->config.caps); + switch (cfg->array_type) { + case NHLT_MIC_ARRAY_2CH_SMALL: + case NHLT_MIC_ARRAY_2CH_BIG: + dmic_geo = MIC_ARRAY_2CH; + break; + + case NHLT_MIC_ARRAY_4CH_1ST_GEOM: + case NHLT_MIC_ARRAY_4CH_L_SHAPED: + case NHLT_MIC_ARRAY_4CH_2ND_GEOM: + dmic_geo = MIC_ARRAY_4CH; + break; + + default: + dev_warn(dev, "undefined DMIC array_type 0x%0x\n", + cfg->array_type); + } + } + epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); + } + + return dmic_geo; +} +EXPORT_SYMBOL_GPL(intel_nhlt_get_dmic_geo); + +MODULE_LICENSE("GPL v2"); +MODULE_DESCRIPTION("Intel NHLT driver");
Move parts of the code outside of the Skylake driver to help detect the presence of DMICs (which are not supported by the HDaudio legacy driver). No functionality change (except for the removal of useless OR operations), only indentation and checkpatch fixes, and making sure that the code compiles without ACPI Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- include/sound/intel-nhlt.h | 41 ++++++++++++---- sound/hda/Kconfig | 3 ++ sound/hda/Makefile | 3 ++ sound/hda/intel-nhlt.c | 98 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 137 insertions(+), 8 deletions(-) create mode 100644 sound/hda/intel-nhlt.c