[PATCH v2 0/5] ALSA/HDA: abort probe when DMICs are detected

[PATCH v2 0/5] ALSA/HDA: abort probe when DMICs are detected

[Pierre-Louis Bossart]

This is the second take on same problem of detecting when the HDaudio
legacy driver can be used and when the SST or SOF drivers are
required.

The previous attempt based on a the PCI Class information was a
resounding failure and broke audio on Linus' laptop, so we need
additional information to avoid enabling a DSP-based driver without a
good reason to do so.

This patchset suggests the use of the NHLT information which *in
theory* exposes DMIC endpoints. The legacy HDaudio driver cannot
handle DMICs and will not provide any capture capabilities. Since it's
typically the first one to probe due to the Makefile order, aborting
the probe will let the PCI subsystem look for the next driver which
hopefully will support this capability.

I tested this patch on 5 devices (SKL, KBL, APL, GLK, WHL), three
without DMICs and two with, and the detection seems to work as
planned. Additional testing by Canonical and Endless folks did not
expose any issues.

Changes since v1 (Feedback from Takashi):
Squashed patch3 in patch2
Changed log to dbg_info
Fixed module parameter handling

Changes since RFC:
Cosmetic code improvements
Moved intel-nhlt.h to include/sound
Moved intel-nhlt.c to sound/hda
Removed SOC prefixes
Added full-support for vendor-defined geometries
Added Kconfig and kernel module parameter to opt-in

Pierre-Louis Bossart (5):
  ASoC: Intel: Skylake: move NHLT header to common directory
  ALSA: hda: move parts of NHLT code to new module
  ALSA: hda: intel-nhlt: handle NHLT VENDOR_DEFINED DMIC geometry
  ASoC: Intel: Skylake: use common NHLT module
  ALSA: hda/intel: stop probe if DMICS are detected on Skylake+
    platforms

 .../skl-nhlt.h => include/sound/intel-nhlt.h  |  51 +++++++--
 sound/hda/Kconfig                             |   3 +
 sound/hda/Makefile                            |   3 +
 sound/hda/intel-nhlt.c                        | 102 ++++++++++++++++++
 sound/pci/hda/Kconfig                         |  10 ++
 sound/pci/hda/hda_intel.c                     |  34 ++++++
 sound/soc/intel/Kconfig                       |   1 +
 sound/soc/intel/skylake/skl-nhlt.c            |  91 +---------------
 sound/soc/intel/skylake/skl-ssp-clk.c         |   1 +
 sound/soc/intel/skylake/skl-topology.c        |   1 +
 sound/soc/intel/skylake/skl.c                 |  12 ++-
 sound/soc/intel/skylake/skl.h                 |   4 -
 12 files changed, 205 insertions(+), 108 deletions(-)
 rename sound/soc/intel/skylake/skl-nhlt.h => include/sound/intel-nhlt.h (65%)
 create mode 100644 sound/hda/intel-nhlt.c

-- 
2.20.1

[PATCH v2 1/5] ASoC: Intel: Skylake: move NHLT header to common directory

[Pierre-Louis Bossart]

Prepare move from NHLT code to common directory, starting with header.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/skylake/skl-nhlt.h => include/sound/intel-nhlt.h | 0
 sound/soc/intel/skylake/skl-nhlt.c                               | 1 +
 sound/soc/intel/skylake/skl-ssp-clk.c                            | 1 +
 sound/soc/intel/skylake/skl-topology.c                           | 1 +
 sound/soc/intel/skylake/skl.h                                    | 1 -
 5 files changed, 3 insertions(+), 1 deletion(-)
 rename sound/soc/intel/skylake/skl-nhlt.h => include/sound/intel-nhlt.h (100%)

diff --git a/sound/soc/intel/skylake/skl-nhlt.h b/include/sound/intel-nhlt.h
similarity index 100%
rename from sound/soc/intel/skylake/skl-nhlt.h
rename to include/sound/intel-nhlt.h
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
index 1132109cb992..aabc5d71650e 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -9,6 +9,7 @@
  * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  */
 #include <linux/pci.h>
+#include <sound/intel-nhlt.h>
 #include "skl.h"
 #include "skl-i2s.h"
 
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c
index 5bb6e40d4d3e..5bfcd46452f9 100644
--- a/sound/soc/intel/skylake/skl-ssp-clk.c
+++ b/sound/soc/intel/skylake/skl-ssp-clk.c
@@ -11,6 +11,7 @@
 #include <linux/platform_device.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
+#include <sound/intel-nhlt.h>
 #include "skl.h"
 #include "skl-ssp-clk.h"
 #include "skl-topology.h"
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 6241e35213af..f8a501cf5fbd 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -12,6 +12,7 @@
 #include <linux/types.h>
 #include <linux/firmware.h>
 #include <linux/uuid.h>
+#include <sound/intel-nhlt.h>
 #include <sound/soc.h>
 #include <sound/soc-topology.h>
 #include <uapi/sound/snd_sst_tokens.h>
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index 6070666a6392..928e8115a1a7 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -16,7 +16,6 @@
 #include <sound/hdaudio_ext.h>
 #include <sound/hda_codec.h>
 #include <sound/soc.h>
-#include "skl-nhlt.h"
 #include "skl-ssp-clk.h"
 
 #define SKL_SUSPEND_DELAY 2000
-- 
2.20.1

[PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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

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");
-- 
2.20.1

[PATCH v2 3/5] ALSA: hda: intel-nhlt: handle NHLT VENDOR_DEFINED DMIC geometry

[Pierre-Louis Bossart]

The NHLT spec defines a VENDOR_DEFINED geometry, which requires
reading additional information to figure out the number of
microphones.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 include/sound/intel-nhlt.h | 10 ++++++++--
 sound/hda/intel-nhlt.c     |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h
index 857922f03931..f657fd8fc0ad 100644
--- a/include/sound/intel-nhlt.h
+++ b/include/sound/intel-nhlt.h
@@ -96,16 +96,22 @@ struct nhlt_resource_desc  {
 #define MIC_ARRAY_2CH 2
 #define MIC_ARRAY_4CH 4
 
-struct nhlt_tdm_config {
+struct nhlt_device_specific_config {
 	u8 virtual_slot;
 	u8 config_type;
 } __packed;
 
 struct nhlt_dmic_array_config {
-	struct nhlt_tdm_config tdm_config;
+	struct nhlt_device_specific_config device_config;
 	u8 array_type;
 } __packed;
 
+struct nhlt_vendor_dmic_array_config {
+	struct nhlt_dmic_array_config dmic_config;
+	u8 nb_mics;
+	/* TODO add vendor mic config */
+} __packed;
+
 enum {
 	NHLT_MIC_ARRAY_2CH_SMALL = 0xa,
 	NHLT_MIC_ARRAY_2CH_BIG = 0xb,
diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c
index 7ba871e470f2..441ee39520a8 100644
--- a/sound/hda/intel-nhlt.c
+++ b/sound/hda/intel-nhlt.c
@@ -58,6 +58,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
 {
 	struct nhlt_endpoint *epnt;
 	struct nhlt_dmic_array_config *cfg;
+	struct nhlt_vendor_dmic_array_config *cfg_vendor;
 	unsigned int dmic_geo = 0;
 	u8 j;
 
@@ -81,7 +82,10 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
 			case NHLT_MIC_ARRAY_4CH_2ND_GEOM:
 				dmic_geo = MIC_ARRAY_4CH;
 				break;
-
+			case NHLT_MIC_ARRAY_VENDOR_DEFINED:
+				cfg_vendor = (struct nhlt_vendor_dmic_array_config *)cfg;
+				dmic_geo = cfg_vendor->nb_mics;
+				break;
 			default:
 				dev_warn(dev, "undefined DMIC array_type 0x%0x\n",
 					 cfg->array_type);
-- 
2.20.1

[PATCH v2 4/5] ASoC: Intel: Skylake: use common NHLT module

[Pierre-Louis Bossart]

No functionality change, only use common functions now.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/Kconfig            |  1 +
 sound/soc/intel/skylake/skl-nhlt.c | 90 ------------------------------
 sound/soc/intel/skylake/skl.c      | 12 ++--
 sound/soc/intel/skylake/skl.h      |  3 -
 4 files changed, 9 insertions(+), 97 deletions(-)

diff --git a/sound/soc/intel/Kconfig b/sound/soc/intel/Kconfig
index 96a00a9d4cf8..a3ec17fd63cd 100644
--- a/sound/soc/intel/Kconfig
+++ b/sound/soc/intel/Kconfig
@@ -215,6 +215,7 @@ config SND_SOC_INTEL_SKYLAKE_COMMON
 	select SND_SOC_INTEL_SST
 	select SND_SOC_HDAC_HDA if SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC
 	select SND_SOC_ACPI_INTEL_MATCH
+	select SND_INTEL_NHLT
 	help
 	  If you have a Intel Skylake/Broxton/ApolloLake/KabyLake/
 	  GeminiLake or CannonLake platform with the DSP enabled in the BIOS
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c
index aabc5d71650e..6f57ceb9efb7 100644
--- a/sound/soc/intel/skylake/skl-nhlt.c
+++ b/sound/soc/intel/skylake/skl-nhlt.c
@@ -13,54 +13,6 @@
 #include "skl.h"
 #include "skl-i2s.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 *skl_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_err(dev, "device specific method to extract NHLT blob failed\n");
-	return NULL;
-}
-
-void skl_nhlt_free(struct nhlt_acpi_table *nhlt)
-{
-	memunmap((void *) nhlt);
-}
-
 static struct nhlt_specific_cfg *skl_get_specific_cfg(
 		struct device *dev, struct nhlt_fmt *fmt,
 		u8 no_ch, u32 rate, u16 bps, u8 linktype)
@@ -163,48 +115,6 @@ struct nhlt_specific_cfg
 	return NULL;
 }
 
-int skl_get_dmic_geo(struct skl *skl)
-{
-	struct nhlt_acpi_table *nhlt = (struct nhlt_acpi_table *)skl->nhlt;
-	struct nhlt_endpoint *epnt;
-	struct nhlt_dmic_array_config *cfg;
-	struct device *dev = &skl->pci->dev;
-	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;
-}
-
 static void skl_nhlt_trim_space(char *trim)
 {
 	char *s = trim;
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 3362e71b4563..2b5159890a57 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -26,9 +26,11 @@
 #include <sound/hdaudio.h>
 #include <sound/hda_i915.h>
 #include <sound/hda_codec.h>
+#include <sound/intel-nhlt.h>
 #include "skl.h"
 #include "skl-sst-dsp.h"
 #include "skl-sst-ipc.h"
+
 #if IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
 #include "../../../soc/codecs/hdac_hda.h"
 #endif
@@ -516,7 +518,9 @@ static int skl_find_machine(struct skl *skl, void *driver_data)
 
 	if (pdata) {
 		skl->use_tplg_pcm = pdata->use_tplg_pcm;
-		mach->mach_params.dmic_num = skl_get_dmic_geo(skl);
+		mach->mach_params.dmic_num =
+			intel_nhlt_get_dmic_geo(&skl->pci->dev,
+						skl->nhlt);
 	}
 
 	return 0;
@@ -1029,7 +1033,7 @@ static int skl_probe(struct pci_dev *pci,
 
 	device_disable_async_suspend(bus->dev);
 
-	skl->nhlt = skl_nhlt_init(bus->dev);
+	skl->nhlt = intel_nhlt_init(bus->dev);
 
 	if (skl->nhlt == NULL) {
 #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC)
@@ -1095,7 +1099,7 @@ static int skl_probe(struct pci_dev *pci,
 out_clk_free:
 	skl_clock_device_unregister(skl);
 out_nhlt_free:
-	skl_nhlt_free(skl->nhlt);
+	intel_nhlt_free(skl->nhlt);
 out_free:
 	skl_free(bus);
 
@@ -1144,7 +1148,7 @@ static void skl_remove(struct pci_dev *pci)
 	skl_dmic_device_unregister(skl);
 	skl_clock_device_unregister(skl);
 	skl_nhlt_remove_sysfs(skl);
-	skl_nhlt_free(skl->nhlt);
+	intel_nhlt_free(skl->nhlt);
 	skl_free(bus);
 	dev_set_drvdata(&pci->dev, NULL);
 }
diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h
index 928e8115a1a7..f4dd6c767993 100644
--- a/sound/soc/intel/skylake/skl.h
+++ b/sound/soc/intel/skylake/skl.h
@@ -128,13 +128,10 @@ struct skl_dsp_ops {
 int skl_platform_unregister(struct device *dev);
 int skl_platform_register(struct device *dev);
 
-struct nhlt_acpi_table *skl_nhlt_init(struct device *dev);
-void skl_nhlt_free(struct nhlt_acpi_table *addr);
 struct nhlt_specific_cfg *skl_get_ep_blob(struct skl *skl, u32 instance,
 					u8 link_type, u8 s_fmt, u8 no_ch,
 					u32 s_rate, u8 dirn, u8 dev_type);
 
-int skl_get_dmic_geo(struct skl *skl);
 int skl_nhlt_update_topology_bin(struct skl *skl);
 int skl_init_dsp(struct skl *skl);
 int skl_free_dsp(struct skl *skl);
-- 
2.20.1

[PATCH v2 5/5] ALSA: hda/intel: stop probe if DMICS are detected on Skylake+ platforms

[Pierre-Louis Bossart]

The legacy HD-Audio driver cannot handle Skylake+ platforms with
digital microphones. For those platforms, the SOF or SST drivers need
to be used.

This patch provides an automatic way of detecting the presence of
DMICs using NHTL information reported by the BIOS. A kernel kconfig
option or a kernel module parameter provide an opt-in means of
stopping the probe. The kernel would then look for an alternate driver
registered for the same PCI ID to probe.

With this capability, distros no longer have to blacklist
snd-hda-intel, but still need to make sure the SOF/SST drivers are
functional by providing the relevant firmware and topology files in
/lib/firmware/intel

The coexistence between SOF and SST drivers and their dynamic
detection is not addressed by this patch, different mechanisms need to
be used, e.g. DMI-based quirks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/pci/hda/Kconfig     | 10 ++++++++++
 sound/pci/hda/hda_intel.c | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/sound/pci/hda/Kconfig b/sound/pci/hda/Kconfig
index 35d934309cb2..b5966014b5f7 100644
--- a/sound/pci/hda/Kconfig
+++ b/sound/pci/hda/Kconfig
@@ -12,6 +12,7 @@ config SND_HDA_INTEL
 	tristate "HD Audio PCI"
 	depends on SND_PCI
 	select SND_HDA
+	select SND_INTEL_NHLT if ACPI
 	help
 	  Say Y here to include support for Intel "High Definition
 	  Audio" (Azalia) and its compatible devices.
@@ -22,6 +23,15 @@ config SND_HDA_INTEL
 	  To compile this driver as a module, choose M here: the module
 	  will be called snd-hda-intel.
 
+config SND_HDA_INTEL_DETECT_DMIC
+	bool "DMIC detection and probe abort"
+	depends on SND_HDA_INTEL
+	help
+	  Say Y to detect digital microphones on SKL+ devices. DMICs
+	  cannot be handled by the HDaudio legacy driver and are
+	  currently only supported by the SOF driver.
+	  If unsure say N.
+
 config SND_HDA_TEGRA
 	tristate "NVIDIA Tegra HD Audio"
 	depends on ARCH_TEGRA
diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index cb8b0945547c..ae9c13248a1d 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -46,6 +46,7 @@
 #include <sound/initval.h>
 #include <sound/hdaudio.h>
 #include <sound/hda_i915.h>
+#include <sound/intel-nhlt.h>
 #include <linux/vgaarb.h>
 #include <linux/vga_switcheroo.h>
 #include <linux/firmware.h>
@@ -124,6 +125,7 @@ static char *patch[SNDRV_CARDS];
 static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
 					CONFIG_SND_HDA_INPUT_BEEP_MODE};
 #endif
+static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC);
 
 module_param_array(index, int, NULL, 0444);
 MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
@@ -158,6 +160,8 @@ module_param_array(beep_mode, bool, NULL, 0444);
 MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode "
 			    "(0=off, 1=on) (default=1).");
 #endif
+module_param(dmic_detect, bool, 0444);
+MODULE_PARM_DESC(dmic_detect, "DMIC detect on SKL+ platforms");
 
 #ifdef CONFIG_PM
 static int param_set_xint(const char *val, const struct kernel_param *kp);
@@ -2025,6 +2029,25 @@ static const struct hda_controller_ops pci_hda_ops = {
 	.position_check = azx_position_check,
 };
 
+static int azx_check_dmic(struct pci_dev *pci, struct azx *chip)
+{
+	struct nhlt_acpi_table *nhlt;
+	int ret = 0;
+
+	if (chip->driver_type == AZX_DRIVER_SKL &&
+	    pci->class != 0x040300) {
+		nhlt = intel_nhlt_init(&pci->dev);
+		if (nhlt) {
+			if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) {
+				ret = -ENODEV;
+				dev_info(&pci->dev, "Digital mics found on Skylake+ platform, aborting probe\n");
+			}
+			intel_nhlt_free(nhlt);
+		}
+	}
+	return ret;
+}
+
 static int azx_probe(struct pci_dev *pci,
 		     const struct pci_device_id *pci_id)
 {
@@ -2055,6 +2078,17 @@ static int azx_probe(struct pci_dev *pci,
 	card->private_data = chip;
 	hda = container_of(chip, struct hda_intel, chip);
 
+	/*
+	 * stop probe if digital microphones detected on Skylake+ platform
+	 * with the DSP enabled. This is an opt-in behavior defined at build
+	 * time or at run-time with a module parameter
+	 */
+	if (dmic_detect) {
+		err = azx_check_dmic(pci, chip);
+		if (err < 0)
+			goto out_free;
+	}
+
 	pci_set_drvdata(pci, card);
 
 	err = register_vga_switcheroo(chip);
-- 
2.20.1

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Cezary Rojewski]

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?

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Takashi Iwai]

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

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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.

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Takashi Iwai]

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

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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
> 

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Takashi Iwai]

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

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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.

Re: [PATCH v2 2/5] ALSA: hda: move parts of NHLT code to new module

[Pierre-Louis Bossart]

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.
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel