At Tue, 2 Aug 2011 12:29:48 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 8:10 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
At Tue, 2 Aug 2011 11:15:28 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 7:09 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Yes, it is raw register index here. The case is that cache array is [reg0, reg1, reg2, reg3], So the size is 4. But when step is 2, reg0 is 0x0, reg1 is 0x2, reg3 is 0x6. So check if reg3 > reg_cache_size is not correct. I mean this mismatch. Am I correct?
No, the (flat) cache array held in codec instance contains padding, e.g. [reg0, 0, reg1, 0, reg2, 0, reg3, 0] in the case above. cache_reg_step is there just for avoiding the access to invalid registers in the table-lookup code in soc-*.c.
I saw most codec drivers with cache_reg_step of 2 do not have padding such as wm9705, wm9712 driver(you can check the code). So It looks the cache_reg_step here causes some misleading.
Yeah, there have been confusion about the usage of these (I vaguely remember I complained years ago :), and I guess something is significantly broken there now.
Thus it's a bug of the driver if it passes reg_cache_default with a packed array with reg_cache_step > 1. If the size matters, we may fix it in soc-core.c to accept packed arrays, but I'm not sure whether it's worth alone.
(For the sparse data like wm8995, it's a different question; Obviously it can be better packed in a form of {index,data} pairs instead of the whole sparse array as initial data. Then we'd need to introduce another type of default-data copier in soc-core.c.)
And, in principle, the driver shouldn't access codec->reg_cache contents directly but use helper functions. Then the most of inconsistency issue should go away.
Yes. The problem is that if the cache array is not padded, the snd_soc_read/write may work incorrectly.
Well, the problem is that there are inconsistencies in the code. Especially soc-cache.c doesn't consider about reg_cache_step at all (e.g. snd_soc_flat_cache_sync()).
Mark, Liam, could you guys take a deeper look and clean these messes up?
thanks,
Takashi