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 > --- /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. > +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@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
