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;