Dne 07. 10. 19 v 3:50 Takashi Iwai napsal(a): > 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@xxxxxxxx> >> Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx> >> Cc: Cezary Rojewski <cezary.rojewski@xxxxxxxxx> > > 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 Yes, I added the ACPI condition later and didn't return back to Makefile. Fixed in v4 (will post after comments from Pierre-Louis). >> --- /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@xxxxxxxx> >> + >> +#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. Uff. Rules and rules for rules. Will fix that in v4. >> +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. It' s really an error. This path should not be executed with the known, functional hardware. The user might use dsp_driver module parameter to change the behaviour. > >> --- 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 just tried to copy the dmic_detect value, but yes, it might be confusion. I will change that in v4. Jaroslav -- Jaroslav Kysela <perex@xxxxxxxx> Linux Sound Maintainer; ALSA Project; Red Hat, Inc. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
