[alsa-devel] [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.