[alsa-devel] [PATCH] ASoC: Intel: Add Cherrytrail & Braswell machine driver cht_bsw_rt5672

[Lin, Mengdong]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: Saturday, October 25, 2014 12:24 AM
 
> > Add machine driver for two Intel Cherryview-based platforms,
> > Cherrytrail and Braswell, with RT5672 codec.
> 
> This doesn't seem to have any ACPI stuff - how does the driver get loaded?

The ACPI stuff will be upstream later.
The machine device will be created by the SST ACPI loader sst-acpi.c, like Broadwell and Baytrail.
We're trying to merge ACPI stuff of new machines to the ACPI loader.

> > +static int cht_aif1_hw_params(struct snd_pcm_substream *substream,
> > +					struct snd_pcm_hw_params *params) {
> > +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> > +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> > +	int ret;
> > +	unsigned int fmt;
> > +
> > +	if (strncmp(codec_dai->name, "rt5670-aif1", 11))
> > +		return 0;
> > +
> > +	/* 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,
> > +				       SNDRV_PCM_FORMAT_GSM);
> > +	if (ret < 0) {
> > +		dev_err(rtd->dev, "can't set codec TDM slot %d\n", ret);
> > +		return ret;
> > +	}
> > +
> 
> This doesn't depend on the parameters so should be set on init and your use of
> SNDRV_PCM_FORMAT_GSM doesn't seem to make much sense here - what's
> going on?

Sorry, this is a misuse of SNDRV_PCM_FORMAT_GSM which happen to be 24
I'll correct this to snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0xF, 4, 24) , create a dai_link init function cht_codec_init() and move the code there.

> > +	/* TDM slave Mode */
> > +	fmt =   SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF
> > +		| SND_SOC_DAIFMT_CBS_CFS;
> > +	ret = snd_soc_dai_set_fmt(codec_dai, fmt);
> > +	if (ret < 0) {
> > +		dev_err(rtd->dev, "can't set codec DAI fmt %d\n", ret);
> > +		return ret;
> > +	}
> 
> Likewise, set on init (in the dai_link)

I'll move it to back end dai link .dai_fmt as below:
.dai_fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF
					| SND_SOC_DAIFMT_CBS_CFS,

> > +static int cht_set_bias_level(struct snd_soc_card *card,
> > +				struct snd_soc_dapm_context *dapm,
> > +				enum snd_soc_bias_level level)
> > +{
> > +	switch (level) {
> > +	case SND_SOC_BIAS_ON:
> > +	case SND_SOC_BIAS_PREPARE:
> > +	case SND_SOC_BIAS_STANDBY:
> > +	case SND_SOC_BIAS_OFF:
> > +		break;
> > +	default:
> > +		dev_err(card->dev, "Invalid bias level=%d\n", level);
> > +		return -EINVAL;
> > +	}
> > +	card->dapm.bias_level = level;
> > +	return 0;
> > +}
> 
> This does nothing and can be removed.

Okay.

> > +static int cht_init(struct snd_soc_pcm_runtime *runtime) {
> > +	int ret;
> > +	struct snd_soc_card *card = runtime->card;
> > +
> > +	/* Set card bias level */
> > +	cht_set_bias_level(card, &card->dapm, SND_SOC_BIAS_OFF);
> > +	card->dapm.idle_bias_off = true;
> > +
> > +	ret = snd_soc_dapm_sync(&card->dapm);
> > +	if (ret) {
> > +		dev_err(card->dev, "unable to sync dapm\n");
> > +		return ret;
> > +	}
> > +	return ret;
> > +}
> 
> This also does nothing and can be removed.

Okay.

> > +static struct snd_pcm_hw_constraint_list constraints_48000 = {
> > +	.count = ARRAY_SIZE(rates_48000),
> > +	.list  = rates_48000,
> > +};
> > +
> > +static int cht_aif1_startup(struct snd_pcm_substream *substream) {
> > +	return snd_pcm_hw_constraint_list(substream->runtime, 0,
> > +			SNDRV_PCM_HW_PARAM_RATE,
> > +			&constraints_48000);
> > +}
> 
> If the rates are restricted to 48kHz then is there any need for rate dependent
> code in hw_params() at all?

Currently, we only tested 48kHz, but later will support 8KHz and 16KHz for VoIP application.
So hw_params() be reserved for reuse later?

> 
> > +static int snd_cht_mc_remove(struct platform_device *pdev) {
> > +	struct snd_soc_card *soc_card = platform_get_drvdata(pdev);
> > +
> > +	snd_soc_card_set_drvdata(soc_card, NULL);
> > +	snd_soc_unregister_card(soc_card);
> > +	platform_set_drvdata(pdev, NULL);
> > +	return 0;
> > +}
> 
> No need to set the driver data to NULL, the driver core will do it anyway and
> it's buggy to look at the driver data for unbound devices.

Okay.

Thanks for your review! I'll submit the v2 patch based on your comment. 
But I lost the message ID of my original patch, sorry for this.

Regards
Mengdong