[PATCH 00/11] ASoC: codecs: Series of fixes for realtek codecs used on RVPs

[PATCH 00/11] ASoC: codecs: Series of fixes for realtek codecs used on RVPs

[Amadeusz Sławiński]

Our tests platforms do use realtek codecs, while implementing avs driver
and machine boards for it, we identified quite a lot of problems with
those codec drivers.

This series aims to fix those issues.

Amadeusz Sławiński (8):
  ASoC: codecs: rt274: Move irq registration and cleanup
  ASoC: codecs: rt286: Move irq registration and cleanup
  ASoC: codecs: rt298: Move irq registration and cleanup
  ASoC: codecs: rt274: Enable irq only when needed
  ASoC: codecs: rt286: Enable irq only when needed
  ASoC: codecs: rt298: Enable irq only when needed
  ASoC: codecs: rt298: Fix NULL jack in interrupt
  ASoC: codecs: rt298: Fix jack detection

Cezary Rojewski (3):
  ASoC: codecs: rt274: Always init jack_detect_work
  ASoC: codecs: rt286: Reorganize jack detect handling
  ASoC: codecs: rt298: Reorganize jack detect handling

 sound/soc/codecs/rt274.c           | 67 +++++++++++----------
 sound/soc/codecs/rt286.c           | 56 ++++++++----------
 sound/soc/codecs/rt286.h           |  2 -
 sound/soc/codecs/rt298.c           | 93 +++++++++++++-----------------
 sound/soc/codecs/rt298.h           |  2 -
 sound/soc/intel/boards/broadwell.c |  6 +-
 sound/soc/intel/boards/bxt_rt298.c |  2 +-
 sound/soc/intel/boards/skl_rt286.c |  2 +-
 8 files changed, 108 insertions(+), 122 deletions(-)

-- 
2.25.1

[PATCH 01/11] ASoC: codecs: rt274: Always init jack_detect_work

[Amadeusz Sławiński]

From: Cezary Rojewski <cezary.rojewski@intel.com>

Improves readability by making sure the work is always initialized.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/codecs/rt274.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/rt274.c b/sound/soc/codecs/rt274.c
index ab093bdb5552..a5615e94ec7d 100644
--- a/sound/soc/codecs/rt274.c
+++ b/sound/soc/codecs/rt274.c
@@ -980,14 +980,11 @@ static int rt274_probe(struct snd_soc_component *component)
 	struct rt274_priv *rt274 = snd_soc_component_get_drvdata(component);
 
 	rt274->component = component;
+	INIT_DELAYED_WORK(&rt274->jack_detect_work, rt274_jack_detect_work);
 
-	if (rt274->i2c->irq) {
-		INIT_DELAYED_WORK(&rt274->jack_detect_work,
-					rt274_jack_detect_work);
+	if (rt274->i2c->irq)
 		schedule_delayed_work(&rt274->jack_detect_work,
-					msecs_to_jiffies(1250));
-	}
-
+				      msecs_to_jiffies(1250));
 	return 0;
 }
 
-- 
2.25.1

[PATCH 02/11] ASoC: codecs: rt286: Reorganize jack detect handling

[Amadeusz Sławiński]

From: Cezary Rojewski <cezary.rojewski@intel.com>

Clean up in order to use and expose .set_jack callback.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/codecs/rt286.c           | 17 ++++++-----------
 sound/soc/codecs/rt286.h           |  2 --
 sound/soc/intel/boards/broadwell.c |  6 +++---
 sound/soc/intel/boards/skl_rt286.c |  2 +-
 4 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index ad8ea1fa7c23..0534a073ee69 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -311,7 +311,8 @@ static void rt286_jack_detect_work(struct work_struct *work)
 		SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
 }
 
-int rt286_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *jack)
+static int rt286_mic_detect(struct snd_soc_component *component,
+			    struct snd_soc_jack *jack, void *data)
 {
 	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
 	struct rt286_priv *rt286 = snd_soc_component_get_drvdata(component);
@@ -335,7 +336,6 @@ int rt286_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *j
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rt286_mic_detect);
 
 static int is_mclk_mode(struct snd_soc_dapm_widget *source,
 			 struct snd_soc_dapm_widget *sink)
@@ -947,17 +947,11 @@ static int rt286_probe(struct snd_soc_component *component)
 	struct rt286_priv *rt286 = snd_soc_component_get_drvdata(component);
 
 	rt286->component = component;
+	INIT_DELAYED_WORK(&rt286->jack_detect_work, rt286_jack_detect_work);
 
-	if (rt286->i2c->irq) {
-		regmap_update_bits(rt286->regmap,
-					RT286_IRQ_CTRL, 0x2, 0x2);
-
-		INIT_DELAYED_WORK(&rt286->jack_detect_work,
-					rt286_jack_detect_work);
+	if (rt286->i2c->irq)
 		schedule_delayed_work(&rt286->jack_detect_work,
-					msecs_to_jiffies(1250));
-	}
-
+				      msecs_to_jiffies(50));
 	return 0;
 }
 
@@ -1055,6 +1049,7 @@ static const struct snd_soc_component_driver soc_component_dev_rt286 = {
 	.suspend		= rt286_suspend,
 	.resume			= rt286_resume,
 	.set_bias_level		= rt286_set_bias_level,
+	.set_jack		= rt286_mic_detect,
 	.controls		= rt286_snd_controls,
 	.num_controls		= ARRAY_SIZE(rt286_snd_controls),
 	.dapm_widgets		= rt286_dapm_widgets,
diff --git a/sound/soc/codecs/rt286.h b/sound/soc/codecs/rt286.h
index f27a4e71d5b6..4b7a3bd6043d 100644
--- a/sound/soc/codecs/rt286.h
+++ b/sound/soc/codecs/rt286.h
@@ -196,7 +196,5 @@ enum {
 	RT286_AIFS,
 };
 
-int rt286_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *jack);
-
 #endif /* __RT286_H__ */
 
diff --git a/sound/soc/intel/boards/broadwell.c b/sound/soc/intel/boards/broadwell.c
index b29d77dfb281..48bf3241b3e6 100644
--- a/sound/soc/intel/boards/broadwell.c
+++ b/sound/soc/intel/boards/broadwell.c
@@ -75,7 +75,7 @@ static int broadwell_rt286_codec_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	rt286_mic_detect(component, &broadwell_headset);
+	snd_soc_component_set_jack(component, &broadwell_headset, NULL);
 	return 0;
 }
 
@@ -235,7 +235,7 @@ static void broadwell_disable_jack(struct snd_soc_card *card)
 		if (!strcmp(component->name, "i2c-INT343A:00")) {
 
 			dev_dbg(component->dev, "disabling jack detect before going to suspend.\n");
-			rt286_mic_detect(component, NULL);
+			snd_soc_component_set_jack(component, NULL, NULL);
 			break;
 		}
 	}
@@ -255,7 +255,7 @@ static int broadwell_resume(struct snd_soc_card *card){
 		if (!strcmp(component->name, "i2c-INT343A:00")) {
 
 			dev_dbg(component->dev, "enabling jack detect for resume.\n");
-			rt286_mic_detect(component, &broadwell_headset);
+			snd_soc_component_set_jack(component, &broadwell_headset, NULL);
 			break;
 		}
 	}
diff --git a/sound/soc/intel/boards/skl_rt286.c b/sound/soc/intel/boards/skl_rt286.c
index e9f9520dcea4..4f3d655e2bfa 100644
--- a/sound/soc/intel/boards/skl_rt286.c
+++ b/sound/soc/intel/boards/skl_rt286.c
@@ -133,7 +133,7 @@ static int skylake_rt286_codec_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	rt286_mic_detect(component, &skylake_headset);
+	snd_soc_component_set_jack(component, &skylake_headset, NULL);
 
 	snd_soc_dapm_ignore_suspend(&rtd->card->dapm, "SoC DMIC");
 
-- 
2.25.1

[PATCH 03/11] ASoC: codecs: rt298: Reorganize jack detect handling

[Amadeusz Sławiński]

From: Cezary Rojewski <cezary.rojewski@intel.com>

Clean up in order to use and expose .set_jack callback.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/codecs/rt298.c           | 17 ++++++-----------
 sound/soc/codecs/rt298.h           |  2 --
 sound/soc/intel/boards/bxt_rt298.c |  2 +-
 3 files changed, 7 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c
index c291786dc82d..1a27e5e63289 100644
--- a/sound/soc/codecs/rt298.c
+++ b/sound/soc/codecs/rt298.c
@@ -326,7 +326,8 @@ static void rt298_jack_detect_work(struct work_struct *work)
 		SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
 }
 
-int rt298_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *jack)
+static int rt298_mic_detect(struct snd_soc_component *component,
+			    struct snd_soc_jack *jack, void *data)
 {
 	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
 	struct snd_soc_dapm_context *dapm;
@@ -358,7 +359,6 @@ int rt298_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *j
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(rt298_mic_detect);
 
 static int is_mclk_mode(struct snd_soc_dapm_widget *source,
 			 struct snd_soc_dapm_widget *sink)
@@ -1011,17 +1011,11 @@ static int rt298_probe(struct snd_soc_component *component)
 	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
 
 	rt298->component = component;
+	INIT_DELAYED_WORK(&rt298->jack_detect_work, rt298_jack_detect_work);
 
-	if (rt298->i2c->irq) {
-		regmap_update_bits(rt298->regmap,
-					RT298_IRQ_CTRL, 0x2, 0x2);
-
-		INIT_DELAYED_WORK(&rt298->jack_detect_work,
-					rt298_jack_detect_work);
+	if (rt298->i2c->irq)
 		schedule_delayed_work(&rt298->jack_detect_work,
-					msecs_to_jiffies(1250));
-	}
-
+				      msecs_to_jiffies(1250));
 	return 0;
 }
 
@@ -1120,6 +1114,7 @@ static const struct snd_soc_component_driver soc_component_dev_rt298 = {
 	.suspend		= rt298_suspend,
 	.resume			= rt298_resume,
 	.set_bias_level		= rt298_set_bias_level,
+	.set_jack		= rt298_mic_detect,
 	.controls		= rt298_snd_controls,
 	.num_controls		= ARRAY_SIZE(rt298_snd_controls),
 	.dapm_widgets		= rt298_dapm_widgets,
diff --git a/sound/soc/codecs/rt298.h b/sound/soc/codecs/rt298.h
index ed2b8fd87f4c..f1be9c135401 100644
--- a/sound/soc/codecs/rt298.h
+++ b/sound/soc/codecs/rt298.h
@@ -207,7 +207,5 @@ enum {
 	RT298_AIFS,
 };
 
-int rt298_mic_detect(struct snd_soc_component *component, struct snd_soc_jack *jack);
-
 #endif /* __RT298_H__ */
 
diff --git a/sound/soc/intel/boards/bxt_rt298.c b/sound/soc/intel/boards/bxt_rt298.c
index 75995d17597d..4bd93c3ba377 100644
--- a/sound/soc/intel/boards/bxt_rt298.c
+++ b/sound/soc/intel/boards/bxt_rt298.c
@@ -176,7 +176,7 @@ static int broxton_rt298_codec_init(struct snd_soc_pcm_runtime *rtd)
 	if (ret)
 		return ret;
 
-	rt298_mic_detect(component, &broxton_headset);
+	snd_soc_component_set_jack(component, &broxton_headset, NULL);
 
 	snd_soc_dapm_ignore_suspend(&rtd->card->dapm, "SoC DMIC");
 
-- 
2.25.1

[PATCH 04/11] ASoC: codecs: rt274: Move irq registration and cleanup

[Amadeusz Sławiński]

Currently irq is registered when i2c driver is loaded, it is unnecessary
when component is unused. Initialize irq only when we probe ASoC
component.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt274.c | 44 ++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/sound/soc/codecs/rt274.c b/sound/soc/codecs/rt274.c
index a5615e94ec7d..144a6f775c21 100644
--- a/sound/soc/codecs/rt274.c
+++ b/sound/soc/codecs/rt274.c
@@ -978,13 +978,22 @@ static irqreturn_t rt274_irq(int irq, void *data)
 static int rt274_probe(struct snd_soc_component *component)
 {
 	struct rt274_priv *rt274 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt274->component = component;
 	INIT_DELAYED_WORK(&rt274->jack_detect_work, rt274_jack_detect_work);
 
-	if (rt274->i2c->irq)
-		schedule_delayed_work(&rt274->jack_detect_work,
-				      msecs_to_jiffies(1250));
+	if (rt274->i2c->irq) {
+		ret = request_threaded_irq(rt274->i2c->irq, NULL, rt274_irq,
+					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt274", rt274);
+		if (ret) {
+			dev_err(&rt274->i2c->dev, "Failed to reguest IRQ: %d\n", ret);
+			return ret;
+		}
+
+		schedule_delayed_work(&rt274->jack_detect_work, msecs_to_jiffies(1250));
+	}
+
 	return 0;
 }
 
@@ -992,7 +1001,12 @@ static void rt274_remove(struct snd_soc_component *component)
 {
 	struct rt274_priv *rt274 = snd_soc_component_get_drvdata(component);
 
-	cancel_delayed_work_sync(&rt274->jack_detect_work);
+	if (rt274->i2c->irq) {
+		cancel_delayed_work_sync(&rt274->jack_detect_work);
+
+		disable_irq(rt274->i2c->irq);
+		free_irq(rt274->i2c->irq, rt274);
+	}
 }
 
 #ifdef CONFIG_PM
@@ -1187,16 +1201,6 @@ static int rt274_i2c_probe(struct i2c_client *i2c)
 	regmap_write(rt274->regmap, RT274_UNSOLICITED_HP_OUT, 0x81);
 	regmap_write(rt274->regmap, RT274_UNSOLICITED_MIC, 0x82);
 
-	if (rt274->i2c->irq) {
-		ret = request_threaded_irq(rt274->i2c->irq, NULL, rt274_irq,
-			IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt274", rt274);
-		if (ret != 0) {
-			dev_err(&i2c->dev,
-				"Failed to reguest IRQ: %d\n", ret);
-			return ret;
-		}
-	}
-
 	ret = devm_snd_soc_register_component(&i2c->dev,
 				     &soc_component_dev_rt274,
 				     rt274_dai, ARRAY_SIZE(rt274_dai));
@@ -1204,17 +1208,6 @@ static int rt274_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static int rt274_i2c_remove(struct i2c_client *i2c)
-{
-	struct rt274_priv *rt274 = i2c_get_clientdata(i2c);
-
-	if (i2c->irq)
-		free_irq(i2c->irq, rt274);
-
-	return 0;
-}
-
-
 static struct i2c_driver rt274_i2c_driver = {
 	.driver = {
 		   .name = "rt274",
@@ -1224,7 +1217,6 @@ static struct i2c_driver rt274_i2c_driver = {
 #endif
 		   },
 	.probe_new = rt274_i2c_probe,
-	.remove = rt274_i2c_remove,
 	.id_table = rt274_i2c_id,
 };
 
-- 
2.25.1

Re: [PATCH 04/11] ASoC: codecs: rt274: Move irq registration and cleanup

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 590 bytes --]

On Thu, Jun 09, 2022 at 03:35:34PM +0200, Amadeusz Sławiński wrote:
> Currently irq is registered when i2c driver is loaded, it is unnecessary
> when component is unused. Initialize irq only when we probe ASoC
> component.

No, this makes things worse - we should acquire resources in the device
level probe so that we handle deferred probe more gracefully, triggering
a defer from the device trying to acquire the resource makes it clearer
what's going on and reduces the amount of work we do on deferred probe
attempts.  Having an interrupt registered has no meaningful cost.

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

[PATCH 05/11] ASoC: codecs: rt286: Move irq registration and cleanup

[Amadeusz Sławiński]

Currently irq is registered when i2c driver is loaded, it is unnecessary
when component is unused. Initialize irq only when we probe ASoC
component.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt286.c | 44 ++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 26 deletions(-)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index 0534a073ee69..02a41c915776 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -945,13 +945,22 @@ static irqreturn_t rt286_irq(int irq, void *data)
 static int rt286_probe(struct snd_soc_component *component)
 {
 	struct rt286_priv *rt286 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt286->component = component;
 	INIT_DELAYED_WORK(&rt286->jack_detect_work, rt286_jack_detect_work);
 
-	if (rt286->i2c->irq)
-		schedule_delayed_work(&rt286->jack_detect_work,
-				      msecs_to_jiffies(50));
+	if (rt286->i2c->irq) {
+		ret = request_threaded_irq(rt286->i2c->irq, NULL, rt286_irq,
+					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt286", rt286);
+		if (ret) {
+			dev_err(&rt286->i2c->dev, "Failed to reguest IRQ: %d\n", ret);
+			return ret;
+		}
+
+		schedule_delayed_work(&rt286->jack_detect_work, msecs_to_jiffies(50));
+	}
+
 	return 0;
 }
 
@@ -959,7 +968,12 @@ static void rt286_remove(struct snd_soc_component *component)
 {
 	struct rt286_priv *rt286 = snd_soc_component_get_drvdata(component);
 
-	cancel_delayed_work_sync(&rt286->jack_detect_work);
+	if (rt286->i2c->irq) {
+		cancel_delayed_work_sync(&rt286->jack_detect_work);
+
+		disable_irq(rt286->i2c->irq);
+		free_irq(rt286->i2c->irq, rt286);
+	}
 }
 
 #ifdef CONFIG_PM
@@ -1232,16 +1246,6 @@ static int rt286_i2c_probe(struct i2c_client *i2c)
 			RT286_GPIO_CTRL, 0xc, 0x8);
 	}
 
-	if (rt286->i2c->irq) {
-		ret = request_threaded_irq(rt286->i2c->irq, NULL, rt286_irq,
-			IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt286", rt286);
-		if (ret != 0) {
-			dev_err(&i2c->dev,
-				"Failed to reguest IRQ: %d\n", ret);
-			return ret;
-		}
-	}
-
 	ret = devm_snd_soc_register_component(&i2c->dev,
 				     &soc_component_dev_rt286,
 				     rt286_dai, ARRAY_SIZE(rt286_dai));
@@ -1249,24 +1253,12 @@ static int rt286_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static int rt286_i2c_remove(struct i2c_client *i2c)
-{
-	struct rt286_priv *rt286 = i2c_get_clientdata(i2c);
-
-	if (i2c->irq)
-		free_irq(i2c->irq, rt286);
-
-	return 0;
-}
-
-
 static struct i2c_driver rt286_i2c_driver = {
 	.driver = {
 		   .name = "rt286",
 		   .acpi_match_table = ACPI_PTR(rt286_acpi_match),
 		   },
 	.probe_new = rt286_i2c_probe,
-	.remove = rt286_i2c_remove,
 	.id_table = rt286_i2c_id,
 };
 
-- 
2.25.1

[PATCH 06/11] ASoC: codecs: rt298: Move irq registration and cleanup

[Amadeusz Sławiński]

Currently irq is registered when i2c driver is loaded, it is unnecessary
when component is unused. Initialize irq only when we probe ASoC
component.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt298.c | 43 +++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c
index 1a27e5e63289..078810f2ec7b 100644
--- a/sound/soc/codecs/rt298.c
+++ b/sound/soc/codecs/rt298.c
@@ -1009,13 +1009,22 @@ static irqreturn_t rt298_irq(int irq, void *data)
 static int rt298_probe(struct snd_soc_component *component)
 {
 	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
+	int ret;
 
 	rt298->component = component;
 	INIT_DELAYED_WORK(&rt298->jack_detect_work, rt298_jack_detect_work);
 
-	if (rt298->i2c->irq)
-		schedule_delayed_work(&rt298->jack_detect_work,
-				      msecs_to_jiffies(1250));
+	if (rt298->i2c->irq) {
+		ret = request_threaded_irq(rt298->i2c->irq, NULL, rt298_irq,
+					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt298", rt298);
+		if (ret) {
+			dev_err(&rt298->i2c->dev, "Failed to reguest IRQ: %d\n", ret);
+			return ret;
+		}
+
+		schedule_delayed_work(&rt298->jack_detect_work, msecs_to_jiffies(1250));
+	}
+
 	return 0;
 }
 
@@ -1023,7 +1032,12 @@ static void rt298_remove(struct snd_soc_component *component)
 {
 	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
 
-	cancel_delayed_work_sync(&rt298->jack_detect_work);
+	if (rt298->i2c->irq) {
+		cancel_delayed_work_sync(&rt298->jack_detect_work);
+
+		disable_irq(rt298->i2c->irq);
+		free_irq(rt298->i2c->irq, rt298);
+	}
 }
 
 #ifdef CONFIG_PM
@@ -1275,16 +1289,6 @@ static int rt298_i2c_probe(struct i2c_client *i2c)
 
 	rt298->is_hp_in = -1;
 
-	if (rt298->i2c->irq) {
-		ret = request_threaded_irq(rt298->i2c->irq, NULL, rt298_irq,
-			IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt298", rt298);
-		if (ret != 0) {
-			dev_err(&i2c->dev,
-				"Failed to reguest IRQ: %d\n", ret);
-			return ret;
-		}
-	}
-
 	ret = devm_snd_soc_register_component(&i2c->dev,
 				     &soc_component_dev_rt298,
 				     rt298_dai, ARRAY_SIZE(rt298_dai));
@@ -1292,17 +1296,6 @@ static int rt298_i2c_probe(struct i2c_client *i2c)
 	return ret;
 }
 
-static int rt298_i2c_remove(struct i2c_client *i2c)
-{
-	struct rt298_priv *rt298 = i2c_get_clientdata(i2c);
-
-	if (i2c->irq)
-		free_irq(i2c->irq, rt298);
-
-	return 0;
-}
-
-
 static struct i2c_driver rt298_i2c_driver = {
 	.driver = {
 		   .name = "rt298",
-- 
2.25.1

[PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Amadeusz Sławiński]

Interrupt is only needed when jack detection is enabled, so enable it
then, similarly disable it when jack detection is being disabled.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt274.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/rt274.c b/sound/soc/codecs/rt274.c
index 144a6f775c21..730de9452333 100644
--- a/sound/soc/codecs/rt274.c
+++ b/sound/soc/codecs/rt274.c
@@ -16,6 +16,7 @@
 #include <linux/spi/spi.h>
 #include <linux/dmi.h>
 #include <linux/acpi.h>
+#include <linux/irq.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -395,28 +396,42 @@ static void rt274_jack_detect_work(struct work_struct *work)
 		SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
 }
 
-static irqreturn_t rt274_irq(int irq, void *data);
-
 static int rt274_mic_detect(struct snd_soc_component *component,
 	struct snd_soc_jack *jack,  void *data)
 {
 	struct rt274_priv *rt274 = snd_soc_component_get_drvdata(component);
+	bool mic = false;
+	bool hp = false;
+	int status = 0;
+	int ret;
 
 	rt274->jack = jack;
 
 	if (jack == NULL) {
 		/* Disable jack detection */
+		disable_irq(rt274->i2c->irq);
 		regmap_update_bits(rt274->regmap, RT274_EAPD_GPIO_IRQ_CTRL,
 					RT274_IRQ_EN, RT274_IRQ_DIS);
 
 		return 0;
 	}
 
+	/* Enable jack detection */
 	regmap_update_bits(rt274->regmap, RT274_EAPD_GPIO_IRQ_CTRL,
 				RT274_IRQ_EN, RT274_IRQ_EN);
+	enable_irq(rt274->i2c->irq);
 
 	/* Send an initial report */
-	rt274_irq(0, rt274);
+	ret = rt274_jack_detect(rt274, &hp, &mic);
+	if (!ret) {
+		if (hp)
+			status |= SND_JACK_HEADPHONE;
+
+		if (mic)
+			status |= SND_JACK_MICROPHONE;
+
+		snd_soc_jack_report(rt274->jack, status, SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
+	}
 
 	return 0;
 }
@@ -984,6 +999,9 @@ static int rt274_probe(struct snd_soc_component *component)
 	INIT_DELAYED_WORK(&rt274->jack_detect_work, rt274_jack_detect_work);
 
 	if (rt274->i2c->irq) {
+		/* irq will be enabled in rt274_mic_detect */
+		irq_set_status_flags(rt274->i2c->irq, IRQ_NOAUTOEN);
+
 		ret = request_threaded_irq(rt274->i2c->irq, NULL, rt274_irq,
 					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt274", rt274);
 		if (ret) {
-- 
2.25.1

Re: [PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Thu, Jun 09, 2022 at 03:35:37PM +0200, Amadeusz Sławiński wrote:
> Interrupt is only needed when jack detection is enabled, so enable it
> then, similarly disable it when jack detection is being disabled.

>  	if (jack == NULL) {
>  		/* Disable jack detection */
> +		disable_irq(rt274->i2c->irq);

There is absolutely no need to do this, it'll interfere with any sharing
of the interrupt and if the interrupt isn't firing then there is no cost
to having the interrupt registered.

The driver could use some cleanup of the interrupt handler, it currently
unconditionally clears anything that fires and reports IRQ_HANDLED but
should only report IRQ_HANDLED if there was anything from the device.
Practically speaking it shouldn't make much difference unless there's
spurious interrupts or the interrupt gets shared.

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

Re: [PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Amadeusz Sławiński]

On 6/9/2022 4:18 PM, Mark Brown wrote:
> On Thu, Jun 09, 2022 at 03:35:37PM +0200, Amadeusz Sławiński wrote:
>> Interrupt is only needed when jack detection is enabled, so enable it
>> then, similarly disable it when jack detection is being disabled.
> 
>>   	if (jack == NULL) {
>>   		/* Disable jack detection */
>> +		disable_irq(rt274->i2c->irq);
> 
> There is absolutely no need to do this, it'll interfere with any sharing
> of the interrupt and if the interrupt isn't firing then there is no cost
> to having the interrupt registered.
> 
> The driver could use some cleanup of the interrupt handler, it currently
> unconditionally clears anything that fires and reports IRQ_HANDLED but
> should only report IRQ_HANDLED if there was anything from the device.
> Practically speaking it shouldn't make much difference unless there's
> spurious interrupts or the interrupt gets shared.

I will recheck this again, but if I remember correctly we may have had 
problems that codec kept firing interrupts when we unloaded machine 
board and codec driver kept spamming dmesg due to _dbg message present 
in irq handler.

Re: [PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1397 bytes --]

On Fri, Jun 10, 2022 at 11:33:34AM +0200, Amadeusz Sławiński wrote:
> On 6/9/2022 4:18 PM, Mark Brown wrote:
> > On Thu, Jun 09, 2022 at 03:35:37PM +0200, Amadeusz Sławiński wrote:
> > > Interrupt is only needed when jack detection is enabled, so enable it
> > > then, similarly disable it when jack detection is being disabled.

> > >   	if (jack == NULL) {
> > >   		/* Disable jack detection */
> > > +		disable_irq(rt274->i2c->irq);

> > There is absolutely no need to do this, it'll interfere with any sharing
> > of the interrupt and if the interrupt isn't firing then there is no cost
> > to having the interrupt registered.

> > The driver could use some cleanup of the interrupt handler, it currently
> > unconditionally clears anything that fires and reports IRQ_HANDLED but
> > should only report IRQ_HANDLED if there was anything from the device.
> > Practically speaking it shouldn't make much difference unless there's
> > spurious interrupts or the interrupt gets shared.

> I will recheck this again, but if I remember correctly we may have had
> problems that codec kept firing interrupts when we unloaded machine board
> and codec driver kept spamming dmesg due to _dbg message present in irq
> handler.

If there's issues there they should be fixed in the interrupt handler -
the driver should be able to handle the interrupt sensibly either way.

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

Re: [PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Amadeusz Sławiński]

On 6/9/2022 4:18 PM, Mark Brown wrote:
> On Thu, Jun 09, 2022 at 03:35:37PM +0200, Amadeusz Sławiński wrote:
>> Interrupt is only needed when jack detection is enabled, so enable it
>> then, similarly disable it when jack detection is being disabled.
> 
>>   	if (jack == NULL) {
>>   		/* Disable jack detection */
>> +		disable_irq(rt274->i2c->irq);
> 
> There is absolutely no need to do this, it'll interfere with any sharing
> of the interrupt and if the interrupt isn't firing then there is no cost
> to having the interrupt registered.
> 
> The driver could use some cleanup of the interrupt handler, it currently
> unconditionally clears anything that fires and reports IRQ_HANDLED but
> should only report IRQ_HANDLED if there was anything from the device.
> Practically speaking it shouldn't make much difference unless there's
> spurious interrupts or the interrupt gets shared.

While this sounds fine, in tests I see that irq handler gets called 
around ~800 times per second even when we unload platform driver and 
there is no one caring about jack detection... in this case I would 
consider this to be a waste of CPU time and would prefer to just 
outright to disable it. Is there some better way to avoid unnecessary 
calls to irq handler?

Main reason why I even looked at this is pr_debug() present in 
rt298_jack_detect() which kept spamming our debug logs (~800 lines per 
second fills up logs rather fast...). It should probably be removed, as 
rt286 and rt274 do fine without having this logged, but they also call 
irq handler quite a lot if you add message log for debug.

Re: [PATCH 07/11] ASoC: codecs: rt274: Enable irq only when needed

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1525 bytes --]

On Thu, Jun 23, 2022 at 03:53:09PM +0200, Amadeusz Sławiński wrote:
> On 6/9/2022 4:18 PM, Mark Brown wrote:
> > On Thu, Jun 09, 2022 at 03:35:37PM +0200, Amadeusz Sławiński wrote:

> > The driver could use some cleanup of the interrupt handler, it currently
> > unconditionally clears anything that fires and reports IRQ_HANDLED but
> > should only report IRQ_HANDLED if there was anything from the device.
> > Practically speaking it shouldn't make much difference unless there's
> > spurious interrupts or the interrupt gets shared.

> While this sounds fine, in tests I see that irq handler gets called around
> ~800 times per second even when we unload platform driver and there is no
> one caring about jack detection... in this case I would consider this to be
> a waste of CPU time and would prefer to just outright to disable it. Is
> there some better way to avoid unnecessary calls to irq handler?

> Main reason why I even looked at this is pr_debug() present in
> rt298_jack_detect() which kept spamming our debug logs (~800 lines per
> second fills up logs rather fast...). It should probably be removed, as
> rt286 and rt274 do fine without having this logged, but they also call irq
> handler quite a lot if you add message log for debug.

If the jack detection is firing hundreds of times a second without there
being an event that seems like a serious problem with the way the
hardware is set up which should be fixed, I'm surprised that this isn't
disrupting things normally.

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

[PATCH 08/11] ASoC: codecs: rt286: Enable irq only when needed

[Amadeusz Sławiński]

Interrupt is only needed when jack detection is enabled, so enable it
then, similarly disable it when jack detection is being disabled.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt286.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c
index 02a41c915776..6384b4cb9eaf 100644
--- a/sound/soc/codecs/rt286.c
+++ b/sound/soc/codecs/rt286.c
@@ -16,6 +16,7 @@
 #include <linux/spi/spi.h>
 #include <linux/dmi.h>
 #include <linux/acpi.h>
+#include <linux/irq.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -324,11 +325,14 @@ static int rt286_mic_detect(struct snd_soc_component *component,
 		if (rt286->jack->status & SND_JACK_HEADPHONE)
 			snd_soc_dapm_force_enable_pin(dapm, "LDO1");
 		regmap_update_bits(rt286->regmap, RT286_IRQ_CTRL, 0x2, 0x2);
+		enable_irq(rt286->i2c->irq);
+
 		/* Send an initial empty report */
 		snd_soc_jack_report(rt286->jack, rt286->jack->status,
 			SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
 	} else {
 		/* disable IRQ */
+		disable_irq(rt286->i2c->irq);
 		regmap_update_bits(rt286->regmap, RT286_IRQ_CTRL, 0x2, 0x0);
 		snd_soc_dapm_disable_pin(dapm, "LDO1");
 	}
@@ -951,6 +955,9 @@ static int rt286_probe(struct snd_soc_component *component)
 	INIT_DELAYED_WORK(&rt286->jack_detect_work, rt286_jack_detect_work);
 
 	if (rt286->i2c->irq) {
+		/* irq will be enabled in rt286_mic_detect */
+		irq_set_status_flags(rt286->i2c->irq, IRQ_NOAUTOEN);
+
 		ret = request_threaded_irq(rt286->i2c->irq, NULL, rt286_irq,
 					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt286", rt286);
 		if (ret) {
-- 
2.25.1

[PATCH 09/11] ASoC: codecs: rt298: Enable irq only when needed

[Amadeusz Sławiński]

Interrupt is only needed when jack detection is enabled, so enable it
then, similarly disable it when jack detection is being disabled.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt298.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c
index 078810f2ec7b..3c13b6dc3be9 100644
--- a/sound/soc/codecs/rt298.c
+++ b/sound/soc/codecs/rt298.c
@@ -16,6 +16,7 @@
 #include <linux/spi/spi.h>
 #include <linux/dmi.h>
 #include <linux/acpi.h>
+#include <linux/irq.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
@@ -337,6 +338,7 @@ static int rt298_mic_detect(struct snd_soc_component *component,
 
 	/* If jack in NULL, disable HS jack */
 	if (!jack) {
+		disable_irq(rt298->i2c->irq);
 		regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x0);
 		dapm = snd_soc_component_get_dapm(component);
 		snd_soc_dapm_disable_pin(dapm, "LDO1");
@@ -346,6 +348,7 @@ static int rt298_mic_detect(struct snd_soc_component *component,
 
 	rt298->jack = jack;
 	regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
+	enable_irq(rt298->i2c->irq);
 
 	rt298_jack_detect(rt298, &hp, &mic);
 	if (hp)
@@ -1015,6 +1018,9 @@ static int rt298_probe(struct snd_soc_component *component)
 	INIT_DELAYED_WORK(&rt298->jack_detect_work, rt298_jack_detect_work);
 
 	if (rt298->i2c->irq) {
+		/* irq will be enabled in rt298_mic_detect */
+		irq_set_status_flags(rt298->i2c->irq, IRQ_NOAUTOEN);
+
 		ret = request_threaded_irq(rt298->i2c->irq, NULL, rt298_irq,
 					   IRQF_TRIGGER_HIGH | IRQF_ONESHOT, "rt298", rt298);
 		if (ret) {
-- 
2.25.1

[PATCH 10/11] ASoC: codecs: rt298: Fix NULL jack in interrupt

[Amadeusz Sławiński]

Set rt298->jack to passed value in mic_detect, otherwise when jack is
set to NULL on next interrupt call, we may use freed pointer.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt298.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c
index 3c13b6dc3be9..2af037536bc9 100644
--- a/sound/soc/codecs/rt298.c
+++ b/sound/soc/codecs/rt298.c
@@ -336,6 +336,8 @@ static int rt298_mic_detect(struct snd_soc_component *component,
 	bool mic = false;
 	int status = 0;
 
+	rt298->jack = jack;
+
 	/* If jack in NULL, disable HS jack */
 	if (!jack) {
 		disable_irq(rt298->i2c->irq);
@@ -346,7 +348,6 @@ static int rt298_mic_detect(struct snd_soc_component *component,
 		return 0;
 	}
 
-	rt298->jack = jack;
 	regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
 	enable_irq(rt298->i2c->irq);
 
-- 
2.25.1

Re: [PATCH 10/11] ASoC: codecs: rt298: Fix NULL jack in interrupt

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 366 bytes --]

On Thu, Jun 09, 2022 at 03:35:40PM +0200, Amadeusz Sławiński wrote:
> Set rt298->jack to passed value in mic_detect, otherwise when jack is
> set to NULL on next interrupt call, we may use freed pointer.

This is an actual fix so should go towards the start of the series,
putting fixes at the start of the series avoids them getting spurious
dependencies.

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

[PATCH 11/11] ASoC: codecs: rt298: Fix jack detection

[Amadeusz Sławiński]

On our RVP platforms using rt298 with combojack we've seen issues with
controls being in incorrect state after suspend/resume cycle. This is
caused by codec driver not setting pins to correct state and causing
codec suspend method to not be called. Which on resume caused codec
registers to be in undefined state. Fix this by setting pins correctly
in jack detect function.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
---
 sound/soc/codecs/rt298.c | 39 +++++++++++++++++----------------------
 1 file changed, 17 insertions(+), 22 deletions(-)

diff --git a/sound/soc/codecs/rt298.c b/sound/soc/codecs/rt298.c
index 2af037536bc9..f1c3dfdea5e7 100644
--- a/sound/soc/codecs/rt298.c
+++ b/sound/soc/codecs/rt298.c
@@ -330,36 +330,31 @@ static void rt298_jack_detect_work(struct work_struct *work)
 static int rt298_mic_detect(struct snd_soc_component *component,
 			    struct snd_soc_jack *jack, void *data)
 {
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
 	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
-	struct snd_soc_dapm_context *dapm;
-	bool hp = false;
-	bool mic = false;
-	int status = 0;
 
 	rt298->jack = jack;
 
-	/* If jack in NULL, disable HS jack */
-	if (!jack) {
+	if (jack) {
+		/* enable IRQ */
+		if (rt298->jack->status & SND_JACK_HEADPHONE)
+			snd_soc_dapm_force_enable_pin(dapm, "LDO1");
+		if (rt298->jack->status & SND_JACK_MICROPHONE) {
+			snd_soc_dapm_force_enable_pin(dapm, "HV");
+			snd_soc_dapm_force_enable_pin(dapm, "VREF");
+		}
+		regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
+		enable_irq(rt298->i2c->irq);
+		snd_soc_jack_report(rt298->jack, rt298->jack->status,
+				    SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
+	} else {
 		disable_irq(rt298->i2c->irq);
 		regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x0);
-		dapm = snd_soc_component_get_dapm(component);
+		snd_soc_dapm_disable_pin(dapm, "HV");
+		snd_soc_dapm_disable_pin(dapm, "VREF");
 		snd_soc_dapm_disable_pin(dapm, "LDO1");
-		snd_soc_dapm_sync(dapm);
-		return 0;
 	}
-
-	regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
-	enable_irq(rt298->i2c->irq);
-
-	rt298_jack_detect(rt298, &hp, &mic);
-	if (hp)
-		status |= SND_JACK_HEADPHONE;
-
-	if (mic)
-		status |= SND_JACK_MICROPHONE;
-
-	snd_soc_jack_report(rt298->jack, status,
-		SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
+	snd_soc_dapm_sync(dapm);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH 11/11] ASoC: codecs: rt298: Fix jack detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1762 bytes --]

On Thu, Jun 09, 2022 at 03:35:41PM +0200, Amadeusz Sławiński wrote:
> On our RVP platforms using rt298 with combojack we've seen issues with
> controls being in incorrect state after suspend/resume cycle. This is
> caused by codec driver not setting pins to correct state and causing
> codec suspend method to not be called. Which on resume caused codec
> registers to be in undefined state. Fix this by setting pins correctly
> in jack detect function.

Again fixes should go before cleanups.  Could you be more specific about
what was wrong with the existing code and how this fixes it?  

>  static int rt298_mic_detect(struct snd_soc_component *component,
>  			    struct snd_soc_jack *jack, void *data)
>  {
> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>  	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
> -	struct snd_soc_dapm_context *dapm;
> -	bool hp = false;
> -	bool mic = false;
> -	int status = 0;
>  
>  	rt298->jack = jack;
>  
> -	/* If jack in NULL, disable HS jack */
> -	if (!jack) {
> +	if (jack) {
> +		/* enable IRQ */
> +		if (rt298->jack->status & SND_JACK_HEADPHONE)
> +			snd_soc_dapm_force_enable_pin(dapm, "LDO1");
> +		if (rt298->jack->status & SND_JACK_MICROPHONE) {
> +			snd_soc_dapm_force_enable_pin(dapm, "HV");
> +			snd_soc_dapm_force_enable_pin(dapm, "VREF");
> +		}
> +		regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
> +		enable_irq(rt298->i2c->irq);
> +		snd_soc_jack_report(rt298->jack, rt298->jack->status,
> +				    SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);

It looks rt298_jack_detect() already forces the pins on?  It's not clear
to me what the relationship between this code and the existing code is.

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

Re: [PATCH 11/11] ASoC: codecs: rt298: Fix jack detection

[Amadeusz Sławiński]

On 6/9/2022 4:55 PM, Mark Brown wrote:
> On Thu, Jun 09, 2022 at 03:35:41PM +0200, Amadeusz Sławiński wrote:
>> On our RVP platforms using rt298 with combojack we've seen issues with
>> controls being in incorrect state after suspend/resume cycle. This is
>> caused by codec driver not setting pins to correct state and causing
>> codec suspend method to not be called. Which on resume caused codec
>> registers to be in undefined state. Fix this by setting pins correctly
>> in jack detect function.
> 
> Again fixes should go before cleanups.  Could you be more specific about
> what was wrong with the existing code and how this fixes it?
> 
>>   static int rt298_mic_detect(struct snd_soc_component *component,
>>   			    struct snd_soc_jack *jack, void *data)
>>   {
>> +	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
>>   	struct rt298_priv *rt298 = snd_soc_component_get_drvdata(component);
>> -	struct snd_soc_dapm_context *dapm;
>> -	bool hp = false;
>> -	bool mic = false;
>> -	int status = 0;
>>   
>>   	rt298->jack = jack;
>>   
>> -	/* If jack in NULL, disable HS jack */
>> -	if (!jack) {
>> +	if (jack) {
>> +		/* enable IRQ */
>> +		if (rt298->jack->status & SND_JACK_HEADPHONE)
>> +			snd_soc_dapm_force_enable_pin(dapm, "LDO1");
>> +		if (rt298->jack->status & SND_JACK_MICROPHONE) {
>> +			snd_soc_dapm_force_enable_pin(dapm, "HV");
>> +			snd_soc_dapm_force_enable_pin(dapm, "VREF");
>> +		}
>> +		regmap_update_bits(rt298->regmap, RT298_IRQ_CTRL, 0x2, 0x2);
>> +		enable_irq(rt298->i2c->irq);
>> +		snd_soc_jack_report(rt298->jack, rt298->jack->status,
>> +				    SND_JACK_MICROPHONE | SND_JACK_HEADPHONE);
> 
> It looks rt298_jack_detect() already forces the pins on?  It's not clear
> to me what the relationship between this code and the existing code is.

This aligns the code to be similar to other two rt2xx drivers and fixes 
a problem on our side.

Original code doesn't reach rt298_jack_detect() when jack == NULL, so it 
never disables those pins in this case.

I could probably fix this by moving rt298_jack_detect() call, but as 
drivers for rt2xx codecs are quite similar to each other I opted to fix 
the issue by minimizing the differences between them.

Re: [PATCH 11/11] ASoC: codecs: rt298: Fix jack detection

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Fri, Jun 10, 2022 at 11:46:19AM +0200, Amadeusz Sławiński wrote:
> On 6/9/2022 4:55 PM, Mark Brown wrote:

> > It looks rt298_jack_detect() already forces the pins on?  It's not clear
> > to me what the relationship between this code and the existing code is.

> This aligns the code to be similar to other two rt2xx drivers and fixes a
> problem on our side.

> Original code doesn't reach rt298_jack_detect() when jack == NULL, so it
> never disables those pins in this case.

> I could probably fix this by moving rt298_jack_detect() call, but as drivers
> for rt2xx codecs are quite similar to each other I opted to fix the issue by
> minimizing the differences between them.

It would be good to at least clarify what's going on in the changelog -
it's not really clear what exactly the problem is or how this fixes it.

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

Re: (subset) [PATCH 00/11] ASoC: codecs: Series of fixes for realtek codecs used on RVPs

[Mark Brown]

On Thu, 9 Jun 2022 15:35:30 +0200, Amadeusz Sławiński wrote:
> Our tests platforms do use realtek codecs, while implementing avs driver
> and machine boards for it, we identified quite a lot of problems with
> those codec drivers.
> 
> This series aims to fix those issues.
> 
> Amadeusz Sławiński (8):
>   ASoC: codecs: rt274: Move irq registration and cleanup
>   ASoC: codecs: rt286: Move irq registration and cleanup
>   ASoC: codecs: rt298: Move irq registration and cleanup
>   ASoC: codecs: rt274: Enable irq only when needed
>   ASoC: codecs: rt286: Enable irq only when needed
>   ASoC: codecs: rt298: Enable irq only when needed
>   ASoC: codecs: rt298: Fix NULL jack in interrupt
>   ASoC: codecs: rt298: Fix jack detection
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[01/11] ASoC: codecs: rt274: Always init jack_detect_work
        commit: a43b4394bb35391b74486a788be6634ed91e221a
[02/11] ASoC: codecs: rt286: Reorganize jack detect handling
        commit: 3082afe097cc5d794c28a629f3492a0133ee4891
[03/11] ASoC: codecs: rt298: Reorganize jack detect handling
        commit: 1eb73102da280b28bc3899f694e673bf3e4d0afd

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