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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 7/3/23 11:02, shumingf@realtek.com wrote:
> From: Shuming Fan <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.

> 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.

> 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.

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