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

[Peter Rosin <peda@axentia.se>]

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