On Tue, 8 Feb 2022, Mark Brown wrote:
On Mon, Feb 07, 2022 at 06:12:06PM +0100, Ricard Wanderlof wrote:
- /*
* Coefficient RAM registers for miniDSP are marked as volatile* mainly because they must be written in pairs, so we don't want* them to be cached. Updates are not likely to occur very often,* so the performance penalty is minimal.*/- if (reg >= ADC3XXX_REG(4, 2) && reg <= ADC3XXX_REG(4, 128))
return true;This is typically done for suspend/resume handling as much as for performance, and note that reads do tend to be a bit more frequent than writes since things get displayed in UI. The driver doesn't currently handle suspend/resume but it seems like something someone might want. Other than resyncing the cache (and see below for that) a cache will affect reads not writes, writes should be affected unless the driver turns on cache only mode.
Isn't one consequence of caching that writing to a register which is known to already have the value to be written are simply skipped?
I remember having that problem with a codec which did not have any means of resetting the codec other than power-on-reset (i.e. no reset pin or software controlled reset). If the system was rebooted without cycling the power, the registers would potentially contain non-default values, and this meant that for instance attempting to explicitly set the sample rate to the default value was not possible, as the regcache assumed that the default value was already set and thus skipped the corresponding register write. (A workaround was to write another sample rate and then default).
- while (index < numcoeff) {
...
- while (index < numcoeff) {
unsigned int value = ucontrol->value.integer.value[index++];unsigned int value_msb = (value >> 8) & 0xff;unsigned int value_lsb = value & 0xff;ret = snd_soc_component_write(component, reg++, value_msb);if (ret)return ret;ret = snd_soc_component_write(component, reg++, value_lsb);if (ret)return ret;- }
Again, this looks like it should be a for loop.
Agreed, I'll rewrite it (and the previous one).
This also seems to be doing single register (though sequential) updates for the values so I really don't see any reason why you couldn't enable caching - the only gotcha I can see would be providing register defaults causing only the MSB to be written if the LSB were the same as the default, that could be avoided by just not providing defaults for these registers.
I'm not sure I follow you (or more likely I've misunderstood something about the regcache). Each register has its own address in the I2C address space, so for instance assuming that a sequence of four registers has been written and the registers currently have the values 0x12 0x34 0x56 0x78, corresponding to the two 16-bit integers 0x1234 and 0x5678, say one wants to update these to 0x1298 and 0x5643, with register caching enabled, this would mean in this case that the writes to the MSB registers (holding 0x12 and 0x56 respectively) would not be performed as the regcache would assume that the values do not need updating.
/Ricard