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