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.
- while (index < numcoeff) {
unsigned int value_msb, value_lsb, value;value_msb = snd_soc_component_read(component, reg++);if ((int)value_msb < 0)return (int)value_msb;value_lsb = snd_soc_component_read(component, reg++);if ((int)value_lsb < 0)return (int)value_lsb;value = (value_msb << 8) | value_lsb;ucontrol->value.integer.value[index++] = value;- }
Why is this not written as a for loop? It's pretty hard to spot the increment as written.
- 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. 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.