diff mbox series

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

Message ID 20220609133541.3984886-8-amadeuszx.slawinski@linux.intel.com (mailing list archive)
State Superseded
Headers show
Series ASoC: codecs: Series of fixes for realtek codecs used on RVPs | expand

Commit Message

Amadeusz Sławiński June 9, 2022, 1:35 p.m. UTC 
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(-)

Comments

Mark Brown June 9, 2022, 2:18 p.m. UTC | #1 
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.
Amadeusz Sławiński June 10, 2022, 9:33 a.m. UTC | #2 
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.
Mark Brown June 10, 2022, 10:38 a.m. UTC | #3 
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.
Amadeusz Sławiński June 23, 2022, 1:53 p.m. UTC | #4 
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.
Mark Brown June 23, 2022, 2:23 p.m. UTC | #5 
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.
diff mbox series

Patch

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) {