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
--
Ricard Wolf Wanderlof ricardw(at)axis.com
Axis Communications AB, Lund, Sweden www.axis.com
Phone +46 46 272 2016 Fax +46 46 13 61 30
