Rick,

On Thu, Oct 1, 2015 at 2:12 AM, Rick Mann rmann@latencyzero.com wrote:

Benoît,

I've discovered some additional minor bugs in the tlv320aic3104 support in the tlv320aic3x driver. In this instance, the reg_default values aren't quite right.

I've written a corrected list, but the set specified includes the reserved registers. I don't know if devm_regmap_init_i2c() attempts to write the default values, or uses them as the initial values in a cache (and attempts to avoid writes unless specific registers change). In my debugging, it does not appear to write the entire register map at any point to the IC, so I think it's the latter.

Yes, it's the latter. The defaults are supposed to be the reset values. This avoids dumping all registers on power up or reset to initialize the cache. regcache_sync() then synchronizes all cached non-default values. See the comments in regmap.c/h and regcache.c/h and how these functions are used by the driver of this CODEC.

This means that if some default values are wrong, then some registers that should have been written would not be in some cases, but this is very unlikely here. Fixing this would be better though.

I also don't know how the regmap is intended to be used. It is passed to devm_regmap_init_i2c() as .reg_defaults, implying the array provides starting values, but other parts of the code refer to the array as if the values are changed in that array as they're read and written.

Which parts? *reg_defaults is const-qualified, so no. Only the cached values associated to these default values change through the snd_soc_read()/snd_soc_write() calls. devm_regmap_init_i2c() only ties the regmap to the I²C device so that the following I²C register accesses go through the cache.

Documentation for regmap suggests regmaps can be "sparse," which I presume would be done by omitting the reserved registers from the defaults array.

Correct. That's why the individual register addresses are given in struct reg_default and not just a register address range.

In any case, the fix is not trivial, and I don't want to go down that road not knowing if it's the right approach. Does this make sense?

If it's just to exclude the reserved registers, this is useless because the driver is not supposed to access them anyway. However, if it's to fix some wrong default values (detectable by dumping all registers following a reset without Linux accessing the device, and excluding the reserved ones from the diff against aic3x_reg[]) of non-reserved registers, then it makes sense to define part-specific alternative struct reg_default values, and in that case the reserved registers can be dropped.

For reference, the registers with different default values (than the ones specified in that file) are 32, 33, 51, 58, 65, 72, 93.

32 and 33 are read-only and not even used by this driver, so they could just be dropped. You could check the datasheets of all the supported parts other than 3104, but it's the same on aic33, so I doubt that 0x18 is a valid value for any of these CODECs, which would mean that this table is just a register dump including values of volatile read-only registers, which is wrong.

51, 58, 65, 72 and 93: You are probably referring to the read-only bit D1. If so, it does not matter since this bit is not used by the driver.

Best regards, Benoît