Re: [PATCH] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S_2 registers

[Peter Rosin]

On 2021-09-21 10:10, Peter Rosin wrote:
>>> Can you try the attached patch w/o without the defaults for i2s_1/2?
>>> Not even compile tested...
>>
>> [added a couple of underscores]
>>
>> I get this early during boot/probe
>> [    1.506291] pcm512x 0-004c: pcm512x_set_fmt: failed to read I2S_1: -16
>> [    1.512905] pcm512x 0-004c: pcm512x_set_fmt: failed to read PLL_EN: -16
>> [    1.519517] pcm512x 0-004c: Failed to set data format: -16
>> [    1.525045] pcm512x 0-004c: Failed to set data offset: -16
>>
>> and then this later, triggered from userspace when an app opens
>> the device
>> [   14.620344] pcm512x 0-004c: pcm512x_hw_params: I2S_1: 0x2
>>
>> So, reading *can* work.
> 
> I added some traces and verified that accesses to I2S_1/2 fail (as do
> PLL_EN accesses) when the chip is in Powerdown mode (pcm512x_suspend
> has set the RQPD bit in the POWER register), which it is when
> pcm512x_set_fmt runs. During pcm512x_hw_params the chip is in Standby
> mode instead (pcm512x_resume has cleared the RQPD bit, but the RQST
> bit is set).
> 
> So, my patch seems wrong, and the I2S_1/2 accesses instead need to
> move to some point where the chip is not in Powerdown mode.
> 
> Or, is the problem that pcm512x_set_fmt is called while the codec is
> suspended and the chip thus is in Powerdown mode? Because, that
> seems problematic to me?

Ok, so the attached works for me as well. But I don't know if it's
appropriate to resume/suspend like that?

Cheers,
Peter
From ac4d55c7741a13d4f209a63cce94a9acbbbf4f25 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@xxxxxxxxxx>
Date: Tue, 21 Sep 2021 10:31:27 +0200
Subject: [PATCH v2] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S_2
 registers

Commit 25d27c4f68d2 ("ASoC: pcm512x: Add support for more data formats")
breaks the TSE-850 device, which is using a pcm5142 in I2S and
CBM_CFS mode (maybe not relevant). Without this fix, the result
is:

pcm512x 0-004c: Failed to set data format: -16

The root cause is that the chip is in Powerdown mode when
pcm512x_set_fmt runs. So, bring the chip out of suspend for
the update of the format.

Fixes: 25d27c4f68d2 ("ASoC: pcm512x: Add support for more data formats")
Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
---
 sound/soc/codecs/pcm512x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4dc844f3c1fc..07cde6d45233 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1339,6 +1339,7 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	int offset = 0;
 	int clock_output;
 	int master_mode;
+	int resuspend = 0;
 	int ret;
 
 	switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
@@ -1396,6 +1397,11 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return -EINVAL;
 	}
 
+	if (pm_runtime_suspended(component->dev)) {
+		resuspend = 1;
+		pm_runtime_resume(component->dev);
+	}
+
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1,
 				 PCM512x_AFMT, afmt);
 	if (ret != 0) {
@@ -1410,6 +1416,9 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return ret;
 	}
 
+	if (resuspend)
+		pm_runtime_suspend(component->dev);
+
 	pcm512x->fmt = fmt;
 
 	return 0;
-- 
2.20.1