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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel