On Tue, 17 Sep 2019 at 01:02, Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com wrote:

On 9/16/19 2:15 AM, Sam McNally wrote:

...

As of commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL"), the cht_bsw_rt5645 driver needs to enable the clock it's using for the codec's mclk. It does this from commit 7735bce05a9c ("ASoC: Intel: boards: use devm_clk_get() unconditionally"), enabling pmc_plt_clk_3. However, Strago family Chromebooks use pmc_plt_clk_0 for the codec mclk, resulting in white noise with some digital microphones. Add a DMI-based quirk for Strago family Chromebooks to use pmc_plt_clk_0 instead.

Sounds good, thanks for the patch. You will need to Cc: maintainers (Takashi and Mark) if you want them to see your patches.

Maybe you should mention in the commit message that this mirrors the changes made in cht_bsw_max98090_ti?

Will do.

Also see more important comment below

...

Signed-off-by: Sam McNally sammc@chromium.org

sound/soc/intel/boards/cht_bsw_rt5645.c | 26 +++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_rt5645.c b/sound/soc/intel/boards/cht_bsw_rt5645.c index 8879c3be29d5..c68a5b85a4a0 100644 --- a/sound/soc/intel/boards/cht_bsw_rt5645.c +++ b/sound/soc/intel/boards/cht_bsw_rt5645.c @@ -48,6 +48,7 @@ struct cht_mc_private { #define CHT_RT5645_SSP2_AIF2 BIT(16) /* default is using AIF1 */ #define CHT_RT5645_SSP0_AIF1 BIT(17) #define CHT_RT5645_SSP0_AIF2 BIT(18) +#define CHT_RT5645_PMC_PLT_CLK_0 BIT(19)

static unsigned long cht_rt5645_quirk = 0;

@@ -59,6 +60,8 @@ static void log_quirks(struct device *dev) dev_info(dev, "quirk SSP0_AIF1 enabled"); if (cht_rt5645_quirk & CHT_RT5645_SSP0_AIF2) dev_info(dev, "quirk SSP0_AIF2 enabled");

• if (cht_rt5645_quirk & CHT_RT5645_PMC_PLT_CLK_0)
  

•         dev_info(dev, "quirk PMC_PLT_CLK_0 enabled");
  

  }

  static int platform_clock_control(struct snd_soc_dapm_widget *w,

@@ -226,15 +229,21 @@ static int cht_aif1_hw_params(struct snd_pcm_substream *substream, return 0; }

-/* uncomment when we have a real quirk static int cht_rt5645_quirk_cb(const struct dmi_system_id *id) { cht_rt5645_quirk = (unsigned long)id->driver_data; return 1; } -*/

static const struct dmi_system_id cht_rt5645_quirk_table[] = {

• {
  

•         /* Strago family Chromebooks */
  

•         .callback = cht_rt5645_quirk_cb,
  

•         .matches = {
  

•                 DMI_MATCH(DMI_PRODUCT_FAMILY, "Intel_Strago"),
  

•         },
  

•         .driver_data = (void *)CHT_RT5645_PMC_PLT_CLK_0,
  

• },
  {
  },
  

  };

@@ -526,6 +535,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev) int dai_index = 0; int ret_val = 0; int i;

• const char *mclk_name;
  
  drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
  if (!drv)
  

@@ -662,11 +672,15 @@ static int snd_cht_mc_probe(struct platform_device *pdev) if (ret_val) return ret_val;

• drv->mclk = devm_clk_get(&pdev->dev, "pmc_plt_clk_3");
  

• if (cht_rt5645_quirk & CHT_RT5645_PMC_PLT_CLK_0)
  

•         mclk_name = "pmc_plt_clk_0";
  

• else
  

•         mclk_name = "pmc_plt_clk_3";
  

Aren't you missing a call to dmi_first_match() to change the default value of this cht_rt5645_quirk variable?

The rest of the patch looks good but I don't see how the DMI info is actually used.

The existing dmi_check_system() call at https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git/tree/sound... uses the quirks table, calling the previously-commented-out callback to assign to the quirks global. I'll add a mention in the description.

...

• drv->mclk = devm_clk_get(&pdev->dev, mclk_name);
  if (IS_ERR(drv->mclk)) {
  

•         dev_err(&pdev->dev,
  

•                 "Failed to get MCLK from pmc_plt_clk_3: %ld\n",
  

•                 PTR_ERR(drv->mclk));
  

•         dev_err(&pdev->dev, "Failed to get MCLK from %s: %ld\n",
  

•                 mclk_name, PTR_ERR(drv->mclk)); >               return PTR_ERR(drv->mclk);
  }