Re: [PATCH] ASoC: Intel: Add Cherrytrail & Braswell machine driver cht_bsw_rt5672

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3299 bytes --]

On Fri, Oct 24, 2014 at 07:14:07PM +0800, mengdong.lin@intel.com wrote:
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> 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?

> +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?

> +	/* 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)

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

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

> +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?

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]