[PATCH 0/5] ALSA: hda: New NHLT functions and cleanup
Changes add two crucial functions: endpoint presence-check and retrieval of endpoint's BLOB (hardware configuration) to NHLT API.
Few cleanups accompany the above: First, drop device pointer usage in NHLT interface as those function don't really make use of it. Work is done to align NHLT-struct naming with other, commonly used ACPI-structs. While cleaning up, don't forget about "is DMIC in NHLT?" check. No need to check for channel count or anything DMIC-configuration related, just straight up verify link_type presence.
Amadeusz Sławiński (5): ALSA: hda: Drop device-argument in NHLT functions ALSA: hda: Follow ACPI convention in NHLT struct naming ALSA: hda: Fill gaps in NHLT endpoint-interface ALSA: hda: Simplify DMIC-in-NHLT check ASoC: Intel: Skylake: Use NHLT API to search for blob
include/sound/intel-nhlt.h | 37 +++++-- sound/hda/intel-dsp-config.c | 6 +- sound/hda/intel-nhlt.c | 127 ++++++++++++++++++++++--- sound/soc/intel/skylake/skl-nhlt.c | 108 +-------------------- sound/soc/intel/skylake/skl-pcm.c | 3 + sound/soc/intel/skylake/skl-topology.c | 27 +++--- sound/soc/intel/skylake/skl-topology.h | 1 + sound/soc/intel/skylake/skl.c | 5 +- sound/soc/intel/skylake/skl.h | 6 +- sound/soc/sof/intel/hda.c | 6 +- 10 files changed, 173 insertions(+), 153 deletions(-)
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/intel-nhlt.h | 9 ++++----- sound/hda/intel-dsp-config.c | 4 ++-- sound/hda/intel-nhlt.c | 24 +++++++++++++----------- sound/soc/intel/skylake/skl.c | 5 ++--- sound/soc/sof/intel/hda.c | 4 ++-- 5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index d0574805865f..4debab7c1996 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -126,17 +126,17 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); +struct nhlt_acpi_table *intel_nhlt_init(void);
void intel_nhlt_free(struct nhlt_acpi_table *addr);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt);
#else
struct nhlt_acpi_table;
-static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +static inline struct nhlt_acpi_table *intel_nhlt_init(void) { return NULL; } @@ -145,8 +145,7 @@ 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) +static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { return 0; } diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index b9ac9e9e45a4..60cc4735c6ec 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) struct nhlt_acpi_table *nhlt; int ret = 0;
- nhlt = intel_nhlt_init(&pci->dev); + nhlt = intel_nhlt_init(); if (nhlt) { - if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt)) + if (intel_nhlt_get_dmic_geo(nhlt)) ret = 1; intel_nhlt_free(nhlt); } diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index e2237239d922..195d9e193a6c 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2015-2019 Intel Corporation
+#define pr_fmt(fmt) "NHLT: " fmt + #include <linux/acpi.h> #include <sound/intel-nhlt.h>
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +struct nhlt_acpi_table *intel_nhlt_init(void) { struct nhlt_acpi_table *nhlt; acpi_status status; @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)&nhlt); if (ACPI_FAILURE(status)) { - dev_warn(dev, "NHLT table not found\n"); + pr_warn("NHLT table not found\n"); return NULL; }
@@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) } EXPORT_SYMBOL_GPL(intel_nhlt_free);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { struct nhlt_endpoint *epnt; struct nhlt_dmic_array_config *cfg; @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) return 0;
if (nhlt->header.length <= sizeof(struct acpi_table_header)) { - dev_warn(dev, "Invalid DMIC description table\n"); + pr_warn("Invalid DMIC description table\n"); return 0; }
@@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
/* find max number of channels based on format_configuration */ if (fmt_configs->fmt_count) { - dev_dbg(dev, "%s: found %d format definitions\n", + pr_debug("%s: found %d format definitions\n", __func__, fmt_configs->fmt_count);
for (i = 0; i < fmt_configs->fmt_count; i++) { @@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) if (fmt_ext->fmt.channels > max_ch) max_ch = fmt_ext->fmt.channels; } - dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch); + pr_debug("%s: max channels found %d\n", __func__, max_ch); } else { - dev_dbg(dev, "%s: No format information found\n", __func__); + pr_debug("%s: No format information found\n", __func__); }
if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) { @@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) dmic_geo = cfg_vendor->nb_mics; break; default: - dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n", + pr_warn("%s: undefined DMIC array_type 0x%0x\n", __func__, cfg->array_type); }
if (dmic_geo > 0) { - dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo); + pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); } if (max_ch > dmic_geo) { - dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n", + pr_debug("%s: max channels %d exceed dmic number %d\n", __func__, max_ch, dmic_geo); } } }
- dev_dbg(dev, "%s: dmic number %d max_ch %d\n", + pr_debug("%s: dmic number %d max_ch %d\n", __func__, dmic_geo, max_ch);
return dmic_geo; diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..4f122616b636 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; mach->mach_params.dmic_num = - intel_nhlt_get_dmic_geo(&skl->pci->dev, - skl->nhlt); + intel_nhlt_get_dmic_geo(skl->nhlt); }
return 0; @@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci,
device_disable_async_suspend(bus->dev);
- skl->nhlt = intel_nhlt_init(bus->dev); + skl->nhlt = intel_nhlt_init();
if (skl->nhlt == NULL) { #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 883d78dd01b5..75c2ee91bf13 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) struct nhlt_acpi_table *nhlt; int dmic_num;
- nhlt = intel_nhlt_init(sdev->dev); + nhlt = intel_nhlt_init(); if (nhlt) { - dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt); + dmic_num = intel_nhlt_get_dmic_geo(nhlt); intel_nhlt_free(nhlt); if (dmic_num >= 1 && dmic_num <= 4) return dmic_num;
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
include/sound/intel-nhlt.h | 9 ++++----- sound/hda/intel-dsp-config.c | 4 ++-- sound/hda/intel-nhlt.c | 24 +++++++++++++----------- sound/soc/intel/skylake/skl.c | 5 ++--- sound/soc/sof/intel/hda.c | 4 ++-- 5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index d0574805865f..4debab7c1996 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -126,17 +126,17 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); +struct nhlt_acpi_table *intel_nhlt_init(void);
void intel_nhlt_free(struct nhlt_acpi_table *addr);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt);
#else
struct nhlt_acpi_table;
-static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +static inline struct nhlt_acpi_table *intel_nhlt_init(void) { return NULL; } @@ -145,8 +145,7 @@ 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)+static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { return 0; } diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index b9ac9e9e45a4..60cc4735c6ec 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) struct nhlt_acpi_table *nhlt; int ret = 0;
- nhlt = intel_nhlt_init(&pci->dev);
- nhlt = intel_nhlt_init(); if (nhlt) {
if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt))
if (intel_nhlt_get_dmic_geo(nhlt)) ret = 1;diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index e2237239d922..195d9e193a6c 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2015-2019 Intel Corporation
+#define pr_fmt(fmt) "NHLT: " fmt
#include <linux/acpi.h> #include <sound/intel-nhlt.h>
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +struct nhlt_acpi_table *intel_nhlt_init(void) { struct nhlt_acpi_table *nhlt; acpi_status status; @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)&nhlt); if (ACPI_FAILURE(status)) {
dev_warn(dev, "NHLT table not found\n");
pr_warn("NHLT table not found\n");@@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) } EXPORT_SYMBOL_GPL(intel_nhlt_free);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { struct nhlt_endpoint *epnt; struct nhlt_dmic_array_config *cfg; @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) return 0;
if (nhlt->header.length <= sizeof(struct acpi_table_header)) {
dev_warn(dev, "Invalid DMIC description table\n");
pr_warn("Invalid DMIC description table\n");@@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
/* find max number of channels based on format_configuration */ if (fmt_configs->fmt_count) {
dev_dbg(dev, "%s: found %d format definitions\n",
pr_debug("%s: found %d format definitions\n", __func__, fmt_configs->fmt_count); for (i = 0; i < fmt_configs->fmt_count; i++) {@@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) if (fmt_ext->fmt.channels > max_ch) max_ch = fmt_ext->fmt.channels; }
dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch);
pr_debug("%s: max channels found %d\n", __func__, max_ch);
dev_dbg(dev, "%s: No format information found\n", __func__);
pr_debug("%s: No format information found\n", __func__);}
if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) {
@@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) dmic_geo = cfg_vendor->nb_mics; break; default:
dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n",
pr_warn("%s: undefined DMIC array_type 0x%0x\n", __func__, cfg->array_type); } if (dmic_geo > 0) {
dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo);
pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); } if (max_ch > dmic_geo) {
dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n",
pr_debug("%s: max channels %d exceed dmic number %d\n", __func__, max_ch, dmic_geo); }
- dev_dbg(dev, "%s: dmic number %d max_ch %d\n",
pr_debug("%s: dmic number %d max_ch %d\n", __func__, dmic_geo, max_ch);
return dmic_geo;
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..4f122616b636 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; mach->mach_params.dmic_num =
intel_nhlt_get_dmic_geo(&skl->pci->dev,skl->nhlt);
intel_nhlt_get_dmic_geo(skl->nhlt);}
return 0;
@@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci,
device_disable_async_suspend(bus->dev);
- skl->nhlt = intel_nhlt_init(bus->dev);
skl->nhlt = intel_nhlt_init();
if (skl->nhlt == NULL) {
#if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 883d78dd01b5..75c2ee91bf13 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) struct nhlt_acpi_table *nhlt; int dmic_num;
- nhlt = intel_nhlt_init(sdev->dev);
- nhlt = intel_nhlt_init(); if (nhlt) {
dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt);
dmic_num = intel_nhlt_get_dmic_geo(nhlt);
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote:
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it.
It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do.
thanks,
Takashi
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com
include/sound/intel-nhlt.h | 9 ++++----- sound/hda/intel-dsp-config.c | 4 ++-- sound/hda/intel-nhlt.c | 24 +++++++++++++----------- sound/soc/intel/skylake/skl.c | 5 ++--- sound/soc/sof/intel/hda.c | 4 ++-- 5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index d0574805865f..4debab7c1996 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -126,17 +126,17 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev); +struct nhlt_acpi_table *intel_nhlt_init(void);
void intel_nhlt_free(struct nhlt_acpi_table *addr);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt); +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt);
#else
struct nhlt_acpi_table;
-static inline struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +static inline struct nhlt_acpi_table *intel_nhlt_init(void) { return NULL; } @@ -145,8 +145,7 @@ 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)+static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { return 0; } diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index b9ac9e9e45a4..60cc4735c6ec 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -382,9 +382,9 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci) struct nhlt_acpi_table *nhlt; int ret = 0;
- nhlt = intel_nhlt_init(&pci->dev);
- nhlt = intel_nhlt_init(); if (nhlt) {
if (intel_nhlt_get_dmic_geo(&pci->dev, nhlt))
if (intel_nhlt_get_dmic_geo(nhlt)) ret = 1;diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index e2237239d922..195d9e193a6c 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -1,10 +1,12 @@ // SPDX-License-Identifier: GPL-2.0-only // Copyright (c) 2015-2019 Intel Corporation
+#define pr_fmt(fmt) "NHLT: " fmt
#include <linux/acpi.h> #include <sound/intel-nhlt.h>
-struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) +struct nhlt_acpi_table *intel_nhlt_init(void) { struct nhlt_acpi_table *nhlt; acpi_status status; @@ -12,7 +14,7 @@ struct nhlt_acpi_table *intel_nhlt_init(struct device *dev) status = acpi_get_table(ACPI_SIG_NHLT, 0, (struct acpi_table_header **)&nhlt); if (ACPI_FAILURE(status)) {
dev_warn(dev, "NHLT table not found\n");
pr_warn("NHLT table not found\n");@@ -26,7 +28,7 @@ void intel_nhlt_free(struct nhlt_acpi_table *nhlt) } EXPORT_SYMBOL_GPL(intel_nhlt_free);
-int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) +int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) { struct nhlt_endpoint *epnt; struct nhlt_dmic_array_config *cfg; @@ -40,7 +42,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) return 0;
if (nhlt->header.length <= sizeof(struct acpi_table_header)) {
dev_warn(dev, "Invalid DMIC description table\n");
pr_warn("Invalid DMIC description table\n");@@ -55,7 +57,7 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt)
/* find max number of channels based on format_configuration */ if (fmt_configs->fmt_count) {
dev_dbg(dev, "%s: found %d format definitions\n",
pr_debug("%s: found %d format definitions\n", __func__, fmt_configs->fmt_count); for (i = 0; i < fmt_configs->fmt_count; i++) {@@ -66,9 +68,9 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) if (fmt_ext->fmt.channels > max_ch) max_ch = fmt_ext->fmt.channels; }
dev_dbg(dev, "%s: max channels found %d\n", __func__, max_ch);
pr_debug("%s: max channels found %d\n", __func__, max_ch);
dev_dbg(dev, "%s: No format information found\n", __func__);
pr_debug("%s: No format information found\n", __func__);}
if (cfg->device_config.config_type != NHLT_CONFIG_TYPE_MIC_ARRAY) {
@@ -90,21 +92,21 @@ int intel_nhlt_get_dmic_geo(struct device *dev, struct nhlt_acpi_table *nhlt) dmic_geo = cfg_vendor->nb_mics; break; default:
dev_warn(dev, "%s: undefined DMIC array_type 0x%0x\n",
pr_warn("%s: undefined DMIC array_type 0x%0x\n", __func__, cfg->array_type); } if (dmic_geo > 0) {
dev_dbg(dev, "%s: Array with %d dmics\n", __func__, dmic_geo);
pr_debug("%s: Array with %d dmics\n", __func__, dmic_geo); } if (max_ch > dmic_geo) {
dev_dbg(dev, "%s: max channels %d exceed dmic number %d\n",
pr_debug("%s: max channels %d exceed dmic number %d\n", __func__, max_ch, dmic_geo); }
- dev_dbg(dev, "%s: dmic number %d max_ch %d\n",
pr_debug("%s: dmic number %d max_ch %d\n", __func__, dmic_geo, max_ch);
return dmic_geo;
diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c index 5b1a15e39912..4f122616b636 100644 --- a/sound/soc/intel/skylake/skl.c +++ b/sound/soc/intel/skylake/skl.c @@ -517,8 +517,7 @@ static int skl_find_machine(struct skl_dev *skl, void *driver_data) if (pdata) { skl->use_tplg_pcm = pdata->use_tplg_pcm; mach->mach_params.dmic_num =
intel_nhlt_get_dmic_geo(&skl->pci->dev,skl->nhlt);
intel_nhlt_get_dmic_geo(skl->nhlt);}
return 0;
@@ -1009,7 +1008,7 @@ static int skl_probe(struct pci_dev *pci,
device_disable_async_suspend(bus->dev);
- skl->nhlt = intel_nhlt_init(bus->dev);
skl->nhlt = intel_nhlt_init();
if (skl->nhlt == NULL) {
#if !IS_ENABLED(CONFIG_SND_SOC_INTEL_SKYLAKE_HDAUDIO_CODEC) diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 883d78dd01b5..75c2ee91bf13 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -651,9 +651,9 @@ static int check_nhlt_dmic(struct snd_sof_dev *sdev) struct nhlt_acpi_table *nhlt; int dmic_num;
- nhlt = intel_nhlt_init(sdev->dev);
- nhlt = intel_nhlt_init(); if (nhlt) {
dmic_num = intel_nhlt_get_dmic_geo(sdev->dev, nhlt);
dmic_num = intel_nhlt_get_dmic_geo(nhlt);
On 2021-10-17 9:52 AM, Takashi Iwai wrote:
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote:
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it.
It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do.
Isn't the answer as to 'who' used it obvious, though? NHLT is used for I2S and DMIC endpoints only, so the question is 'limited' in the first place. And then, you cannot have several Intel ADSP drivers running simultaneously.
Also, logs found ACPI-table interface are device-less so this patch makes NHLT interface look more ACPI-generic alike.
Regards, Czarek
On Mon, 18 Oct 2021 10:08:30 +0200, Cezary Rojewski wrote:
On 2021-10-17 9:52 AM, Takashi Iwai wrote:
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote:
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it.
It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do.
Isn't the answer as to 'who' used it obvious, though? NHLT is used for I2S and DMIC endpoints only, so the question is 'limited' in the first place. And then, you cannot have several Intel ADSP drivers running simultaneously.
Well, it's not about you or devs -- those must know which driver is relevant very well, of course. Instead, the problem is for *all* others who read the message.
IOW, which user would know and think "hey, it's a NHLT thingy that must be ASoC xxx driver that spewed" only from the text snippet "NHLT table not found"? That's way too much expectation. Some more guidance is needed in the error message. The dev_*() variant gave at least the device names that can help guessing the relevant driver easily.
Also, logs found ACPI-table interface are device-less so this patch makes NHLT interface look more ACPI-generic alike.
The conversion itself is no problem, but the lost information is the problem.
Takashi
On 2021-10-18 10:25 AM, Takashi Iwai wrote:
On Mon, 18 Oct 2021 10:08:30 +0200, Cezary Rojewski wrote:
On 2021-10-17 9:52 AM, Takashi Iwai wrote:
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote:
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it.
It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do.
Isn't the answer as to 'who' used it obvious, though? NHLT is used for I2S and DMIC endpoints only, so the question is 'limited' in the first place. And then, you cannot have several Intel ADSP drivers running simultaneously.
Well, it's not about you or devs -- those must know which driver is relevant very well, of course. Instead, the problem is for *all* others who read the message.
IOW, which user would know and think "hey, it's a NHLT thingy that must be ASoC xxx driver that spewed" only from the text snippet "NHLT table not found"? That's way too much expectation. Some more guidance is needed in the error message. The dev_*() variant gave at least the device names that can help guessing the relevant driver easily.
Also, logs found ACPI-table interface are device-less so this patch makes NHLT interface look more ACPI-generic alike.
The conversion itself is no problem, but the lost information is the problem.
Well, most invocations that end in error will cause stack of error-logs (dev_xxx()) in the parent driver. NHLT reads: Non-HDAudio-Link-Table. The name itself points reader towards sound/hda already. I understand that not everyone will google "NHLT" and such but then there are ton of kernel messages which won't guide reader by the hand anyway.
"NHLT table not found" is configuration problem, not a driver problem. Mixing BIOS/config problems with driver ones leads to incorrect conclusions what we want to avoid.
Regards, Czarek
On Mon, 18 Oct 2021 11:07:15 +0200, Cezary Rojewski wrote:
On 2021-10-18 10:25 AM, Takashi Iwai wrote:
On Mon, 18 Oct 2021 10:08:30 +0200, Cezary Rojewski wrote:
On 2021-10-17 9:52 AM, Takashi Iwai wrote:
On Fri, 15 Oct 2021 18:42:33 +0200, Pierre-Louis Bossart wrote:
On 10/15/21 11:40 AM, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
ACPI is device independent, so printing warnings using device functions is misleading. Replace dev_xxx() with pr_xxx() and remove now unnecessary argument.
the routines in sound/hda/intel-nhtl.c are called from a specific PCI device, why would you remove that information?
This makes no sense to me.
Right, otherwise this change would confuse user, too; they'll be clueless about who triggers it.
It's OK to change to pr_*(), but then it should have more information that can be easily identified and understood what user should do.
Isn't the answer as to 'who' used it obvious, though? NHLT is used for I2S and DMIC endpoints only, so the question is 'limited' in the first place. And then, you cannot have several Intel ADSP drivers running simultaneously.
Well, it's not about you or devs -- those must know which driver is relevant very well, of course. Instead, the problem is for *all* others who read the message.
IOW, which user would know and think "hey, it's a NHLT thingy that must be ASoC xxx driver that spewed" only from the text snippet "NHLT table not found"? That's way too much expectation. Some more guidance is needed in the error message. The dev_*() variant gave at least the device names that can help guessing the relevant driver easily.
Also, logs found ACPI-table interface are device-less so this patch makes NHLT interface look more ACPI-generic alike.
The conversion itself is no problem, but the lost information is the problem.
Well, most invocations that end in error will cause stack of error-logs (dev_xxx()) in the parent driver. NHLT reads: Non-HDAudio-Link-Table. The name itself points reader towards sound/hda already.
But then how dropping the device information would improve things at all? It rather cuts off the link between that message and the later one.
I understand that not everyone will google "NHLT" and such but then there are ton of kernel messages which won't guide reader by the hand anyway.
This can't be an excuse, we don't have to follow that anti pattern :)
"NHLT table not found" is configuration problem, not a driver problem. Mixing BIOS/config problems with driver ones leads to incorrect conclusions what we want to avoid.
The configuration itself makes sense only if it's in actual use, i.e. the problem is always tied with the driver.
So, if any, you can put more information around the message mentioning that it's the configuration problem and what to do for users, e.g. check your BIOS, etc. *That* would be the improvement.
Takashi
On 2021-10-18 2:01 PM, Takashi Iwai wrote:
On Mon, 18 Oct 2021 11:07:15 +0200, Cezary Rojewski wrote:
...
Well, most invocations that end in error will cause stack of error-logs (dev_xxx()) in the parent driver. NHLT reads: Non-HDAudio-Link-Table. The name itself points reader towards sound/hda already.
But then how dropping the device information would improve things at all? It rather cuts off the link between that message and the later one.
I understand that not everyone will google "NHLT" and such but then there are ton of kernel messages which won't guide reader by the hand anyway.
This can't be an excuse, we don't have to follow that anti pattern :)
"NHLT table not found" is configuration problem, not a driver problem. Mixing BIOS/config problems with driver ones leads to incorrect conclusions what we want to avoid.
The configuration itself makes sense only if it's in actual use, i.e. the problem is always tied with the driver.
So, if any, you can put more information around the message mentioning that it's the configuration problem and what to do for users, e.g. check your BIOS, etc. *That* would be the improvement.
Ok, I'll think about better solution. For now I'll just drop this patch from the series and send v2.
Thanks for the discussion!
Czarek
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
All ACPI table structs except for NHLT ones are defined in postfix way. Update NHLT structs naming to make code cohesive.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/intel-nhlt.h | 16 ++++++++-------- sound/hda/intel-dsp-config.c | 2 +- sound/hda/intel-nhlt.c | 8 ++++---- sound/soc/intel/skylake/skl-nhlt.c | 8 ++++---- sound/soc/intel/skylake/skl.h | 2 +- sound/soc/sof/intel/hda.c | 2 +- 6 files changed, 19 insertions(+), 19 deletions(-)
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index 4debab7c1996..b7eec4b3ca01 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -77,7 +77,7 @@ struct nhlt_endpoint { struct nhlt_specific_cfg config; } __packed;
-struct nhlt_acpi_table { +struct acpi_table_nhlt { struct acpi_table_header header; u8 endpoint_count; struct nhlt_endpoint desc[]; @@ -126,26 +126,26 @@ enum { NHLT_MIC_ARRAY_VENDOR_DEFINED = 0xf, };
-struct nhlt_acpi_table *intel_nhlt_init(void); +struct acpi_table_nhlt *intel_nhlt_init(void);
-void intel_nhlt_free(struct nhlt_acpi_table *addr); +void intel_nhlt_free(struct acpi_table_nhlt *addr);
-int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt); +int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt);
#else
-struct nhlt_acpi_table; +struct acpi_table_nhlt;
-static inline struct nhlt_acpi_table *intel_nhlt_init(void) +static inline struct acpi_table_nhlt *intel_nhlt_init(void) { return NULL; }
-static inline void intel_nhlt_free(struct nhlt_acpi_table *addr) +static inline void intel_nhlt_free(struct acpi_table_nhlt *addr) { }
-static inline int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) +static inline int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt) { return 0; } diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index 60cc4735c6ec..4c9d5bc39cbd 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -379,7 +379,7 @@ static const struct config_entry *snd_intel_dsp_find_config
static int snd_intel_dsp_check_dmic(struct pci_dev *pci) { - struct nhlt_acpi_table *nhlt; + struct acpi_table_nhlt *nhlt; int ret = 0;
nhlt = intel_nhlt_init(); diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index 195d9e193a6c..c41d20e1b349 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -6,9 +6,9 @@ #include <linux/acpi.h> #include <sound/intel-nhlt.h>
-struct nhlt_acpi_table *intel_nhlt_init(void) +struct acpi_table_nhlt *intel_nhlt_init(void) { - struct nhlt_acpi_table *nhlt; + struct acpi_table_nhlt *nhlt; acpi_status status;
status = acpi_get_table(ACPI_SIG_NHLT, 0, @@ -22,13 +22,13 @@ struct nhlt_acpi_table *intel_nhlt_init(void) } EXPORT_SYMBOL_GPL(intel_nhlt_init);
-void intel_nhlt_free(struct nhlt_acpi_table *nhlt) +void intel_nhlt_free(struct acpi_table_nhlt *nhlt) { acpi_put_table((struct acpi_table_header *)nhlt); } EXPORT_SYMBOL_GPL(intel_nhlt_free);
-int intel_nhlt_get_dmic_geo(struct nhlt_acpi_table *nhlt) +int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt) { struct nhlt_endpoint *epnt; struct nhlt_dmic_array_config *cfg; diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 64226072f0ee..3033c1bf279f 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -88,7 +88,7 @@ struct nhlt_specific_cfg struct hdac_bus *bus = skl_to_bus(skl); struct device *dev = bus->dev; struct nhlt_specific_cfg *sp_config; - struct nhlt_acpi_table *nhlt = skl->nhlt; + struct acpi_table_nhlt *nhlt = skl->nhlt; u16 bps = (s_fmt == 16) ? 16 : 32; u8 j;
@@ -132,7 +132,7 @@ static void skl_nhlt_trim_space(char *trim)
int skl_nhlt_update_topology_bin(struct skl_dev *skl) { - struct nhlt_acpi_table *nhlt = (struct nhlt_acpi_table *)skl->nhlt; + struct acpi_table_nhlt *nhlt = (struct acpi_table_nhlt *)skl->nhlt; struct hdac_bus *bus = skl_to_bus(skl); struct device *dev = bus->dev;
@@ -155,7 +155,7 @@ static ssize_t platform_id_show(struct device *dev, struct pci_dev *pci = to_pci_dev(dev); struct hdac_bus *bus = pci_get_drvdata(pci); struct skl_dev *skl = bus_to_skl(bus); - struct nhlt_acpi_table *nhlt = (struct nhlt_acpi_table *)skl->nhlt; + struct acpi_table_nhlt *nhlt = (struct acpi_table_nhlt *)skl->nhlt; char platform_id[32];
sprintf(platform_id, "%x-%.6s-%.8s-%d", skl->pci_id, @@ -335,7 +335,7 @@ static void skl_get_mclk(struct skl_dev *skl, struct skl_ssp_clk *mclk,
void skl_get_clks(struct skl_dev *skl, struct skl_ssp_clk *ssp_clks) { - struct nhlt_acpi_table *nhlt = (struct nhlt_acpi_table *)skl->nhlt; + struct acpi_table_nhlt *nhlt = (struct acpi_table_nhlt *)skl->nhlt; struct nhlt_endpoint *epnt; struct nhlt_fmt *fmt; int i; diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 33ed274fc0cb..37195aafbf27 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -67,7 +67,7 @@ struct skl_dev { struct snd_soc_component *component; struct snd_soc_dai_driver *dais;
- struct nhlt_acpi_table *nhlt; /* nhlt ptr */ + struct acpi_table_nhlt *nhlt; /* nhlt ptr */
struct list_head ppl_list; struct list_head bind_list; diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c index 75c2ee91bf13..4a8907fe7f94 100644 --- a/sound/soc/sof/intel/hda.c +++ b/sound/soc/sof/intel/hda.c @@ -648,7 +648,7 @@ static int hda_init(struct snd_sof_dev *sdev)
static int check_nhlt_dmic(struct snd_sof_dev *sdev) { - struct nhlt_acpi_table *nhlt; + struct acpi_table_nhlt *nhlt; int dmic_num;
nhlt = intel_nhlt_init();
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
Two key operations missings are: endpoint presence-check and retrieval of matching endpoint hardware configuration (blob). Add operations for both use cases.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- include/sound/intel-nhlt.h | 20 ++++++++ sound/hda/intel-nhlt.c | 99 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+)
diff --git a/include/sound/intel-nhlt.h b/include/sound/intel-nhlt.h index b7eec4b3ca01..2cd3f27b7a72 100644 --- a/include/sound/intel-nhlt.h +++ b/include/sound/intel-nhlt.h @@ -132,6 +132,12 @@ void intel_nhlt_free(struct acpi_table_nhlt *addr);
int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt);
+bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type); +struct nhlt_specific_cfg * +intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt, + u32 bus_id, u8 link_type, u8 vbps, u8 bps, + u8 num_ch, u32 rate, u8 dir, u8 dev_type); + #else
struct acpi_table_nhlt; @@ -149,6 +155,20 @@ static inline int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt) { return 0; } + +bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) +{ + return false; +} + +struct nhlt_specific_cfg * +intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt, + u32 bus_id, u8 link_type, u8 vbps, u8 bps, + u8 num_ch, u32 rate, u8 dir, u8 dev_type) +{ + return NULL; +} + #endif
#endif diff --git a/sound/hda/intel-nhlt.c b/sound/hda/intel-nhlt.c index c41d20e1b349..d97fe57a7ef2 100644 --- a/sound/hda/intel-nhlt.c +++ b/sound/hda/intel-nhlt.c @@ -112,3 +112,102 @@ int intel_nhlt_get_dmic_geo(struct acpi_table_nhlt *nhlt) return dmic_geo; } EXPORT_SYMBOL_GPL(intel_nhlt_get_dmic_geo); + +bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) +{ + struct nhlt_endpoint *epnt; + int i; + + if (!nhlt) + return false; + + epnt = (struct nhlt_endpoint *)nhlt->desc; + for (i = 0; i < nhlt->endpoint_count; i++) { + if (epnt->linktype == link_type) + return true; + + epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); + } + return false; +} +EXPORT_SYMBOL(intel_nhlt_has_endpoint_type); + +static struct nhlt_specific_cfg * +nhlt_get_specific_cfg(struct nhlt_fmt *fmt, u8 num_ch, u32 rate, u8 vbps, u8 bps) +{ + struct nhlt_fmt_cfg *cfg = fmt->fmt_config; + struct wav_fmt *wfmt; + u16 _bps, _vbps; + int i; + + pr_debug("Endpoint format count=%d\n", fmt->fmt_count); + + for (i = 0; i < fmt->fmt_count; i++) { + wfmt = &cfg->fmt_ext.fmt; + _bps = wfmt->bits_per_sample; + _vbps = cfg->fmt_ext.sample.valid_bits_per_sample; + + pr_debug("Endpoint format: ch=%d fmt=%d/%d rate=%d\n", + wfmt->channels, _vbps, _bps, wfmt->samples_per_sec); + + if (wfmt->channels == num_ch && wfmt->samples_per_sec == rate && + vbps == _vbps && bps == _bps) + return &cfg->config; + + cfg = (struct nhlt_fmt_cfg *)(cfg->config.caps + cfg->config.size); + } + + return NULL; +} + +static bool nhlt_check_ep_match(struct nhlt_endpoint *epnt, + u32 bus_id, u8 link_type, u8 dir, u8 dev_type) +{ + pr_debug("Endpoint: vbus_id=%d link_type=%d dir=%d dev_type = %d\n", + epnt->virtual_bus_id, epnt->linktype, + epnt->direction, epnt->device_type); + + if ((epnt->virtual_bus_id != bus_id) || + (epnt->linktype != link_type) || + (epnt->direction != dir)) + return false; + + /* link of type DMIC bypasses device_type check */ + return epnt->linktype == NHLT_LINK_DMIC || + epnt->device_type == dev_type; +} + +struct nhlt_specific_cfg * +intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt, + u32 bus_id, u8 link_type, u8 vbps, u8 bps, + u8 num_ch, u32 rate, u8 dir, u8 dev_type) +{ + struct nhlt_specific_cfg *cfg; + struct nhlt_endpoint *epnt; + struct nhlt_fmt *fmt; + int i; + + if (!nhlt) + return NULL; + + pr_debug("Looking for configuration:\n"); + pr_debug(" vbus_id=%d link_type=%d dir=%d, dev_type=%d\n", + bus_id, link_type, dir, dev_type); + pr_debug(" ch=%d fmt=%d/%d rate=%d\n", num_ch, vbps, bps, rate); + pr_debug("Endpoint count=%d\n", nhlt->endpoint_count); + + epnt = (struct nhlt_endpoint *)nhlt->desc; + for (i = 0; i < nhlt->endpoint_count; i++) { + if (nhlt_check_ep_match(epnt, bus_id, link_type, dir, dev_type)) { + fmt = (struct nhlt_fmt *)(epnt->config.caps + epnt->config.size); + cfg = nhlt_get_specific_cfg(fmt, num_ch, rate, vbps, bps); + if (cfg) + return cfg; + } + + epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); + } + + return NULL; +} +EXPORT_SYMBOL(intel_nhlt_get_endpoint_blob);
Hi Cezary,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on broonie-sound/for-next] [also build test WARNING on tiwai-sound/for-next v5.15-rc5 next-20211015] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Cezary-Rojewski/ALSA-hda-New-NHLT-f... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next config: riscv-randconfig-r042-20211016 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6069a6a5049497a32a50a49661c2f4169078bdba) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/418a9f8fd0b93d717ac19a0fed367c60fe2e... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Cezary-Rojewski/ALSA-hda-New-NHLT-functions-and-cleanup/20211016-004154 git checkout 418a9f8fd0b93d717ac19a0fed367c60fe2e4352 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
In file included from sound/hda/intel-dsp-config.c:13:
include/sound/intel-nhlt.h:159:6: warning: no previous prototype for function 'intel_nhlt_has_endpoint_type' [-Wmissing-prototypes]
bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) ^ include/sound/intel-nhlt.h:159:1: note: declare 'static' if the function is not intended to be used outside of this translation unit bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) ^ static
include/sound/intel-nhlt.h:165:1: warning: no previous prototype for function 'intel_nhlt_get_endpoint_blob' [-Wmissing-prototypes]
intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt, ^ include/sound/intel-nhlt.h:164:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct nhlt_specific_cfg * ^ static 2 warnings generated.
vim +/intel_nhlt_has_endpoint_type +159 include/sound/intel-nhlt.h
158
159 bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type)
160 { 161 return false; 162 } 163 164 struct nhlt_specific_cfg *
165 intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt,
166 u32 bus_id, u8 link_type, u8 vbps, u8 bps, 167 u8 num_ch, u32 rate, u8 dir, u8 dev_type) 168 { 169 return NULL; 170 } 171
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
Only DMIC endpoint presence is relevant, not its configuration.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/hda/intel-dsp-config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/hda/intel-dsp-config.c b/sound/hda/intel-dsp-config.c index 4c9d5bc39cbd..e8fc5557436b 100644 --- a/sound/hda/intel-dsp-config.c +++ b/sound/hda/intel-dsp-config.c @@ -384,7 +384,7 @@ static int snd_intel_dsp_check_dmic(struct pci_dev *pci)
nhlt = intel_nhlt_init(); if (nhlt) { - if (intel_nhlt_get_dmic_geo(nhlt)) + if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_DMIC)) ret = 1; intel_nhlt_free(nhlt); }
Hi Cezary,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on broonie-sound/for-next] [also build test ERROR on tiwai-sound/for-next v5.15-rc5 next-20211015] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Cezary-Rojewski/ALSA-hda-New-NHLT-f... base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next config: riscv-randconfig-r042-20211016 (attached as .config) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 6069a6a5049497a32a50a49661c2f4169078bdba) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/2d319eabf52e504c98ef5c0fdd812347b41e... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Cezary-Rojewski/ALSA-hda-New-NHLT-functions-and-cleanup/20211016-004154 git checkout 2d319eabf52e504c98ef5c0fdd812347b41edd00 # save the attached .config to linux build tree mkdir build_dir COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=riscv SHELL=/bin/bash sound/hda/
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All errors (new ones prefixed by >>):
In file included from sound/hda/intel-dsp-config.c:13: include/sound/intel-nhlt.h:159:6: warning: no previous prototype for function 'intel_nhlt_has_endpoint_type' [-Wmissing-prototypes] bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) ^ include/sound/intel-nhlt.h:159:1: note: declare 'static' if the function is not intended to be used outside of this translation unit bool intel_nhlt_has_endpoint_type(struct acpi_table_nhlt *nhlt, u8 link_type) ^ static include/sound/intel-nhlt.h:165:1: warning: no previous prototype for function 'intel_nhlt_get_endpoint_blob' [-Wmissing-prototypes] intel_nhlt_get_endpoint_blob(struct acpi_table_nhlt *nhlt, ^ include/sound/intel-nhlt.h:164:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct nhlt_specific_cfg * ^ static
sound/hda/intel-dsp-config.c:387:42: error: use of undeclared identifier 'NHLT_LINK_DMIC'
if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_DMIC)) ^
sound/hda/intel-dsp-config.c:387:42: error: use of undeclared identifier 'NHLT_LINK_DMIC' sound/hda/intel-dsp-config.c:387:42: error: use of undeclared identifier 'NHLT_LINK_DMIC'
2 warnings and 3 errors generated.
vim +/NHLT_LINK_DMIC +387 sound/hda/intel-dsp-config.c
379 380 static int snd_intel_dsp_check_dmic(struct pci_dev *pci) 381 { 382 struct acpi_table_nhlt *nhlt; 383 int ret = 0; 384 385 nhlt = intel_nhlt_init(); 386 if (nhlt) {
387 if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_DMIC))
388 ret = 1; 389 intel_nhlt_free(nhlt); 390 } 391 return ret; 392 } 393
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
With NHLT enriched with new search functions, remove local code in favour of them. This also fixes broken behaviour: search should be based on significant bits count rather than container size.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/intel/skylake/skl-nhlt.c | 102 ------------------------- sound/soc/intel/skylake/skl-pcm.c | 3 + sound/soc/intel/skylake/skl-topology.c | 27 ++++--- sound/soc/intel/skylake/skl-topology.h | 1 + sound/soc/intel/skylake/skl.h | 4 - 5 files changed, 19 insertions(+), 118 deletions(-)
diff --git a/sound/soc/intel/skylake/skl-nhlt.c b/sound/soc/intel/skylake/skl-nhlt.c index 3033c1bf279f..16f70554b2a0 100644 --- a/sound/soc/intel/skylake/skl-nhlt.c +++ b/sound/soc/intel/skylake/skl-nhlt.c @@ -13,108 +13,6 @@ #include "skl.h" #include "skl-i2s.h"
-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) -{ - struct nhlt_specific_cfg *sp_config; - struct wav_fmt *wfmt; - struct nhlt_fmt_cfg *fmt_config = fmt->fmt_config; - int i; - - dev_dbg(dev, "Format count =%d\n", fmt->fmt_count); - - for (i = 0; i < fmt->fmt_count; i++) { - wfmt = &fmt_config->fmt_ext.fmt; - dev_dbg(dev, "ch=%d fmt=%d s_rate=%d\n", wfmt->channels, - wfmt->bits_per_sample, wfmt->samples_per_sec); - if (wfmt->channels == no_ch && wfmt->bits_per_sample == bps) { - /* - * if link type is dmic ignore rate check as the blob is - * generic for all rates - */ - sp_config = &fmt_config->config; - if (linktype == NHLT_LINK_DMIC) - return sp_config; - - if (wfmt->samples_per_sec == rate) - return sp_config; - } - - fmt_config = (struct nhlt_fmt_cfg *)(fmt_config->config.caps + - fmt_config->config.size); - } - - return NULL; -} - -static void dump_config(struct device *dev, u32 instance_id, u8 linktype, - u8 s_fmt, u8 num_channels, u32 s_rate, u8 dirn, u16 bps) -{ - dev_dbg(dev, "Input configuration\n"); - dev_dbg(dev, "ch=%d fmt=%d s_rate=%d\n", num_channels, s_fmt, s_rate); - dev_dbg(dev, "vbus_id=%d link_type=%d\n", instance_id, linktype); - dev_dbg(dev, "bits_per_sample=%d\n", bps); -} - -static bool skl_check_ep_match(struct device *dev, struct nhlt_endpoint *epnt, - u32 instance_id, u8 link_type, u8 dirn, u8 dev_type) -{ - dev_dbg(dev, "vbus_id=%d link_type=%d dir=%d dev_type = %d\n", - epnt->virtual_bus_id, epnt->linktype, - epnt->direction, epnt->device_type); - - if ((epnt->virtual_bus_id == instance_id) && - (epnt->linktype == link_type) && - (epnt->direction == dirn)) { - /* do not check dev_type for DMIC link type */ - if (epnt->linktype == NHLT_LINK_DMIC) - return true; - - if (epnt->device_type == dev_type) - return true; - } - - return false; -} - -struct nhlt_specific_cfg -*skl_get_ep_blob(struct skl_dev *skl, u32 instance, u8 link_type, - u8 s_fmt, u8 num_ch, u32 s_rate, - u8 dirn, u8 dev_type) -{ - struct nhlt_fmt *fmt; - struct nhlt_endpoint *epnt; - struct hdac_bus *bus = skl_to_bus(skl); - struct device *dev = bus->dev; - struct nhlt_specific_cfg *sp_config; - struct acpi_table_nhlt *nhlt = skl->nhlt; - u16 bps = (s_fmt == 16) ? 16 : 32; - u8 j; - - dump_config(dev, instance, link_type, s_fmt, num_ch, s_rate, dirn, bps); - - epnt = (struct nhlt_endpoint *)nhlt->desc; - - dev_dbg(dev, "endpoint count =%d\n", nhlt->endpoint_count); - - for (j = 0; j < nhlt->endpoint_count; j++) { - if (skl_check_ep_match(dev, epnt, instance, link_type, - dirn, dev_type)) { - fmt = (struct nhlt_fmt *)(epnt->config.caps + - epnt->config.size); - sp_config = skl_get_specific_cfg(dev, fmt, num_ch, - s_rate, bps, link_type); - if (sp_config) - return sp_config; - } - - epnt = (struct nhlt_endpoint *)((u8 *)epnt + epnt->length); - } - - return NULL; -} - static void skl_nhlt_trim_space(char *trim) { char *s = trim; diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c index 9ecaf6a1e847..34908af50046 100644 --- a/sound/soc/intel/skylake/skl-pcm.c +++ b/sound/soc/intel/skylake/skl-pcm.c @@ -317,6 +317,7 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream, dev_dbg(dai->dev, "dma_id=%d\n", dma_id);
p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.s_cont = snd_pcm_format_physical_width(params_format(params)); p_params.ch = params_channels(params); p_params.s_freq = params_rate(params); p_params.host_dma_id = dma_id; @@ -405,6 +406,7 @@ static int skl_be_hw_params(struct snd_pcm_substream *substream, struct skl_pipe_params p_params = {0};
p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.s_cont = snd_pcm_format_physical_width(params_format(params)); p_params.ch = params_channels(params); p_params.s_freq = params_rate(params); p_params.stream = substream->stream; @@ -569,6 +571,7 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream, snd_soc_dai_set_tdm_slot(codec_dai, 0, stream_tag, 0, 0);
p_params.s_fmt = snd_pcm_format_width(params_format(params)); + p_params.s_cont = snd_pcm_format_physical_width(params_format(params)); p_params.ch = params_channels(params); p_params.s_freq = params_rate(params); p_params.stream = substream->stream; diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c index 89e4231304dd..4a92f046925d 100644 --- a/sound/soc/intel/skylake/skl-topology.c +++ b/sound/soc/intel/skylake/skl-topology.c @@ -285,7 +285,7 @@ static int skl_tplg_update_be_blob(struct snd_soc_dapm_widget *w, { struct skl_module_cfg *m_cfg = w->priv; int link_type, dir; - u32 ch, s_freq, s_fmt; + u32 ch, s_freq, s_fmt, s_cont; struct nhlt_specific_cfg *cfg; u8 dev_type = skl_tplg_be_dev_type(m_cfg->dev_type); int fmt_idx = m_cfg->fmt_idx; @@ -301,7 +301,8 @@ static int skl_tplg_update_be_blob(struct snd_soc_dapm_widget *w, link_type = NHLT_LINK_DMIC; dir = SNDRV_PCM_STREAM_CAPTURE; s_freq = m_iface->inputs[0].fmt.s_freq; - s_fmt = m_iface->inputs[0].fmt.bit_depth; + s_fmt = m_iface->inputs[0].fmt.valid_bit_depth; + s_cont = m_iface->inputs[0].fmt.bit_depth; ch = m_iface->inputs[0].fmt.channels; break;
@@ -310,12 +311,14 @@ static int skl_tplg_update_be_blob(struct snd_soc_dapm_widget *w, if (m_cfg->hw_conn_type == SKL_CONN_SOURCE) { dir = SNDRV_PCM_STREAM_PLAYBACK; s_freq = m_iface->outputs[0].fmt.s_freq; - s_fmt = m_iface->outputs[0].fmt.bit_depth; + s_fmt = m_iface->outputs[0].fmt.valid_bit_depth; + s_cont = m_iface->outputs[0].fmt.bit_depth; ch = m_iface->outputs[0].fmt.channels; } else { dir = SNDRV_PCM_STREAM_CAPTURE; s_freq = m_iface->inputs[0].fmt.s_freq; - s_fmt = m_iface->inputs[0].fmt.bit_depth; + s_fmt = m_iface->inputs[0].fmt.valid_bit_depth; + s_cont = m_iface->inputs[0].fmt.bit_depth; ch = m_iface->inputs[0].fmt.channels; } break; @@ -325,16 +328,16 @@ static int skl_tplg_update_be_blob(struct snd_soc_dapm_widget *w, }
/* update the blob based on virtual bus_id and default params */ - cfg = skl_get_ep_blob(skl, m_cfg->vbus_id, link_type, - s_fmt, ch, s_freq, dir, dev_type); + cfg = intel_nhlt_get_endpoint_blob(skl->nhlt, m_cfg->vbus_id, link_type, + s_fmt, s_cont, ch, s_freq, dir, dev_type); if (cfg) { m_cfg->formats_config[SKL_PARAM_INIT].caps_size = cfg->size; m_cfg->formats_config[SKL_PARAM_INIT].caps = (u32 *)&cfg->caps; } else { dev_err(skl->dev, "Blob NULL for id %x type %d dirn %d\n", m_cfg->vbus_id, link_type, dir); - dev_err(skl->dev, "PCM: ch %d, freq %d, fmt %d\n", - ch, s_freq, s_fmt); + dev_err(skl->dev, "PCM: ch %d, freq %d, fmt %d/%d\n", + ch, s_freq, s_fmt, s_cont); return -EIO; }
@@ -1849,10 +1852,10 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai, pipe_fmt = &pipe->configs[pipe->pipe_config_idx].in_fmt;
/* update the blob based on virtual bus_id*/ - cfg = skl_get_ep_blob(skl, mconfig->vbus_id, link_type, - pipe_fmt->bps, pipe_fmt->channels, - pipe_fmt->freq, pipe->direction, - dev_type); + cfg = intel_nhlt_get_endpoint_blob(skl->nhlt, mconfig->vbus_id, link_type, + pipe_fmt->bps, params->s_cont, + pipe_fmt->channels, pipe_fmt->freq, + pipe->direction, dev_type); if (cfg) { mconfig->formats_config[SKL_PARAM_INIT].caps_size = cfg->size; mconfig->formats_config[SKL_PARAM_INIT].caps = (u32 *)&cfg->caps; diff --git a/sound/soc/intel/skylake/skl-topology.h b/sound/soc/intel/skylake/skl-topology.h index f0695b2ac5dd..22963634fbea 100644 --- a/sound/soc/intel/skylake/skl-topology.h +++ b/sound/soc/intel/skylake/skl-topology.h @@ -284,6 +284,7 @@ struct skl_pipe_params { u32 ch; u32 s_freq; u32 s_fmt; + u32 s_cont; u8 linktype; snd_pcm_format_t format; int link_index; diff --git a/sound/soc/intel/skylake/skl.h b/sound/soc/intel/skylake/skl.h index 37195aafbf27..3b92dfc23731 100644 --- a/sound/soc/intel/skylake/skl.h +++ b/sound/soc/intel/skylake/skl.h @@ -165,10 +165,6 @@ struct skl_dsp_ops { int skl_platform_unregister(struct device *dev); int skl_platform_register(struct device *dev);
-struct nhlt_specific_cfg *skl_get_ep_blob(struct skl_dev *skl, u32 instance, - u8 link_type, u8 s_fmt, u8 num_ch, - u32 s_rate, u8 dirn, u8 dev_type); - int skl_nhlt_update_topology_bin(struct skl_dev *skl); int skl_init_dsp(struct skl_dev *skl); int skl_free_dsp(struct skl_dev *skl);
On Fri, Oct 15, 2021 at 06:40:47PM +0200, Cezary Rojewski wrote:
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
With NHLT enriched with new search functions, remove local code in favour of them. This also fixes broken behaviour: search should be based on significant bits count rather than container size.
Acked-by: Mark Brown broonie@kernel.org
participants (5)
-
Cezary Rojewski -
kernel test robot -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai