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@xxxxxxxxxxxxxxxx
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
