Alsa-Devel Archive on lore.kernel.org
 help / color / Atom feed
From: Takashi Iwai <tiwai@suse.de>
To: Jaroslav Kysela <perex@perex.cz>
Cc: Cezary Rojewski <cezary.rojewski@intel.com>,
	ALSA development <alsa-devel@alsa-project.org>,
	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Subject: Re: [alsa-devel] [PATCH v3] ALSA: hda: add Intel DSP configuration / probe code
Date: Mon, 07 Oct 2019 03:50:22 +0200
Message-ID: <s5h5zl1jpcx.wl-tiwai@suse.de> (raw)
In-Reply-To: <20191006152232.17701-1-perex@perex.cz>

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

  reply index

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-06 15:22 Jaroslav Kysela
2019-10-07  1:50 ` Takashi Iwai [this message]
2019-10-07 16:13   ` Jaroslav Kysela
2019-10-07 14:05 ` Pierre-Louis Bossart
2019-10-07 16:03   ` Jaroslav Kysela
2019-10-07 16:44     ` Pierre-Louis Bossart
2019-10-08 18:44     ` Pierre-Louis Bossart
2019-10-09  6:26       ` Jaroslav Kysela

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=s5h5zl1jpcx.wl-tiwai@suse.de \
    --to=tiwai@suse.de \
    --cc=alsa-devel@alsa-project.org \
    --cc=cezary.rojewski@intel.com \
    --cc=perex@perex.cz \
    --cc=pierre-louis.bossart@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Alsa-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/alsa-devel/0 alsa-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 alsa-devel alsa-devel/ https://lore.kernel.org/alsa-devel \
		alsa-devel@alsa-project.org
	public-inbox-index alsa-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.alsa-project.alsa-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git