Re: [PATCH] ASoC: tlv320adc3xxx: Add IIR filter configuration

From: Mark Brown <broonie@kernel.org>
To: Ricard Wanderlof <ricardw@axis.com>
Cc: alsa-devel <alsa-devel@alsa-project.org>,
	kernel@axis.com, Liam Girdwood <lgirdwood@gmail.com>
Subject: Re: [PATCH] ASoC: tlv320adc3xxx: Add IIR filter configuration
Date: Tue, 8 Feb 2022 18:56:21 +0000	[thread overview]
Message-ID: <YgK81R6ipwLagmoE@sirena.org.uk> (raw)
In-Reply-To: <alpine.DEB.2.21.2202071806580.31604@lnxricardw1.se.axis.com>

[-- Attachment #1: Type: text/plain, Size: 2199 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]