[alsa-devel] [PATCH 2/2] ASoC: Intel: Add machine driver for CX2072X on BYT/CHT platforms

[Takashi Iwai]

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