Re: [alsa-devel] [PATCH v2] ASoC: imx-wm8958: add imx-wm8958 machine driver

On Fri, Feb 26, 2016 at 11:42:42AM +0800, Zidan Wang wrote:

This is the initial imx-wm8958 device-tree-only machine driver working with fsl_sai driver. This sound card has three dai link, HIFI, VOICE and BT dai. HIFI dai link will support codec master and slave mode. VOICE an BT dai link have dummy cpu dai, and just support codec master mode.

Why VOICE and BT are using dummy cpu dai? Where can I find the schematics for wm8958 from NXP website?

diff --git a/Documentation/devicetree/bindings/sound/imx-audio-wm8958.txt +Example:

+sound {

• compatible = "fsl,imx6ul-evk-wm8958",
  

• 	   "fsl,imx-audio-wm8958";
  

• model = "wm8960-audio";
  

8960?

diff --git a/arch/arm/configs/imx_v6_v7_defconfig +config SND_SOC_IMX_WM8958

• tristate "SoC Audio support for i.MX boards with wm8958"
• depends on OF && I2C
• select MFD_WM8994
• select SND_SOC_WM8994
• select SND_SOC_IMX_PCM_DMA
• select SND_SOC_FSL_SAI
• select SND_SOC_FSL_UTILS
• select SND_KCTL_JACK

The driver doesn't seem to use kcontrol....

+static u32 imx_wm8958_rates[] = { 8000, 16000, 32000, 48000 };

+static struct snd_pcm_hw_constraint_list imx_wm8958_rate_constraints = {

• .count = ARRAY_SIZE(imx_wm8958_rates),
• .list = imx_wm8958_rates,

+};

Where are these rate constraints coming from?

+static int imx_wm8958_hw_params(struct snd_pcm_substream *substream,

• 		struct snd_pcm_hw_params *params)
  

+{

• if (data->mclk_freq[id])

• mclk_id = WM8994_FLL_SRC_MCLK(id);
  

• else if (id == HIFI_DAI)

• mclk_id = WM8994_FLL_SRC_MCLK2;
  

• else

• mclk_id = WM8994_FLL_SRC_MCLK1;
  

I can barely follow the MCLK assigning logic here...

Is it trying to use the alternative MCLK if the corresponding one is unavailable? (Since id could be only 0 or 1). And what if both are unavailable?

• if (id == HIFI_DAI) {

• /*
  

•  * Set GPIO1 pin function to reserve, so that DAC1 and ADC1
  

•  * using shared LRCLK from DACLRCK1.
  

•  */
  

• snd_soc_update_bits(codec, WM8994_GPIO_1, 0x1f, 0x2);
  

I don't see any reverse operation against this GPIO configuration here. So is it really an operation that has to be done every hw_params()?

• } else if (id == VOICE_DAI) {

• /*
  

•  * Set GPIO6 pin function to reserve, so that DAC2 and ADC2
  

•  * using shared LRCLK from DACLRCK2.
  

•  */
  

• snd_soc_update_bits(codec, WM8994_GPIO_6, 0x1f, 0x2);
  

Same here.

+static int imx_wm8958_set_bias_level(struct snd_soc_card *card,

• 		struct snd_soc_dapm_context *dapm,
  

• 		enum snd_soc_bias_level level)
  

+{

• struct snd_soc_pcm_runtime *rtd;
• struct snd_soc_dai *codec_dai;
• struct imx_wm8958_data *data = snd_soc_card_get_drvdata(card);
• int ret, i;
• rtd = snd_soc_get_pcm_runtime(card, card->dai_link[0].name);
• codec_dai = rtd->codec_dai;
• if (dapm->dev != codec_dai->dev)

• return 0;
  

• switch (level) {
• case SND_SOC_BIAS_STANDBY:

• if (card->dapm.bias_level == SND_SOC_BIAS_OFF) {
  

Using if (card->dapm.bias_level != SND_SOC_BIAS_OFF) break;

may save one \t for each line below.

• 	/* need to enable mclk to write/read wm8958 register */
  

• 	for (i = 0; i < WM8958_MCLK_MAX; i++) {
  

• 		if (!IS_ERR(data->mclk[i])) {
  

• 			ret = clk_prepare_enable(data->mclk[i]);
  

• 			if (ret)
  

• 				dev_warn(card->dev,
  

• 					 "Failed to enable MCLK%d: %d\n",
  

• 					 i + 1, ret);
  

• 		}
  

• 	}
  

• }
  

• break;
  

• default:

• break;
  

• }
• return 0;

+}

+static int imx_wm8958_set_bias_level_post(struct snd_soc_card *card,

• 		     struct snd_soc_dapm_context *dapm,
  

• 		     enum snd_soc_bias_level level)
  

+{

• struct snd_soc_pcm_runtime *rtd;
• struct snd_soc_dai *codec_dai;
• struct imx_wm8958_data *data = snd_soc_card_get_drvdata(card);
• int i;
• rtd = snd_soc_get_pcm_runtime(card, card->dai_link[0].name);
• codec_dai = rtd->codec_dai;
• if (dapm->dev != codec_dai->dev)

• return 0;
  

• switch (level) {
• case SND_SOC_BIAS_OFF:

• if (card->dapm.bias_level == SND_SOC_BIAS_STANDBY)
  

• 	for (i = 0; i < WM8958_MCLK_MAX; i++) {
  

• 		if (!IS_ERR(data->mclk[i]))
  

• 			clk_disable_unprepare(data->mclk[i]);
  

The set_bias() funcs are merely en/disabling mclk. Would be possible to do it in the codec driver?

+static struct snd_soc_dai_link imx_wm8958_dai_link[] = {

• [HIFI_DAI] = {

• 	.name = "HiFi",
  

• 	.stream_name = "HiFi",
  

• 	.codec_name = "wm8994-codec",
  

• 	.codec_dai_name = "wm8994-aif1",
  

• 	.ops = &imx_hifi_ops,
  

• 	.dai_fmt = SND_SOC_DAIFMT_I2S |
  

• 		   SND_SOC_DAIFMT_NB_NF,
  

• },
• [VOICE_DAI] = {

• 	.name = "Voice",
  

• 	.stream_name = "Voice",
  

• 	.cpu_dai_name = "snd-soc-dummy-dai",
  

• 	.codec_name = "wm8994-codec",
  

• 	.codec_dai_name = "wm8994-aif2",
  

• 	.platform_name = "snd-soc-dummy",
  

• 	.ignore_pmdown_time = 1,
  

• 	.ops = &imx_voice_ops,
  

• 	.dai_fmt = SND_SOC_DAIFMT_I2S |
  

• 		   SND_SOC_DAIFMT_NB_NF |
  

• 		   SND_SOC_DAIFMT_CBM_CFM,
  

• },
• [BT_DAI] = {

• 	.name = "Bluetooth",
  

• 	.stream_name = "Bluetooth",
  

• 	.cpu_dai_name = "snd-soc-dummy-dai",
  

• 	.codec_name = "wm8994-codec",
  

• 	.codec_dai_name = "wm8994-aif3",
  

• 	.platform_name = "snd-soc-dummy",
  

• 	.ignore_pmdown_time = 1,
  

• },

A little curious...how is the BT gonna work for you?

+};

+static int imx_wm8958_probe(struct platform_device *pdev) +{

• for (i = 0; i < WM8958_MCLK_MAX; i++) {

• sprintf(tmp, "mclk%d", i + 1);
  

• data->mclk[i] = devm_clk_get(&codec_dev->dev, tmp);
  

• if (IS_ERR(data->mclk[i])) {
  

• 	ret = PTR_ERR(data->mclk[i]);
  

• 	dev_err(&pdev->dev, "failed to get mclk%d clock: %d\n",
  

• 		i + 1, ret);
  

What's the point to have ret = PTR_ERR without doing any error out?

• } else {
  

• 	data->mclk_freq[i] = clk_get_rate(data->mclk[i]);
  

• }
  

• }

Thanks Nicolin