lkml.org 
[lkml]   [2021]   [Sep]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patches in this message
/
Date
From
SubjectRe: [PATCH] ASoC: pcm512x: Mend accesses to the I2S_1 and I2S_2 registers
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éterFrom 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
\
 
 \ /
  Last update: 2021-09-21 06:21    [W:0.126 / U:0.672 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site