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

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

[Peter Rosin]

From 625f858894af2b7e547cc723b97361081438b123 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>

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

And after that, no sound.

This fix is not 100% correct. The datasheet of at least the pcm5142
states that four bits (0xcc) in the I2S_1 register are "RSV"
("Reserved. Do not access.") and no hint is given as to what the
initial values are supposed to be. So, specifying defaults for
these bits is wrong. But perhaps better than a broken driver?

Fixes: 25d27c4f68d2 ("ASoC: pcm512x: Add support for more data formats")
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Jaroslav Kysela <perex@perex.cz>
Cc: Takashi Iwai <tiwai@suse.com>
Cc: Kirill Marinushkin <kmarinushkin@birdec.com>
Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
Cc: alsa-devel@alsa-project.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Peter Rosin <peda@axentia.se>
---
 sound/soc/codecs/pcm512x.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4dc844f3c1fc..60dee41816dc 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -116,6 +116,8 @@ static const struct reg_default pcm512x_reg_defaults[] = {
 	{ PCM512x_FS_SPEED_MODE,     0x00 },
 	{ PCM512x_IDAC_1,            0x01 },
 	{ PCM512x_IDAC_2,            0x00 },
+	{ PCM512x_I2S_1,             0x02 },
+	{ PCM512x_I2S_2,             0x00 },
 };
 
 static bool pcm512x_readable(struct device *dev, unsigned int reg)
-- 
2.20.1

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

[Péter Ujfalusi]

Hi Peter,

On 9/20/21 17:49, Peter Rosin wrote:
> From 625f858894af2b7e547cc723b97361081438b123 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> 
> 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
> 
> And after that, no sound.

Is it possible that on the board the pcm5142 is in hardwired mode (MODE1
and MODE2 pin is pulled low)? If it is and FMT pin is also low then the
codec is in i2s mode.

I can imagine that the codec would ignore a write to AFMT part of I2S_1
register - it is wired for 00b.

> This fix is not 100% correct. The datasheet of at least the pcm5142
> states that four bits (0xcc) in the I2S_1 register are "RSV"
> ("Reserved. Do not access.") and no hint is given as to what the
> initial values are supposed to be. So, specifying defaults for
> these bits is wrong. But perhaps better than a broken driver?

The default of 0x02 (AFMT = 00b - I2S, ALEN = 10b - 24bits) is correct
for I2S_1 and the 0 is the default of I2S_2.

The failure happens when updating the AFMT (bit4-5) and when regmap is
doing a i2c read since the default is not specified.

It would be interesting to see what it is reading... Out of interest:
can you mar the I2S_1 and I2S_2 as volatile and read / print the value
just before the afmt and alen update?

> Fixes: 25d27c4f68d2 ("ASoC: pcm512x: Add support for more data formats")
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Jaroslav Kysela <perex@perex.cz>
> Cc: Takashi Iwai <tiwai@suse.com>
> Cc: Kirill Marinushkin <kmarinushkin@birdec.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
> Cc: alsa-devel@alsa-project.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Peter Rosin <peda@axentia.se>

The defaults for the two registers are OK, should be safe ;)

Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>

> ---
>  sound/soc/codecs/pcm512x.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
> index 4dc844f3c1fc..60dee41816dc 100644
> --- a/sound/soc/codecs/pcm512x.c
> +++ b/sound/soc/codecs/pcm512x.c
> @@ -116,6 +116,8 @@ static const struct reg_default pcm512x_reg_defaults[] = {
>  	{ PCM512x_FS_SPEED_MODE,     0x00 },
>  	{ PCM512x_IDAC_1,            0x01 },
>  	{ PCM512x_IDAC_2,            0x00 },
> +	{ PCM512x_I2S_1,             0x02 },
> +	{ PCM512x_I2S_2,             0x00 },
>  };
>  
>  static bool pcm512x_readable(struct device *dev, unsigned int reg)
> 

-- 
Péter

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

[Peter Rosin]

Hi Péter,

On 2021-09-20 20:37, Péter Ujfalusi wrote:
> Hi Peter,
> 
> On 9/20/21 17:49, Peter Rosin wrote:
>> From 625f858894af2b7e547cc723b97361081438b123 Mon Sep 17 00:00:00 2001
>> From: Peter Rosin <peda@axentia.se>
>>
>> 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
>>
>> And after that, no sound.
> 
> Is it possible that on the board the pcm5142 is in hardwired mode (MODE1
> and MODE2 pin is pulled low)? If it is and FMT pin is also low then the
> codec is in i2s mode.
> 
> I can imagine that the codec would ignore a write to AFMT part of I2S_1
> register - it is wired for 00b.

Nope, MODE1/2 are wired for I2C -> FMT is just the last I2C address
bit. So, nothing to do with I2S. And I can't see how that would explain
the same problem with the I2S_2 register?

>> This fix is not 100% correct. The datasheet of at least the pcm5142
>> states that four bits (0xcc) in the I2S_1 register are "RSV"
>> ("Reserved. Do not access.") and no hint is given as to what the
>> initial values are supposed to be. So, specifying defaults for
>> these bits is wrong. But perhaps better than a broken driver?
> 
> The default of 0x02 (AFMT = 00b - I2S, ALEN = 10b - 24bits) is correct
> for I2S_1 and the 0 is the default of I2S_2.
> 
> The failure happens when updating the AFMT (bit4-5) and when regmap is
> doing a i2c read since the default is not specified.
> 
> It would be interesting to see what it is reading... Out of interest:
> can you mar the I2S_1 and I2S_2 as volatile and read / print the value
> just before the afmt and alen update?

My first attempt was to mark the register volatile, and then read and
compare if the update was needed at all. But marking volatile wasn't
enough. I also tried to set both a default and mark as volatile,
but it seems every read fails with -16 (EBUSY). I don't get why, to me
it almost feels like a regmap issue of some sort (probably the regmap
config is bad in some way), but I'm not fluent in regmap...

So, since I can't read, I can't get to the initial values of the four
reserved bits. So, I winged them as zero.

Cheers,
Peter

>> Fixes: 25d27c4f68d2 ("ASoC: pcm512x: Add support for more data formats")
>> Cc: Liam Girdwood <lgirdwood@gmail.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Jaroslav Kysela <perex@perex.cz>
>> Cc: Takashi Iwai <tiwai@suse.com>
>> Cc: Kirill Marinushkin <kmarinushkin@birdec.com>
>> Cc: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> Cc: alsa-devel@alsa-project.org
>> Cc: linux-kernel@vger.kernel.org
>> Signed-off-by: Peter Rosin <peda@axentia.se>
> 
> The defaults for the two registers are OK, should be safe ;)
> 
> Reviewed-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
> 
>> ---
>>  sound/soc/codecs/pcm512x.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
>> index 4dc844f3c1fc..60dee41816dc 100644
>> --- a/sound/soc/codecs/pcm512x.c
>> +++ b/sound/soc/codecs/pcm512x.c
>> @@ -116,6 +116,8 @@ static const struct reg_default pcm512x_reg_defaults[] = {
>>  	{ PCM512x_FS_SPEED_MODE,     0x00 },
>>  	{ PCM512x_IDAC_1,            0x01 },
>>  	{ PCM512x_IDAC_2,            0x00 },
>> +	{ PCM512x_I2S_1,             0x02 },
>> +	{ PCM512x_I2S_2,             0x00 },
>>  };
>>  
>>  static bool pcm512x_readable(struct device *dev, unsigned int reg)
>>
> 

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Mon, Sep 20, 2021 at 09:37:37PM +0200, Peter Rosin wrote:

> compare if the update was needed at all. But marking volatile wasn't
> enough. I also tried to set both a default and mark as volatile,
> but it seems every read fails with -16 (EBUSY). I don't get why, to me
> it almost feels like a regmap issue of some sort (probably the regmap
> config is bad in some way), but I'm not fluent in regmap...

Having a default for a volatile register isn't really a sensible
configuration since the whole point with volatile registers is
that they change underneath us, I'd not be surprised if we had
some error checking code in there that was trying to tell you
there was a problem though it does seem like it should at least
be more verbose about it since returning -EBUSY isn't exactly
helpful by itself.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Peter Rosin]

On 2021-09-20 23:29, Mark Brown wrote:
> On Mon, Sep 20, 2021 at 09:37:37PM +0200, Peter Rosin wrote:
> 
>> compare if the update was needed at all. But marking volatile wasn't
>> enough. I also tried to set both a default and mark as volatile,
>> but it seems every read fails with -16 (EBUSY). I don't get why, to me
>> it almost feels like a regmap issue of some sort (probably the regmap
>> config is bad in some way), but I'm not fluent in regmap...
> 
> Having a default for a volatile register isn't really a sensible
> configuration since the whole point with volatile registers is
> that they change underneath us, I'd not be surprised if we had
> some error checking code in there that was trying to tell you
> there was a problem though it does seem like it should at least
> be more verbose about it since returning -EBUSY isn't exactly
> helpful by itself.

I totally agree that it's not a sensible config to set up a register
with a default when it's marked as volatile. That was just a wild
attempt.

I expected it to just work to mark the register as readable and do
without the default value (i.e. the way it was before my patch). What
I don't understand is why regmap returns -EBUSY in that case. That
doesn't make sense to me. Perhaps that -EBUSY is propagated from the
I2C layer, but in that case, why is it then ok to do a write to
another register at the same spot in the code? So, why -EBUSY?

Something is going on that is not understood. At least not by me.

Cheers,
Peter

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 763 bytes --]

On Tue, Sep 21, 2021 at 08:37:28AM +0200, Peter Rosin wrote:

> I expected it to just work to mark the register as readable and do
> without the default value (i.e. the way it was before my patch). What
> I don't understand is why regmap returns -EBUSY in that case. That
> doesn't make sense to me. Perhaps that -EBUSY is propagated from the
> I2C layer, but in that case, why is it then ok to do a write to
> another register at the same spot in the code? So, why -EBUSY?

Actually one thing that can trigger this now I think about it is
attempting to access a volatile register when the device is in cache
only mode for power management reasons - if the device is in cache only
mode then you can't do a hardware read so volatile registers become
inaccessible.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Péter Ujfalusi]

[-- Attachment #1: Type: text/plain, Size: 4825 bytes --]

Hi Peter.

On 20/09/2021 22:37, Peter Rosin wrote:

> Nope, MODE1/2 are wired for I2C -> FMT is just the last I2C address
> bit. So, nothing to do with I2S. And I can't see how that would explain
> the same problem with the I2S_2 register?

true, but worth the question ;)
 
>>> This fix is not 100% correct. The datasheet of at least the pcm5142
>>> states that four bits (0xcc) in the I2S_1 register are "RSV"
>>> ("Reserved. Do not access.") and no hint is given as to what theHi
>>> initial values are supposed to be. So, specifying defaults for
>>> these bits is wrong. But perhaps better than a broken driver?
>>
>> The default of 0x02 (AFMT = 00b - I2S, ALEN = 10b - 24bits) is correct
>> for I2S_1 and the 0 is the default of I2S_2.
>>
>> The failure happens when updating the AFMT (bit4-5) and when regmap is
>> doing a i2c read since the default is not specified.
>>
>> It would be interesting to see what it is reading... Out of interest:
>> can you mar the I2S_1 and I2S_2 as volatile and read / print the value
>> just before the afmt and alen update?
> 
> My first attempt was to mark the register volatile, and then read and
> compare if the update was needed at all. But marking volatile wasn't
> enough.

If the value is not provided in the defaults then the first read is reading out to the chip anyways.

> I also tried to set both a default and mark as volatile,

Volatile always skips the cache, default would be ignored.

> but it seems every read fails with -16 (EBUSY). I don't get why, to me
> it almost feels like a regmap issue of some sort (probably the regmap
> config is bad in some way), but I'm not fluent in regmap...

Or most likely the chip is not powered at pcm512x_set_fmt() time?

> So, since I can't read, I can't get to the initial values of the four
> reserved bits. So, I winged them as zero.

The value of the reserved bits are don't care.

Can you try the attached patch w/o without the defaults for i2s_1/2?
Not even compile tested...

From e013a03286b6a744914c50239d3123b7723068df Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi <peter.ujfalusi@gmail.com>
Date: Tue, 21 Sep 2021 07:12:06 +0300
Subject: [PATCH] ASoC: pcm512x: test regmap register accesses

read i2c_1 in different stages.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
---
 sound/soc/codecs/pcm512x.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4dc844f3c1fc..d0382e9ac329 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1166,6 +1166,7 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_component *component = dai->component;
 	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+	unsigned int val;
 	int alen;
 	int gpio;
 	int ret;
@@ -1193,6 +1194,18 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	ret = regmap_read(pcm512x->regmap, PCM512x_I2S_1, &val);
+	if (ret) {
+		dev_err(component->dev, "%s: failed to read I2S_1: %d\n", __func_, ret);
+		ret = regmap_read(pcm512x->regmap, PCM512x_PLL_EN, &val);
+		if (ret)
+			dev_err(component->dev, "%s: failed to read PLL_EN: %d\n", __func__, ret);
+		else
+			dev_err(component->dev, "%s: PLL_EN: %#x\n", __func__, val);
+	} else {
+		dev_err(component->dev, "%s: I2S_1: %#x\n", __func__, val);
+	}
+
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1,
 				 PCM512x_ALEN, alen);
 	if (ret != 0) {
@@ -1335,6 +1348,7 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct snd_soc_component *component = dai->component;
 	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+	unsigned int val;
 	int afmt;
 	int offset = 0;
 	int clock_output;
@@ -1396,18 +1410,28 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return -EINVAL;
 	}
 
+	ret = regmap_read(pcm512x->regmap, PCM512x_I2S_1, &val);
+	if (ret) {
+		dev_err(component->dev, "%s: failed to read I2S_1: %d\n", __func_, ret);
+		ret = regmap_read(pcm512x->regmap, PCM512x_PLL_EN, &val);
+		if (ret)
+			dev_err(component->dev, "%s: failed to read PLL_EN: %d\n", __func__, ret);
+		else
+			dev_err(component->dev, "%s: PLL_EN: %#x\n", __func__, val);
+	} else {
+		dev_err(component->dev, "%s: I2S_1: %#x\n", __func__, val);
+	}
+
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1,
 				 PCM512x_AFMT, afmt);
 	if (ret != 0) {
 		dev_err(component->dev, "Failed to set data format: %d\n", ret);
-		return ret;
 	}
 
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_2,
 				 0xFF, offset);
 	if (ret != 0) {
 		dev_err(component->dev, "Failed to set data offset: %d\n", ret);
-		return ret;
 	}
 
 	pcm512x->fmt = fmt;
-- 
2.33.0



-- 
Péter

[-- Attachment #2: 0001-ASoC-pcm512x-test-regmap-register-accesses.patch --]
[-- Type: text/x-patch, Size: 2832 bytes --]

From e013a03286b6a744914c50239d3123b7723068df Mon Sep 17 00:00:00 2001
From: Peter Ujfalusi <peter.ujfalusi@gmail.com>
Date: Tue, 21 Sep 2021 07:12:06 +0300
Subject: [PATCH] ASoC: pcm512x: test regmap register accesses

read i2c_1 in different stages.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@gmail.com>
---
 sound/soc/codecs/pcm512x.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/pcm512x.c b/sound/soc/codecs/pcm512x.c
index 4dc844f3c1fc..d0382e9ac329 100644
--- a/sound/soc/codecs/pcm512x.c
+++ b/sound/soc/codecs/pcm512x.c
@@ -1166,6 +1166,7 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
 {
 	struct snd_soc_component *component = dai->component;
 	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+	unsigned int val;
 	int alen;
 	int gpio;
 	int ret;
@@ -1193,6 +1194,18 @@ static int pcm512x_hw_params(struct snd_pcm_substream *substream,
 		return -EINVAL;
 	}
 
+	ret = regmap_read(pcm512x->regmap, PCM512x_I2S_1, &val);
+	if (ret) {
+		dev_err(component->dev, "%s: failed to read I2S_1: %d\n", __func_, ret);
+		ret = regmap_read(pcm512x->regmap, PCM512x_PLL_EN, &val);
+		if (ret)
+			dev_err(component->dev, "%s: failed to read PLL_EN: %d\n", __func__, ret);
+		else
+			dev_err(component->dev, "%s: PLL_EN: %#x\n", __func__, val);
+	} else {
+		dev_err(component->dev, "%s: I2S_1: %#x\n", __func__, val);
+	}
+
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1,
 				 PCM512x_ALEN, alen);
 	if (ret != 0) {
@@ -1335,6 +1348,7 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct snd_soc_component *component = dai->component;
 	struct pcm512x_priv *pcm512x = snd_soc_component_get_drvdata(component);
+	unsigned int val;
 	int afmt;
 	int offset = 0;
 	int clock_output;
@@ -1396,18 +1410,28 @@ static int pcm512x_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		return -EINVAL;
 	}
 
+	ret = regmap_read(pcm512x->regmap, PCM512x_I2S_1, &val);
+	if (ret) {
+		dev_err(component->dev, "%s: failed to read I2S_1: %d\n", __func_, ret);
+		ret = regmap_read(pcm512x->regmap, PCM512x_PLL_EN, &val);
+		if (ret)
+			dev_err(component->dev, "%s: failed to read PLL_EN: %d\n", __func__, ret);
+		else
+			dev_err(component->dev, "%s: PLL_EN: %#x\n", __func__, val);
+	} else {
+		dev_err(component->dev, "%s: I2S_1: %#x\n", __func__, val);
+	}
+
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_1,
 				 PCM512x_AFMT, afmt);
 	if (ret != 0) {
 		dev_err(component->dev, "Failed to set data format: %d\n", ret);
-		return ret;
 	}
 
 	ret = regmap_update_bits(pcm512x->regmap, PCM512x_I2S_2,
 				 0xFF, offset);
 	if (ret != 0) {
 		dev_err(component->dev, "Failed to set data offset: %d\n", ret);
-		return ret;
 	}
 
 	pcm512x->fmt = fmt;
-- 
2.33.0

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

[Peter Rosin]

On 2021-09-21 06:20, Péter Ujfalusi wrote:
> Hi Peter.
> 
> On 20/09/2021 22:37, Peter Rosin wrote:
> 
>> Nope, MODE1/2 are wired for I2C -> FMT is just the last I2C address
>> bit. So, nothing to do with I2S. And I can't see how that would explain
>> the same problem with the I2S_2 register?
> 
> true, but worth the question ;)

Right :-)

>>>> This fix is not 100% correct. The datasheet of at least the pcm5142
>>>> states that four bits (0xcc) in the I2S_1 register are "RSV"
>>>> ("Reserved. Do not access.") and no hint is given as to what theHi
>>>> initial values are supposed to be. So, specifying defaults for
>>>> these bits is wrong. But perhaps better than a broken driver?
>>>
>>> The default of 0x02 (AFMT = 00b - I2S, ALEN = 10b - 24bits) is correct
>>> for I2S_1 and the 0 is the default of I2S_2.
>>>
>>> The failure happens when updating the AFMT (bit4-5) and when regmap is
>>> doing a i2c read since the default is not specified.
>>>
>>> It would be interesting to see what it is reading... Out of interest:
>>> can you mar the I2S_1 and I2S_2 as volatile and read / print the value
>>> just before the afmt and alen update?
>>
>> My first attempt was to mark the register volatile, and then read and
>> compare if the update was needed at all. But marking volatile wasn't
>> enough.
> 
> If the value is not provided in the defaults then the first read is reading out to the chip anyways.

Yeah, but why is accessing I2S_1/2 returning -EBUSY when accessing e.g.
the PCM512x_MASTER_MODE register is not?

>> I also tried to set both a default and mark as volatile,
> 
> Volatile always skips the cache, default would be ignored.
> 
>> but it seems every read fails with -16 (EBUSY). I don't get why, to me
>> it almost feels like a regmap issue of some sort (probably the regmap
>> config is bad in some way), but I'm not fluent in regmap...
> 
> Or most likely the chip is not powered at pcm512x_set_fmt() time?

The chip is always powered, at least externally. Are you hinting that
relevant parts of the chip may be powered down internally when
pcm512x_set_fmt() executes and that this is the -EBUSY cause? Why does
that only happen to me in that case?

>> So, since I can't read, I can't get to the initial values of the four
>> reserved bits. So, I winged them as zero.
> 
> The value of the reserved bits are don't care.
> 
> 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.

Cheers,
Peter

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

[Peter Rosin]

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

Cheers,
Peter

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

[Peter Rosin]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

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

[-- Attachment #2: 0001-ASoC-pcm512x-Mend-accesses-to-the-I2S_1-and-I2S_2-re.patch --]
[-- Type: text/plain, Size: 1778 bytes --]

From ac4d55c7741a13d4f209a63cce94a9acbbbf4f25 Mon Sep 17 00:00:00 2001
From: Peter Rosin <peda@axentia.se>
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@axentia.se>
---
 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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 681 bytes --]

On Tue, Sep 21, 2021 at 10:48:01AM +0200, Peter Rosin wrote:
> On 2021-09-21 10:10, Peter Rosin wrote:

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

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

How would this work if the device looses power while in suspend (eg, due
to the regulators being software controllable)?  If the data isn't being
stored in the cache then it'll need to be stored somewhere else and
restored on resume.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Peter Rosin]

On 2021-09-21 14:01, Mark Brown wrote:
> On Tue, Sep 21, 2021 at 10:48:01AM +0200, Peter Rosin wrote:
>> On 2021-09-21 10:10, Peter Rosin wrote:
> 
>> Ok, so the attached works for me as well. But I don't know if it's
>> appropriate to resume/suspend like that?
> 
>> 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.
> 
> How would this work if the device looses power while in suspend (eg, due
> to the regulators being software controllable)?  If the data isn't being
> stored in the cache then it'll need to be stored somewhere else and
> restored on resume.

Right. Scratch v2. I'd go with the original patch. We have verified
that the original content of the I2S_1 register is the expected 0x02
(at least on one pcm5142) and besides, the four RSV bits are probably
don't care anyway.

Péter might have a different opinion?

Cheers,
Peter

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

[Péter Ujfalusi]

On 21/09/2021 16:30, Peter Rosin wrote:
> On 2021-09-21 14:01, Mark Brown wrote:
>> On Tue, Sep 21, 2021 at 10:48:01AM +0200, Peter Rosin wrote:
>>> On 2021-09-21 10:10, Peter Rosin wrote:
>>
>>> Ok, so the attached works for me as well. But I don't know if it's
>>> appropriate to resume/suspend like that?
>>
>>> 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.
>>
>> How would this work if the device looses power while in suspend (eg, due
>> to the regulators being software controllable)?  If the data isn't being
>> stored in the cache then it'll need to be stored somewhere else and
>> restored on resume.
> 
> Right. Scratch v2. I'd go with the original patch. We have verified
> that the original content of the I2S_1 register is the expected 0x02
> (at least on one pcm5142) and besides, the four RSV bits are probably
> don't care anyway.
> 
> Péter might have a different opinion?

The original patch is correct, the defaults are those exactly (by TRM
and by your register read).

This at some point needs to be addressed when someone comes along with
wanting something else than I2S as default format.

Thanks for the debugging!

-- 
Péter

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

[Mark Brown]

On Mon, 20 Sep 2021 16:49:39 +0200, Peter Rosin wrote:
> From 625f858894af2b7e547cc723b97361081438b123 Mon Sep 17 00:00:00 2001
> From: Peter Rosin <peda@axentia.se>
> 
> 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:
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S_2 registers
      commit: 3f4b57ad07d9237acf1b8cff3f8bf530cacef87a

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark