[alsa-devel] [PATCH 0/5] BYT/CHT-Realtek remaining fixes
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(-)
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);
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);
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));
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) {
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 MaxB3 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);elseret = clk_set_rate(priv->mclk, 19200000);if (ret)dev_err(card->dev, "unable to set MCLKrate\n");
- }
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 MaxB3 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);elseret = clk_set_rate(priv->mclk, 19200000);if (ret)dev_err(card->dev, "unable to set MCLKrate\n");
- }
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);
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) { ?
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) { ?
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
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
participants (4)
-
Andy Shevchenko -
Mark Brown -
Pierre-Louis Bossart -
Vinod Koul