[alsa-devel] [PATCH 2/2] ALSA: HD-Audio: SKL+: force HDaudio legacy or SKL+ driver selection

[Takashi Iwai]

On Sat, 08 Dec 2018 01:00:39 +0100,
Pierre-Louis Bossart wrote:
> 
> For HDaudio and Skylake drivers, add module parameter "pci_binding"
> 
> When pci_binding == 0 (AUTO), the PCI class/subclass info is used to
> select drivers based on the presence of the DSP.
> 
> pci_binding == 1 (LEGACY) forces the use of the HDAudio legacy driver,
> even if the DSP is present.
> 
> pci_binding == 2 (ASOC) forces the use of the ASOC driver. The
> information on the DSP presence is bypassed.
> 
> The value for the module parameter needs to be identical for both
> drivers. This parameter is intended as a back-up solution if the
> automatic detection fails or when the DSP usage fails. Such cases
> should be reported on the alsa-devel mailing list for analysis.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart at linux.intel.com>

In theory, we can put this module option into snd-hda-core that is
used by both legacy and ASoC drivers, too, if we want to have a single
common option.  But it's not always intuitive, so I'm fine to keep the
option to both drivers.

> index 0c76713ac5c0..4e1aee5167a1 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -172,6 +172,9 @@ 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
> +static int skl_pci_binding;
> +module_param_named(pci_binding, skl_pci_binding, int, 0444);
> +MODULE_PARM_DESC(legacy, "PCI binding (0=auto, 1=only legacy, 2=only asoc");

The parameter is "pci_binding", not "legacy", right?
The same error is seen in asoc skl.


>  #ifdef CONFIG_PM
>  static int param_set_xint(const char *val, const struct kernel_param *kp);
> @@ -2093,9 +2096,25 @@ static int azx_probe(struct pci_dev *pci,
>  	int err;
>  
>  	/* check if this driver can be used on SKL+ Intel platforms */
> -	if ((pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) &&
> -	    pci->class != 0x040300)
> -		return -ENODEV;
> +	if (pci_id->driver_data & AZX_DCAPS_INTEL_SHARED) {
> +		switch (skl_pci_binding) {
> +		case SND_SKL_PCI_BIND_AUTO:
> +			if (pci->class != 0x040300) {
> +				dev_warn(&pci->dev, "The DSP is enabled on this platform, aborting probe\n");

I'd use dev_info() for these messages.
Loading this module is done by udev, not user, so it's no user's fault.

The reason I care about the printk level is that this message would
appear on the boot screen even if you set "quiet" kernel option
(although the quiet level became adjustable recently).

> +				return -ENODEV;
> +			}
> +			break;
> +		case SND_SKL_PCI_BIND_LEGACY:
> +			dev_warn(&pci->dev, "Module parameter forced binding with HDaudio legacy, bypassed detection logic\n");

Ditto.  This is information-only.

> +			break;
> +		case SND_SKL_PCI_BIND_ASOC:
> +			dev_err(&pci->dev, "Module parameter forced binding with SKL+ ASoC driver, aborting probe\n");
> +			return -ENODEV;

Ditto.

> +		default:
> +			dev_err(&pci->dev, "invalid value for skl_pci_binding module parameter, ignored\n");
> +			break;

This should be kept as error.

The similar logic applied to asoc driver.

In general, the dev_err() and dev_warn() should be used for the things
the user really has to care explicitly.


thanks,

Takashi