All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH resend 0/3] ASoC: Intel: Misc. fixes
@ 2019-04-02 10:20 Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 1/3] ASoC: rt5651: Add support for active-high jack detect Hans de Goede
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Hans de Goede @ 2019-04-02 10:20 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang, Mark Brown
  Cc: Hans de Goede, alsa-devel

Hi Pierre-Louis, Mark,

These 3 patches (originally 2 different patch-sets) seem to have
fallen through the cracks, hence this resend.

Pierre-Louis, can you review these 3 patches please?

Regards,

Hans

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH resend 1/3] ASoC: rt5651: Add support for active-high jack detect
  2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
@ 2019-04-02 10:20 ` Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 2/3] ASoC: Intel: bytcr_rt5651: Add BYT_RT5651_JD_NOT_INV quirk Hans de Goede
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2019-04-02 10:20 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang, Mark Brown
  Cc: Hans de Goede, alsa-devel

Some boards use a jack-receptacle with a switch which reports the
jack-inserted status as active-high, rather then the standard active-low
reporting most jacks use.

This commit adds support for it. This is activated by a boolean
"realtek,jack-detect-not-inverted" device-property. The not-inverted
in the device-property name, rather then active-high, was chosen to keep
the device-property naming consistent with the rt5640 codec driver.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../devicetree/bindings/sound/rt5651.txt      |  5 ++
 sound/soc/codecs/rt5651.c                     | 47 ++++++++++++++++---
 sound/soc/codecs/rt5651.h                     |  1 +
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/rt5651.txt b/Documentation/devicetree/bindings/sound/rt5651.txt
index a41199a5cd79..56e736a1cba9 100644
--- a/Documentation/devicetree/bindings/sound/rt5651.txt
+++ b/Documentation/devicetree/bindings/sound/rt5651.txt
@@ -22,6 +22,11 @@ Optional properties:
   2: Use JD1_2 pin for jack-detect
   3: Use JD2 pin for jack-detect
 
+- realtek,jack-detect-not-inverted
+  bool. Normal jack-detect switches give an inverted (active-low) signal,
+  set this bool in the rare case you've a jack-detect switch which is not
+  inverted.
+
 - realtek,over-current-threshold-microamp
   u32, micbias over-current detection threshold in µA, valid values are
   600, 1500 and 2000µA.
diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c
index 29b2d60076b0..cb8252ff31cb 100644
--- a/sound/soc/codecs/rt5651.c
+++ b/sound/soc/codecs/rt5651.c
@@ -1645,7 +1645,10 @@ static bool rt5651_jack_inserted(struct snd_soc_component *component)
 		break;
 	}
 
-	return val == 0;
+	if (rt5651->jd_active_high)
+		return val != 0;
+	else
+		return val == 0;
 }
 
 /* Jack detect and button-press timings */
@@ -1868,20 +1871,47 @@ static void rt5651_enable_jack_detect(struct snd_soc_component *component,
 	case RT5651_JD1_1:
 		snd_soc_component_update_bits(component, RT5651_JD_CTRL2,
 			RT5651_JD_TRG_SEL_MASK, RT5651_JD_TRG_SEL_JD1_1);
-		snd_soc_component_update_bits(component, RT5651_IRQ_CTRL1,
-			RT5651_JD1_1_IRQ_EN, RT5651_JD1_1_IRQ_EN);
+		/* active-low is normal, set inv flag for active-high */
+		if (rt5651->jd_active_high)
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD1_1_IRQ_EN | RT5651_JD1_1_INV,
+				RT5651_JD1_1_IRQ_EN | RT5651_JD1_1_INV);
+		else
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD1_1_IRQ_EN | RT5651_JD1_1_INV,
+				RT5651_JD1_1_IRQ_EN);
 		break;
 	case RT5651_JD1_2:
 		snd_soc_component_update_bits(component, RT5651_JD_CTRL2,
 			RT5651_JD_TRG_SEL_MASK, RT5651_JD_TRG_SEL_JD1_2);
-		snd_soc_component_update_bits(component, RT5651_IRQ_CTRL1,
-			RT5651_JD1_2_IRQ_EN, RT5651_JD1_2_IRQ_EN);
+		/* active-low is normal, set inv flag for active-high */
+		if (rt5651->jd_active_high)
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD1_2_IRQ_EN | RT5651_JD1_2_INV,
+				RT5651_JD1_2_IRQ_EN | RT5651_JD1_2_INV);
+		else
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD1_2_IRQ_EN | RT5651_JD1_2_INV,
+				RT5651_JD1_2_IRQ_EN);
 		break;
 	case RT5651_JD2:
 		snd_soc_component_update_bits(component, RT5651_JD_CTRL2,
 			RT5651_JD_TRG_SEL_MASK, RT5651_JD_TRG_SEL_JD2);
-		snd_soc_component_update_bits(component, RT5651_IRQ_CTRL1,
-			RT5651_JD2_IRQ_EN, RT5651_JD2_IRQ_EN);
+		/* active-low is normal, set inv flag for active-high */
+		if (rt5651->jd_active_high)
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD2_IRQ_EN | RT5651_JD2_INV,
+				RT5651_JD2_IRQ_EN | RT5651_JD2_INV);
+		else
+			snd_soc_component_update_bits(component,
+				RT5651_IRQ_CTRL1,
+				RT5651_JD2_IRQ_EN | RT5651_JD2_INV,
+				RT5651_JD2_IRQ_EN);
 		break;
 	default:
 		dev_err(component->dev, "Currently only JD1_1 / JD1_2 / JD2 are supported\n");
@@ -1986,6 +2016,9 @@ static void rt5651_apply_properties(struct snd_soc_component *component)
 				     "realtek,jack-detect-source", &val) == 0)
 		rt5651->jd_src = val;
 
+	if (device_property_read_bool(component->dev, "realtek,jack-detect-not-inverted"))
+		rt5651->jd_active_high = true;
+
 	/*
 	 * Testing on various boards has shown that good defaults for the OVCD
 	 * threshold and scale-factor are 2000µA and 0.75. For an effective
diff --git a/sound/soc/codecs/rt5651.h b/sound/soc/codecs/rt5651.h
index 41fcb8b5eb40..05b0f6f8b95d 100644
--- a/sound/soc/codecs/rt5651.h
+++ b/sound/soc/codecs/rt5651.h
@@ -2083,6 +2083,7 @@ struct rt5651_priv {
 	int release_count;
 	int poll_count;
 	unsigned int jd_src;
+	bool jd_active_high;
 	unsigned int ovcd_th;
 	unsigned int ovcd_sf;
 
-- 
2.21.0

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH resend 2/3] ASoC: Intel: bytcr_rt5651: Add BYT_RT5651_JD_NOT_INV quirk
  2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 1/3] ASoC: rt5651: Add support for active-high jack detect Hans de Goede
@ 2019-04-02 10:20 ` Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 3/3] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled Hans de Goede
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2019-04-02 10:20 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang, Mark Brown
  Cc: Hans de Goede, alsa-devel

Add BYT_RT5651_JD_NOT_INV quirk for devices with an inverted
(active-high instead of the normal active-low) jack-detect switch.

And add a quirk for the Complet Electro Serv MY8307 tablet which has
an inverted jack-detect switch (and a mono-speaker).

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/bytcr_rt5651.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5651.c b/sound/soc/intel/boards/bytcr_rt5651.c
index b0a4d297176e..4ed59f41ee83 100644
--- a/sound/soc/intel/boards/bytcr_rt5651.c
+++ b/sound/soc/intel/boards/bytcr_rt5651.c
@@ -79,14 +79,15 @@ enum {
 #define BYT_RT5651_SSP0_AIF2		BIT(21)
 #define BYT_RT5651_HP_LR_SWAPPED	BIT(22)
 #define BYT_RT5651_MONO_SPEAKER		BIT(23)
+#define BYT_RT5651_JD_NOT_INV		BIT(24)
 
 #define BYT_RT5651_DEFAULT_QUIRKS	(BYT_RT5651_MCLK_EN | \
 					 BYT_RT5651_JD1_1   | \
 					 BYT_RT5651_OVCD_TH_2000UA | \
 					 BYT_RT5651_OVCD_SF_0P75)
 
-/* jack-detect-source + dmic-en + ovcd-th + -sf + terminating empty entry */
-#define MAX_NO_PROPS 5
+/* jack-detect-source + inv + dmic-en + ovcd-th + -sf + terminating entry */
+#define MAX_NO_PROPS 6
 
 struct byt_rt5651_private {
 	struct clk *mclk;
@@ -137,6 +138,8 @@ static void log_quirks(struct device *dev)
 		dev_info(dev, "quirk SSP0_AIF2 enabled\n");
 	if (byt_rt5651_quirk & BYT_RT5651_MONO_SPEAKER)
 		dev_info(dev, "quirk MONO_SPEAKER enabled\n");
+	if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV)
+		dev_info(dev, "quirk JD_NOT_INV enabled\n");
 }
 
 #define BYT_CODEC_DAI1	"rt5651-aif1"
@@ -414,6 +417,18 @@ static const struct dmi_system_id byt_rt5651_quirk_table[] = {
 					BYT_RT5651_HP_LR_SWAPPED |
 					BYT_RT5651_MONO_SPEAKER),
 	},
+	{
+		/* Complet Electro Serv MY8307 */
+		.callback = byt_rt5651_quirk_cb,
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Complet Electro Serv"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MY8307"),
+		},
+		.driver_data = (void *)(BYT_RT5651_DEFAULT_QUIRKS |
+					BYT_RT5651_IN2_MAP |
+					BYT_RT5651_MONO_SPEAKER |
+					BYT_RT5651_JD_NOT_INV),
+	},
 	{
 		/* I.T.Works TW701, Ployer Momo7w and Trekstor ST70416-6
 		 * (these all use the same mainboard) */
@@ -525,6 +540,9 @@ static int byt_rt5651_add_codec_device_props(struct device *i2c_dev)
 	if (byt_rt5651_quirk & BYT_RT5651_DMIC_EN)
 		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,dmic-en");
 
+	if (byt_rt5651_quirk & BYT_RT5651_JD_NOT_INV)
+		props[cnt++] = PROPERTY_ENTRY_BOOL("realtek,jack-detect-not-inverted");
+
 	return device_add_properties(i2c_dev, props);
 }
 
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH resend 3/3] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled
  2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 1/3] ASoC: rt5651: Add support for active-high jack detect Hans de Goede
  2019-04-02 10:20 ` [PATCH resend 2/3] ASoC: Intel: bytcr_rt5651: Add BYT_RT5651_JD_NOT_INV quirk Hans de Goede
@ 2019-04-02 10:20 ` Hans de Goede
  2019-04-03  4:41   ` Applied "ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled" to the asoc tree Mark Brown
  2019-04-02 14:00 ` [PATCH resend 0/3] ASoC: Intel: Misc. fixes Pierre-Louis Bossart
  2019-04-03  4:40 ` Mark Brown
  4 siblings, 1 reply; 8+ messages in thread
From: Hans de Goede @ 2019-04-02 10:20 UTC (permalink / raw)
  To: Pierre-Louis Bossart, Liam Girdwood, Jie Yang, Mark Brown
  Cc: Hans de Goede, alsa-devel, Dean Wallace, Mogens Jensen

Users have been seeing sound stability issues with max98090 codecs since:
commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")

At first that commit broke sound for Chromebook Swanky and Clapper models,
the problem was that the machine-driver has been controlling the wrong
clock on those models since support for them was added. This was hidden by
clk-pmc-atom.c keeping the actual clk on unconditionally.

With the machine-driver controlling the proper clock, sound works again
but we are seeing bug reports describing it as: low volume,
"sounds like played at 10x speed" and instable.

When these issues are hit the following message is seen in dmesg:
"max98090 i2c-193C9890:00: PLL unlocked".

Attempts have been made to fix this by inserting a delay between enabling
the clk and enabling and checking the pll, but this has not helped.

It seems that at least on boards which use pmc_plt_clk_0 as clock,
if we ever disable the clk, the pll looses its lock and after that we get
various issues.

This commit fixes this by enabling the clock once at probe time on
these boards. In essence this restores the old behavior of clk-pmc-atom.c
always keeping the clk on on these boards.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Reported-by: Mogens Jensen <mogens-jensen@protonmail.com>
Reported-by: Dean Wallace <duffydack73@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Only enable the codec clock once and keep it enabled on boards which use
 pmc_plt_clk_0 as clock and leave the behavior unchanged on other boards
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 47 +++++++++++++++++---
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index 3263b0495853..c0e0844f75b9 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -43,6 +43,7 @@ struct cht_mc_private {
 	struct clk *mclk;
 	struct snd_soc_jack jack;
 	bool ts3a227e_present;
+	int quirks;
 };
 
 static int platform_clock_control(struct snd_soc_dapm_widget *w,
@@ -54,6 +55,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
 	int ret;
 
+	/* See the comment in snd_cht_mc_probe() */
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		return 0;
+
 	codec_dai = snd_soc_card_get_codec_dai(card, CHT_CODEC_DAI);
 	if (!codec_dai) {
 		dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n");
@@ -223,6 +228,10 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 			"jack detection gpios not added, error %d\n", ret);
 	}
 
+	/* See the comment in snd_cht_mc_probe() */
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		return 0;
+
 	/*
 	 * The firmware might enable the clock at
 	 * boot (this information may or may not
@@ -423,16 +432,15 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	const char *mclk_name;
 	struct snd_soc_acpi_mach *mach;
 	const char *platform_name;
-	int quirks = 0;
-
-	dmi_id = dmi_first_match(cht_max98090_quirk_table);
-	if (dmi_id)
-		quirks = (unsigned long)dmi_id->driver_data;
 
 	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return -ENOMEM;
 
+	dmi_id = dmi_first_match(cht_max98090_quirk_table);
+	if (dmi_id)
+		drv->quirks = (unsigned long)dmi_id->driver_data;
+
 	drv->ts3a227e_present = acpi_dev_found("104C227E");
 	if (!drv->ts3a227e_present) {
 		/* no need probe TI jack detection chip */
@@ -458,7 +466,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	snd_soc_card_cht.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-	if (quirks & QUIRK_PMC_PLT_CLK_0)
+	if (drv->quirks & QUIRK_PMC_PLT_CLK_0)
 		mclk_name = "pmc_plt_clk_0";
 	else
 		mclk_name = "pmc_plt_clk_3";
@@ -471,6 +479,21 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		return PTR_ERR(drv->mclk);
 	}
 
+	/*
+	 * Boards which have the MAX98090's clk connected to clk_0 do not seem
+	 * to like it if we muck with the clock. If we disable the clock when
+	 * it is unused we get "max98090 i2c-193C9890:00: PLL unlocked" errors
+	 * and the PLL never seems to lock again.
+	 * So for these boards we enable it here once and leave it at that.
+	 */
+	if (drv->quirks & QUIRK_PMC_PLT_CLK_0) {
+		ret_val = clk_prepare_enable(drv->mclk);
+		if (ret_val < 0) {
+			dev_err(&pdev->dev, "MCLK enable error: %d\n", ret_val);
+			return ret_val;
+		}
+	}
+
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht);
 	if (ret_val) {
 		dev_err(&pdev->dev,
@@ -481,11 +504,23 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	return ret_val;
 }
 
+static int snd_cht_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
+
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		clk_disable_unprepare(ctx->mclk);
+
+	return 0;
+}
+
 static struct platform_driver snd_cht_mc_driver = {
 	.driver = {
 		.name = "cht-bsw-max98090",
 	},
 	.probe = snd_cht_mc_probe,
+	.remove = snd_cht_mc_remove,
 };
 
 module_platform_driver(snd_cht_mc_driver)
-- 
2.21.0

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH resend 0/3] ASoC: Intel: Misc. fixes
  2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
                   ` (2 preceding siblings ...)
  2019-04-02 10:20 ` [PATCH resend 3/3] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled Hans de Goede
@ 2019-04-02 14:00 ` Pierre-Louis Bossart
  2019-04-03  4:40 ` Mark Brown
  4 siblings, 0 replies; 8+ messages in thread
From: Pierre-Louis Bossart @ 2019-04-02 14:00 UTC (permalink / raw)
  To: Hans de Goede, Liam Girdwood, Jie Yang, Mark Brown; +Cc: alsa-devel


On 4/2/19 5:20 AM, Hans de Goede wrote:
> Hi Pierre-Louis, Mark,
>
> These 3 patches (originally 2 different patch-sets) seem to have
> fallen through the cracks, hence this resend.
>
> Pierre-Louis, can you review these 3 patches please?

Those 3 look good to me (and I do recall the thread on the last one with the clk_0)

Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH resend 0/3] ASoC: Intel: Misc. fixes
  2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
                   ` (3 preceding siblings ...)
  2019-04-02 14:00 ` [PATCH resend 0/3] ASoC: Intel: Misc. fixes Pierre-Louis Bossart
@ 2019-04-03  4:40 ` Mark Brown
  2019-04-03  6:33   ` Hans de Goede
  4 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2019-04-03  4:40 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Liam Girdwood, alsa-devel, Jie Yang, Pierre-Louis Bossart


[-- Attachment #1.1: Type: text/plain, Size: 491 bytes --]

On Tue, Apr 02, 2019 at 12:20:46PM +0200, Hans de Goede wrote:
> Hi Pierre-Louis, Mark,
> 
> These 3 patches (originally 2 different patch-sets) seem to have
> fallen through the cracks, hence this resend.

Not sure what makes you say that for patch 1 and 2, they're already
there?  

8a68a509ae6b5d7d18c6bfc88553ca7761029ada ASoC: rt5651: Add support for active-high jack detect
a0cb2d4357e483b7bca3c06f790d8f5d4cc20d84 ASoC: Intel: bytcr_rt5651: Add BYT_RT5651_JD_NOT_INV quirk

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 8+ messages in thread

* Applied "ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled" to the asoc tree
  2019-04-02 10:20 ` [PATCH resend 3/3] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled Hans de Goede
@ 2019-04-03  4:41   ` Mark Brown
  0 siblings, 0 replies; 8+ messages in thread
From: Mark Brown @ 2019-04-03  4:41 UTC (permalink / raw)
  To: Hans de Goede
  Cc: alsa-devel, Mogens Jensen, Jie Yang, Pierre-Louis Bossart,
	Dean Wallace, Liam Girdwood, Mark Brown

The patch

   ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled

has been applied to the asoc tree at

   https://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 4bcdec39c454c4e8f9512115bdcc3efec1ba5f55 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Tue, 2 Apr 2019 12:20:49 +0200
Subject: [PATCH] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and
 keep it enabled

Users have been seeing sound stability issues with max98090 codecs since:
commit 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")

At first that commit broke sound for Chromebook Swanky and Clapper models,
the problem was that the machine-driver has been controlling the wrong
clock on those models since support for them was added. This was hidden by
clk-pmc-atom.c keeping the actual clk on unconditionally.

With the machine-driver controlling the proper clock, sound works again
but we are seeing bug reports describing it as: low volume,
"sounds like played at 10x speed" and instable.

When these issues are hit the following message is seen in dmesg:
"max98090 i2c-193C9890:00: PLL unlocked".

Attempts have been made to fix this by inserting a delay between enabling
the clk and enabling and checking the pll, but this has not helped.

It seems that at least on boards which use pmc_plt_clk_0 as clock,
if we ever disable the clk, the pll looses its lock and after that we get
various issues.

This commit fixes this by enabling the clock once at probe time on
these boards. In essence this restores the old behavior of clk-pmc-atom.c
always keeping the clk on on these boards.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Reported-by: Mogens Jensen <mogens-jensen@protonmail.com>
Reported-by: Dean Wallace <duffydack73@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/intel/boards/cht_bsw_max98090_ti.c | 47 +++++++++++++++++---
 1 file changed, 41 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/cht_bsw_max98090_ti.c b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
index 3263b0495853..c0e0844f75b9 100644
--- a/sound/soc/intel/boards/cht_bsw_max98090_ti.c
+++ b/sound/soc/intel/boards/cht_bsw_max98090_ti.c
@@ -43,6 +43,7 @@ struct cht_mc_private {
 	struct clk *mclk;
 	struct snd_soc_jack jack;
 	bool ts3a227e_present;
+	int quirks;
 };
 
 static int platform_clock_control(struct snd_soc_dapm_widget *w,
@@ -54,6 +55,10 @@ static int platform_clock_control(struct snd_soc_dapm_widget *w,
 	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
 	int ret;
 
+	/* See the comment in snd_cht_mc_probe() */
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		return 0;
+
 	codec_dai = snd_soc_card_get_codec_dai(card, CHT_CODEC_DAI);
 	if (!codec_dai) {
 		dev_err(card->dev, "Codec dai not found; Unable to set platform clock\n");
@@ -223,6 +228,10 @@ static int cht_codec_init(struct snd_soc_pcm_runtime *runtime)
 			"jack detection gpios not added, error %d\n", ret);
 	}
 
+	/* See the comment in snd_cht_mc_probe() */
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		return 0;
+
 	/*
 	 * The firmware might enable the clock at
 	 * boot (this information may or may not
@@ -423,16 +432,15 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	const char *mclk_name;
 	struct snd_soc_acpi_mach *mach;
 	const char *platform_name;
-	int quirks = 0;
-
-	dmi_id = dmi_first_match(cht_max98090_quirk_table);
-	if (dmi_id)
-		quirks = (unsigned long)dmi_id->driver_data;
 
 	drv = devm_kzalloc(&pdev->dev, sizeof(*drv), GFP_KERNEL);
 	if (!drv)
 		return -ENOMEM;
 
+	dmi_id = dmi_first_match(cht_max98090_quirk_table);
+	if (dmi_id)
+		drv->quirks = (unsigned long)dmi_id->driver_data;
+
 	drv->ts3a227e_present = acpi_dev_found("104C227E");
 	if (!drv->ts3a227e_present) {
 		/* no need probe TI jack detection chip */
@@ -458,7 +466,7 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	snd_soc_card_cht.dev = &pdev->dev;
 	snd_soc_card_set_drvdata(&snd_soc_card_cht, drv);
 
-	if (quirks & QUIRK_PMC_PLT_CLK_0)
+	if (drv->quirks & QUIRK_PMC_PLT_CLK_0)
 		mclk_name = "pmc_plt_clk_0";
 	else
 		mclk_name = "pmc_plt_clk_3";
@@ -471,6 +479,21 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 		return PTR_ERR(drv->mclk);
 	}
 
+	/*
+	 * Boards which have the MAX98090's clk connected to clk_0 do not seem
+	 * to like it if we muck with the clock. If we disable the clock when
+	 * it is unused we get "max98090 i2c-193C9890:00: PLL unlocked" errors
+	 * and the PLL never seems to lock again.
+	 * So for these boards we enable it here once and leave it at that.
+	 */
+	if (drv->quirks & QUIRK_PMC_PLT_CLK_0) {
+		ret_val = clk_prepare_enable(drv->mclk);
+		if (ret_val < 0) {
+			dev_err(&pdev->dev, "MCLK enable error: %d\n", ret_val);
+			return ret_val;
+		}
+	}
+
 	ret_val = devm_snd_soc_register_card(&pdev->dev, &snd_soc_card_cht);
 	if (ret_val) {
 		dev_err(&pdev->dev,
@@ -481,11 +504,23 @@ static int snd_cht_mc_probe(struct platform_device *pdev)
 	return ret_val;
 }
 
+static int snd_cht_mc_remove(struct platform_device *pdev)
+{
+	struct snd_soc_card *card = platform_get_drvdata(pdev);
+	struct cht_mc_private *ctx = snd_soc_card_get_drvdata(card);
+
+	if (ctx->quirks & QUIRK_PMC_PLT_CLK_0)
+		clk_disable_unprepare(ctx->mclk);
+
+	return 0;
+}
+
 static struct platform_driver snd_cht_mc_driver = {
 	.driver = {
 		.name = "cht-bsw-max98090",
 	},
 	.probe = snd_cht_mc_probe,
+	.remove = snd_cht_mc_remove,
 };
 
 module_platform_driver(snd_cht_mc_driver)
-- 
2.20.1

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH resend 0/3] ASoC: Intel: Misc. fixes
  2019-04-03  4:40 ` Mark Brown
@ 2019-04-03  6:33   ` Hans de Goede
  0 siblings, 0 replies; 8+ messages in thread
From: Hans de Goede @ 2019-04-03  6:33 UTC (permalink / raw)
  To: Mark Brown; +Cc: Liam Girdwood, alsa-devel, Jie Yang, Pierre-Louis Bossart

Hi,

On 03-04-19 06:40, Mark Brown wrote:
> On Tue, Apr 02, 2019 at 12:20:46PM +0200, Hans de Goede wrote:
>> Hi Pierre-Louis, Mark,
>>
>> These 3 patches (originally 2 different patch-sets) seem to have
>> fallen through the cracks, hence this resend.
> 
> Not sure what makes you say that for patch 1 and 2, they're already
> there?

My bad, I somehow overlooked them.

Thank you for merging them (and for merging the 3th patch).

Regards,

Hans

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-04-03  6:33 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-02 10:20 [PATCH resend 0/3] ASoC: Intel: Misc. fixes Hans de Goede
2019-04-02 10:20 ` [PATCH resend 1/3] ASoC: rt5651: Add support for active-high jack detect Hans de Goede
2019-04-02 10:20 ` [PATCH resend 2/3] ASoC: Intel: bytcr_rt5651: Add BYT_RT5651_JD_NOT_INV quirk Hans de Goede
2019-04-02 10:20 ` [PATCH resend 3/3] ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled Hans de Goede
2019-04-03  4:41   ` Applied "ASoC: Intel: cht_bsw_max98090_ti: Enable codec clock once and keep it enabled" to the asoc tree Mark Brown
2019-04-02 14:00 ` [PATCH resend 0/3] ASoC: Intel: Misc. fixes Pierre-Louis Bossart
2019-04-03  4:40 ` Mark Brown
2019-04-03  6:33   ` Hans de Goede

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.