[PATCH 00/11] ASoC: codecs: Series of fixes for realtek codecs used on RVPs
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(-)
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; }
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");
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");
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, };
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.
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, };
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",
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) {
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.
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.
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.
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.
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.
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) {
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) {
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);
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.
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; }
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.
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.
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.
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
participants (2)
-
Amadeusz Sławiński -
Mark Brown