Re: [alsa-devel] [PATCH v3] ALSA: hda: add Intel DSP configuration / probe code

[Takashi Iwai <tiwai@suse.de>]

On Sun, 06 Oct 2019 17:22:32 +0200,
Jaroslav Kysela wrote:
> 
> For distributions, we need one place where we can decide
> which driver will be activated for the auto-configation of the
> Intel's HDA hardware with DSP. Actually, we cover three drivers:
> 
> * Legacy HDA
> * Intel SST
> * Intel Sound Open Firmware (SOF)
> 
> All those drivers registers similar PCI IDs, so the first
> driver probed from the PCI stack can win. But... it is not
> guaranteed that the correct driver wins.
> 
> This commit changes Intel's NHLT ACPI module to a common
> DSP probe module for the Intel's hardware. All above sound
> drivers calls this code. The user can force another behaviour
> using the module parameter 'dsp_driver' located in
> the 'snd-intel-dspcfg' module.
> 
> This change allows to add specific dmi checks for the specific
> systems. The examples are taken from the pull request:
> 
>   https://github.com/thesofproject/linux/pull/927
> 
> Tested on Lenovo Carbon X1 7th gen.
> 
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Cc: Cezary Rojewski <cezary.rojewski@intel.com>

Since I've been still traveling, just a few nitpicking:

> --- a/sound/hda/Makefile
> +++ b/sound/hda/Makefile
> @@ -14,5 +14,8 @@ 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
> +snd-intel-dspcfg-objs := intel-dsp-config.o
> +ifneq ($(CONFIG_ACPI),)
> +  snd-intel-dspcfg-objs += intel-nhlt.o
> +endif

This can be simplified by
  snd-intel-dspcfg-$(CONFIG_SND_INTEL_NHLT) += intel-nhlt.o

> --- /dev/null
> +++ b/sound/hda/intel-dsp-config.c
> @@ -0,0 +1,247 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Jaroslav Kysela <perex@perex.cz>
> +
> +#include <sound/core.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/dmi.h>
> +#include <sound/intel-nhlt.h>
> +#include <sound/intel-dsp-config.h>

Please put sound/core.h after linux/*.h inclusions.
Also in general the inclusions are arranged in the alphabetical order
nowadays.

> +int snd_intel_dsp_driver_probe(struct pci_dev *pci)
> +{
> +	const struct config_entry *cfg;
> +
> +	if (dsp_driver > 0 && dsp_driver <= SND_INTEL_DSP_DRIVER_LAST)
> +		return dsp_driver;
> +
> +	/* Intel vendor only */
> +	if (snd_BUG_ON(pci->vendor != 0x8086))
> +		return SND_INTEL_DSP_DRIVER_ANY;
> +
> +	/*
> +	 * detect DSP by checking class/subclass/prog-id information
> +	 * class=04 subclass 03 prog-if 00: no DSP, use legacy driver
> +	 * class=04 subclass 01 prog-if 00: DSP is present
> +	 *  (and may be required e.g. for DMIC or SSP support)
> +	 * class=04 subclass 03 prog-if 80: use DSP or legacy mode
> +	 */
> +	if (pci->class == 0x040300)
> +		return SND_INTEL_DSP_DRIVER_LEGACY;
> +	if (pci->class != 0x040100 && pci->class != 0x040380) {
> +		dev_err(&pci->dev, "Unknown PCI class/subclass/prog-if information (0x%06x) found, selecting HDA legacy driver\n", pci->class);
> +		return SND_INTEL_DSP_DRIVER_LEGACY;
> +	}

If we treat this as an error, we should provide a code to work around
this (e.g. quirk entry or such), so that the error can be avoided
later.  Or, just make it dev_info() for now so that it won't appear in
the splash screen.

> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
....
> @@ -124,7 +124,7 @@ static char *patch[SNDRV_CARDS];
>  static bool beep_mode[SNDRV_CARDS] = {[0 ... (SNDRV_CARDS-1)] =
>  					CONFIG_SND_HDA_INPUT_BEEP_MODE};
>  #endif
> -static bool dmic_detect = IS_ENABLED(CONFIG_SND_HDA_INTEL_DETECT_DMIC);
> +static bool no_dsp_driver;
>  
>  module_param_array(index, int, NULL, 0444);
>  MODULE_PARM_DESC(index, "Index value for Intel HD audio interface.");
> @@ -159,8 +159,8 @@ module_param_array(beep_mode, bool, NULL, 0444);
>  MODULE_PARM_DESC(beep_mode, "Select HDA Beep registration mode "
>  			    "(0=off, 1=on) (default=1).");
>  #endif
> -module_param(dmic_detect, bool, 0444);
> -MODULE_PARM_DESC(dmic_detect, "DMIC detect on SKL+ platforms");
> +module_param(no_dsp_driver, bool, 0444);
> +MODULE_PARM_DESC(no_dsp_driver, "Force this driver to be used and bypass the DSP driver selection");

A negative option is often confusing, e.g. you may pass like
no_dsp_driver=no.  Better to use a positive form if possible.

I'll be still off until the Thursday in the next week, so the further
reply may be delayed.

thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel