[PATCH] ASoC: da7219: Improve the relability of AAD IRQ process
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(-)
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;
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
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
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.
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.
participants (5)
-
Baili Deng -
David Rau -
David.Rau.opensource
-
Guenter Roeck -
Mark Brown