[PATCH 0/5] BYT/CHT-Realtek remaining fixes

[PATCH 0/5] BYT/CHT-Realtek remaining fixes

[Pierre-Louis Bossart]

This last batch for the week is about simplification of the
MCLK-related code for byt/cht-realtek machine drivers and fixes for
rt5651. This is mostly a v2 of patches I submitted earlier, with
improvements suggested by Andy Shevchenko, a fixed mistake on quirk
management flagged by Vinod Koul and better commit messages. No new
functionality otherwise.

Pierre-Louis Bossart (5):
  ASoC: Intel: boards: use devm_clk_get() unconditionally
  ASoC: Intel: bytcr-rt5651: fix capture routes
  ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks
  ASoC: Intel: bytcr_rt5651: filter codec name
  ASoC: Intel: bytcr_rt5640: simplify MCLK quirk tests

 sound/soc/intel/boards/bytcr_rt5640.c   |   8 +-
 sound/soc/intel/boards/bytcr_rt5651.c   | 277 ++++++++++++++++++++++++++++----
 sound/soc/intel/boards/cht_bsw_rt5645.c |  14 +-
 sound/soc/intel/boards/cht_bsw_rt5672.c |  27 +---
 4 files changed, 265 insertions(+), 61 deletions(-)

-- 
2.9.3

[PATCH 1/5] ASoC: Intel: boards: use devm_clk_get() unconditionally

[Pierre-Louis Bossart]

The clock framework was only used in Baytrail, on Cherrytrail
the firmware takes care of the MCLK/plt_clk_3.

With the fix in 'commit d31fd43c0f9a
("clk: x86: Do not gate clocks enabled by the firmware")'

the firmware-managed clocks are not impacted by enable/disable
requests make at the driver level, and the rates are identical.

Remove all checks for Baytrail and use devm_clk_get()
unconditionally. Tested on Asus T100HA (CHT) and Asus T100TAF (BYT)

Note that the RT5640 and RT5645 machine drivers need to keep some
checks for Valleyview to check for Baytrail-CR.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c   |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5645.c | 14 ++++++--------
 sound/soc/intel/boards/cht_bsw_rt5672.c | 27 ++++++---------------------
 3 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 4a76b09..15b1e29 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -891,7 +891,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 			byt_rt5640_cpu_dai_name;
 	}
 
-	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && (is_valleyview())) {
+	if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
 		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index 5bcde01..d553e2b 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -682,14 +682,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 			cht_rt5645_cpu_dai_name;
 	}
 
-	if (is_valleyview()) {
-		drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-		if (IS_ERR(drv->mclk)) {
-			dev_err(&pdev->dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-				PTR_ERR(drv->mclk));
-			return PTR_ERR(drv->mclk);
-		}
+	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	if (IS_ERR(drv->mclk)) {
+		dev_err(&pdev->dev,
+			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+			PTR_ERR(drv->mclk));
+		return PTR_ERR(drv->mclk);
 	}
 
 	snd_soc_card_set_drvdata(card, drv);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c
index b13d6222..f799b76 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5672.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5672.c
@@ -20,7 +20,6 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
-#include <asm/cpu_device_id.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -398,18 +397,6 @@ static struct snd_soc_card snd_soc_card_cht = {
 	.resume_post = cht_resume_post,
 };
 
-static bool is_valleyview(void)
-{
-	static const struct x86_cpu_id cpu_ids[] = {
-		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
-		{}
-	};
-
-	if (!x86_match_cpu(cpu_ids))
-		return false;
-	return true;
-}
-
 #define RT5672_I2C_DEFAULT	"i2c-10EC5670:00"
 
 static int snd_cht_mc_probe(struct platform_device *pdev)
@@ -443,14 +430,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (is_valleyview()) {
-		drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-		if (IS_ERR(drv->mclk)) {
-			dev_err(&pdev->dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-				PTR_ERR(drv->mclk));
-			return PTR_ERR(drv->mclk);
-		}
+	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	if (IS_ERR(drv->mclk)) {
+		dev_err(&pdev->dev,
+			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+			PTR_ERR(drv->mclk));
+		return PTR_ERR(drv->mclk);
 	}
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-- 
2.9.3

Applied "ASoC: Intel: boards: use devm_clk_get() unconditionally" to the asoc tree

[Mark Brown]

The patch

   ASoC: Intel: boards: use devm_clk_get() unconditionally

has been applied to the asoc tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 7735bce05a9c0bb0eb0f08c9002d65843a7c5798 Mon Sep 17 00:00:00 2001
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Date: Fri, 8 Sep 2017 12:43:52 -0500
Subject: [PATCH] ASoC: Intel: boards: use devm_clk_get() unconditionally

The clock framework was only used in Baytrail, on Cherrytrail
the firmware takes care of the MCLK/plt_clk_3.

With the fix in 'commit d31fd43c0f9a
("clk: x86: Do not gate clocks enabled by the firmware")'

the firmware-managed clocks are not impacted by enable/disable
requests make at the driver level, and the rates are identical.

Remove all checks for Baytrail and use devm_clk_get()
unconditionally. Tested on Asus T100HA (CHT) and Asus T100TAF (BYT)

Note that the RT5640 and RT5645 machine drivers need to keep some
checks for Valleyview to check for Baytrail-CR.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Acked-By: Vinod Koul <vinod.koul@intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/bytcr_rt5640.c   |  2 +-
 sound/soc/intel/boards/cht_bsw_rt5645.c | 14 ++++++--------
 sound/soc/intel/boards/cht_bsw_rt5672.c | 27 ++++++---------------------
 3 files changed, 13 insertions(+), 30 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 4a76b099a508..15b1e292b0c3 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -891,7 +891,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 			byt_rt5640_cpu_dai_name;
 	}
 
-	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && (is_valleyview())) {
+	if (byt_rt5640_quirk & BYT_RT5640_MCLK_EN) {
 		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
 		if (IS_ERR(priv->mclk)) {
 			ret_val = PTR_ERR(priv->mclk);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c
index 5bcde01d15e6..d553e2b67c92 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5645.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5645.c
@@ -682,14 +682,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 			cht_rt5645_cpu_dai_name;
 	}
 
-	if (is_valleyview()) {
-		drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-		if (IS_ERR(drv->mclk)) {
-			dev_err(&pdev->dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-				PTR_ERR(drv->mclk));
-			return PTR_ERR(drv->mclk);
-		}
+	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	if (IS_ERR(drv->mclk)) {
+		dev_err(&pdev->dev,
+			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+			PTR_ERR(drv->mclk));
+		return PTR_ERR(drv->mclk);
 	}
 
 	snd_soc_card_set_drvdata(card, drv);
diff --git a/sound/soc/intel/boards/cht_bsw_rt5672.c b/sound/soc/intel/boards/cht_bsw_rt5672.c
index f597d5582223..a0e60bc1f84f 100644
--- a/sound/soc/intel/boards/cht_bsw_rt5672.c
+++ b/sound/soc/intel/boards/cht_bsw_rt5672.c
@@ -20,7 +20,6 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
-#include <asm/cpu_device_id.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -394,18 +393,6 @@ static struct snd_soc_card snd_soc_card_cht = {
 	.resume_post = cht_resume_post,
 };
 
-static bool is_valleyview(void)
-{
-	static const struct x86_cpu_id cpu_ids[] = {
-		{ X86_VENDOR_INTEL, 6, 55 }, /* Valleyview, Bay Trail */
-		{}
-	};
-
-	if (!x86_match_cpu(cpu_ids))
-		return false;
-	return true;
-}
-
 #define RT5672_I2C_DEFAULT	"i2c-10EC5670:00"
 
 static int snd_cht_mc_probe(struct platform_device *pdev)
@@ -439,14 +426,12 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		}
 	}
 
-	if (is_valleyview()) {
-		drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
-		if (IS_ERR(drv->mclk)) {
-			dev_err(&pdev->dev,
-				"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
-				PTR_ERR(drv->mclk));
-			return PTR_ERR(drv->mclk);
-		}
+	drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+	if (IS_ERR(drv->mclk)) {
+		dev_err(&pdev->dev,
+			"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+			PTR_ERR(drv->mclk));
+		return PTR_ERR(drv->mclk);
 	}
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-- 
2.14.1

[PATCH 2/5] ASoC: Intel: bytcr-rt5651: fix capture routes

[Pierre-Louis Bossart]

There is only one dmic path and the routes were not added.
Probably a copy-paste mistake when initially creating the
file

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 4a3516b..441f735 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -54,12 +54,9 @@ static const struct snd_soc_dapm_route byt_rt5651_audio_map[] = {
 	{"Speaker", NULL, "LOUTR"},
 };
 
-static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic1_map[] = {
-	{"DMIC1", NULL, "Internal Mic"},
-};
-
-static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic2_map[] = {
-	{"DMIC2", NULL, "Internal Mic"},
+static const struct snd_soc_dapm_route byt_rt5651_intmic_dmic_map[] = {
+	{"DMIC L1", NULL, "Internal Mic"},
+	{"DMIC R1", NULL, "Internal Mic"},
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_map[] = {
@@ -133,14 +130,13 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 		custom_map = byt_rt5651_intmic_in1_map;
 		num_routes = ARRAY_SIZE(byt_rt5651_intmic_in1_map);
 		break;
-	case BYT_RT5651_DMIC2_MAP:
-		custom_map = byt_rt5651_intmic_dmic2_map;
-		num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic2_map);
-		break;
 	default:
-		custom_map = byt_rt5651_intmic_dmic1_map;
-		num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic1_map);
+		custom_map = byt_rt5651_intmic_dmic_map;
+		num_routes = ARRAY_SIZE(byt_rt5651_intmic_dmic_map);
 	}
+	ret = snd_soc_dapm_add_routes(&card->dapm, custom_map, num_routes);
+	if (ret)
+		return ret;
 
 	ret = snd_soc_add_card_controls(card, byt_rt5651_controls,
 					ARRAY_SIZE(byt_rt5651_controls));
-- 
2.9.3

[PATCH 3/5] ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks

[Pierre-Louis Bossart]

Same as for other codecs, enable MCLK by default. When it is not
present, e.g. on MinnowBoard B3 since it's not routed on the LSE
connector, we fall back to blck-based clocking.

The DMIC quirks are also fixed, there is a single DMIC input of the
codec

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 231 +++++++++++++++++++++++++++++++---
 1 file changed, 215 insertions(+), 16 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 441f735..224e608 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -24,6 +24,9 @@
 #include <linux/device.h>
 #include <linux/dmi.h>
 #include <linux/slab.h>
+#include <asm/cpu_device_id.h>
+#include <asm/platform_sst_audio.h>
+#include <linux/clk.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -31,14 +34,119 @@
 #include "../../codecs/rt5651.h"
 #include "../atom/sst-atom-controls.h"
 
+enum {
+	BYT_RT5651_DMIC_MAP,
+	BYT_RT5651_IN1_MAP,
+};
+
+#define BYT_RT5651_MAP(quirk)	((quirk) & 0xff)
+#define BYT_RT5651_DMIC_EN	BIT(16)
+#define BYT_RT5651_MCLK_EN	BIT(17)
+#define BYT_RT5651_MCLK_25MHZ	BIT(18)
+
+struct byt_rt5651_private {
+	struct clk *mclk;
+};
+
+static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC_MAP |
+					BYT_RT5651_DMIC_EN |
+					BYT_RT5651_MCLK_EN;
+
+static void log_quirks(struct device *dev)
+{
+	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_DMIC_MAP)
+		dev_info(dev, "quirk DMIC_MAP enabled");
+	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)
+		dev_info(dev, "quirk IN1_MAP enabled");
+	if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
+		dev_info(dev, "quirk DMIC enabled");
+	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
+		dev_info(dev, "quirk MCLK_EN enabled");
+	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
+		dev_info(dev, "quirk MCLK_25MHZ enabled");
+}
+
+#define BYT_CODEC_DAI1	"rt5651-aif1"
+
+static inline struct snd_soc_dai *byt_get_codec_dai(struct snd_soc_card *card)
+{
+	struct snd_soc_pcm_runtime *rtd;
+
+	list_for_each_entry(rtd, &card->rtd_list, list) {
+		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI1,
+			     strlen(BYT_CODEC_DAI1)))
+			return rtd->codec_dai;
+	}
+	return NULL;
+}
+
+static int platform_clock_control(struct snd_soc_dapm_widget *w,
+				  struct snd_kcontrol *k, int  event)
+{
+	struct snd_soc_dapm_context *dapm = w->dapm;
+	struct snd_soc_card *card = dapm->card;
+	struct snd_soc_dai *codec_dai;
+	struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card);
+	int ret;
+
+	codec_dai = byt_get_codec_dai(card);
+	if (!codec_dai) {
+		dev_err(card->dev,
+			"Codec dai not found; Unable to set platform clock\n");
+		return -EIO;
+	}
+
+	if (SND_SOC_DAPM_EVENT_ON(event)) {
+		if (priv->mclk) {
+			ret = clk_prepare_enable(priv->mclk);
+			if (ret < 0) {
+				dev_err(card->dev,
+					"could not configure MCLK state");
+				return ret;
+			}
+		}
+		ret = snd_soc_dai_set_sysclk(codec_dai, RT5651_SCLK_S_PLL1,
+					     48000 * 512,
+					     SND_SOC_CLOCK_IN);
+	} else {
+		/*
+		 * Set codec clock source to internal clock before
+		 * turning off the platform clock. Codec needs clock
+		 * for Jack detection and button press
+		 */
+		ret = snd_soc_dai_set_sysclk(codec_dai, RT5651_SCLK_S_RCCLK,
+					     48000 * 512,
+					     SND_SOC_CLOCK_IN);
+		if (!ret)
+			if (priv->mclk)
+				clk_disable_unprepare(priv->mclk);
+	}
+
+	if (ret < 0) {
+		dev_err(card->dev, "can't set codec sysclk: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
 static const struct snd_soc_dapm_widget byt_rt5651_widgets[] = {
 	SND_SOC_DAPM_HP("Headphone", NULL),
 	SND_SOC_DAPM_MIC("Headset Mic", NULL),
 	SND_SOC_DAPM_MIC("Internal Mic", NULL),
 	SND_SOC_DAPM_SPK("Speaker", NULL),
+	SND_SOC_DAPM_SUPPLY("Platform Clock", SND_SOC_NOPM, 0, 0,
+			    platform_clock_control, SND_SOC_DAPM_PRE_PMU |
+			    SND_SOC_DAPM_POST_PMD),
+
 };
 
 static const struct snd_soc_dapm_route byt_rt5651_audio_map[] = {
+	{"Headphone", NULL, "Platform Clock"},
+	{"Headset Mic", NULL, "Platform Clock"},
+	{"Internal Mic", NULL, "Platform Clock"},
+	{"Speaker", NULL, "Platform Clock"},
+
 	{"AIF1 Playback", NULL, "ssp2 Tx"},
 	{"ssp2 Tx", NULL, "codec_out0"},
 	{"ssp2 Tx", NULL, "codec_out1"},
@@ -64,18 +172,6 @@ static const struct snd_soc_dapm_route byt_rt5651_intmic_in1_map[] = {
 	{"IN1P", NULL, "Internal Mic"},
 };
 
-enum {
-	BYT_RT5651_DMIC1_MAP,
-	BYT_RT5651_DMIC2_MAP,
-	BYT_RT5651_IN1_MAP,
-};
-
-#define BYT_RT5651_MAP(quirk)	((quirk) & 0xff)
-#define BYT_RT5651_DMIC_EN	BIT(16)
-
-static unsigned long byt_rt5651_quirk = BYT_RT5651_DMIC1_MAP |
-					BYT_RT5651_DMIC_EN;
-
 static const struct snd_kcontrol_new byt_rt5651_controls[] = {
 	SOC_DAPM_PIN_SWITCH("Headphone"),
 	SOC_DAPM_PIN_SWITCH("Headset Mic"),
@@ -100,9 +196,26 @@ static int byt_rt5651_aif1_hw_params(struct snd_pcm_substream *substream,
 		return ret;
 	}
 
-	ret = snd_soc_dai_set_pll(codec_dai, 0, RT5651_PLL1_S_BCLK1,
-				  params_rate(params) * 50,
-				  params_rate(params) * 512);
+	if (!(byt_rt5651_quirk & BYT_RT5651_MCLK_EN)) {
+		/* 2x25 bit slots on SSP2 */
+		ret = snd_soc_dai_set_pll(codec_dai, 0,
+					RT5651_PLL1_S_BCLK1,
+					params_rate(params) * 50,
+					params_rate(params) * 512);
+	} else {
+		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ) {
+			ret = snd_soc_dai_set_pll(codec_dai, 0,
+						RT5651_PLL1_S_MCLK,
+						25000000,
+						params_rate(params) * 512);
+		} else {
+			ret = snd_soc_dai_set_pll(codec_dai, 0,
+						RT5651_PLL1_S_MCLK,
+						19200000,
+						params_rate(params) * 512);
+		}
+	}
+
 	if (ret < 0) {
 		dev_err(rtd->dev, "can't set codec pll: %d\n", ret);
 		return ret;
@@ -111,7 +224,22 @@ static int byt_rt5651_aif1_hw_params(struct snd_pcm_substream *substream,
 	return 0;
 }
 
+static int byt_rt5651_quirk_cb(const struct dmi_system_id *id)
+{
+	byt_rt5651_quirk = (unsigned long)id->driver_data;
+	return 1;
+}
+
 static const struct dmi_system_id byt_rt5651_quirk_table[] = {
+	{
+		.callback = byt_rt5651_quirk_cb,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Max B3 PLATFORM"),
+		},
+		.driver_data = (unsigned long *)(BYT_RT5651_DMIC_MAP |
+						 BYT_RT5651_DMIC_EN),
+	},
 	{}
 };
 
@@ -120,11 +248,11 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 	int ret;
 	struct snd_soc_card *card = runtime->card;
 	const struct snd_soc_dapm_route *custom_map;
+	struct byt_rt5651_private *priv = snd_soc_card_get_drvdata(card);
 	int num_routes;
 
 	card->dapm.idle_bias_off = true;
 
-	dmi_check_system(byt_rt5651_quirk_table);
 	switch (BYT_RT5651_MAP(byt_rt5651_quirk)) {
 	case BYT_RT5651_IN1_MAP:
 		custom_map = byt_rt5651_intmic_in1_map;
@@ -147,6 +275,30 @@ static int byt_rt5651_init(struct snd_soc_pcm_runtime *runtime)
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
 
+	if (priv->mclk) {
+		/*
+		 * The firmware might enable the clock at
+		 * boot (this information may or may not
+		 * be reflected in the enable clock register).
+		 * To change the rate we must disable the clock
+		 * first to cover these cases. Due to common
+		 * clock framework restrictions that do not allow
+		 * to disable a clock that has not been enabled,
+		 * we need to enable the clock first.
+		 */
+		ret = clk_prepare_enable(priv->mclk);
+		if (!ret)
+			clk_disable_unprepare(priv->mclk);
+
+		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
+			ret = clk_set_rate(priv->mclk, 25000000);
+		else
+			ret = clk_set_rate(priv->mclk, 19200000);
+
+		if (ret)
+			dev_err(card->dev, "unable to set MCLK rate\n");
+	}
+
 	return ret;
 }
 
@@ -295,10 +447,57 @@ static struct snd_soc_card byt_rt5651_card = {
 static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 {
 	int ret_val = 0;
+	struct byt_rt5651_private *priv;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
+	if (!priv)
+		return -ENOMEM;
 
 	/* register the soc card */
 	byt_rt5651_card.dev = &pdev->dev;
 
+	mach = byt_rt5651_card.dev->platform_data;
+	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
+
+	/* fix index of codec dai */
+	dai_index = MERR_DPCM_COMPR + 1;
+	for (i = 0; i < ARRAY_SIZE(byt_rt5651_dais); i++) {
+		if (!strcmp(byt_rt5651_dais[i].codec_name, "i2c-10EC5651:00")) {
+			dai_index = i;
+			break;
+		}
+	}
+
+	/* fixup codec name based on HID */
+	i2c_name = sst_acpi_find_name_from_hid(mach->id);
+	if (i2c_name != NULL) {
+		snprintf(byt_rt5651_codec_name, sizeof(byt_rt5651_codec_name),
+			"%s%s", "i2c-", i2c_name);
+
+		byt_rt5651_dais[dai_index].codec_name = byt_rt5651_codec_name;
+	}
+
+	/* check quirks before creating card */
+	dmi_check_system(byt_rt5651_quirk_table);
+	log_quirks(&pdev->dev);
+
+	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN) {
+		priv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
+		if (IS_ERR(priv->mclk)) {
+			dev_err(&pdev->dev,
+				"Failed to get MCLK from pmc_plt_clk_3: %ld\n",
+				PTR_ERR(priv->mclk));
+			/*
+			 * Fall back to bit clock usage for -ENOENT (clock not
+			 * available likely due to missing dependencies), bail
+			 * for all other errors, including -EPROBE_DEFER
+			 */
+			if (ret_val != -ENOENT)
+				return ret_val;
+			byt_rt5651_quirk &= ~BYT_RT5651_MCLK_EN;
+		}
+	}
+
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &byt_rt5651_card);
 
 	if (ret_val) {
-- 
2.9.3

Re: [PATCH 3/5] ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks

[Andy Shevchenko]

On Fri, 2017-09-08 at 12:43 -0500, Pierre-Louis Bossart wrote:
> Same as for other codecs, enable MCLK by default. When it is not
> present, e.g. on MinnowBoard B3 since it's not routed on the LSE
> connector, we fall back to blck-based clocking.
> 
> The DMIC quirks are also fixed, there is a single DMIC input of the
> codec

>  #include <linux/device.h>
>  #include <linux/dmi.h>
>  #include <linux/slab.h>
> +#include <asm/cpu_device_id.h>
> +#include <asm/platform_sst_audio.h>

> +#include <linux/clk.h>

Just a nit: I would rather squeeze this to somewhere above device
(alphabetical ordering)

>  #include <sound/pcm.h>
>  #include <sound/pcm_params.h>
>  #include <sound/soc.h>

> +#define BYT_RT5651_MAP(quirk)	((quirk) & 0xff)

GENMASK(7, 0) ?

> +#define BYT_RT5651_DMIC_EN	BIT(16)
> +#define BYT_RT5651_MCLK_EN	BIT(17)
> +#define BYT_RT5651_MCLK_25MHZ	BIT(18)

> +static void log_quirks(struct device *dev)
> +{
> +	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_DMIC_MAP)
> +		dev_info(dev, "quirk DMIC_MAP enabled");
> +	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)
> +		dev_info(dev, "quirk IN1_MAP enabled");
> +	if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
> +		dev_info(dev, "quirk DMIC enabled");
> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
> +		dev_info(dev, "quirk MCLK_EN enabled");
> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> +		dev_info(dev, "quirk MCLK_25MHZ enabled");
> +}
> +
> +#define BYT_CODEC_DAI1	"rt5651-aif1"
> +
> +static inline struct snd_soc_dai *byt_get_codec_dai(struct
> snd_soc_card *card)
> +{
> +	struct snd_soc_pcm_runtime *rtd;
> +
> +	list_for_each_entry(rtd, &card->rtd_list, list) {
> +		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI1,
> +			     strlen(BYT_CODEC_DAI1)))

Still same comment about str_n_cmp vs. strcmp() in this case.

(strlen() also can be calculated once size_t dai1_len = strlen(...); )

> +			return rtd->codec_dai;
> +	}
> +	return NULL;


> +	if (SND_SOC_DAPM_EVENT_ON(event)) {

> +		if (priv->mclk) {

Do we need this check?

I'm not sure I have read a comment why this is left as in initial
series.

> 				     SND_SOC_CLOCK_IN);
> +		if (!ret)
> +			if (priv->mclk)

Ditto ?

> +				clk_disable_unprepare(priv->mclk);
> +	}

> 

>  static const struct dmi_system_id byt_rt5651_quirk_table[] = {
> +	{
> +		.callback = byt_rt5651_quirk_cb,
> +		.matches = {
> +			DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"),
> +			DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Max
> B3 PLATFORM"),
> +		},
> +		.driver_data = (unsigned long *)(BYT_RT5651_DMIC_MAP
> |

Shouldn't be just (void *)  [and maybe fit one line]?

> +						 BYT_RT5651_DMIC_EN),

>  
> +	if (priv->mclk) {

Same question as above.

> +		/*
> +		 * The firmware might enable the clock at
> +		 * boot (this information may or may not
> +		 * be reflected in the enable clock register).
> +		 * To change the rate we must disable the clock
> +		 * first to cover these cases. Due to common
> +		 * clock framework restrictions that do not allow
> +		 * to disable a clock that has not been enabled,
> +		 * we need to enable the clock first.
> +		 */
> +		ret = clk_prepare_enable(priv->mclk);
> +		if (!ret)
> +			clk_disable_unprepare(priv->mclk);
> +
> +		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
> +			ret = clk_set_rate(priv->mclk, 25000000);
> +		else
> +			ret = clk_set_rate(priv->mclk, 19200000);
> +
> +		if (ret)
> +			dev_err(card->dev, "unable to set MCLK
> rate\n");
> +	}
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 3/5] ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks

[Pierre-Louis Bossart]

On 9/18/17 2:10 AM, Andy Shevchenko wrote:
> On Fri, 2017-09-08 at 12:43 -0500, Pierre-Louis Bossart wrote:
>> Same as for other codecs, enable MCLK by default. When it is not
>> present, e.g. on MinnowBoard B3 since it's not routed on the LSE
>> connector, we fall back to blck-based clocking.
>>
>> The DMIC quirks are also fixed, there is a single DMIC input of the
>> codec
> 
>>   #include <linux/device.h>
>>   #include <linux/dmi.h>
>>   #include <linux/slab.h>
>> +#include <asm/cpu_device_id.h>
>> +#include <asm/platform_sst_audio.h>
> 
>> +#include <linux/clk.h>
> 
> Just a nit: I would rather squeeze this to somewhere above device
> (alphabetical ordering)
> 
>>   #include <sound/pcm.h>
>>   #include <sound/pcm_params.h>
>>   #include <sound/soc.h>
> 
>> +#define BYT_RT5651_MAP(quirk)	((quirk) & 0xff)
> 
> GENMASK(7, 0) ?

again a copy-paste from rt5640, I don't mind changing this but it should 
be done in both drivers for alignment.

> 
>> +#define BYT_RT5651_DMIC_EN	BIT(16)
>> +#define BYT_RT5651_MCLK_EN	BIT(17)
>> +#define BYT_RT5651_MCLK_25MHZ	BIT(18)
> 
>> +static void log_quirks(struct device *dev)
>> +{
>> +	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_DMIC_MAP)
>> +		dev_info(dev, "quirk DMIC_MAP enabled");
>> +	if (BYT_RT5651_MAP(byt_rt5651_quirk) == BYT_RT5651_IN1_MAP)
>> +		dev_info(dev, "quirk IN1_MAP enabled");
>> +	if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
>> +		dev_info(dev, "quirk DMIC enabled");
>> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_EN)
>> +		dev_info(dev, "quirk MCLK_EN enabled");
>> +	if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
>> +		dev_info(dev, "quirk MCLK_25MHZ enabled");
>> +}
>> +
>> +#define BYT_CODEC_DAI1	"rt5651-aif1"
>> +
>> +static inline struct snd_soc_dai *byt_get_codec_dai(struct
>> snd_soc_card *card)
>> +{
>> +	struct snd_soc_pcm_runtime *rtd;
>> +
>> +	list_for_each_entry(rtd, &card->rtd_list, list) {
>> +		if (!strncmp(rtd->codec_dai->name, BYT_CODEC_DAI1,
>> +			     strlen(BYT_CODEC_DAI1)))
> 
> Still same comment about str_n_cmp vs. strcmp() in this case.
> 
> (strlen() also can be calculated once size_t dai1_len = strlen(...); )

same, the same code is used in multiple places so I'd like to fix it in 
one step later.

> 
>> +			return rtd->codec_dai;
>> +	}
>> +	return NULL;
> 
> 
>> +	if (SND_SOC_DAPM_EVENT_ON(event)) {
> 
>> +		if (priv->mclk) {
> 
> Do we need this check?
> 
> I'm not sure I have read a comment why this is left as in initial
> series.

yes, we need it. I use it to check if we are using the MCLK or the bclk 
for the PLL reference. In the latter case there is no point in 
programming the PMC clocks. You might argue this is harmless but I like 
to avoid unnecessary programming sequences.

> 
>> 				     SND_SOC_CLOCK_IN);
>> +		if (!ret)
>> +			if (priv->mclk)
> 
> Ditto ?
> 
>> +				clk_disable_unprepare(priv->mclk);
>> +	}
> 
>>
> 
>>   static const struct dmi_system_id byt_rt5651_quirk_table[] = {
>> +	{
>> +		.callback = byt_rt5651_quirk_cb,
>> +		.matches = {
>> +			DMI_MATCH(DMI_SYS_VENDOR, "Circuitco"),
>> +			DMI_MATCH(DMI_PRODUCT_NAME, "Minnowboard Max
>> B3 PLATFORM"),
>> +		},
>> +		.driver_data = (unsigned long *)(BYT_RT5651_DMIC_MAP
>> |
> 
> Shouldn't be just (void *)  [and maybe fit one line]?

we use this in every single driver, not sure there is a point in making 
this pretty?

> 
>> +						 BYT_RT5651_DMIC_EN),
> 
>>   
>> +	if (priv->mclk) {
> 
> Same question as above.
> 
>> +		/*
>> +		 * The firmware might enable the clock at
>> +		 * boot (this information may or may not
>> +		 * be reflected in the enable clock register).
>> +		 * To change the rate we must disable the clock
>> +		 * first to cover these cases. Due to common
>> +		 * clock framework restrictions that do not allow
>> +		 * to disable a clock that has not been enabled,
>> +		 * we need to enable the clock first.
>> +		 */
>> +		ret = clk_prepare_enable(priv->mclk);
>> +		if (!ret)
>> +			clk_disable_unprepare(priv->mclk);
>> +
>> +		if (byt_rt5651_quirk & BYT_RT5651_MCLK_25MHZ)
>> +			ret = clk_set_rate(priv->mclk, 25000000);
>> +		else
>> +			ret = clk_set_rate(priv->mclk, 19200000);
>> +
>> +		if (ret)
>> +			dev_err(card->dev, "unable to set MCLK
>> rate\n");
>> +	}
>>
> 

[PATCH 4/5] ASoC: Intel: bytcr_rt5651: filter codec name

[Pierre-Louis Bossart]

Use same fix as other codecs to work around BIOS/ACPI issues

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index 224e608..e4ece0b9 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -33,6 +33,7 @@
 #include <sound/jack.h>
 #include "../../codecs/rt5651.h"
 #include "../atom/sst-atom-controls.h"
+#include "../common/sst-acpi.h"
 
 enum {
 	BYT_RT5651_DMIC_MAP,
@@ -444,9 +445,15 @@ static struct snd_soc_card byt_rt5651_card = {
 	.fully_routed = true,
 };
 
+static char byt_rt5651_codec_name[16]; /* i2c-<HID>:00 with HID being 8 chars */
+
 static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 {
 	int ret_val = 0;
+	struct sst_acpi_mach *mach;
+	const char *i2c_name = NULL;
+	int i;
+	int dai_index;
 	struct byt_rt5651_private *priv;
 
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_ATOMIC);
@@ -455,6 +462,25 @@ static int snd_byt_rt5651_mc_probe(struct platform_device *pdev)
 
 	/* register the soc card */
 	byt_rt5651_card.dev = &pdev->dev;
+	mach = byt_rt5651_card.dev->platform_data;
+
+	/* fix index of codec dai */
+	dai_index = MERR_DPCM_COMPR + 1;
+	for (i = 0; i < ARRAY_SIZE(byt_rt5651_dais); i++) {
+		if (!strcmp(byt_rt5651_dais[i].codec_name, "i2c-10EC5651:00")) {
+			dai_index = i;
+			break;
+		}
+	}
+
+	/* fixup codec name based on HID */
+	i2c_name = sst_acpi_find_name_from_hid(mach->id);
+	if (i2c_name != NULL) {
+		snprintf(byt_rt5651_codec_name, sizeof(byt_rt5651_codec_name),
+			"%s%s", "i2c-", i2c_name);
+
+		byt_rt5651_dais[dai_index].codec_name = byt_rt5651_codec_name;
+	}
 
 	mach = byt_rt5651_card.dev->platform_data;
 	snd_soc_card_set_drvdata(&byt_rt5651_card, priv);
-- 
2.9.3

Re: [PATCH 4/5] ASoC: Intel: bytcr_rt5651: filter codec name

[Andy Shevchenko]

On Fri, 2017-09-08 at 12:43 -0500, Pierre-Louis Bossart wrote:
> Use same fix as other codecs to work around BIOS/ACPI issues
> 
> 

>  {
>  	int ret_val = 0;
> +	struct sst_acpi_mach *mach;
> +	const char *i2c_name = NULL;
> +	int i;
> +	int dai_index;
>  	struct byt_rt5651_private *priv;

I would rather put variables in reversed xmas tree order or close to it.

+
> +	/* fixup codec name based on HID */
> +	i2c_name = sst_acpi_find_name_from_hid(mach->id);

> +	if (i2c_name != NULL) {

if (i2c_name) { ?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 4/5] ASoC: Intel: bytcr_rt5651: filter codec name

[Pierre-Louis Bossart]

On 9/18/17 2:13 AM, Andy Shevchenko wrote:
> On Fri, 2017-09-08 at 12:43 -0500, Pierre-Louis Bossart wrote:
>> Use same fix as other codecs to work around BIOS/ACPI issues
>>
>>
> 
>>   {
>>   	int ret_val = 0;
>> +	struct sst_acpi_mach *mach;
>> +	const char *i2c_name = NULL;
>> +	int i;
>> +	int dai_index;
>>   	struct byt_rt5651_private *priv;
> 
> I would rather put variables in reversed xmas tree order or close to it.

yes you are right, this looks awful. I'll send a cleaned-up series.

> 
> +
>> +	/* fixup codec name based on HID */
>> +	i2c_name = sst_acpi_find_name_from_hid(mach->id);
> 
>> +	if (i2c_name != NULL) {
> 
> if (i2c_name) { ?
> 

[PATCH 5/5] ASoC: Intel: bytcr_rt5640: simplify MCLK quirk tests

[Pierre-Louis Bossart]

remove redundant tests to check MCLK (align with other
machine drivers). some checks remain since when the MCLK is
disabled we fall back to using the bclk as PLL reference

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 15b1e29..7cee09d 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -178,7 +178,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	}
 
 	if (SND_SOC_DAPM_EVENT_ON(event)) {
-		if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
+		if (priv->mclk) {
 			ret = clk_prepare_enable(priv->mclk);
 			if (ret < 0) {
 				dev_err(card->dev,
@@ -199,7 +199,7 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 					     48000 * 512,
 					     SND_SOC_CLOCK_IN);
 		if (!ret) {
-			if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk)
+			if (priv->mclk)
 				clk_disable_unprepare(priv->mclk);
 		}
 	}
@@ -549,7 +549,7 @@ static int byt_rt5640_init(struct snd_soc_pcm_runtime *runtime)
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Headphone");
 	snd_soc_dapm_ignore_suspend(&card->dapm, "Speaker");
 
-	if ((byt_rt5640_quirk & BYT_RT5640_MCLK_EN) && priv->mclk) {
+	if (priv->mclk) {
 		/*
 		 * The firmware might enable the clock at
 		 * boot (this information may or may not
-- 
2.9.3

Re: [PATCH 0/5] BYT/CHT-Realtek remaining fixes

[Vinod Koul]

On Fri, Sep 08, 2017 at 12:43:51PM -0500, Pierre-Louis Bossart wrote:
> This last batch for the week is about simplification of the
> MCLK-related code for byt/cht-realtek machine drivers and fixes for
> rt5651. This is mostly a v2 of patches I submitted earlier, with
> improvements suggested by Andy Shevchenko, a fixed mistake on quirk
> management flagged by Vinod Koul and better commit messages. No new
> functionality otherwise.

Acked-By: Vinod Koul <vinod.koul@intel.com>

> 
> Pierre-Louis Bossart (5):
>   ASoC: Intel: boards: use devm_clk_get() unconditionally
>   ASoC: Intel: bytcr-rt5651: fix capture routes
>   ASoC: Intel: bytcr_rt5651: add MCLK support and fix quirks
>   ASoC: Intel: bytcr_rt5651: filter codec name
>   ASoC: Intel: bytcr_rt5640: simplify MCLK quirk tests
> 
>  sound/soc/intel/boards/bytcr_rt5640.c   |   8 +-
>  sound/soc/intel/boards/bytcr_rt5651.c   | 277 ++++++++++++++++++++++++++++----
>  sound/soc/intel/boards/cht_bsw_rt5645.c |  14 +-
>  sound/soc/intel/boards/cht_bsw_rt5672.c |  27 +---
>  4 files changed, 265 insertions(+), 61 deletions(-)
> 
> -- 
> 2.9.3
> 

-- 
~Vinod