[alsa-devel] Additional bugs in tlv320aic3x.c driver (register defaults)

[Benoît Thébaudeau]

Rick,

On Thu, Oct 1, 2015 at 2:12 AM, Rick Mann <rmann at 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