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

[Peter Rosin <peda@axentia.se>]

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