RE: [PATCH 1/5] ASoC: rt5682-sdw: fix for JD event handling in ClockStop Mode0

["Shuming [范書銘]" <shumingf@realtek.com>]

> > During ClockStop Mode0, peripheral interrupts are disabled.
> 
> I can see that the interrupts are disabled in rt5682_dev_system_suspend(),
> which is NOT a mode where the clock stop is used... I don't think this commit
> message is correct.
> 
> The IMPL_DEF interrupt which is used for jack detection is not disabled at all
> during any clock stop mode, and it shouldn't otherwise that would break the
> jack detection.

You are right. The commit message is wrong and not clear.
The situation is that the manager driver uses the clock stop mode0 to do system suspension.
The SdW device will not be re-attached when the system resume.
Therefore, the INT1_IMPL_DEF/SDCA_INTMASK interrupt will need to be enabled when the system resumes.

> > When system level resume is invoked, Peripheral SDCA interrupts should
> > be enabled to handle JD events.
> 
> The initial 'when system level resume is invoked' is aligned with my comment
> above, this interrupt disabling is only for system suspend.
> 
> In addition, I think it's a copy paste here, there is no SDCA support in
> RT5682 or the initial RT711.

Will fix copy paste issue.

> > Enable SDCA interrupts in resume sequence when ClockStop Mode0 is
> applied.
> 
> same comments for rt711-sdw.c and other codecs which use the same pattern
> with the same comment
> 
>         /*
>          * prevent new interrupts from being handled after the
>          * deferred work completes and before the parent disables
>          * interrupts on the link
>          */
> 
> BTW if this is an issue for system suspend, maybe we should disable the
> interrupts on the link in the .prepare stage, that would remove this step in all
> codec drivers? The .prepare deals with the parent first, while .suspend deal
> with child devices first. The drawback would be that the codec driver would
> depend on the manager driver doing the right thing, which isn't great.

I think the codec driver could handle that. The SDCA codec driver already re-enabled the SDCA INT mask when the device is re-attached.

> > Signed-off-by: Shuming Fan <shumingf@realtek.com>
> > ---
> >  sound/soc/codecs/rt5682-sdw.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/sound/soc/codecs/rt5682-sdw.c
> > b/sound/soc/codecs/rt5682-sdw.c index 67404f45389f..4968a8c0064d
> > 100644
> > --- a/sound/soc/codecs/rt5682-sdw.c
> > +++ b/sound/soc/codecs/rt5682-sdw.c
> > @@ -750,8 +750,15 @@ static int __maybe_unused
> rt5682_dev_resume(struct device *dev)
> >       if (!rt5682->first_hw_init)
> >               return 0;
> >
> > -     if (!slave->unattach_request)
> > +     if (!slave->unattach_request) {
> > +             if (rt5682->disable_irq == true) {
> > +                     mutex_lock(&rt5682->disable_irq_lock);
> > +                     sdw_write_no_pm(slave, SDW_SCP_INTMASK1,
> SDW_SCP_INT1_IMPL_DEF);
> > +                     rt5682->disable_irq = false;
> > +                     mutex_unlock(&rt5682->disable_irq_lock);
> > +             }
> >               goto regmap_sync;
> > +     }
> >
> >       time =
> wait_for_completion_timeout(&slave->initialization_complete,
> >
> msecs_to_jiffies(RT5682_PROBE_TIMEOUT));
> 
> ------Please consider the environment before printing this e-mail.