ASoC: Intel: cht_bsw_rt5645: Add quirk for boards using pmc_plt_clk_0
diff mbox series

Message ID 20190916071511.5909-1-sammc@chromium.org
State New
Headers show
Series
  • ASoC: Intel: cht_bsw_rt5645: Add quirk for boards using pmc_plt_clk_0
Related show

Commit Message

Sam McNally Sept. 16, 2019, 7:15 a.m. UTC
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.

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

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

Comments

Pierre-Louis Bossart Sept. 16, 2019, 3:02 p.m. UTC | #1
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?

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.

> +	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);
>   	}
>   
>
Sam McNally Sept. 17, 2019, 5:46 a.m. UTC | #2
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/soc/intel/boards/cht_bsw_rt5645.c?h=for-5.4&id=fca11622d600228bec405456f41590155b3a3eca#n630
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);
> >       }
> >
> >
>

Patch
diff mbox series

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