[alsa-devel] [PATCH v2] ASoC: Intel: kbl_rt5660: Add a new machine driver for kbl with rt5660

[Pierre-Louis Bossart]

Hi Hui,

Looks mostly good, couple of comments below. I didn't look at the GPIO 
parts since I am not familiar with the framework.

Thanks

-Pierre


> +++ b/sound/soc/intel/boards/kbl_rt5660.c
> @@ -0,0 +1,536 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright(c) 2018-19 Intel Corporation.

Not sure this is correct since you did the work?

> +static const struct snd_kcontrol_new kabylake_rt5660_controls[] = {
> +	SOC_DAPM_PIN_SWITCH("Line In"),
> +	SOC_DAPM_PIN_SWITCH("Line Out"),
> +};
> +
> +static const struct snd_soc_dapm_widget kabylake_rt5660_widgets[] = {
> +	SND_SOC_DAPM_MIC("Line In", NULL),
> +	SND_SOC_DAPM_LINE("Line Out", kabylake_5660_event_lineout),
> +	SND_SOC_DAPM_SPK("DP", NULL),
> +	SND_SOC_DAPM_SPK("HDMI", NULL),

I see in other machine drivers that when this DP stuff is used there is 
also a matching route which you don't have.

kbl_da7219_max98357a.c:94:    SND_SOC_DAPM_SPK("DP", NULL),
kbl_da7219_max98357a.c:113:    { "DP", NULL, "hif6 Output" },
kbl_da7219_max98927.c:109:    SND_SOC_DAPM_SPK("DP", NULL),
kbl_da7219_max98927.c:125:    { "DP", NULL, "hif6 Output" },
kbl_rt5663_max98927.c:205:    SND_SOC_DAPM_SPK("DP", NULL),
kbl_rt5663_max98927.c:223:    { "DP", NULL, "hif6 Output" },

I never noticed this in Intel machine drivers, yet another weird thing 
to reverse engineer...I have no idea why it was modeled this way, the 
interface at the DSP level does not know or need to care what the actual 
link is, it first goes over an iDISP/HDAudio link.

Naveen, any idea on this?

> +};
> +
> +static const struct snd_soc_dapm_route kabylake_rt5660_map[] = {
> +	/* other jacks */
> +	{"IN1P", NULL, "Line In"},
> +	{"IN2P", NULL, "Line In"},
> +	{"Line Out", NULL, "LOUTR"},
> +	{"Line Out", NULL, "LOUTL"},
> +
> +	/* CODEC BE connections */
> +	{ "AIF1 Playback", NULL, "ssp0 Tx"},
> +	{ "ssp0 Tx", NULL, "codec0_out"},
> +
> +	{ "codec0_in", NULL, "ssp0 Rx" },
> +	{ "ssp0 Rx", NULL, "AIF1 Capture" },
> +
> +	{ "hifi2", NULL, "iDisp2 Tx"},
> +	{ "iDisp2 Tx", NULL, "iDisp2_out"},
> +	{ "hifi1", NULL, "iDisp1 Tx"},
> +	{ "iDisp1 Tx", NULL, "iDisp1_out"},
aren't you missing the routes for the HDMI3 which are listed in the rest 
of the patch?
> +
> +static int kabylake_rt5660_codec_init(struct snd_soc_pcm_runtime *rtd)
> +{
> +	int ret;
> +	struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card);
> +	struct snd_soc_component *component = rtd->codec_dai->component;
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> +
> +	ret = devm_acpi_dev_add_driver_gpios(component->dev, acpi_rt5660_gpios);
> +	if (ret)
> +		dev_warn(component->dev, "Failed to add driver gpios\n");
> +
> +	/* Request rt5660 GPIO for lineout mute control */
> +	ctx->gpio_lo_mute = devm_gpiod_get(component->dev, "lineout-mute",
> +			GPIOD_OUT_HIGH);
> +	if (IS_ERR(ctx->gpio_lo_mute)) {
> +		dev_err(component->dev, "Can't find GPIO_MUTE# gpio\n");
> +		return PTR_ERR(ctx->gpio_lo_mute);
> +	}
Here you exit with an error, but see [1] below
> +
> +	/* Create and initialize headphone jack */
> +	if (!snd_soc_card_jack_new(rtd->card, "Lineout Jack",
> +			SND_JACK_LINEOUT, &lineout_jack,
> +			&lineout_jack_pin, 1)) {
> +		lineout_jack_gpio.gpiod_dev = component->dev;
> +		if (snd_soc_jack_add_gpios(&lineout_jack, 1,
> +				&lineout_jack_gpio))
> +			dev_err(component->dev, "Can't add Lineout jack gpio\n");
> +	} else
> +		dev_err(component->dev, "Can't create Lineout jack\n");

the tests for snd_soc_jack_new are odd. It's more usual to see

ret = snd_soc_card_jack_new();

if (ret){

     dev_err(blah)

     return ret;

}

I guess you took as example bdw-rt5677 but it's the only one following 
the pattern you used.

> +
> +	/* Create and initialize mic jack */
> +	if (!snd_soc_card_jack_new(rtd->card, "Mic Jack",
> +			SND_JACK_MICROPHONE, &mic_jack,
> +			&mic_jack_pin, 1)) {
> +		mic_jack_gpio.gpiod_dev = component->dev;
> +		if (snd_soc_jack_add_gpios(&mic_jack, 1, &mic_jack_gpio))
> +			dev_err(component->dev, "Can't add mic jack gpio\n");
> +	} else
> +		dev_err(component->dev, "Can't create mic jack\n");

[1] ... for both of the jacks you don't exit. Is this intentional?

> +
> +	snd_soc_dapm_force_enable_pin(dapm, "MICBIAS1");
> +	snd_soc_dapm_force_enable_pin(dapm, "BST1");
> +	snd_soc_dapm_force_enable_pin(dapm, "BST2");

Are those initializations required?

> +
> +	return 0;
> +}
>
> +static int kbl_fe_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_pcm_runtime *runtime = substream->runtime;
> +
> +	/*
> +	 * On this platform for PCM device we support,
> +	 * 48Khz
> +	 * stereo
> +	 * 16 bit audio
> +	 */
> +
> +	runtime->hw.channels_max = DUAL_CHANNEL;
> +	snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS,
> +					   &constraints_channels);
> +
> +	runtime->hw.formats = SNDRV_PCM_FMTBIT_S16_LE;
> +	snd_pcm_hw_constraint_msbits(runtime, 0, 16, 16);
that part is odd, the SSP is configured to work with 24 bits but somehow 
it's constrained here to 16 bits. it must be copy/paste from other 
machine drivers, but not sure if this is required here.
> --- a/sound/soc/intel/common/soc-acpi-intel-kbl-match.c
> +++ b/sound/soc/intel/common/soc-acpi-intel-kbl-match.c
> @@ -96,6 +96,11 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_kbl_machines[] = {
>   		.quirk_data = &kbl_7219_98927_codecs,
>   		.pdata = &skl_dmic_data
>   	},
> +	{
> +		.id = "10EC3277",
> +		.drv_name = "kbl_rt5660",
> +		.fw_filename = "intel/dsp_fw_kbl.bin",
can we also add an entry for 10EC5660 since it's already a known ACPI ID?