[v2,2/5] ALSA: hda: move parts of NHLT code to new module
diff mbox series

Message ID 20190719203752.11151-3-pierre-louis.bossart@linux.intel.com
State New
Headers show
Series
  • ALSA/HDA: abort probe when DMICs are detected
Related show

Commit Message

Pierre-Louis Bossart July 19, 2019, 8:37 p.m. UTC
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

Comments

Cezary Rojewski July 20, 2019, 9:06 p.m. UTC | #1
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?
Takashi Iwai July 22, 2019, 8:54 a.m. UTC | #2
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
Pierre-Louis Bossart July 22, 2019, 12:14 p.m. UTC | #3
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.
Takashi Iwai July 22, 2019, 12:26 p.m. UTC | #4
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
Pierre-Louis Bossart July 22, 2019, 12:58 p.m. UTC | #5
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
>
Takashi Iwai July 22, 2019, 1:01 p.m. UTC | #6
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
Pierre-Louis Bossart July 22, 2019, 1:17 p.m. UTC | #7
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.
Pierre-Louis Bossart July 26, 2019, 10:09 p.m. UTC | #8
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.

Patch
diff mbox series

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