Thanks for the kind feedback.
Would you please let me know what kinds of environment such error appears you ever meet?
Ex: da7219_aad->gnd_switch_delay = ?
-----Original Message-----
From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
Sent: Friday, January 20, 2023 00:12
To: David Rau <david.rau.zg@xxxxxxxxxxx>
Cc: David Rau <we730128@xxxxxxxxx>; perex@xxxxxxxx; lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx; tiwai@xxxxxxxx; support.opensource@xxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [PATCH] ASoC: da7219: Fix pole orientation detection on OMTP headsets when playing music
On Thu, Jan 19, 2023 at 11:02:25AM +0000, David Rau wrote:
> Would you please provide me the related error messages when hung task crashes in da7219_aad_irq_thread()?
> BTW, "gnd_switch_delay = 256" is an unusual use case of the longer jack detection latency.
>
Here is a typical traceback.
<3>[ 246.919057] INFO: task irq/105-da7219-:2854 blocked for more than 122 seconds.
<3>[ 246.919065] Not tainted 5.10.159-20927-g317f62e2494d #1 <3>[ 246.919068] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
<6>[ $PHONE_NUMBER] task:irq/105-da7219- state:D stack: 0 pid: 2854 ppid: 2 flags:0x00004080 <6>[ 246.919075] Call Trace:
<6>[ 246.919084] __schedule+0x3b0/0xdaf
<6>[ 246.919090] schedule+0x44/0xa8
<6>[ 246.919093] schedule_timeout+0xb6/0x290 <6>[ 246.919098] ? run_local_timers+0x4e/0x4e <6>[ 246.919102] msleep+0x2c/0x38 <6>[ 246.919108] da7219_aad_irq_thread+0x66/0x2b0 [snd_soc_da7219 cd5a76eef6e777074216b9d61f7918f7561bf7ec]
<6>[ 246.919113] ? irq_forced_thread_fn+0x5f/0x5f <6>[ 246.919116] irq_thread_fn+0x22/0x4d <6>[ 246.919120] irq_thread+0x120/0x19d <6>[ 246.919123] ? irq_thread_fn+0x4d/0x4d <6>[ 246.919128] kthread+0x142/0x153 <6>[ 246.919132] ? irq_forced_secondary_handler+0x21/0x21
<6>[ 246.919135] ? kthread_blkcg+0x31/0x31 <6>[ 246.919139] ret_from_fork+0x1f/0x30
The underlying question is if it really appropriate to have an
msleep() of any kind in an interrupt handler. If this is about debouncing a signal, it should be handled with a delayed timer.
Guenter
> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Wednesday, January 18, 2023 03:57
> To: David Rau <we730128@xxxxxxxxx>
> Cc: perex@xxxxxxxx; lgirdwood@xxxxxxxxx; broonie@xxxxxxxxxx;
> tiwai@xxxxxxxx; support.opensource@xxxxxxxxxxx;
> alsa-devel@xxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; David Rau
> <david.rau.zg@xxxxxxxxxxx>
> Subject: Re: [PATCH] ASoC: da7219: Fix pole orientation detection on
> OMTP headsets when playing music
>
> On Mon, Nov 21, 2022 at 05:07:44AM +0000, David Rau wrote:
> > The OMTP pin define headsets can be mis-detected as line out instead
> > of OMTP, causing obvious issues with audio quality.
> > This patch is to put increased resistances within the device at a
> > suitable point.
> >
> > To solve this issue better, the new mechanism setup ground switches
> > with conditional delay control and these allow for more stabile
> > detection process to operate as intended. This conditional delay
> > control will not impact the hardware process but use extra system
> > resource.
> >
> > This commit improves control of ground switches in the AAD logic.
> >
> > Signed-off-by: David Rau <david.rau.zg@xxxxxxxxxxx>
> > ---
> > sound/soc/codecs/da7219-aad.c | 42
> > ++++++++++++++++++++++++++++++-----
> > sound/soc/codecs/da7219-aad.h | 1 +
> > 2 files changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/sound/soc/codecs/da7219-aad.c
> > b/sound/soc/codecs/da7219-aad.c index bba73c44c219..08200ec259f9
> > 100644
> > --- a/sound/soc/codecs/da7219-aad.c
> > +++ b/sound/soc/codecs/da7219-aad.c
> > @@ -352,9 +352,14 @@ static irqreturn_t da7219_aad_irq_thread(int irq, void *data)
> > struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
> > struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
> > u8 events[DA7219_AAD_IRQ_REG_MAX];
> > - u8 statusa;
> > + u8 statusa, srm_st;
> > int i, report = 0, mask = 0;
> >
> > + 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);
>
> Ever since this patch was applied to ChromeOS, we have observed hung task crashes in da7219_aad_irq_thread().
>
> Is it really appropriate to sleep up to (256 * 2) - 4 = 508 ms in an interrupt handler ?
>
> Thanks,
> Guenter
>
> > + /* Enable ground switch */
> > + snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
> > +
> > /* Read current IRQ events */
> > regmap_bulk_read(da7219->regmap, DA7219_ACCDET_IRQ_EVENT_A,
> > events, DA7219_AAD_IRQ_REG_MAX); @@ -454,8 +459,8 @@ static
> > irqreturn_t da7219_aad_irq_thread(int irq, void *data)
> > snd_soc_dapm_disable_pin(dapm, "Mic Bias");
> > snd_soc_dapm_sync(dapm);
> >
> > - /* Enable ground switch */
> > - snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
> > + /* Disable ground switch */
> > + snd_soc_component_update_bits(component, 0xFB, 0x01, 0x00);
> > }
> > }
> >
> > @@ -831,6 +836,32 @@ static void da7219_aad_handle_pdata(struct snd_soc_component *component)
> > }
> > }
> >
> > +static void da7219_aad_handle_gnd_switch_time(struct
> > +snd_soc_component *component) {
> > + struct da7219_priv *da7219 = snd_soc_component_get_drvdata(component);
> > + struct da7219_aad_priv *da7219_aad = da7219->aad;
> > + u8 jack_det;
> > +
> > + jack_det = snd_soc_component_read(component, DA7219_ACCDET_CONFIG_2)
> > + & DA7219_JACK_DETECT_RATE_MASK;
> > + switch (jack_det) {
> > + case 0x00:
> > + da7219_aad->gnd_switch_delay = 32;
> > + break;
> > + case 0x10:
> > + da7219_aad->gnd_switch_delay = 64;
> > + break;
> > + case 0x20:
> > + da7219_aad->gnd_switch_delay = 128;
> > + break;
> > + case 0x30:
> > + da7219_aad->gnd_switch_delay = 256;
> > + break;
> > + default:
> > + da7219_aad->gnd_switch_delay = 32;
> > + break;
> > + }
> > +}
> >
> > /*
> > * Suspend/Resume
> > @@ -908,9 +939,6 @@ int da7219_aad_init(struct snd_soc_component *component)
> > snd_soc_component_update_bits(component, DA7219_ACCDET_CONFIG_1,
> > DA7219_BUTTON_CONFIG_MASK, 0);
> >
> > - /* Enable ground switch */
> > - snd_soc_component_update_bits(component, 0xFB, 0x01, 0x01);
> > -
> > INIT_WORK(&da7219_aad->btn_det_work, da7219_aad_btn_det_work);
> > INIT_WORK(&da7219_aad->hptest_work, da7219_aad_hptest_work);
> >
> > @@ -928,6 +956,8 @@ 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;
> > }
> > EXPORT_SYMBOL_GPL(da7219_aad_init);
> > diff --git a/sound/soc/codecs/da7219-aad.h
> > b/sound/soc/codecs/da7219-aad.h index f48a12012ef3..21fdf53095cc
> > 100644
> > --- a/sound/soc/codecs/da7219-aad.h
> > +++ b/sound/soc/codecs/da7219-aad.h
> > @@ -187,6 +187,7 @@ enum da7219_aad_event_regs { struct
> > da7219_aad_priv {
> > struct snd_soc_component *component;
> > int irq;
> > + int gnd_switch_delay;
> >
> > u8 micbias_pulse_lvl;
> > u32 micbias_pulse_time;
> > --
> > 2.17.1
> >
