From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 2/2] ASoC: Intel: Add machine driver for CX2072X on BYT/CHT platforms Date: Tue, 23 Apr 2019 21:39:50 +0200 Message-ID: References: <20190423141336.12568-1-tiwai@suse.de> <20190423141336.12568-3-tiwai@suse.de> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.suse.de (mx2.suse.de [195.135.220.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by alsa1.perex.cz (Postfix) with ESMTPS id E0032F80730 for ; Tue, 23 Apr 2019 21:39:51 +0200 (CEST) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: "Alsa-devel" To: Pierre-Louis Bossart Cc: alsa-devel@alsa-project.org, Mark Brown List-Id: alsa-devel@alsa-project.org On Tue, 23 Apr 2019 21:20:24 +0200, Pierre-Louis Bossart wrote: > > On 4/23/19 9:13 AM, Takashi Iwai wrote: > > This is an implementation of a machine driver needed for Conexant > > CX2072X codec on Intel Baytrail and Cherrytrail platforms. The > > current patch is based on the initial work by Pierre-Louis Bossart and > > the other Intel machine drivers. > > > > The jack detection support (driven via the standard GPIO) was added on > > top of the original work. > > > > No SOF support yet, so the corresponding entries are commented out. > > SOF support is trivial to add, can we help you here? I just had no time to take a deeper look at that direction, so it's a thing I'd like to play at my next spare time. > > +config SND_SOC_INTEL_BYT_CHT_CX2072X_MACH > > + tristate "Baytrail & Cherrytrail with CX2072X codec" > > + depends on X86_INTEL_LPSS && I2C && ACPI > > Didn't Mark recently split this in two, with X86_INTEL_LPSS || COMPILE_TEST? The entry I just copied, CONFIG_SND_SOC_INTEL_BYT_CHT_ES8316_MACH, still has this style, so no, it doesn't seem applied. > > +static int byt_cht_cx2072x_fixup(struct snd_soc_pcm_runtime *rtd, > > + struct snd_pcm_hw_params *params) > > +{ > > + struct snd_interval *rate = > > + hw_param_interval(params, SNDRV_PCM_HW_PARAM_RATE); > > + struct snd_interval *channels = > > + hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS); > > + int ret; > > + > > + /* The DSP will covert the FE rate to 48k, stereo, 24bits */ > > + rate->min = rate->max = 48000; > > + channels->min = channels->max = 2; > > + > > + /* set SSP2 to 24-bit */ > > + params_set_format(params, SNDRV_PCM_FORMAT_S24_LE); > > + > > + /* > > + * Default mode for SSP configuration is TDM 4 slot, override config > > + * with explicit setting to I2S 2ch 24-bit. The word length is set with > > + * dai_set_tdm_slot() since there is no other API exposed > > + */ > > + ret = snd_soc_dai_set_fmt(rtd->cpu_dai, > > + SND_SOC_DAIFMT_I2S | > > + SND_SOC_DAIFMT_NB_NF | > > + SND_SOC_DAIFMT_CBS_CFS); > > + if (ret < 0) { > > + dev_err(rtd->dev, "can't set format to I2S, err %d\n", ret); > > + return ret; > > + } > > + > > + ret = snd_soc_dai_set_tdm_slot(rtd->cpu_dai, 0x3, 0x3, 2, 24); > > + if (ret < 0) { > > + dev_err(rtd->dev, "can't set I2S config, err %d\n", ret); > > + return ret; > > + } > > + > > + snd_soc_dai_set_bclk_ratio(rtd->codec_dai, 50); > > that part would be problematic for SOF. IIRC we put all clock-related > stuff in the init, and ignore the fixups to use topology-based > information instead. If this call to _set_bclk_ratio can be moved to > the init it's more future-proof. Is there a reason to do this here in > the fixup? No particular reason from my side. I just took over your code ;) That is, it'd be appreciated if you can give a fixup patch that can be applied on top. > > +static struct snd_soc_dai_link byt_cht_cx2072x_dais[] = { > > + [MERR_DPCM_AUDIO] = { > > + .name = "Audio Port", > > + .stream_name = "Audio", > > + .cpu_dai_name = "media-cpu-dai", > > + .codec_dai_name = "snd-soc-dummy-dai", > > + .codec_name = "snd-soc-dummy", > > + .platform_name = "sst-mfld-platform", > > + .nonatomic = true, > > + .dynamic = 1, > > + .dpcm_playback = 1, > > + .dpcm_capture = 1, > > + .ops = &byt_cht_cx2072x_aif1_ops, > > + }, > > + [MERR_DPCM_DEEP_BUFFER] = { > > + .name = "Deep-Buffer Audio Port", > > + .stream_name = "Deep-Buffer Audio", > > + .cpu_dai_name = "deepbuffer-cpu-dai", > > + .codec_dai_name = "snd-soc-dummy-dai", > > + .codec_name = "snd-soc-dummy", > > + .platform_name = "sst-mfld-platform", > > + .nonatomic = true, > > + .dynamic = 1, > > + .dpcm_playback = 1, > > + .ops = &byt_cht_cx2072x_aif1_ops, > > + }, > > + /* back ends */ > > + { > > + .name = "SSP2-Codec", > > + .id = 1, > > + .cpu_dai_name = "ssp2-port", > > + .platform_name = "sst-mfld-platform", > > + .no_pcm = 1, > > + .codec_dai_name = "cx2072x-hifi", > > + .codec_name = "i2c-14F10720:00", > > + .dai_fmt = SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF > > + | SND_SOC_DAIFMT_CBS_CFS, > > + .init = byt_cht_cx2072x_init, > > + .be_hw_params_fixup = byt_cht_cx2072x_fixup, > > + .nonatomic = true, > > I can't recall if this .atomic is needed or not for back-ends. If FE requires it, BE must set it, too, in general. > > + { > > + .id = "14F10720", > > + .drv_name = "bytcht_cx2072x", > > + .fw_filename = "intel/fw_sst_0f28.bin", > > + .board = "bytcht_cx2072x", > > + /* .sof_fw_filename = "sof-byt.ri", */ > > + /* .sof_tplg_filename = "sof-byt-cx2072x.tplg", */ > > + }, > > #if IS_ENABLED(CONFIG_SND_SOC_INTEL_BYT_CHT_NOCODEC_MACH) > > /* > > * This is always last in the table so that it is selected only when > > diff --git a/sound/soc/intel/common/soc-acpi-intel-cht-match.c b/sound/soc/intel/common/soc-acpi-intel-cht-match.c > > index deafd87cc764..69ecbb88c171 100644 > > --- a/sound/soc/intel/common/soc-acpi-intel-cht-match.c > > +++ b/sound/soc/intel/common/soc-acpi-intel-cht-match.c > > @@ -169,6 +169,14 @@ struct snd_soc_acpi_mach snd_soc_acpi_intel_cherrytrail_machines[] = { > > .sof_fw_filename = "sof-cht.ri", > > .sof_tplg_filename = "sof-cht-rt5651.tplg", > > }, > > + { > > + .id = "14F10720", > > + .drv_name = "bytcht_cx2072x", > > + .fw_filename = "intel/fw_sst_22a8.bin", > > + .board = "bytcht_cx2072x", > > + /* .sof_fw_filename = "sof-cht.ri", */ > > + /* .sof_tplg_filename = "sof-cht-cx2072x.tplg", */ > > + }, > > I'd uncomment those two ACPI machine parts. There is not risk unless > SOF is actually enabled. OK, will do that. Thanks for a quick review! Takashi