Re: [PATCH v2] ASoC: Intel: Boards: Add CNL RT274 I2S machine driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 3/15/18 10:40 PM, Guneshwor Singh wrote:
Hi Pierre,

On Thu, Mar 15, 2018 at 07:23:05AM -0500, Pierre-Louis Bossart wrote:

+
+static int cnl_rt274_clock_control(struct snd_soc_dapm_widget *w,
+				   struct snd_kcontrol *k, int event)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct snd_soc_card *card = dapm->card;
+	struct snd_soc_dai *codec_dai =
+		snd_soc_card_get_codec_dai(card, RT274_CODEC_DAI);
+	int ret, ratio = 100;
+
+	if (!codec_dai)
+		return -EINVAL;
+
+	/* Codec needs clock for Jack detection and button press */
+	ret = snd_soc_dai_set_sysclk(codec_dai, RT274_SCLK_S_PLL2,
+				     CNL_FREQ_OUT, SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "set codec sysclk failed: %d\n", ret);
+		return ret;
+	}
+
+	if (SND_SOC_DAPM_EVENT_ON(event)) {
+		ret = snd_soc_dai_set_bclk_ratio(codec_dai, ratio);
+		if (ret) {
+			dev_err(codec_dai->dev,
+				"set bclk ratio failed: %d\n", ret);
+			return ret;
+		}
+
+		ret = snd_soc_dai_set_pll(codec_dai, 0, RT274_PLL2_S_BCLK,
+					  CNL_BE_FIXUP_RATE * ratio,
+					  CNL_FREQ_OUT);
+		if (ret) {
+			dev_err(codec_dai->dev,
+				"enable PLL2 failed: %d\n", ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
it's not clear to me why you need a clock control? You are not changing
anything that really depends on DAPM events, to e.g. take the MCLK down and
use a local clock, so could this be moved to hw_params?

Yes, we can do in hw_params too. When we implemented it we thought
during simultaneous playback and capture, hw_params will be called twice
but clock clock event will be called just once.

Yes, I remember this discussion but what's missing here is that PLL is always set to the max value, in most cases we fall back to a local clock to save power a bit. And it's probably wrong to leave a PLL on if the clock reference is turned off on the SOC side.


It does not harm to call twice as done in other machine drivers. Do you
suggest to move it to hw_params?

+static const struct snd_soc_dapm_route cnl_map[] = {
+	{"Headphone Jack", NULL, "HPO Pin"},
+	{"MIC", NULL, "Mic Jack"},
+	{"DMic", NULL, "SoC DMIC"},
+	{"DMIC01 Rx", NULL, "Capture"},
+	{"dmic01_hifi", NULL, "DMIC01 Rx"},
+
+	{"AIF1 Playback", NULL, "ssp0 Tx"},
+	{"ssp0 Tx", NULL, "codec1_out"},
+	{"ssp0 Tx", NULL, "codec0_out"},
I get the routes to connect firmware widgets to codec ones, but why do we
need SSP0 TX-> codec1_out? shouldn't this be part of the topology?

We still have routes for BEs defined here. Only FE ones come from
topology.

The comment was about the codec1_out->SSP0 TX. Why is this hard-coded?
You would only need SSP0 TX->AIF1 playback


+
+	{"ssp0 Rx", NULL, "AIF1 Capture"},
+	{"codec0_in", NULL, "ssp0 Rx"},
+
+	{"Headphone Jack", NULL, "Platform Clock"},
+	{"Mic Jack", NULL, "Platform Clock"},
+};
+
+static	struct snd_soc_jack_pin cnl_headset_pins[] = {
+	{
+		.pin = "Mic Jack",
+		.mask = SND_JACK_MICROPHONE,
+	},
+	{
+		.pin = "Headphone Jack",
+		.mask = SND_JACK_HEADPHONE,
+	},
+};
+
+static struct snd_soc_jack cnl_headset;
+
+static int cnl_rt274_init(struct snd_soc_pcm_runtime *runtime)
+{
+	struct snd_soc_card *card = runtime->card;
+	struct snd_soc_dai *codec_dai = runtime->codec_dai;
+	struct snd_soc_component *component = codec_dai->component;
+	int ret;
+
+	ret = snd_soc_card_jack_new(runtime->card, "Headset",
+		SND_JACK_HEADSET, &cnl_headset,
+		cnl_headset_pins, ARRAY_SIZE(cnl_headset_pins));
+	if (ret)
+		return ret;
+
+	ret = snd_soc_component_set_jack(component, &cnl_headset, NULL);
+	if (ret)
+		return ret;
+
+	/* TDM 4 slots 24 bit, set Rx & Tx bitmask to 4 active slots */
+	ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xf, 0xf, 4, 24);
what are the 4 slots used for?

Ah, this is a mistake. Thanks for pointing out. It should be
snd_soc_dai_set_tdm_slot(codec_dai, 0x3, 0x3, 2, 24) as we do not have
speakers on rt274. Will correct in v3.

then it also begs the question why you needed to have both codec0_out and codec1_out mentioned above - same issue really about hard-coding things that should be topology defined.



_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel


_______________________________________________
Alsa-devel mailing list
Alsa-devel@xxxxxxxxxxxxxxxx
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel



[Index of Archives]     [ALSA User]     [Linux Audio Users]     [Kernel Archive]     [Asterisk PBX]     [Photo Sharing]     [Linux Sound]     [Video 4 Linux]     [Gimp]     [Yosemite News]

  Powered by Linux