[alsa-devel] [PATCH v2] ASoC: Intel: Boards: Add CNL RT274 I2S machine driver

[Guneshwor Singh]

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.

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.

> >+
> >+	{"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.

>