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";elsemclk_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); }