diff mbox series

ASoC: da7219: Improve the relability of AAD IRQ process

Message ID 20230410092634.4870-1-David.Rau.opensource@dm.renesas.com (mailing list archive)
State Superseded
Headers show
Series ASoC: da7219: Improve the relability of AAD IRQ process | expand

Commit Message

David Rau April 10, 2023, 9:26 a.m. UTC 
This commit improves the control of ground switches in AAD IRQ

Signed-off-by: David Rau <David.Rau.opensource@dm.renesas.com>
---
 sound/soc/codecs/da7219-aad.c | 60 +++++++++++++++++------------------
 sound/soc/codecs/da7219-aad.h |  5 ++-
 2 files changed, 31 insertions(+), 34 deletions(-)

Comments

Mark Brown April 11, 2023, 11:32 a.m. UTC | #1 
On Mon, Apr 10, 2023 at 09:26:34AM +0000, David Rau wrote:

> This commit improves the control of ground switches in AAD IRQ

In what way does it do this - what was previously unrelabile and how
does this change address that?
Guenter Roeck April 11, 2023, 2:28 p.m. UTC | #2 
On Tue, Apr 11, 2023 at 4:32 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Apr 10, 2023 at 09:26:34AM +0000, David Rau wrote:
>
> > This commit improves the control of ground switches in AAD IRQ
>
> In what way does it do this - what was previously unrelabile and how
> does this change address that?

One very specific problem is that da7219_aad_handle_gnd_switch_time()
is currently called after interrupts were enabled. As a result, the
delay time is not initialized if there is an interrupt before the
initialization. This results in a negative value passed to msleep().
Since the parameter to msleep() is unsigned, this causes it to sleep
forever which in turn causes a substantial number of hung task crashes
in ChromeOS. Plus, of course, the code doesn't really do anything in
this situation.

A secondary problem may be that calling msleep() with a potentially
large sleep time on a system worker isn't really a good idea, but I
didn't explore the impact further.

Guenter
Baili Deng April 12, 2023, 2:32 a.m. UTC | #3 
The change in the patch done to address the issue Geunter mentioned is that
da7219_aad_handle_gnd_switch_time() is now called before interrupts are
enabled. To address the msleep() issue, the delay is now added as a delayed
work of its own workqueue.

On Tue, Apr 11, 2023 at 10:28 PM Guenter Roeck <groeck@google.com> wrote:

> On Tue, Apr 11, 2023 at 4:32 AM Mark Brown <broonie@kernel.org> wrote:
> >
> > On Mon, Apr 10, 2023 at 09:26:34AM +0000, David Rau wrote:
> >
> > > This commit improves the control of ground switches in AAD IRQ
> >
> > In what way does it do this - what was previously unrelabile and how
> > does this change address that?
>
> One very specific problem is that da7219_aad_handle_gnd_switch_time()
> is currently called after interrupts were enabled. As a result, the
> delay time is not initialized if there is an interrupt before the
> initialization. This results in a negative value passed to msleep().
> Since the parameter to msleep() is unsigned, this causes it to sleep
> forever which in turn causes a substantial number of hung task crashes
> in ChromeOS. Plus, of course, the code doesn't really do anything in
> this situation.
>
> A secondary problem may be that calling msleep() with a potentially
> large sleep time on a system worker isn't really a good idea, but I
> didn't explore the impact further.
>
> Guenter
>
Mark Brown April 12, 2023, 12:09 p.m. UTC | #4 
On Wed, Apr 12, 2023 at 10:32:47AM +0800, Baili Deng wrote:
> The change in the patch done to address the issue Geunter mentioned is that
> da7219_aad_handle_gnd_switch_time() is now called before interrupts are
> enabled. To address the msleep() issue, the delay is now added as a delayed
> work of its own workqueue.

The point with the question was that this sort of stuff should be in the
commit messages.  I can't really tell what the change is supposed to do
as things stand.

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.
David Rau April 12, 2023, 3:27 p.m. UTC | #5 
>On Wed, Apr 12, 2023 at 10:32:47AM +0800, Baili Deng wrote:
>> The change in the patch done to address the issue Geunter mentioned is 
>> that
>> da7219_aad_handle_gnd_switch_time() is now called before interrupts 
>> are enabled. To address the msleep() issue, the delay is now added as 
>> a delayed work of its own workqueue.

>The point with the question was that this sort of stuff should be in the commit messages.  I can't really tell what the change is supposed to do as things stand.
Sorry for missing such needed information in the previous commit message.
May I send another commit that includes the complete information again?
Thanks.
Mark Brown April 12, 2023, 3:58 p.m. UTC | #6 
On Wed, Apr 12, 2023 at 03:27:01PM +0000, David.Rau.opensource wrote:

> Sorry for missing such needed information in the previous commit message.
> May I send another commit that includes the complete information again?

Yes, please.
diff mbox series

Patch

diff --git a/sound/soc/codecs/da7219-aad.c b/sound/soc/codecs/da7219-aad.c
index e3d398b8f54e..993a0d00bc48 100644
--- a/sound/soc/codecs/da7219-aad.c
+++ b/sound/soc/codecs/da7219-aad.c
@@ -342,36 +342,17 @@  static void da7219_aad_hptest_work(struct work_struct *work)
 static void da7219_aad_jack_det_work(struct work_struct *work)
 {
 	struct da7219_aad_priv *da7219_aad =
-		container_of(work, struct da7219_aad_priv, jack_det_work);
+		container_of(work, struct da7219_aad_priv, jack_det_work.work);
 	struct snd_soc_component *component = da7219_aad->component;
-	u8 srm_st;
 
-	mutex_lock(&da7219_aad->jack_det_mutex);
-
-	srm_st = snd_soc_component_read(component, DA7219_PLL_SRM_STS) & DA7219_PLL_SRM_STS_MCLK;
-	msleep(da7219_aad->gnd_switch_delay * ((srm_st == 0x0) ? 2 : 1) - 4);
 	/* Enable ground switch */
 	snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
-
-	mutex_unlock(&da7219_aad->jack_det_mutex);
 }
 
-
 /*
  * IRQ
  */
 
-static irqreturn_t da7219_aad_pre_irq_thread(int irq, void *data)
-{
-
-	struct da7219_aad_priv *da7219_aad = data;
-
-	if (!da7219_aad->jack_inserted)
-		schedule_work(&da7219_aad->jack_det_work);
-
-	return IRQ_WAKE_THREAD;
-}
-
 static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 {
 	struct da7219_aad_priv *da7219_aad = data;
@@ -392,6 +373,18 @@  static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 	/* Read status register for jack insertion & type status */
 	statusa = snd_soc_component_read(component, DA7219_ACCDET_STATUS_A);
 
+	if (events[DA7219_AAD_IRQ_REG_A] & DA7219_E_JACK_INSERTED_MASK) {
+		u8 srm_st;
+		int delay = 0;
+
+		srm_st = snd_soc_component_read(component,
+					DA7219_PLL_SRM_STS) & DA7219_PLL_SRM_STS_MCLK;
+		delay = (da7219_aad->gnd_switch_delay * ((srm_st == 0x0) ? 2 : 1) - 2);
+		queue_delayed_work(da7219_aad->aad_wq,
+							&da7219_aad->jack_det_work,
+							msecs_to_jiffies(delay));
+	}
+
 	/* Clear events */
 	regmap_bulk_write(da7219->regmap, DA7219_ACCDET_IRQ_EVENT_A,
 			  events, DA7219_AAD_IRQ_REG_MAX);
@@ -400,9 +393,6 @@  static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 		events[DA7219_AAD_IRQ_REG_A], events[DA7219_AAD_IRQ_REG_B],
 		statusa);
 
-	if (!da7219_aad->jack_inserted)
-		cancel_work_sync(&da7219_aad->jack_det_work);
-
 	if (statusa & DA7219_JACK_INSERTION_STS_MASK) {
 		/* Jack Insertion */
 		if (events[DA7219_AAD_IRQ_REG_A] &
@@ -430,9 +420,9 @@  static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 			if (statusa & DA7219_JACK_TYPE_STS_MASK) {
 				report |= SND_JACK_HEADSET;
 				mask |=	SND_JACK_HEADSET | SND_JACK_LINEOUT;
-				schedule_work(&da7219_aad->btn_det_work);
+				queue_work(da7219_aad->aad_wq, &da7219_aad->btn_det_work);
 			} else {
-				schedule_work(&da7219_aad->hptest_work);
+				queue_work(da7219_aad->aad_wq, &da7219_aad->hptest_work);
 			}
 		}
 
@@ -465,6 +455,7 @@  static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
 			da7219_aad->jack_inserted = false;
 
 			/* Cancel any pending work */
+			cancel_delayed_work_sync(&da7219_aad->jack_det_work);
 			cancel_work_sync(&da7219_aad->btn_det_work);
 			cancel_work_sync(&da7219_aad->hptest_work);
 
@@ -964,13 +955,19 @@  int da7219_aad_init(struct snd_soc_component *component)
 	snd_soc_component_update_bits(component, DA7219_ACCDET_CONFIG_1,
 			    DA7219_BUTTON_CONFIG_MASK, 0);
 
+	da7219_aad_handle_gnd_switch_time(component);
+
+	da7219_aad->aad_wq = create_singlethread_workqueue("da7219-aad");
+	if (!da7219_aad->aad_wq) {
+		dev_err(component->dev, "Failed to create aad workqueue\n");
+		return -ENOMEM;
+	}
+
+	INIT_DELAYED_WORK(&da7219_aad->jack_det_work, da7219_aad_jack_det_work);
 	INIT_WORK(&da7219_aad->btn_det_work, da7219_aad_btn_det_work);
 	INIT_WORK(&da7219_aad->hptest_work, da7219_aad_hptest_work);
-	INIT_WORK(&da7219_aad->jack_det_work, da7219_aad_jack_det_work);
-
-	mutex_init(&da7219_aad->jack_det_mutex);
 
-	ret = request_threaded_irq(da7219_aad->irq, da7219_aad_pre_irq_thread,
+	ret = request_threaded_irq(da7219_aad->irq, NULL,
 				   da7219_aad_irq_thread,
 				   IRQF_TRIGGER_LOW | IRQF_ONESHOT,
 				   "da7219-aad", da7219_aad);
@@ -984,8 +981,6 @@  int da7219_aad_init(struct snd_soc_component *component)
 	regmap_bulk_write(da7219->regmap, DA7219_ACCDET_IRQ_MASK_A,
 			  &mask, DA7219_AAD_IRQ_REG_MAX);
 
-	da7219_aad_handle_gnd_switch_time(component);
-
 	return 0;
 }
 
@@ -1002,8 +997,10 @@  void da7219_aad_exit(struct snd_soc_component *component)
 
 	free_irq(da7219_aad->irq, da7219_aad);
 
+	cancel_delayed_work_sync(&da7219_aad->jack_det_work);
 	cancel_work_sync(&da7219_aad->btn_det_work);
 	cancel_work_sync(&da7219_aad->hptest_work);
+	destroy_workqueue(da7219_aad->aad_wq);
 }
 
 /*
@@ -1031,4 +1028,5 @@  int da7219_aad_probe(struct i2c_client *i2c)
 
 MODULE_DESCRIPTION("ASoC DA7219 AAD Driver");
 MODULE_AUTHOR("Adam Thomson <Adam.Thomson.Opensource@diasemi.com>");
+MODULE_AUTHOR("David Rau <David.Rau.opensource@dm.renesas.com>");
 MODULE_LICENSE("GPL");
diff --git a/sound/soc/codecs/da7219-aad.h b/sound/soc/codecs/da7219-aad.h
index be87ee47edde..fbfbf3e67918 100644
--- a/sound/soc/codecs/da7219-aad.h
+++ b/sound/soc/codecs/da7219-aad.h
@@ -197,9 +197,8 @@  struct da7219_aad_priv {
 
 	struct work_struct btn_det_work;
 	struct work_struct hptest_work;
-	struct work_struct jack_det_work;
-
-	struct mutex  jack_det_mutex;
+	struct delayed_work jack_det_work;
+	struct workqueue_struct *aad_wq;
 
 	struct snd_soc_jack *jack;
 	bool micbias_resume_enable;