On 2018/12/11 下午10:30, Pierre-Louis Bossart wrote:
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.
For GPIO part, I mainly referred to the bdw-rt5677.c, and so far this part works perfectly on the Dell IoT machine.
Thank you for reviewing my patch, please see my reply below.
Thanks.
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?
will change to the Canonical Corporation.
+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" },
Originally I copied the matching route like { "DP", NULL, "hif5/6 Output" }, but the kernel will output a warning that there is no dapm named "hif5" or "hif6", So I dropped it.
And looks like I should drop SND_SOC_DAPM_SPK("DP", NULL) and SND_SOC_DAPM_SPK("HDMI", NULL) too, since it is useless at all for this machine.
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?
Ok, will add the HDMI3 in the next version.
+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.
Will change this part as suggested.
+ /* 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?
Yes, it is intentional.
The gpio_lo_mute is mandatory, otherwise this driver can't play any sound via lineout jack since the jack is muted in hardware level. So if we can't get this gpio, the driver will return an error.
But the Jack and the GPIO of Jack detection are not mandatory, even jack or jack detection is failed to register, we can still use aplay/arecord to work. And the version 0x00 of the Dell IoT machine doesn't have jack detection GPIOs, to support both the version 0x00 and 0x01 boards, we don't let it return here intentionally.
+ 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?
Yes, they are required. Without enabling these dapms in advance, there will be 1~2 seconds noise at the beginning of the recorded sound.
I will add a comment here in the next version.
+ 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.
OK, will set to be 24bits here and do a test on the machine.
--- 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?
OK, will add it.
Alsa-devel mailing list Alsa-devel@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel