[PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Sylwester Nawrocki]

This patch adds the sound machine driver for TM2 and TM2E board.
Speaker and headphone playback, Main Mic capture, Bluetooth,
Voice call and external accessory are supported.

Signed-off-by: Inha Song <ideal.song@samsung.com>
[k.kozlowski: rebased on 4.1]
Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
[s.nawrocki: rebased to 4.7, adjustment to the ASoC core changes,
 removed unused ops and direct calls to the max98504 function,
 added parsing of "audio-amplifier" and "audio-codec"
 properties, added TDM API calls, switched to gpiod API]
Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Changes since v3:
 - removed SND_SOC_SAMSUNG_AUDSS from Kconfig.

Changes since v2:
 - added missing Kconfig dependencies.

Changes since initial version:
 - added PDM Tx channels setup through TDM API
 - adaptation to renamed 'samsung,model', 'samsung,i2s-controller',
   'samsung,speaker-amplifier' properties,
 - removed some dev_dbg() calls,
 - cleaned up mic-bias GPIO handling and switched to gpiod API,
 - added parsing of 'audio-codec' property,
 - initialized codec_of_node of dai_link instead of codec_name,
 - switched to using clock, clock-names properties from the wm5110
   codec node,
 - fixed error paths in probe() (of_node reference counting).
---
 sound/soc/samsung/Kconfig      |   9 +
 sound/soc/samsung/Makefile     |   2 +
 sound/soc/samsung/tm2_wm5110.c | 579 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 590 insertions(+)
 create mode 100644 sound/soc/samsung/tm2_wm5110.c

diff --git a/sound/soc/samsung/Kconfig b/sound/soc/samsung/Kconfig
index 78baa26..a711605 100644
--- a/sound/soc/samsung/Kconfig
+++ b/sound/soc/samsung/Kconfig
@@ -237,3 +237,12 @@ config SND_SOC_ARNDALE_RT5631_ALC5631
         depends on SND_SOC_SAMSUNG && I2C
         select SND_SAMSUNG_I2S
         select SND_SOC_RT5631
+
+config SND_SOC_SAMSUNG_TM2_WM5110
+	tristate "SoC I2S Audio support for WM5110 on TM2 board"
+	depends on SND_SOC_SAMSUNG && MFD_ARIZONA && I2C && SPI_MASTER
+	select SND_SOC_MAX98504
+	select SND_SOC_WM5110
+	select SND_SAMSUNG_I2S
+	help
+	  Say Y if you want to add support for SoC audio on the TM2 board.
diff --git a/sound/soc/samsung/Makefile b/sound/soc/samsung/Makefile
index 052fe71..c15a759 100644
--- a/sound/soc/samsung/Makefile
+++ b/sound/soc/samsung/Makefile
@@ -45,6 +45,7 @@ snd-soc-littlemill-objs := littlemill.o
 snd-soc-bells-objs := bells.o
 snd-soc-odroidx2-max98090-objs := odroidx2_max98090.o
 snd-soc-arndale-rt5631-objs := arndale_rt5631.o
+snd-soc-tm2-wm5110-objs := tm2_wm5110.o
 
 obj-$(CONFIG_SND_SOC_SAMSUNG_JIVE_WM8750) += snd-soc-jive-wm8750.o
 obj-$(CONFIG_SND_SOC_SAMSUNG_NEO1973_WM8753) += snd-soc-neo1973-wm8753.o
@@ -71,3 +72,4 @@ obj-$(CONFIG_SND_SOC_LITTLEMILL) += snd-soc-littlemill.o
 obj-$(CONFIG_SND_SOC_BELLS) += snd-soc-bells.o
 obj-$(CONFIG_SND_SOC_ODROIDX2) += snd-soc-odroidx2-max98090.o
 obj-$(CONFIG_SND_SOC_ARNDALE_RT5631_ALC5631) += snd-soc-arndale-rt5631.o
+obj-$(CONFIG_SND_SOC_SAMSUNG_TM2_WM5110) += snd-soc-tm2-wm5110.o
diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
new file mode 100644
index 0000000..9728b3c
--- /dev/null
+++ b/sound/soc/samsung/tm2_wm5110.c
@@ -0,0 +1,579 @@
+/*
+ * Copyright (C) 2015 - 2016 Samsung Electronics Co., Ltd.
+ *
+ * Authors: Inha Song <ideal.song@samsung.com>
+ *          Sylwester Nawrocki <s.nawrocki@samsung.com>
+ *
+ * This program is free software; you can redistribute  it and/or modify it
+ * under  the terms of  the GNU General  Public License as published by the
+ * Free Software Foundation;  either version 2 of the  License, or (at your
+ * option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <sound/pcm_params.h>
+#include <sound/soc.h>
+
+#include "i2s.h"
+#include "../codecs/wm5110.h"
+
+struct tm2_machine_priv {
+	struct snd_soc_codec *codec;
+	struct clk *codec_mclk1;
+	struct clk *codec_mclk2;
+
+	unsigned int sysclk_rate;
+
+	struct gpio_desc *gpio_mic_bias;
+};
+
+static int tm2_start_sysclk(struct snd_soc_card *card)
+{
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+	struct snd_soc_codec *codec = priv->codec;
+	unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1);
+	int ret;
+
+	ret = clk_prepare_enable(priv->codec_mclk1);
+	if (ret < 0) {
+		dev_err(card->dev, "Failed to enable mclk: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1,
+				    ARIZONA_FLL_SRC_MCLK1,
+				    mclk_rate,
+				    priv->sysclk_rate);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1_REFCLK,
+				    ARIZONA_FLL_SRC_MCLK1,
+				    mclk_rate,
+				    priv->sysclk_rate);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_sysclk(codec, ARIZONA_CLK_SYSCLK,
+				       ARIZONA_CLK_SRC_FLL1,
+				       priv->sysclk_rate,
+				       SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set SYSCLK Source: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tm2_stop_sysclk(struct snd_soc_card *card)
+{
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+	struct snd_soc_codec *codec = priv->codec;
+	int ret;
+
+	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1, 0, 0, 0);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to stop FLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_sysclk(codec, ARIZONA_CLK_SYSCLK,
+				       ARIZONA_CLK_SRC_FLL1, 0, 0);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to stop SYSCLK: %d\n", ret);
+		return ret;
+	}
+
+	clk_disable_unprepare(priv->codec_mclk1);
+
+	return 0;
+}
+
+static int tm2_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;
+	struct snd_soc_codec *codec = rtd->codec;
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(rtd->card);
+	int ret;
+
+	switch (params_rate(params)) {
+	case 4000:
+	case 8000:
+	case 12000:
+	case 16000:
+	case 24000:
+	case 32000:
+	case 48000:
+	case 96000:
+	case 192000:
+		/* Highest possible SYSCLK frequency: 147.456MHz */
+		priv->sysclk_rate = 147456000U;
+		break;
+	case 11025:
+	case 22050:
+	case 44100:
+	case 88200:
+	case 176400:
+		/* Highest possible SYSCLK frequency: 135.4752 MHz */
+		priv->sysclk_rate = 135475200U;
+		break;
+	default:
+		dev_err(codec->dev, "Not supported sample rate: %d\n",
+			params_rate(params));
+		return -EINVAL;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK, 0, 0);
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "Failed to set SYSCLK: %d\n", ret);
+		return ret;
+	}
+
+	return tm2_start_sysclk(rtd->card);
+}
+
+static struct snd_soc_ops tm2_aif1_ops = {
+	.hw_params = tm2_aif1_hw_params,
+};
+
+static int tm2_aif2_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;
+	struct snd_soc_codec *codec = rtd->codec;
+	struct tm2_machine_priv *priv =	snd_soc_card_get_drvdata(rtd->card);
+	unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1);
+	unsigned int asyncclk_rate;
+	int ret;
+
+	switch (params_rate(params)) {
+	case 8000:
+	case 12000:
+	case 16000:
+		/* Highest possible ASYNCCLK frequency: 49.152MHz */
+		asyncclk_rate = 49152000U;
+		break;
+	case 11025:
+		/* Highest possible ASYNCCLK frequency: 45.1584 MHz */
+		asyncclk_rate = 45158400U;
+		break;
+	default:
+		dev_err(codec->dev, "Not supported sample rate: %d\n",
+			params_rate(params));
+		return -EINVAL;
+	}
+
+	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2,
+				    ARIZONA_FLL_SRC_MCLK1,
+				    mclk_rate,
+				    asyncclk_rate);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2_REFCLK,
+				    ARIZONA_FLL_SRC_MCLK1,
+				    mclk_rate,
+				    asyncclk_rate);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_ASYNCCLK, 0, 0);
+
+	if (ret < 0) {
+		dev_err(codec_dai->dev, "Failed to set ASYNCCLK: %d\n", ret);
+		return ret;
+	}
+
+	ret = snd_soc_codec_set_sysclk(codec, ARIZONA_CLK_ASYNCCLK,
+				       ARIZONA_CLK_SRC_FLL2,
+				       asyncclk_rate,
+				       SND_SOC_CLOCK_IN);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set ASYNCCLK Source: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct snd_soc_ops tm2_aif2_ops = {
+	.hw_params = tm2_aif2_hw_params,
+};
+
+static int tm2_mic_bias(struct snd_soc_dapm_widget *w,
+				struct snd_kcontrol *kcontrol, int event)
+{
+	struct snd_soc_card *card = w->dapm->card;
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+
+	switch (event) {
+	case SND_SOC_DAPM_PRE_PMU:
+		gpiod_set_value_cansleep(priv->gpio_mic_bias,  1);
+		break;
+	case SND_SOC_DAPM_POST_PMD:
+		gpiod_set_value_cansleep(priv->gpio_mic_bias,  0);
+		break;
+	}
+
+	return 0;
+}
+
+static int tm2_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;
+
+	rtd = snd_soc_get_pcm_runtime(card, card->dai_link[0].name);
+
+	if (dapm->dev != rtd->codec_dai->dev)
+		return 0;
+
+	switch (level) {
+	case SND_SOC_BIAS_STANDBY:
+		if (card->dapm.bias_level == SND_SOC_BIAS_OFF)
+			tm2_start_sysclk(card);
+		break;
+	case SND_SOC_BIAS_OFF:
+		tm2_stop_sysclk(card);
+		break;
+	case SND_SOC_BIAS_PREPARE:
+		break;
+	default:
+		break;
+	}
+
+	card->dapm.bias_level = level;
+
+	return 0;
+}
+
+static struct snd_soc_aux_dev tm2_speaker_amp_dev;
+
+static int tm2_late_probe(struct snd_soc_card *card)
+{
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+	struct snd_soc_dai_link_component dlc = { 0 };
+	struct snd_soc_dai *amp_pdm_dai;
+	struct snd_soc_pcm_runtime *rtd;
+	unsigned int ch_map[] = { 0, 1 };
+	int ret;
+
+	rtd = snd_soc_get_pcm_runtime(card, card->dai_link[0].name);
+	priv->codec = rtd->codec;
+
+	/* 32 kHz must be enabled for jack detection */
+	if (!IS_ERR(priv->codec_mclk2))
+		clk_prepare_enable(priv->codec_mclk2);
+
+	dlc.of_node = tm2_speaker_amp_dev.codec_of_node;
+	amp_pdm_dai = snd_soc_find_dai(&dlc);
+	if (!amp_pdm_dai)
+		return -ENODEV;
+
+	/* Set the MAX98504 V/I sense PDM Tx DAI channel mapping */
+	ret = snd_soc_dai_set_channel_map(amp_pdm_dai, ARRAY_SIZE(ch_map),
+					  ch_map, 0, NULL);
+	if (ret < 0)
+		return ret;
+
+	ret = snd_soc_dai_set_tdm_slot(amp_pdm_dai, 0x3, 0x0, 2, 16);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tm2_suspend_post(struct snd_soc_card *card)
+{
+	return tm2_stop_sysclk(card);
+}
+
+static int tm2_resume_pre(struct snd_soc_card *card)
+{
+	return tm2_start_sysclk(card);
+}
+
+static const struct snd_kcontrol_new tm2_controls[] = {
+	SOC_DAPM_PIN_SWITCH("HP"),
+	SOC_DAPM_PIN_SWITCH("SPK"),
+	SOC_DAPM_PIN_SWITCH("RCV"),
+	SOC_DAPM_PIN_SWITCH("VPS"),
+	SOC_DAPM_PIN_SWITCH("HDMI"),
+
+	SOC_DAPM_PIN_SWITCH("Main Mic"),
+	SOC_DAPM_PIN_SWITCH("Sub Mic"),
+	SOC_DAPM_PIN_SWITCH("Third Mic"),
+
+	SOC_DAPM_PIN_SWITCH("Headset Mic"),
+};
+
+const struct snd_soc_dapm_widget tm2_dapm_widgets[] = {
+	SND_SOC_DAPM_HP("HP", NULL),
+	SND_SOC_DAPM_SPK("SPK", NULL),
+	SND_SOC_DAPM_SPK("RCV", NULL),
+	SND_SOC_DAPM_LINE("VPS", NULL),
+	SND_SOC_DAPM_LINE("HDMI", NULL),
+
+	SND_SOC_DAPM_MIC("Main Mic", tm2_mic_bias),
+	SND_SOC_DAPM_MIC("Sub Mic", NULL),
+	SND_SOC_DAPM_MIC("Third Mic", NULL),
+
+	SND_SOC_DAPM_MIC("Headset Mic", NULL),
+};
+
+static const struct snd_soc_component_driver tm2_component = {
+	.name	= "tm2-audio",
+};
+
+static struct snd_soc_dai_driver tm2_ext_dai[] = {
+	{
+		.name = "Voice call",
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 4,
+			.rate_min = 8000,
+			.rate_max = 48000,
+			.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
+					SNDRV_PCM_RATE_48000),
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.capture = {
+			.channels_min = 1,
+			.channels_max = 4,
+			.rate_min = 8000,
+			.rate_max = 48000,
+			.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000 |
+					SNDRV_PCM_RATE_48000),
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+	},
+	{
+		.name = "Bluetooth",
+		.playback = {
+			.channels_min = 1,
+			.channels_max = 4,
+			.rate_min = 8000,
+			.rate_max = 16000,
+			.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000),
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+		.capture = {
+			.channels_min = 1,
+			.channels_max = 2,
+			.rate_min = 8000,
+			.rate_max = 16000,
+			.rates = (SNDRV_PCM_RATE_8000 | SNDRV_PCM_RATE_16000),
+			.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		},
+	},
+};
+
+static struct snd_soc_dai_link tm2_dai_links[] = {
+	{
+		.name		= "WM5110 AIF1",
+		.stream_name	= "HiFi Primary",
+		.codec_dai_name = "wm5110-aif1",
+		.ops		= &tm2_aif1_ops,
+		.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				  SND_SOC_DAIFMT_CBM_CFM,
+	}, {
+		.name		= "WM5110 Voice",
+		.stream_name	= "Voice call",
+		.codec_dai_name = "wm5110-aif2",
+		.ops		= &tm2_aif2_ops,
+		.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				  SND_SOC_DAIFMT_CBM_CFM,
+		.ignore_suspend = 1,
+	}, {
+		.name		= "WM5110 BT",
+		.stream_name	= "Bluetooth",
+		.codec_dai_name = "wm5110-aif3",
+		.dai_fmt	= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_NB_NF |
+				  SND_SOC_DAIFMT_CBM_CFM,
+		.ignore_suspend = 1,
+	}
+};
+
+static struct snd_soc_card tm2_card = {
+	.owner			= THIS_MODULE,
+
+	.dai_link		= tm2_dai_links,
+	.num_links		= ARRAY_SIZE(tm2_dai_links),
+	.controls		= tm2_controls,
+	.num_controls		= ARRAY_SIZE(tm2_controls),
+	.dapm_widgets		= tm2_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(tm2_dapm_widgets),
+
+	.aux_dev		= &tm2_speaker_amp_dev,
+	.num_aux_devs		= 1,
+
+	.late_probe		= tm2_late_probe,
+
+	.set_bias_level		= tm2_set_bias_level,
+
+	.suspend_post		= tm2_suspend_post,
+	.resume_pre		= tm2_resume_pre,
+};
+
+static int tm2_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct snd_soc_card *card = &tm2_card;
+	struct tm2_machine_priv *priv;
+	struct device_node *cpu_dai_node, *codec_dai_node;
+	int ret, i;
+
+	if (!dev->of_node) {
+		dev_err(dev, "DT node is missing\n");
+		return -ENODEV;
+	}
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	snd_soc_card_set_drvdata(card, priv);
+	card->dev = dev;
+
+	priv->gpio_mic_bias = devm_gpiod_get(dev, "mic-bias",
+						GPIOF_OUT_INIT_LOW);
+	if (IS_ERR(priv->gpio_mic_bias)) {
+		dev_err(dev, "Failed to get mic bias gpio\n");
+		return PTR_ERR(priv->gpio_mic_bias);
+	}
+
+	ret = snd_soc_of_parse_card_name(card, "model");
+	if (ret < 0) {
+		dev_err(dev, "Card name is not specified\n");
+		return ret;
+	}
+
+	ret = snd_soc_of_parse_audio_routing(card, "samsung,audio-routing");
+	if (ret < 0) {
+		dev_err(dev, "Audio routing is not specified or invalid\n");
+		return ret;
+	}
+
+	card->aux_dev[0].codec_of_node = of_parse_phandle(dev->of_node,
+							"audio-amplifier", 0);
+	if (!card->aux_dev[0].codec_of_node) {
+		dev_err(dev, "audio-amplifier property invalid or missing\n");
+		return -EINVAL;
+	}
+
+	cpu_dai_node = of_parse_phandle(dev->of_node, "i2s-controller", 0);
+	if (!cpu_dai_node) {
+		dev_err(dev, "i2s-controllers property invalid or missing\n");
+		ret = -EINVAL;
+		goto err_put_amp;
+	}
+
+	codec_dai_node = of_parse_phandle(dev->of_node, "audio-codec", 0);
+	if (!codec_dai_node) {
+		dev_err(dev, "audio-codec property invalid or missing\n");
+		ret = -EINVAL;
+		goto err_put_cpu_dai;
+	}
+
+	for (i = 0; i < card->num_links; i++) {
+		card->dai_link[i].cpu_dai_name = NULL;
+		card->dai_link[i].cpu_name = NULL;
+		card->dai_link[i].platform_name = NULL;
+		card->dai_link[i].codec_of_node = codec_dai_node;
+		card->dai_link[i].cpu_of_node = cpu_dai_node;
+		card->dai_link[i].platform_of_node = cpu_dai_node;
+	}
+
+	priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
+	if (IS_ERR(priv->codec_mclk1)) {
+		dev_err(dev, "Failed to get mclk1 clock\n");
+		ret = PTR_ERR(priv->codec_mclk1);
+		goto err_put_codec_dai;
+	}
+
+	/* mclk2 is optional */
+	priv->codec_mclk2 = of_clk_get_by_name(codec_dai_node, "mclk2");
+	if (IS_ERR(priv->codec_mclk2))
+		dev_info(dev, "Not using mclk2 clock\n");
+
+	ret = devm_snd_soc_register_component(dev, &tm2_component,
+				tm2_ext_dai, ARRAY_SIZE(tm2_ext_dai));
+	if (ret < 0) {
+		dev_err(dev, "Failed to register component: %d\n", ret);
+		goto err_put_mclk;
+	}
+
+	ret = devm_snd_soc_register_card(dev, card);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register card: %d\n", ret);
+		goto err_put_mclk;
+	}
+
+	return 0;
+
+err_put_mclk:
+	clk_put(priv->codec_mclk1);
+	if (!IS_ERR(priv->codec_mclk2))
+		clk_put(priv->codec_mclk2);
+err_put_codec_dai:
+	of_node_put(codec_dai_node);
+err_put_cpu_dai:
+	of_node_put(cpu_dai_node);
+err_put_amp:
+	of_node_put(card->aux_dev[0].codec_of_node);
+	return ret;
+}
+
+static int tm2_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = &tm2_card;
+	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
+
+	clk_put(priv->codec_mclk1);
+	if (!IS_ERR(priv->codec_mclk2))
+		clk_put(priv->codec_mclk2);
+
+	of_node_put(card->dai_link[0].codec_of_node);
+	of_node_put(card->dai_link[0].cpu_of_node);
+	of_node_put(card->aux_dev[0].codec_of_node);
+
+	return 0;
+}
+
+static const struct of_device_id tm2_of_match[] = {
+	{ .compatible = "samsung,tm2-audio" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, tm2_of_match);
+
+static struct platform_driver tm2_driver = {
+	.driver = {
+		.name		= "tm2-audio",
+		.pm		= &snd_soc_pm_ops,
+		.of_match_table	= tm2_of_match,
+	},
+	.probe	= tm2_probe,
+	.remove	= tm2_remove,
+};
+
+module_platform_driver(tm2_driver);
+
+MODULE_AUTHOR("Inha Song <ideal.song@samsung.com>");
+MODULE_DESCRIPTION("ALSA SoC Exynos TM2 Audio Support");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Chanwoo Choi]

Hi Sylwester,

[snip]
> +static int tm2_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct snd_soc_card *card = &tm2_card;
> +	struct tm2_machine_priv *priv;
> +	struct device_node *cpu_dai_node, *codec_dai_node;
> +	int ret, i;
> +
> +	if (!dev->of_node) {
> +		dev_err(dev, "DT node is missing\n");
> +		return -ENODEV;
> +	}
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	snd_soc_card_set_drvdata(card, priv);
> +	card->dev = dev;
> +
> +	priv->gpio_mic_bias = devm_gpiod_get(dev, "mic-bias",
> +						GPIOF_OUT_INIT_LOW);
> +	if (IS_ERR(priv->gpio_mic_bias)) {
> +		dev_err(dev, "Failed to get mic bias gpio\n");
> +		return PTR_ERR(priv->gpio_mic_bias);
> +	}
> +
> +	ret = snd_soc_of_parse_card_name(card, "model");
> +	if (ret < 0) {
> +		dev_err(dev, "Card name is not specified\n");
> +		return ret;
> +	}
> +
> +	ret = snd_soc_of_parse_audio_routing(card, "samsung,audio-routing");
> +	if (ret < 0) {
> +		dev_err(dev, "Audio routing is not specified or invalid\n");
> +		return ret;
> +	}
> +
> +	card->aux_dev[0].codec_of_node = of_parse_phandle(dev->of_node,
> +							"audio-amplifier", 0);
> +	if (!card->aux_dev[0].codec_of_node) {
> +		dev_err(dev, "audio-amplifier property invalid or missing\n");
> +		return -EINVAL;
> +	}
> +
> +	cpu_dai_node = of_parse_phandle(dev->of_node, "i2s-controller", 0);
> +	if (!cpu_dai_node) {
> +		dev_err(dev, "i2s-controllers property invalid or missing\n");
> +		ret = -EINVAL;
> +		goto err_put_amp;
> +	}
> +
> +	codec_dai_node = of_parse_phandle(dev->of_node, "audio-codec", 0);
> +	if (!codec_dai_node) {
> +		dev_err(dev, "audio-codec property invalid or missing\n");
> +		ret = -EINVAL;
> +		goto err_put_cpu_dai;
> +	}
> +
> +	for (i = 0; i < card->num_links; i++) {
> +		card->dai_link[i].cpu_dai_name = NULL;
> +		card->dai_link[i].cpu_name = NULL;
> +		card->dai_link[i].platform_name = NULL;
> +		card->dai_link[i].codec_of_node = codec_dai_node;
> +		card->dai_link[i].cpu_of_node = cpu_dai_node;
> +		card->dai_link[i].platform_of_node = cpu_dai_node;
> +	}
> +
> +	priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
> +	if (IS_ERR(priv->codec_mclk1)) {
> +		dev_err(dev, "Failed to get mclk1 clock\n");
> +		ret = PTR_ERR(priv->codec_mclk1);
> +		goto err_put_codec_dai;
> +	}

I think that you better to use the devm_clk_get() instead of of_clk_get_by_name()
because you don't need to handle the clk_put() when error happen and remove the this driver.

	priv->codec_mclk1 = devm_clk_get(dev, "mclk1");

> +
> +	/* mclk2 is optional */
> +	priv->codec_mclk2 = of_clk_get_by_name(codec_dai_node, "mclk2");
> +	if (IS_ERR(priv->codec_mclk2))
> +		dev_info(dev, "Not using mclk2 clock\n");

	priv->codec_mclk2 = devm_clk_get(dev, "mclk2");

> +
> +	ret = devm_snd_soc_register_component(dev, &tm2_component,
> +				tm2_ext_dai, ARRAY_SIZE(tm2_ext_dai));
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register component: %d\n", ret);
> +		goto err_put_mclk;
> +	}
> +
> +	ret = devm_snd_soc_register_card(dev, card);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register card: %d\n", ret);
> +		goto err_put_mclk;
> +	}
> +
> +	return 0;
> +
> +err_put_mclk:
> +	clk_put(priv->codec_mclk1);
> +	if (!IS_ERR(priv->codec_mclk2))
> +		clk_put(priv->codec_mclk2);

If using the devm_clk_get(), clk_put() is not needed

> +err_put_codec_dai:
> +	of_node_put(codec_dai_node);
> +err_put_cpu_dai:
> +	of_node_put(cpu_dai_node);
> +err_put_amp:
> +	of_node_put(card->aux_dev[0].codec_of_node);
> +	return ret;
> +}
> +
> +static int tm2_remove(struct platform_device *pdev)
> +{
> +	struct snd_soc_card *card = &tm2_card;
> +	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
> +
> +	clk_put(priv->codec_mclk1);
> +	if (!IS_ERR(priv->codec_mclk2))
> +		clk_put(priv->codec_mclk2);

ditto.

> +
> +	of_node_put(card->dai_link[0].codec_of_node);
> +	of_node_put(card->dai_link[0].cpu_of_node);
> +	of_node_put(card->aux_dev[0].codec_of_node);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tm2_of_match[] = {
> +	{ .compatible = "samsung,tm2-audio" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, tm2_of_match);
> +
> +static struct platform_driver tm2_driver = {
> +	.driver = {
> +		.name		= "tm2-audio",
> +		.pm		= &snd_soc_pm_ops,
> +		.of_match_table	= tm2_of_match,
> +	},
> +	.probe	= tm2_probe,
> +	.remove	= tm2_remove,
> +};
> +
> +module_platform_driver(tm2_driver);
> +
> +MODULE_AUTHOR("Inha Song <ideal.song@samsung.com>");
> +MODULE_DESCRIPTION("ALSA SoC Exynos TM2 Audio Support");
> +MODULE_LICENSE("GPL v2");
> 

Thanks,
Chanwoo Choi

Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Sylwester Nawrocki]

Hi Chanwoo,

On 07/15/2016 07:18 AM, Chanwoo Choi wrote:
>> +static int tm2_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;

>> +	codec_dai_node = of_parse_phandle(dev->of_node, "audio-codec", 0);
>> +	if (!codec_dai_node) {
>> +		dev_err(dev, "audio-codec property invalid or missing\n");
>> +		ret = -EINVAL;
>> +		goto err_put_cpu_dai;
>> +	}

>> +	priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
>> +	if (IS_ERR(priv->codec_mclk1)) {
>> +		dev_err(dev, "Failed to get mclk1 clock\n");
>> +		ret = PTR_ERR(priv->codec_mclk1);
>> +		goto err_put_codec_dai;
>> +	}
> 
> I think that you better to use the devm_clk_get() instead of of_clk_get_by_name()
> because you don't need to handle the clk_put() when error happen and remove the 
> this driver.
> 
> 	priv->codec_mclk1 = devm_clk_get(dev, "mclk1");

The clocks are from the CODEC DT node, for which we don't have struct
device pointer here, that's why I used of_clk_get_by_name().

--
Thanks,
Sylwester

Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Chanwoo Choi]

Hi Sylwester,

On 2016년 07월 18일 19:41, Sylwester Nawrocki wrote:
> Hi Chanwoo,
> 
> On 07/15/2016 07:18 AM, Chanwoo Choi wrote:
>>> +static int tm2_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device *dev = &pdev->dev;
> 
>>> +	codec_dai_node = of_parse_phandle(dev->of_node, "audio-codec", 0);
>>> +	if (!codec_dai_node) {
>>> +		dev_err(dev, "audio-codec property invalid or missing\n");
>>> +		ret = -EINVAL;
>>> +		goto err_put_cpu_dai;
>>> +	}
> 
>>> +	priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
>>> +	if (IS_ERR(priv->codec_mclk1)) {
>>> +		dev_err(dev, "Failed to get mclk1 clock\n");
>>> +		ret = PTR_ERR(priv->codec_mclk1);
>>> +		goto err_put_codec_dai;
>>> +	}
>>
>> I think that you better to use the devm_clk_get() instead of of_clk_get_by_name()
>> because you don't need to handle the clk_put() when error happen and remove the 
>> this driver.
>>
>> 	priv->codec_mclk1 = devm_clk_get(dev, "mclk1");
> 
> The clocks are from the CODEC DT node, for which we don't have struct
> device pointer here, that's why I used of_clk_get_by_name().

When I test it, I can get the clock pointer by devm_clk_get() as following:

diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
index 9728b3c5927f..5de4fc554aec 100644
--- a/sound/soc/samsung/tm2_wm5110.c
+++ b/sound/soc/samsung/tm2_wm5110.c
@@ -500,7 +500,7 @@ static int tm2_probe(struct platform_device *pdev)
                card->dai_link[i].platform_of_node = cpu_dai_node;
        }

-       priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
+       priv->codec_mclk1 = devm_clk_get(dev, "mclk1");
        if (IS_ERR(priv->codec_mclk1)) {
                dev_err(dev, "Failed to get mclk1 clock\n");
                ret = PTR_ERR(priv->codec_mclk1);
@@ -508,7 +508,7 @@ static int tm2_probe(struct platform_device *pdev)
        }

        /* mclk2 is optional */
-       priv->codec_mclk2 = of_clk_get_by_name(codec_dai_node, "mclk2");
+       priv->codec_mclk2 = devm_clk_get(dev, "mclk2");
        if (IS_ERR(priv->codec_mclk2))
                dev_info(dev, "Not using mclk2 clock\n");

@@ -516,21 +516,17 @@ static int tm2_probe(struct platform_device *pdev)
                                tm2_ext_dai, ARRAY_SIZE(tm2_ext_dai));
        if (ret < 0) {
                dev_err(dev, "Failed to register component: %d\n", ret);
-               goto err_put_mclk;
+               goto err_put_codec_dai;
        }

        ret = devm_snd_soc_register_card(dev, card);
        if (ret < 0) {
                dev_err(dev, "Failed to register card: %d\n", ret);
-               goto err_put_mclk;
+               goto err_put_codec_dai;
        }

        return 0;

-err_put_mclk:
-       clk_put(priv->codec_mclk1);
-       if (!IS_ERR(priv->codec_mclk2))
-               clk_put(priv->codec_mclk2);
 err_put_codec_dai:
        of_node_put(codec_dai_node);
 err_put_cpu_dai:
@@ -543,11 +539,6 @@ err_put_amp:
 static int tm2_remove(struct platform_device *pdev)
 {
        struct snd_soc_card *card = &tm2_card;
-       struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
-
-       clk_put(priv->codec_mclk1);
-       if (!IS_ERR(priv->codec_mclk2))
-               clk_put(priv->codec_mclk2);


Regards,
Chanwoo Choi

Re: [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Sylwester Nawrocki]

On 07/21/2016 12:28 PM, Chanwoo Choi wrote:
> When I test it, I can get the clock pointer by devm_clk_get() as following:
> 
> diff --git a/sound/soc/samsung/tm2_wm5110.c b/sound/soc/samsung/tm2_wm5110.c
> index 9728b3c5927f..5de4fc554aec 100644
> --- a/sound/soc/samsung/tm2_wm5110.c
> +++ b/sound/soc/samsung/tm2_wm5110.c
> @@ -500,7 +500,7 @@ static int tm2_probe(struct platform_device *pdev)
>                 card->dai_link[i].platform_of_node = cpu_dai_node;
>         }
> 
> -       priv->codec_mclk1 = of_clk_get_by_name(codec_dai_node, "mclk1");
> +       priv->codec_mclk1 = devm_clk_get(dev, "mclk1");

I guess you have and old dts where clock/clock-names are in the sound
node.  Instead the clock/clock-names properties should be specified
in the CODEC's node, the related binding's documentation can be found
in Documentation/devicetree/bindings/mfd/arizona.txt. Also please see
patch 3/4, there is no clock properties in the sound card's DT binding.

--
Thanks,
Sylwester

Re: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Charles Keepax]

On Tue, Jul 05, 2016 at 07:14:37PM +0200, Sylwester Nawrocki wrote:
> This patch adds the sound machine driver for TM2 and TM2E board.
> Speaker and headphone playback, Main Mic capture, Bluetooth,
> Voice call and external accessory are supported.
> 
> Signed-off-by: Inha Song <ideal.song@samsung.com>
> [k.kozlowski: rebased on 4.1]
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> [s.nawrocki: rebased to 4.7, adjustment to the ASoC core changes,
>  removed unused ops and direct calls to the max98504 function,
>  added parsing of "audio-amplifier" and "audio-codec"
>  properties, added TDM API calls, switched to gpiod API]
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> ---
> Changes since v3:
>  - removed SND_SOC_SAMSUNG_AUDSS from Kconfig.
> 
> Changes since v2:
>  - added missing Kconfig dependencies.
> 
> Changes since initial version:
>  - added PDM Tx channels setup through TDM API
>  - adaptation to renamed 'samsung,model', 'samsung,i2s-controller',
>    'samsung,speaker-amplifier' properties,
>  - removed some dev_dbg() calls,
>  - cleaned up mic-bias GPIO handling and switched to gpiod API,
>  - added parsing of 'audio-codec' property,
>  - initialized codec_of_node of dai_link instead of codec_name,
>  - switched to using clock, clock-names properties from the wm5110
>    codec node,
>  - fixed error paths in probe() (of_node reference counting).
> ---

Apologies for my late response I had missed this.

<snip>
> +static int tm2_start_sysclk(struct snd_soc_card *card)
> +{
> +	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
> +	struct snd_soc_codec *codec = priv->codec;
> +	unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1);
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->codec_mclk1);
> +	if (ret < 0) {
> +		dev_err(card->dev, "Failed to enable mclk: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1,
> +				    ARIZONA_FLL_SRC_MCLK1,
> +				    mclk_rate,
> +				    priv->sysclk_rate);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1_REFCLK,
> +				    ARIZONA_FLL_SRC_MCLK1,
> +				    mclk_rate,
> +				    priv->sysclk_rate);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
> +		return ret;
> +	}

It would be better to set the REFCLK first. Setting WM5110_FLL1
actually enables the FLL, where as setting WM5110_FLL1_REFCKL
doesn't. So doing it this way round the first time you bring
up the FLL it will enable it then reconfigure the reference
path. Doing it the other way round it will always enable first
time with the correct settings.

> +
> +	ret = snd_soc_codec_set_sysclk(codec, ARIZONA_CLK_SYSCLK,
> +				       ARIZONA_CLK_SRC_FLL1,
> +				       priv->sysclk_rate,
> +				       SND_SOC_CLOCK_IN);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set SYSCLK Source: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
<snip>
> +static int tm2_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;
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(rtd->card);
> +	int ret;
> +
> +	switch (params_rate(params)) {
> +	case 4000:
> +	case 8000:
> +	case 12000:
> +	case 16000:
> +	case 24000:
> +	case 32000:
> +	case 48000:
> +	case 96000:
> +	case 192000:
> +		/* Highest possible SYSCLK frequency: 147.456MHz */
> +		priv->sysclk_rate = 147456000U;
> +		break;
> +	case 11025:
> +	case 22050:
> +	case 44100:
> +	case 88200:
> +	case 176400:
> +		/* Highest possible SYSCLK frequency: 135.4752 MHz */
> +		priv->sysclk_rate = 135475200U;
> +		break;
> +	default:
> +		dev_err(codec->dev, "Not supported sample rate: %d\n",
> +			params_rate(params));
> +		return -EINVAL;
> +	}
> +
> +	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK, 0, 0);
> +	if (ret < 0) {
> +		dev_err(codec_dai->dev, "Failed to set SYSCLK: %d\n", ret);
> +		return ret;
> +	}

If there is no intention to change which clocking domain the DAI
is attached to I would put this in late probe, although it does
no harm here and if that might get added in the future then here
is better.

> +
> +	return tm2_start_sysclk(rtd->card);
> +}
> +
> +static struct snd_soc_ops tm2_aif1_ops = {
> +	.hw_params = tm2_aif1_hw_params,
> +};
> +
> +static int tm2_aif2_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;
> +	struct snd_soc_codec *codec = rtd->codec;
> +	struct tm2_machine_priv *priv =	snd_soc_card_get_drvdata(rtd->card);
> +	unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1);
> +	unsigned int asyncclk_rate;
> +	int ret;
> +
> +	switch (params_rate(params)) {
> +	case 8000:
> +	case 12000:
> +	case 16000:
> +		/* Highest possible ASYNCCLK frequency: 49.152MHz */
> +		asyncclk_rate = 49152000U;
> +		break;
> +	case 11025:
> +		/* Highest possible ASYNCCLK frequency: 45.1584 MHz */
> +		asyncclk_rate = 45158400U;
> +		break;
> +	default:
> +		dev_err(codec->dev, "Not supported sample rate: %d\n",
> +			params_rate(params));
> +		return -EINVAL;
> +	}
> +
> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2,
> +				    ARIZONA_FLL_SRC_MCLK1,
> +				    mclk_rate,
> +				    asyncclk_rate);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2_REFCLK,
> +				    ARIZONA_FLL_SRC_MCLK1,
> +				    mclk_rate,
> +				    asyncclk_rate);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
> +		return ret;
> +	}

Again as before I would set the REFCLK path first on the FLL.

Also nothing ever turns FLL2 off again here. I would either turn
it off again in set_bias level or add a free callback into
tm2_aif2_ops, probably adding a free callback matches the usage
of this clock better I would guess.

> +
> +	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_ASYNCCLK, 0, 0);
> +
> +	if (ret < 0) {
> +		dev_err(codec_dai->dev, "Failed to set ASYNCCLK: %d\n", ret);
> +		return ret;
> +	}

Again I would do from late probe.

> +
> +	ret = snd_soc_codec_set_sysclk(codec, ARIZONA_CLK_ASYNCCLK,
> +				       ARIZONA_CLK_SRC_FLL2,
> +				       asyncclk_rate,
> +				       SND_SOC_CLOCK_IN);
> +	if (ret < 0) {
> +		dev_err(codec->dev, "Failed to set ASYNCCLK Source: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct snd_soc_ops tm2_aif2_ops = {
> +	.hw_params = tm2_aif2_hw_params,
> +};
> +
<snip>
> +
> +static int tm2_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;
> +
> +	rtd = snd_soc_get_pcm_runtime(card, card->dai_link[0].name);
> +
> +	if (dapm->dev != rtd->codec_dai->dev)
> +		return 0;
> +
> +	switch (level) {
> +	case SND_SOC_BIAS_STANDBY:
> +		if (card->dapm.bias_level == SND_SOC_BIAS_OFF)
> +			tm2_start_sysclk(card);
> +		break;
> +	case SND_SOC_BIAS_OFF:
> +		tm2_stop_sysclk(card);
> +		break;
> +	case SND_SOC_BIAS_PREPARE:
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	card->dapm.bias_level = level;

I believe the core does this for you these days.

> +
> +	return 0;
> +}
> +
<snip>
> +
> +static int tm2_suspend_post(struct snd_soc_card *card)
> +{
> +	return tm2_stop_sysclk(card);

I think you can't really do this from these callbacks, the
trouble is suspend_post is called from snd_soc_suspend which is
set as the suspend callback in snd_soc_pm_ops. So when this is
called you don't actually know if the SPI has already suspended
or not, if it hasn't things will work but if the SPI has already
suspended then this falls over.

A better solution would be to define a local copy of
snd_soc_pm_ops with this functions set as the prepare and
complete callbacks the other callbacks set as snd_soc_pm_ops sets
them. These callback will run before anything is suspended and
after everything has been resumed.

> +}
> +
> +static int tm2_resume_pre(struct snd_soc_card *card)
> +{
> +	return tm2_start_sysclk(card);

Same here with restore you don't know if the SPI has resumed yet
when this runs.

Thanks,
Charles

Re: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board

[Sylwester Nawrocki]

On 07/22/2016 11:51 AM, Charles Keepax wrote:
> On Tue, Jul 05, 2016 at 07:14:37PM +0200, Sylwester Nawrocki wrote:
>> This patch adds the sound machine driver for TM2 and TM2E board.
>> Speaker and headphone playback, Main Mic capture, Bluetooth,
>> Voice call and external accessory are supported.
>>
>> Signed-off-by: Inha Song <ideal.song@samsung.com>
>> [k.kozlowski: rebased on 4.1]
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>> [s.nawrocki: rebased to 4.7, adjustment to the ASoC core changes,
>>  removed unused ops and direct calls to the max98504 function,
>>  added parsing of "audio-amplifier" and "audio-codec"
>>  properties, added TDM API calls, switched to gpiod API]
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>

> 
> Apologies for my late response I had missed this.

Thanks a lot for your comments, this missed the 4.8 merge window
anyway.

> <snip>
>> +static int tm2_start_sysclk(struct snd_soc_card *card)
>> +{
>> +	struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card);
>> +	struct snd_soc_codec *codec = priv->codec;
>> +	unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1);
>> +	int ret;
>> +
>> +	ret = clk_prepare_enable(priv->codec_mclk1);
>> +	if (ret < 0) {
>> +		dev_err(card->dev, "Failed to enable mclk: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1,
>> +				    ARIZONA_FLL_SRC_MCLK1,
>> +				    mclk_rate,
>> +				    priv->sysclk_rate);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL1_REFCLK,
>> +				    ARIZONA_FLL_SRC_MCLK1,
>> +				    mclk_rate,
>> +				    priv->sysclk_rate);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
>> +		return ret;
>> +	}
> 
> It would be better to set the REFCLK first. Setting WM5110_FLL1
> actually enables the FLL, where as setting WM5110_FLL1_REFCKL
> doesn't. So doing it this way round the first time you bring
> up the FLL it will enable it then reconfigure the reference
> path. Doing it the other way round it will always enable first
> time with the correct settings.

OK, thanks for the hint, I will reorder this.


>> +static int tm2_aif1_hw_params(struct snd_pcm_substream *substream,
>> +				struct snd_pcm_hw_params *params)
>> +{

>> +
>> +	ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK, 0, 0);
>> +	if (ret < 0) {
>> +		dev_err(codec_dai->dev, "Failed to set SYSCLK: %d\n", ret);
>> +		return ret;
>> +	}
> 
> If there is no intention to change which clocking domain the DAI
> is attached to I would put this in late probe, although it does
> no harm here and if that might get added in the future then here
> is better.

If the clocking arrangement ever needs to be changed the call could
be added here again, I will move it to late_probe.

>> +	return tm2_start_sysclk(rtd->card);
>> +}
>> +
>> +static struct snd_soc_ops tm2_aif1_ops = {
>> +	.hw_params = tm2_aif1_hw_params,
>> +};
>> +
>> +static int tm2_aif2_hw_params(struct snd_pcm_substream *substream,
>> +				struct snd_pcm_hw_params *params)
>> +{

>> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2,
>> +				    ARIZONA_FLL_SRC_MCLK1,
>> +				    mclk_rate,
>> +				    asyncclk_rate);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "Failed to start FLL: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = snd_soc_codec_set_pll(codec, WM5110_FLL2_REFCLK,
>> +				    ARIZONA_FLL_SRC_MCLK1,
>> +				    mclk_rate,
>> +				    asyncclk_rate);
>> +	if (ret < 0) {
>> +		dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);
>> +		return ret;
>> +	}
> 
> Again as before I would set the REFCLK path first on the FLL.
> 
> Also nothing ever turns FLL2 off again here. I would either turn
> it off again in set_bias level or add a free callback into
> tm2_aif2_ops, probably adding a free callback matches the usage
> of this clock better I would guess.

hw_free sounds like a good option, I'll add it to ensure the WM5110_FLL2
clock is disabled when not in use.

>> +static int tm2_set_bias_level(struct snd_soc_card *card,
>> +				struct snd_soc_dapm_context *dapm,
>> +				enum snd_soc_bias_level level)
>> +{

>> +
>> +	card->dapm.bias_level = level;
> 
> I believe the core does this for you these days.

Indeed, I had missed that, I'll drop this assignment.

>> +static int tm2_suspend_post(struct snd_soc_card *card)
>> +{
>> +	return tm2_stop_sysclk(card);
> 
> I think you can't really do this from these callbacks, the
> trouble is suspend_post is called from snd_soc_suspend which is
> set as the suspend callback in snd_soc_pm_ops. So when this is
> called you don't actually know if the SPI has already suspended
> or not, if it hasn't things will work but if the SPI has already
> suspended then this falls over.
> 
> A better solution would be to define a local copy of
> snd_soc_pm_ops with this functions set as the prepare and
> complete callbacks the other callbacks set as snd_soc_pm_ops sets
> them. These callback will run before anything is suspended and
> after everything has been resumed.

Agreed, dependency on the SPI controller seems to be not modelled and
it looks like it is working by chance now.  I will try to use prepare/
complete callback until better options are available.

-- 
Thanks,
Sylwester