[PATCH] ASoC: tlv320adc3xxx: Add IIR filter configuration

[PATCH] ASoC: tlv320adc3xxx: Add IIR filter configuration

[Ricard Wanderlof]

The TLV320ADC3001/3101 have an internal DSP, which can either be
used in various preset configurations (called "Processing Blocks"
in the data sheet), or as a freely programmable (using the 
"PurePath Studio" graphical programming tool from TI) but rather
small DSP ("miniDSP").

Using the default configuration (PRB_R1) it's possible to set up
filtering using a first-order IIR, which can be useful for adding
a digital high pass filter to the signal chain, for instance.

This patch adds support for configuring the IIR filter coefficients.
The filter itself is always enabled; the default coefficients 
implement a pass-through function.

Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
---
 sound/soc/codecs/tlv320adc3xxx.c | 160 +++++++++++++++++++++++++++++--
 1 file changed, 153 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/tlv320adc3xxx.c b/sound/soc/codecs/tlv320adc3xxx.c
index 4baf3d881633..d0d5206db0c7 100644
--- a/sound/soc/codecs/tlv320adc3xxx.c
+++ b/sound/soc/codecs/tlv320adc3xxx.c
@@ -169,6 +169,23 @@
 #define ADC3XXX_ANALOG_PGA_FLAGS		ADC3XXX_REG(1, 62)
 /* 63-127 Reserved */
 
+/*
+ * Page 4 registers. First page of coefficient memory for the miniDSP.
+ */
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N0_MSB	ADC3XXX_REG(4, 8)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N0_LSB	ADC3XXX_REG(4, 9)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N1_MSB	ADC3XXX_REG(4, 10)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N1_LSB	ADC3XXX_REG(4, 11)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_D1_MSB	ADC3XXX_REG(4, 12)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_D1_LSB	ADC3XXX_REG(4, 13)
+
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_MSB	ADC3XXX_REG(4, 72)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_LSB	ADC3XXX_REG(4, 73)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N1_MSB	ADC3XXX_REG(4, 74)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N1_LSB	ADC3XXX_REG(4, 75)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_D1_MSB	ADC3XXX_REG(4, 76)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_D1_LSB	ADC3XXX_REG(4, 77)
+
 /*
  * Register bits.
  */
@@ -373,22 +390,63 @@ static const struct reg_default adc3xxx_defaults[] = {
 	{ 180, 0xff },  { 181, 0x00 },  { 182, 0x3f },  { 183, 0xff },
 	{ 184, 0x00 },  { 185, 0x3f },  { 186, 0x00 },  { 187, 0x80 },
 	{ 188, 0x80 },  { 189, 0x00 },  { 190, 0x00 },  { 191, 0x00 },
+
+	/* Page 4 */
+	{ 1024, 0x00 },			{ 1026, 0x01 },	{ 1027, 0x17 },
+	{ 1028, 0x01 }, { 1029, 0x17 }, { 1030, 0x7d }, { 1031, 0xd3 },
+	{ 1032, 0x7f }, { 1033, 0xff }, { 1034, 0x00 }, { 1035, 0x00 },
+	{ 1036, 0x00 }, { 1037, 0x00 }, { 1038, 0x7f }, { 1039, 0xff },
+	{ 1040, 0x00 }, { 1041, 0x00 }, { 1042, 0x00 }, { 1043, 0x00 },
+	{ 1044, 0x00 }, { 1045, 0x00 }, { 1046, 0x00 }, { 1047, 0x00 },
+	{ 1048, 0x7f }, { 1049, 0xff }, { 1050, 0x00 }, { 1051, 0x00 },
+	{ 1052, 0x00 }, { 1053, 0x00 }, { 1054, 0x00 }, { 1055, 0x00 },
+	{ 1056, 0x00 }, { 1057, 0x00 }, { 1058, 0x7f }, { 1059, 0xff },
+	{ 1060, 0x00 }, { 1061, 0x00 }, { 1062, 0x00 }, { 1063, 0x00 },
+	{ 1064, 0x00 }, { 1065, 0x00 }, { 1066, 0x00 }, { 1067, 0x00 },
+	{ 1068, 0x7f }, { 1069, 0xff }, { 1070, 0x00 }, { 1071, 0x00 },
+	{ 1072, 0x00 }, { 1073, 0x00 }, { 1074, 0x00 }, { 1075, 0x00 },
+	{ 1076, 0x00 }, { 1077, 0x00 }, { 1078, 0x7f }, { 1079, 0xff },
+	{ 1080, 0x00 }, { 1081, 0x00 }, { 1082, 0x00 }, { 1083, 0x00 },
+	{ 1084, 0x00 }, { 1085, 0x00 }, { 1086, 0x00 }, { 1087, 0x00 },
+	{ 1088, 0x00 }, { 1089, 0x00 }, { 1090, 0x00 }, { 1091, 0x00 },
+	{ 1092, 0x00 }, { 1093, 0x00 }, { 1094, 0x00 }, { 1095, 0x00 },
+	{ 1096, 0x00 }, { 1097, 0x00 }, { 1098, 0x00 }, { 1099, 0x00 },
+	{ 1100, 0x00 }, { 1101, 0x00 }, { 1102, 0x00 }, { 1103, 0x00 },
+	{ 1104, 0x00 }, { 1105, 0x00 }, { 1106, 0x00 }, { 1107, 0x00 },
+	{ 1108, 0x00 }, { 1109, 0x00 }, { 1110, 0x00 }, { 1111, 0x00 },
+	{ 1112, 0x00 }, { 1113, 0x00 }, { 1114, 0x00 }, { 1115, 0x00 },
+	{ 1116, 0x00 }, { 1117, 0x00 }, { 1118, 0x00 }, { 1119, 0x00 },
+	{ 1120, 0x00 }, { 1121, 0x00 }, { 1122, 0x00 }, { 1123, 0x00 },
+	{ 1124, 0x00 }, { 1125, 0x00 }, { 1126, 0x00 }, { 1127, 0x00 },
+	{ 1128, 0x00 }, { 1129, 0x00 }, { 1130, 0x00 }, { 1131, 0x00 },
+	{ 1132, 0x00 }, { 1133, 0x00 }, { 1134, 0x00 }, { 1135, 0x00 },
+	{ 1136, 0x00 }, { 1137, 0x00 }, { 1138, 0x00 }, { 1139, 0x00 },
+	{ 1140, 0x00 }, { 1141, 0x00 }, { 1142, 0x00 }, { 1143, 0x00 },
+	{ 1144, 0x00 }, { 1145, 0x00 }, { 1146, 0x00 }, { 1147, 0x00 },
+	{ 1148, 0x00 }, { 1149, 0x00 }, { 1150, 0x00 }, { 1151, 0x00 },
 };
 
 static bool adc3xxx_volatile_reg(struct device *dev, unsigned int reg)
 {
-	switch (reg) {
-	case ADC3XXX_RESET:
+	if (reg == ADC3XXX_RESET)
 		return true;
-	default:
-		return false;
-	}
+
+	/*
+	 * 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;
+
+	return false;
 }
 
 static const struct regmap_range_cfg adc3xxx_ranges[] = {
 	{
 		.range_min = 0,
-		.range_max = 2 * ADC3XXX_PAGE_SIZE,
+		.range_max = 5 * ADC3XXX_PAGE_SIZE,
 		.selector_reg = ADC3XXX_PAGE_SELECT,
 		.selector_mask = 0xff,
 		.selector_shift = 0,
@@ -410,7 +468,7 @@ static const struct regmap_config adc3xxx_regmap = {
 
 	.ranges = adc3xxx_ranges,
 	.num_ranges = ARRAY_SIZE(adc3xxx_ranges),
-	.max_register = 2 * ADC3XXX_PAGE_SIZE,
+	.max_register = 5 * ADC3XXX_PAGE_SIZE,
 };
 
 struct adc3xxx_rate_divs {
@@ -497,6 +555,83 @@ static int adc3xxx_pll_delay(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int adc3xxx_coefficient_info(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_info *uinfo)
+{
+	int numcoeff = kcontrol->private_value >> 16;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = numcoeff;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 0xffff; /* all coefficients are 16 bit */
+	return 0;
+}
+
+static int adc3xxx_coefficient_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	int numcoeff  = kcontrol->private_value >> 16;
+	int reg = kcontrol->private_value & 0xffff;
+	int index = 0;
+
+	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;
+	}
+
+	return 0;
+}
+
+static int adc3xxx_coefficient_put(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	int numcoeff  = kcontrol->private_value >> 16;
+	int reg = kcontrol->private_value & 0xffff;
+	int index = 0;
+	int ret;
+
+	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;
+	}
+
+	return 0;
+}
+
+/* All on-chip filters have coefficients which are expressed in terms of
+ * 16 bit values, so represent them as strings of 16-bit integers.
+ */
+#define TI_COEFFICIENTS(xname, reg, numcoeffs) { \
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
+	.name = xname, \
+	.info = adc3xxx_coefficient_info, \
+	.get = adc3xxx_coefficient_get,\
+	.put = adc3xxx_coefficient_put, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.private_value = reg | (numcoeffs << 16) \
+}
+
 static const char * const adc_softstepping_text[] = { "1 step", "2 step", "off" };
 static SOC_ENUM_SINGLE_DECL(adc_softstepping_enum, ADC3XXX_ADC_DIGITAL, 0,
 			    adc_softstepping_text);
@@ -640,6 +775,17 @@ static const struct snd_kcontrol_new adc3xxx_snd_controls[] = {
 	SOC_SINGLE("Right ADC Unselected CM Bias Capture Switch",
 		   ADC3XXX_RIGHT_PGA_SEL_2, 6, 1, 0),
 	SOC_ENUM("Dither Control DC Offset", dither_dc_offset_enum),
+
+	/* Coefficient memory for miniDSP. */
+	/* For the default PRB_R1 processing block, the only available
+	 * filter is the first order IIR.
+	 */
+
+	TI_COEFFICIENTS("Left ADC IIR Coefficients N0 N1 D1",
+			ADC3XXX_LEFT_ADC_IIR_COEFF_N0_MSB, 3),
+
+	TI_COEFFICIENTS("Right ADC IIR Coefficients N0 N1 D1",
+			ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_MSB, 3),
 };
 
 /* Left input selection, Single Ended inputs and Differential inputs */
-- 
2.20.1


-- 
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

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

[Mark Brown]

[-- 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 --]

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

[Ricard Wanderlof]

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

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

[Mark Brown]

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

On Wed, Feb 09, 2022 at 03:53:28PM +0100, Ricard Wanderlof wrote:
> On Tue, 8 Feb 2022, Mark Brown wrote:
> > On Mon, Feb 07, 2022 at 06:12:06PM +0100, Ricard Wanderlof wrote:

> > 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? 

No, look at the code for regmap_write().

> 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 

This is during a cache sync, a sync will only write out non-default
values if the device was flagged as having been reset in order to reduce
power on times.  Your driver is not doing a cache sync at any point so
won't be affected by this, but in any case...

> write. (A workaround was to write another sample rate and then default).

If your driver has no way of ensuring that the device has default
register values your driver should just not specify any register
defaults, but in this case it sounds like you have some other bug going
on.  If the device is getting suspended with a default value set in the
registers then comes out of suspend with a non-default value it's hard
to see how that could happen in the hardware, either the device will
retain the value it had or it will reset to power on default but either
way it's the same value.  I have seen drivers bypassing the cache for a
shutdown sequence that wrote non-default values to the hardware without
updating the cache.

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

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

[Ricard Wanderlof]

On Wed, 9 Feb 2022, Mark Brown wrote:

> > 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? 
> 
> No, look at the code for regmap_write().

Thanks. Good. Should have checked. I had the case previosly described 
deeply ingrained in my mind since it was an elusive issue at the time.

> > 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 
> 
> This is during a cache sync, a sync will only write out non-default
> values if the device was flagged as having been reset in order to reduce
> power on times.  Your driver is not doing a cache sync at any point so
> won't be affected by this, but in any case...
> 
> > write. (A workaround was to write another sample rate and then default).
> 
> If your driver has no way of ensuring that the device has default
> register values your driver should just not specify any register
> defaults, but in this case it sounds like you have some other bug going
> on.  If the device is getting suspended with a default value set in the
> registers then comes out of suspend with a non-default value it's hard
> to see how that could happen in the hardware, either the device will
> retain the value it had or it will reset to power on default but either
> way it's the same value.  I have seen drivers bypassing the cache for a
> shutdown sequence that wrote non-default values to the hardware without
> updating the cache.

In this case it was the ADAU1761 driver and it was several years ago
and I don't know if it still is an issue. Basically the sequence was 
something like:

- System boots up.
- Codec is configured (I think it was the sample rate, but it could have 
  been the format or some other I2S parameter) to something that is non 
  default.
- System reboots. Since the codec has no means of getting reset, it
  retains all its register values.
- This time, an attempt is made to configure the codec with the default
  sample rate. Since the driver believes the default is already set
  it does not attempt to write anything, although looking at 
  regmap_write() now I'm not sure how this could be the case.
- When codec is started, the sample rate / format / whatever is wrong
  compared to what was allegedly set up.

It was all rather academic, because the above sequence only occurred in 
the lab under manual control; in the actual production code the same 
format and sample rate was used every time, so it wasn't an issue that was 
pressing to fix. It could as you said have been a bug somewhere else. I 
distinctly remember the last point about the codec running in seemingly 
the wrong mode after a (power-cycle-less) reboot at any rate.

/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

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

[Mark Brown]

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

On Wed, Feb 09, 2022 at 06:20:00PM +0100, Ricard Wanderlof wrote:

> In this case it was the ADAU1761 driver and it was several years ago
> and I don't know if it still is an issue. Basically the sequence was 
> something like:

> - System boots up.
> - Codec is configured (I think it was the sample rate, but it could have 
>   been the format or some other I2S parameter) to something that is non 
>   default.
> - System reboots. Since the codec has no means of getting reset, it
>   retains all its register values.
> - This time, an attempt is made to configure the codec with the default
>   sample rate. Since the driver believes the default is already set
>   it does not attempt to write anything, although looking at 
>   regmap_write() now I'm not sure how this could be the case.

update_bits() could also trigger this if it thinks the value didn't
change, but that's at a higher level before it ever gets as far as
trying to do the write and unrelated to the cache (it will also
suppress redundant writes with no cache present).

> - When codec is started, the sample rate / format / whatever is wrong
>   compared to what was allegedly set up.

> It was all rather academic, because the above sequence only occurred in 
> the lab under manual control; in the actual production code the same 
> format and sample rate was used every time, so it wasn't an issue that was 
> pressing to fix. It could as you said have been a bug somewhere else. I 
> distinctly remember the last point about the codec running in seemingly 
> the wrong mode after a (power-cycle-less) reboot at any rate.

A soft reboot could trigger something like that, the easiest fix would
just be to not specify any register defaults so that there's no default
values to compare with.

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

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

[Ricard Wanderlof]

On Wed, 9 Feb 2022, Mark Brown wrote:

> > In this case it was the ADAU1761 driver and it was several years ago
> > and I don't know if it still is an issue. Basically the sequence was 
> > something like:
> 
> > - System boots up.
> > - Codec is configured (I think it was the sample rate, but it could have 
> >   been the format or some other I2S parameter) to something that is non 
> >   default.
> > - System reboots. Since the codec has no means of getting reset, it
> >   retains all its register values.
> > - This time, an attempt is made to configure the codec with the default
> >   sample rate. Since the driver believes the default is already set
> >   it does not attempt to write anything, although looking at 
> >   regmap_write() now I'm not sure how this could be the case.
> 
> update_bits() could also trigger this if it thinks the value didn't
> change, but that's at a higher level before it ever gets as far as
> trying to do the write and unrelated to the cache (it will also
> suppress redundant writes with no cache present).

Right, that sound even more familiar.

/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

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

[Ricard Wanderlof]

The TLV320ADC3001/3101 have an internal DSP, which can either be
used in various preset configurations (called "Processing Blocks"
in the data sheet), or as a freely programmable (using the 
"PurePath Studio" graphical programming tool from TI) but rather
small DSP ("miniDSP").

Using the default configuration (PRB_R1) it's possible to set up
filtering using a first-order IIR, which can be useful for adding
a digital high pass filter to the signal chain, for instance.

This patch adds support for configuring the IIR filter coefficients.
The filter itself is always enabled; the default coefficients 
implement a pass-through function.

Signed-off-by: Ricard Wanderlof <ricardw@axis.com>
---
Changelog:
v2:
- Removed marking miniDSP coefficient registers as volatile,
  as writes always take place regardless of caching.
- Replace slightly obscure while loops in coefficient read/write
  functions with more obvious for loops. 

 sound/soc/codecs/tlv320adc3xxx.c | 143 ++++++++++++++++++++++++++++++-
 1 file changed, 141 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/tlv320adc3xxx.c b/sound/soc/codecs/tlv320adc3xxx.c
index 4baf3d881633..1d12a3f7074a 100644
--- a/sound/soc/codecs/tlv320adc3xxx.c
+++ b/sound/soc/codecs/tlv320adc3xxx.c
@@ -169,6 +169,23 @@
 #define ADC3XXX_ANALOG_PGA_FLAGS		ADC3XXX_REG(1, 62)
 /* 63-127 Reserved */
 
+/*
+ * Page 4 registers. First page of coefficient memory for the miniDSP.
+ */
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N0_MSB	ADC3XXX_REG(4, 8)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N0_LSB	ADC3XXX_REG(4, 9)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N1_MSB	ADC3XXX_REG(4, 10)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_N1_LSB	ADC3XXX_REG(4, 11)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_D1_MSB	ADC3XXX_REG(4, 12)
+#define ADC3XXX_LEFT_ADC_IIR_COEFF_D1_LSB	ADC3XXX_REG(4, 13)
+
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_MSB	ADC3XXX_REG(4, 72)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_LSB	ADC3XXX_REG(4, 73)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N1_MSB	ADC3XXX_REG(4, 74)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_N1_LSB	ADC3XXX_REG(4, 75)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_D1_MSB	ADC3XXX_REG(4, 76)
+#define ADC3XXX_RIGHT_ADC_IIR_COEFF_D1_LSB	ADC3XXX_REG(4, 77)
+
 /*
  * Register bits.
  */
@@ -373,6 +390,40 @@ static const struct reg_default adc3xxx_defaults[] = {
 	{ 180, 0xff },  { 181, 0x00 },  { 182, 0x3f },  { 183, 0xff },
 	{ 184, 0x00 },  { 185, 0x3f },  { 186, 0x00 },  { 187, 0x80 },
 	{ 188, 0x80 },  { 189, 0x00 },  { 190, 0x00 },  { 191, 0x00 },
+
+	/* Page 4 */
+	{ 1024, 0x00 },			{ 1026, 0x01 },	{ 1027, 0x17 },
+	{ 1028, 0x01 }, { 1029, 0x17 }, { 1030, 0x7d }, { 1031, 0xd3 },
+	{ 1032, 0x7f }, { 1033, 0xff }, { 1034, 0x00 }, { 1035, 0x00 },
+	{ 1036, 0x00 }, { 1037, 0x00 }, { 1038, 0x7f }, { 1039, 0xff },
+	{ 1040, 0x00 }, { 1041, 0x00 }, { 1042, 0x00 }, { 1043, 0x00 },
+	{ 1044, 0x00 }, { 1045, 0x00 }, { 1046, 0x00 }, { 1047, 0x00 },
+	{ 1048, 0x7f }, { 1049, 0xff }, { 1050, 0x00 }, { 1051, 0x00 },
+	{ 1052, 0x00 }, { 1053, 0x00 }, { 1054, 0x00 }, { 1055, 0x00 },
+	{ 1056, 0x00 }, { 1057, 0x00 }, { 1058, 0x7f }, { 1059, 0xff },
+	{ 1060, 0x00 }, { 1061, 0x00 }, { 1062, 0x00 }, { 1063, 0x00 },
+	{ 1064, 0x00 }, { 1065, 0x00 }, { 1066, 0x00 }, { 1067, 0x00 },
+	{ 1068, 0x7f }, { 1069, 0xff }, { 1070, 0x00 }, { 1071, 0x00 },
+	{ 1072, 0x00 }, { 1073, 0x00 }, { 1074, 0x00 }, { 1075, 0x00 },
+	{ 1076, 0x00 }, { 1077, 0x00 }, { 1078, 0x7f }, { 1079, 0xff },
+	{ 1080, 0x00 }, { 1081, 0x00 }, { 1082, 0x00 }, { 1083, 0x00 },
+	{ 1084, 0x00 }, { 1085, 0x00 }, { 1086, 0x00 }, { 1087, 0x00 },
+	{ 1088, 0x00 }, { 1089, 0x00 }, { 1090, 0x00 }, { 1091, 0x00 },
+	{ 1092, 0x00 }, { 1093, 0x00 }, { 1094, 0x00 }, { 1095, 0x00 },
+	{ 1096, 0x00 }, { 1097, 0x00 }, { 1098, 0x00 }, { 1099, 0x00 },
+	{ 1100, 0x00 }, { 1101, 0x00 }, { 1102, 0x00 }, { 1103, 0x00 },
+	{ 1104, 0x00 }, { 1105, 0x00 }, { 1106, 0x00 }, { 1107, 0x00 },
+	{ 1108, 0x00 }, { 1109, 0x00 }, { 1110, 0x00 }, { 1111, 0x00 },
+	{ 1112, 0x00 }, { 1113, 0x00 }, { 1114, 0x00 }, { 1115, 0x00 },
+	{ 1116, 0x00 }, { 1117, 0x00 }, { 1118, 0x00 }, { 1119, 0x00 },
+	{ 1120, 0x00 }, { 1121, 0x00 }, { 1122, 0x00 }, { 1123, 0x00 },
+	{ 1124, 0x00 }, { 1125, 0x00 }, { 1126, 0x00 }, { 1127, 0x00 },
+	{ 1128, 0x00 }, { 1129, 0x00 }, { 1130, 0x00 }, { 1131, 0x00 },
+	{ 1132, 0x00 }, { 1133, 0x00 }, { 1134, 0x00 }, { 1135, 0x00 },
+	{ 1136, 0x00 }, { 1137, 0x00 }, { 1138, 0x00 }, { 1139, 0x00 },
+	{ 1140, 0x00 }, { 1141, 0x00 }, { 1142, 0x00 }, { 1143, 0x00 },
+	{ 1144, 0x00 }, { 1145, 0x00 }, { 1146, 0x00 }, { 1147, 0x00 },
+	{ 1148, 0x00 }, { 1149, 0x00 }, { 1150, 0x00 }, { 1151, 0x00 },
 };
 
 static bool adc3xxx_volatile_reg(struct device *dev, unsigned int reg)
@@ -388,7 +439,7 @@ static bool adc3xxx_volatile_reg(struct device *dev, unsigned int reg)
 static const struct regmap_range_cfg adc3xxx_ranges[] = {
 	{
 		.range_min = 0,
-		.range_max = 2 * ADC3XXX_PAGE_SIZE,
+		.range_max = 5 * ADC3XXX_PAGE_SIZE,
 		.selector_reg = ADC3XXX_PAGE_SELECT,
 		.selector_mask = 0xff,
 		.selector_shift = 0,
@@ -410,7 +461,7 @@ static const struct regmap_config adc3xxx_regmap = {
 
 	.ranges = adc3xxx_ranges,
 	.num_ranges = ARRAY_SIZE(adc3xxx_ranges),
-	.max_register = 2 * ADC3XXX_PAGE_SIZE,
+	.max_register = 5 * ADC3XXX_PAGE_SIZE,
 };
 
 struct adc3xxx_rate_divs {
@@ -497,6 +548,83 @@ static int adc3xxx_pll_delay(struct snd_soc_dapm_widget *w,
 	return 0;
 }
 
+static int adc3xxx_coefficient_info(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_info *uinfo)
+{
+	int numcoeff = kcontrol->private_value >> 16;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = numcoeff;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = 0xffff; /* all coefficients are 16 bit */
+	return 0;
+}
+
+static int adc3xxx_coefficient_get(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	int numcoeff  = kcontrol->private_value >> 16;
+	int reg = kcontrol->private_value & 0xffff;
+	int index = 0;
+
+	for (index = 0; index < numcoeff; index++) {
+		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;
+	}
+
+	return 0;
+}
+
+static int adc3xxx_coefficient_put(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	int numcoeff  = kcontrol->private_value >> 16;
+	int reg = kcontrol->private_value & 0xffff;
+	int index = 0;
+	int ret;
+
+	for (index = 0; index < numcoeff; index++) {
+		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;
+	}
+
+	return 0;
+}
+
+/* All on-chip filters have coefficients which are expressed in terms of
+ * 16 bit values, so represent them as strings of 16-bit integers.
+ */
+#define TI_COEFFICIENTS(xname, reg, numcoeffs) { \
+	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
+	.name = xname, \
+	.info = adc3xxx_coefficient_info, \
+	.get = adc3xxx_coefficient_get,\
+	.put = adc3xxx_coefficient_put, \
+	.access = SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.private_value = reg | (numcoeffs << 16) \
+}
+
 static const char * const adc_softstepping_text[] = { "1 step", "2 step", "off" };
 static SOC_ENUM_SINGLE_DECL(adc_softstepping_enum, ADC3XXX_ADC_DIGITAL, 0,
 			    adc_softstepping_text);
@@ -640,6 +768,17 @@ static const struct snd_kcontrol_new adc3xxx_snd_controls[] = {
 	SOC_SINGLE("Right ADC Unselected CM Bias Capture Switch",
 		   ADC3XXX_RIGHT_PGA_SEL_2, 6, 1, 0),
 	SOC_ENUM("Dither Control DC Offset", dither_dc_offset_enum),
+
+	/* Coefficient memory for miniDSP. */
+	/* For the default PRB_R1 processing block, the only available
+	 * filter is the first order IIR.
+	 */
+
+	TI_COEFFICIENTS("Left ADC IIR Coefficients N0 N1 D1",
+			ADC3XXX_LEFT_ADC_IIR_COEFF_N0_MSB, 3),
+
+	TI_COEFFICIENTS("Right ADC IIR Coefficients N0 N1 D1",
+			ADC3XXX_RIGHT_ADC_IIR_COEFF_N0_MSB, 3),
 };
 
 /* Left input selection, Single Ended inputs and Differential inputs */
-- 
2.20.1


-- 
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

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

[Mark Brown]

On Thu, 10 Feb 2022 18:07:36 +0100, Ricard Wanderlof wrote:
> The TLV320ADC3001/3101 have an internal DSP, which can either be
> used in various preset configurations (called "Processing Blocks"
> in the data sheet), or as a freely programmable (using the
> "PurePath Studio" graphical programming tool from TI) but rather
> small DSP ("miniDSP").
> 
> Using the default configuration (PRB_R1) it's possible to set up
> filtering using a first-order IIR, which can be useful for adding
> a digital high pass filter to the signal chain, for instance.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/1] ASoC: tlv320adc3xxx: Add IIR filter configuration
      commit: 9193bc0558d1812343039b510797b669f054efc5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark