[PATCH 0/2] TDA7419 audio processor driver

[PATCH 0/2] TDA7419 audio processor driver

[Matt Porter]

This series adds an ASoC component driver for the ST TDA7419 audio
processor which is commonly used in automotive audio applications.
The datasheet can be found at
http://www.st.com/resource/en/datasheet/tda7419.pdf

Matt Porter (2):
  ASoC: add tda7419 audio processor binding
  ASoC: add tda7419 audio processor driver

 .../devicetree/bindings/sound/tda7419.txt          |  15 +
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/tda7419.c                         | 571 +++++++++++++++++++++
 4 files changed, 594 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt
 create mode 100644 sound/soc/codecs/tda7419.c

-- 
2.11.0

[PATCH 1/2] ASoC: add tda7419 audio processor binding

[Matt Porter]

DeviceTree binding for the tda7419 audio processor.

Signed-off-by: Matt Porter <mporter@konsulko.com>
---
 Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt

diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt
new file mode 100644
index 000000000000..919318315600
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/tda7419.txt
@@ -0,0 +1,15 @@
+TDA7419 audio processor
+
+This device supports I2C only.
+
+Required properties:
+
+- compatible : "st,tda7419"
+- reg : the I2C address of the device.
+
+Example:
+
+ap: tda7419@44 {
+	compatible = "st,tda7419";
+	reg = <0x44>;
+};
-- 
2.11.0

[PATCH 2/2] ASoC: add tda7419 audio processor driver

[Matt Porter]

Component driver for the tda7419 audio processor.

Signed-off-by: Matt Porter <mporter@konsulko.com>
---
 sound/soc/codecs/Kconfig   |   6 +
 sound/soc/codecs/Makefile  |   2 +
 sound/soc/codecs/tda7419.c | 571 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 579 insertions(+)
 create mode 100644 sound/soc/codecs/tda7419.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 2b331f7266ab..1553cf2b9445 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -151,6 +151,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_TAS571X if I2C
 	select SND_SOC_TAS5720 if I2C
 	select SND_SOC_TAS6424 if I2C
+	select SND_SOC_TDA7419 if I2C
 	select SND_SOC_TFA9879 if I2C
 	select SND_SOC_TLV320AIC23_I2C if I2C
 	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -910,6 +911,11 @@ config SND_SOC_TAS6424
 	  Enable support for Texas Instruments TAS6424 high-efficiency
 	  digital input quad-channel Class-D audio power amplifiers.
 
+config SND_SOC_TDA7419
+	tristate "ST TDA7419 audio processor"
+	depends on I2C
+	select REGMAP_I2C
+
 config SND_SOC_TFA9879
 	tristate "NXP Semiconductors TFA9879 amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index da1571336f1e..6cf3c3b92cb5 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -160,6 +160,7 @@ snd-soc-tas5086-objs := tas5086.o
 snd-soc-tas571x-objs := tas571x.o
 snd-soc-tas5720-objs := tas5720.o
 snd-soc-tas6424-objs := tas6424.o
+snd-soc-tda7419-objs := tda7419.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -405,6 +406,7 @@ obj-$(CONFIG_SND_SOC_TAS5086)	+= snd-soc-tas5086.o
 obj-$(CONFIG_SND_SOC_TAS571X)	+= snd-soc-tas571x.o
 obj-$(CONFIG_SND_SOC_TAS5720)	+= snd-soc-tas5720.o
 obj-$(CONFIG_SND_SOC_TAS6424)	+= snd-soc-tas6424.o
+obj-$(CONFIG_SND_SOC_TDA7419)	+= snd-soc-tda7419.o
 obj-$(CONFIG_SND_SOC_TFA9879)	+= snd-soc-tfa9879.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23)	+= snd-soc-tlv320aic23.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C)	+= snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tda7419.c b/sound/soc/codecs/tda7419.c
new file mode 100644
index 000000000000..97a7d21b8f2a
--- /dev/null
+++ b/sound/soc/codecs/tda7419.c
@@ -0,0 +1,571 @@
+/*
+ * TDA7419 audio processor driver
+ *
+ * Copyright 2018 Konsulko Group
+ *
+ * Author: Matt Porter <mporter@konsulko.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ */
+
+#include <linux/i2c.h>
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <sound/core.h>
+#include <sound/control.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+
+#define TDA7419_MAIN_SRC_REG		0x00
+#define TDA7419_LOUDNESS_REG		0x01
+#define TDA7419_MUTE_CLK_REG		0x02
+#define TDA7419_VOLUME_REG		0x03
+#define TDA7419_TREBLE_REG		0x04
+#define TDA7419_MIDDLE_REG		0x05
+#define TDA7419_BASS_REG		0x06
+#define TDA7419_SECOND_SRC_REG		0x07
+#define TDA7419_SUB_MID_BASS_REG	0x08
+#define TDA7419_MIXING_GAIN_REG		0x09
+#define TDA7419_ATTENUATOR_LF_REG	0x0a
+#define TDA7419_ATTENUATOR_RF_REG	0x0b
+#define TDA7419_ATTENUATOR_LR_REG	0x0c
+#define TDA7419_ATTENUATOR_RR_REG	0x0d
+#define TDA7419_MIXING_LEVEL_REG	0x0e
+#define TDA7419_ATTENUATOR_SUB_REG	0x0f
+#define TDA7419_SA_CLK_AC_REG		0x10
+#define TDA7419_TESTING_REG		0x11
+
+#define TDA7419_MAIN_SRC_SEL		0
+#define TDA7419_MAIN_SRC_GAIN		3
+#define TDA7419_MAIN_SRC_AUTOZERO	7
+
+#define TDA7419_LOUDNESS_ATTEN		0
+#define TDA7419_LOUDNESS_CENTER_FREQ	4
+#define TDA7419_LOUDNESS_BOOST		6
+#define TDA7419_LOUDNESS_SOFT_STEP	7
+
+#define TDA7419_VOLUME_SOFT_STEP	7
+
+#define TDA7419_SOFT_MUTE		0
+#define TDA7419_MUTE_INFLUENCE		1
+#define TDA7419_SOFT_MUTE_TIME		2
+#define TDA7419_SOFT_STEP_TIME		4
+#define TDA7419_CLK_FAST_MODE		7
+
+#define TDA7419_TREBLE_CENTER_FREQ	5
+#define TDA7419_REF_OUT_SELECT		7
+
+#define TDA7419_MIDDLE_Q_FACTOR		5
+#define TDA7419_MIDDLE_SOFT_STEP	7
+
+#define TDA7419_BASS_Q_FACTOR		5
+#define TDA7419_BASS_SOFT_STEP		7
+
+#define TDA7419_SECOND_SRC_SEL		0
+#define TDA7419_SECOND_SRC_GAIN		3
+#define TDA7419_REAR_SPKR_SRC		7
+
+#define TDA7419_SUB_CUT_OFF_FREQ	0
+#define TDA7419_MIDDLE_CENTER_FREQ	2
+#define TDA7419_BASS_CENTER_FREQ	4
+#define TDA7419_BASS_DC_MODE		6
+#define TDA7419_SMOOTHING_FILTER	7
+
+#define TDA7419_MIX_LF			0
+#define TDA7419_MIX_RF			1
+#define TDA7419_MIX_ENABLE		2
+#define TDA7419_SUB_ENABLE		3
+#define TDA7419_HPF_GAIN		4
+
+#define TDA7419_SA_Q_FACTOR		0
+#define TDA7419_RESET_MODE		1
+#define TDA7419_SA_SOURCE		2
+#define TDA7419_SA_RUN			3
+#define TDA7419_RESET			4
+#define TDA7419_CLK_SOURCE		5
+#define TDA7419_COUPLING_MODE		6
+
+struct tda7419_data {
+	struct regmap *regmap;
+};
+
+static bool tda7419_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return true;
+}
+
+static bool tda7419_readable_reg(struct device *dev, unsigned int reg)
+{
+	return false;
+}
+
+static const struct reg_default tda7419_regmap_defaults[] = {
+	{ TDA7419_MAIN_SRC_REG,	0xfe },
+	{ TDA7419_LOUDNESS_REG, 0xfe },
+	{ TDA7419_MUTE_CLK_REG, 0xfe },
+	{ TDA7419_VOLUME_REG, 0xfe },
+	{ TDA7419_TREBLE_REG, 0xfe },
+	{ TDA7419_MIDDLE_REG, 0xfe },
+	{ TDA7419_BASS_REG, 0xfe },
+	{ TDA7419_SECOND_SRC_REG, 0xfe },
+	{ TDA7419_SUB_MID_BASS_REG, 0xfe },
+	{ TDA7419_MIXING_GAIN_REG, 0xfe },
+	{ TDA7419_ATTENUATOR_LF_REG, 0xfe },
+	{ TDA7419_ATTENUATOR_RF_REG, 0xfe },
+	{ TDA7419_ATTENUATOR_LR_REG, 0xfe },
+	{ TDA7419_ATTENUATOR_RR_REG, 0xfe },
+	{ TDA7419_MIXING_LEVEL_REG, 0xfe },
+	{ TDA7419_ATTENUATOR_SUB_REG, 0xfe },
+	{ TDA7419_SA_CLK_AC_REG, 0xfe },
+	{ TDA7419_TESTING_REG, 0xfe },
+};
+
+static const struct regmap_config tda7419_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = TDA7419_TESTING_REG,
+	.cache_type = REGCACHE_RBTREE,
+	.writeable_reg = tda7419_writeable_reg,
+	.readable_reg = tda7419_readable_reg,
+	.reg_defaults = tda7419_regmap_defaults,
+	.num_reg_defaults = ARRAY_SIZE(tda7419_regmap_defaults),
+};
+
+struct tda7419_vol_control {
+	int min, max;
+	unsigned int reg, rreg, mask, thresh;
+	unsigned int invert:1;
+};
+
+static inline bool tda7419_vol_is_stereo(struct tda7419_vol_control *tvc)
+{
+	if (tvc->reg == tvc->rreg)
+		return 0;
+
+	return 1;
+}
+
+static int tda7419_vol_info(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_info *uinfo)
+{
+	struct tda7419_vol_control *tvc =
+		(struct tda7419_vol_control *)kcontrol->private_value;
+
+	uinfo->type = SNDRV_CTL_ELEM_TYPE_INTEGER;
+	uinfo->count = tda7419_vol_is_stereo(tvc) ? 2 : 1;
+	uinfo->value.integer.min = tvc->min;
+	uinfo->value.integer.max = tvc->max;
+
+	return 0;
+}
+
+static inline int tda7419_vol_get_value(int val, unsigned int mask,
+					int thresh, unsigned int invert)
+{
+	val &= mask;
+	if (val < thresh) {
+		if (invert)
+			val = 0 - val;
+	} else if (val > thresh) {
+		if (invert)
+			val = val - thresh;
+		else
+			val = thresh - val;
+	}
+
+	return val;
+}
+
+static int tda7419_vol_get(struct snd_kcontrol *kcontrol,
+			   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct tda7419_vol_control *tvc =
+		(struct tda7419_vol_control *)kcontrol->private_value;
+	unsigned int reg = tvc->reg;
+	unsigned int rreg = tvc->rreg;
+	unsigned int mask = tvc->mask;
+	int thresh = tvc->thresh;
+	unsigned int invert = tvc->invert;
+	int val;
+	int ret;
+
+	ret = snd_soc_component_read(component, reg, &val);
+	if (ret < 0)
+		return ret;
+	ucontrol->value.integer.value[0] =
+		tda7419_vol_get_value(val, mask, thresh, invert);
+
+	if (tda7419_vol_is_stereo(tvc)) {
+		ret = snd_soc_component_read(component, rreg, &val);
+		if (ret < 0)
+			return ret;
+		ucontrol->value.integer.value[1] =
+			tda7419_vol_get_value(val, mask, thresh, invert);
+	}
+
+	return 0;
+}
+
+static inline int tda7419_vol_put_value(int val, int thresh,
+					unsigned int invert)
+{
+	if (val < 0) {
+		if (invert)
+			val = abs(val);
+		else
+			val = thresh - val;
+	} else if ((val > 0) && invert) {
+		val += thresh;
+	}
+
+	return val;
+}
+
+static int tda7419_vol_put(struct snd_kcontrol *kcontrol,
+			   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component =
+		snd_kcontrol_chip(kcontrol);
+	struct tda7419_vol_control *tvc =
+		(struct tda7419_vol_control *)kcontrol->private_value;
+	unsigned int reg = tvc->reg;
+	unsigned int rreg = tvc->rreg;
+	unsigned int mask = tvc->mask;
+	int thresh = tvc->thresh;
+	unsigned int invert = tvc->invert;
+	int val;
+	int ret;
+
+	val = tda7419_vol_put_value(ucontrol->value.integer.value[0],
+				    thresh, invert);
+	ret = snd_soc_component_update_bits(component, reg,
+					    mask, val);
+	if (ret < 0)
+		return ret;
+
+	if (tda7419_vol_is_stereo(tvc)) {
+		val = tda7419_vol_put_value(ucontrol->value.integer.value[1],
+					    thresh, invert);
+		ret = snd_soc_component_update_bits(component, rreg,
+						    mask, val);
+	}
+
+	return ret;
+}
+
+#define TDA7419_SINGLE_VALUE(xreg, xmask, xmin, xmax, xthresh, xinvert) \
+	((unsigned long)&(struct tda7419_vol_control) \
+	{.reg = xreg, .rreg = xreg, .mask = xmask, .min = xmin, \
+	 .max = xmax, .thresh = xthresh, .invert = xinvert})
+
+#define TDA7419_DOUBLE_R_VALUE(xregl, xregr, xmask, xmin, xmax, xthresh, \
+			       xinvert) \
+	((unsigned long)&(struct tda7419_vol_control) \
+	{.reg = xregl, .rreg = xregr, .mask = xmask, .min = xmin, \
+	 .max = xmax, .thresh = xthresh, .invert = xinvert})
+
+#define TDA7419_SINGLE_TLV(xname, xreg, xmask, xmin, xmax, xthresh, \
+			   xinvert, xtlv_array) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
+	.name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
+		SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.tlv.p = (xtlv_array), \
+	.info = tda7419_vol_info, \
+	.get = tda7419_vol_get, \
+	.put = tda7419_vol_put, \
+	.private_value = TDA7419_SINGLE_VALUE(xreg, xmask, xmin, \
+					      xmax, xthresh, xinvert), \
+}
+
+#define TDA7419_DOUBLE_R_TLV(xname, xregl, xregr, xmask, xmin, xmax, \
+			     xthresh, xinvert, xtlv_array) \
+{	.iface = SNDRV_CTL_ELEM_IFACE_MIXER, \
+	.name = xname, \
+	.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
+		SNDRV_CTL_ELEM_ACCESS_READWRITE, \
+	.tlv.p = (xtlv_array), \
+	.info = tda7419_vol_info, \
+	.get = tda7419_vol_get, \
+	.put = tda7419_vol_put, \
+	.private_value = TDA7419_DOUBLE_R_VALUE(xregl, xregr, xmask, \
+						xmin, xmax, xthresh, \
+						xinvert), \
+}
+
+static const char * const enum_src_sel[] = {
+	"QD", "SE1", "SE2", "SE3", "SE", "Mute", "Mute", "Mute"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_main_src_sel,
+	TDA7419_MAIN_SRC_REG, TDA7419_MAIN_SRC_SEL, enum_src_sel);
+static DECLARE_TLV_DB_SCALE(tlv_src_gain, 0, 100, 0);
+
+static DECLARE_TLV_DB_SCALE(tlv_loudness_atten, -1500, 100, 0);
+static const char * const enum_loudness_center_freq[] = {
+	"Flat", "400 Hz", "800 Hz", "2400 Hz"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_loudness_center_freq,
+	TDA7419_LOUDNESS_REG, TDA7419_LOUDNESS_CENTER_FREQ,
+	enum_loudness_center_freq);
+static const char * const enum_mute_influence[] = {
+	"Pin and IIC", "IIC"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_mute_influence,
+	TDA7419_MUTE_CLK_REG, TDA7419_MUTE_INFLUENCE, enum_mute_influence);
+static const char * const enum_soft_mute_time[] = {
+	"0.48 ms", "0.96 ms", "123 ms", "123 ms"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_soft_mute_time,
+	TDA7419_MUTE_CLK_REG, TDA7419_SOFT_MUTE_TIME, enum_soft_mute_time);
+static const char * const enum_soft_step_time[] = {
+	"0.160 ms", "0.321 ms", "0.642 ms", "1.28 ms",
+	"2.56 ms", "5.12 ms", "10.24 ms", "20.48 ms"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_soft_step_time,
+	TDA7419_MUTE_CLK_REG, TDA7419_SOFT_STEP_TIME, enum_soft_step_time);
+static DECLARE_TLV_DB_SCALE(tlv_volume, -8000, 100, 1);
+static const char * const enum_treble_center_freq[] = {
+	"10.0 kHz", "12.5 kHz", "15.0 kHz", "17.5 kHz"};
+static DECLARE_TLV_DB_SCALE(tlv_filter, -1500, 100, 0);
+static SOC_ENUM_SINGLE_DECL(soc_enum_treble_center_freq,
+	TDA7419_TREBLE_REG, TDA7419_TREBLE_CENTER_FREQ,
+	enum_treble_center_freq);
+static const char * const enum_ref_out_select[] = {
+	"External Vref (4 V)", "Internal Vref (3.3 V)"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_ref_out_select,
+	TDA7419_TREBLE_REG, TDA7419_REF_OUT_SELECT, enum_ref_out_select);
+static const char * const enum_middle_q_factor[] = {
+	"0.5", "0.75", "1.0", "1.25"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_middle_q_factor,
+	TDA7419_MIDDLE_REG, TDA7419_MIDDLE_Q_FACTOR, enum_middle_q_factor);
+static const char * const enum_bass_q_factor[] = {
+	"1.0", "1.25", "1.5", "2.0"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_bass_q_factor,
+	TDA7419_BASS_REG, TDA7419_BASS_Q_FACTOR, enum_bass_q_factor);
+static SOC_ENUM_SINGLE_DECL(soc_enum_second_src_sel,
+	TDA7419_SECOND_SRC_REG, TDA7419_SECOND_SRC_SEL, enum_src_sel);
+static const char * const enum_rear_spkr_src[] = {
+	"Main", "Second"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_rear_spkr_src,
+	TDA7419_SECOND_SRC_REG, TDA7419_REAR_SPKR_SRC, enum_rear_spkr_src);
+static const char * const enum_sub_cut_off_freq[] = {
+	"Flat", "80 Hz", "120 Hz", "160 Hz"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_sub_cut_off_freq,
+	TDA7419_SUB_MID_BASS_REG, TDA7419_SUB_CUT_OFF_FREQ,
+	enum_sub_cut_off_freq);
+static const char * const enum_middle_center_freq[] = {
+	"500 Hz", "1000 Hz", "1500 Hz", "2500 Hz"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_middle_center_freq,
+	TDA7419_SUB_MID_BASS_REG, TDA7419_MIDDLE_CENTER_FREQ,
+	enum_middle_center_freq);
+static const char * const enum_bass_center_freq[] = {
+	"60 Hz", "80 Hz", "100 Hz", "200 Hz"};
+static SOC_ENUM_SINGLE_DECL(soc_enum_bass_center_freq,
+	TDA7419_SUB_MID_BASS_REG, TDA7419_BASS_CENTER_FREQ,
+	enum_bass_center_freq);
+static DECLARE_TLV_DB_SCALE(tlv_hpf_gain, 400, 200, 0);
+static const char * const enum_sa_q_factor[] = {
+	"3.5", "1.75" };
+static SOC_ENUM_SINGLE_DECL(soc_enum_sa_q_factor,
+	TDA7419_SA_CLK_AC_REG, TDA7419_SA_Q_FACTOR, enum_sa_q_factor);
+static const char * const enum_reset_mode[] = {
+	"IIC", "Auto" };
+static SOC_ENUM_SINGLE_DECL(soc_enum_reset_mode,
+	TDA7419_SA_CLK_AC_REG, TDA7419_RESET_MODE, enum_reset_mode);
+static const char * const enum_sa_src[] = {
+	"Bass", "In Gain" };
+static SOC_ENUM_SINGLE_DECL(soc_enum_sa_src,
+	TDA7419_SA_CLK_AC_REG, TDA7419_SA_SOURCE, enum_sa_src);
+static const char * const enum_clk_src[] = {
+	"Internal", "External" };
+static SOC_ENUM_SINGLE_DECL(soc_enum_clk_src,
+	TDA7419_SA_CLK_AC_REG, TDA7419_CLK_SOURCE, enum_clk_src);
+static const char * const enum_coupling_mode[] = {
+	"DC Coupling (without HPF)", "AC Coupling after In Gain",
+	"DC Coupling (with HPF)", "AC Coupling after Bass" };
+static SOC_ENUM_SINGLE_DECL(soc_enum_coupling_mode,
+	TDA7419_SA_CLK_AC_REG, TDA7419_COUPLING_MODE, enum_coupling_mode);
+
+/* ASoC Controls */
+static struct snd_kcontrol_new tda7419_controls[] = {
+SOC_ENUM("Main Source Select", soc_enum_main_src_sel),
+SOC_SINGLE_TLV("Main Source Capture Volume", TDA7419_MAIN_SRC_REG,
+	       TDA7419_MAIN_SRC_GAIN, 15, 0, tlv_src_gain),
+SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG,
+	   TDA7419_MAIN_SRC_AUTOZERO, 1, 1),
+SOC_SINGLE_TLV("Loudness Playback Volume", TDA7419_LOUDNESS_REG,
+	       TDA7419_LOUDNESS_ATTEN, 15, 1, tlv_loudness_atten),
+SOC_ENUM("Loudness Center Frequency", soc_enum_loudness_center_freq),
+SOC_SINGLE("Loudness High Boost", TDA7419_LOUDNESS_REG,
+	   TDA7419_LOUDNESS_BOOST, 1, 1),
+SOC_SINGLE("Loudness Soft Step", TDA7419_LOUDNESS_REG,
+	   TDA7419_LOUDNESS_SOFT_STEP, 1, 1),
+SOC_SINGLE("Soft Mute", TDA7419_MUTE_CLK_REG, TDA7419_SOFT_MUTE, 1, 1),
+SOC_ENUM("Mute Influence", soc_enum_mute_influence),
+SOC_ENUM("Soft Mute Time", soc_enum_soft_mute_time),
+SOC_ENUM("Soft Step Time", soc_enum_soft_step_time),
+SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG,
+	   TDA7419_CLK_FAST_MODE, 1, 1),
+TDA7419_SINGLE_TLV("Master Playback Volume", TDA7419_VOLUME_REG,
+		   0x7f, -80, 15, 0x10, 0, tlv_volume),
+SOC_SINGLE("Volume Soft Step", TDA7419_VOLUME_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+TDA7419_SINGLE_TLV("Treble Playback Volume", TDA7419_TREBLE_REG,
+		   0x1f, -15, 15, 0x10, 1, tlv_filter),
+SOC_ENUM("Treble Center Frequency", soc_enum_treble_center_freq),
+SOC_ENUM("Reference Output Select", soc_enum_ref_out_select),
+TDA7419_SINGLE_TLV("Middle Playback Volume", TDA7419_MIDDLE_REG,
+		   0x1f, -15, 15, 0x10, 1, tlv_filter),
+SOC_ENUM("Middle Q Factor", soc_enum_middle_q_factor),
+SOC_SINGLE("Middle Soft Step", TDA7419_MIDDLE_REG,
+	   TDA7419_MIDDLE_SOFT_STEP, 1, 1),
+TDA7419_SINGLE_TLV("Bass Playback Volume", TDA7419_BASS_REG,
+		   0x1f, -15, 15, 0x10, 1, tlv_filter),
+SOC_ENUM("Bass Q Factor", soc_enum_bass_q_factor),
+SOC_SINGLE("Bass Soft Step", TDA7419_BASS_REG,
+	   TDA7419_BASS_SOFT_STEP, 1, 1),
+SOC_ENUM("Second Source Select", soc_enum_second_src_sel),
+SOC_SINGLE_TLV("Second Source Capture Volume", TDA7419_SECOND_SRC_REG,
+	       TDA7419_SECOND_SRC_GAIN, 15, 0, tlv_src_gain),
+SOC_ENUM("Rear Speaker Source", soc_enum_rear_spkr_src),
+SOC_ENUM("Subwoofer Cut-off Frequency", soc_enum_sub_cut_off_freq),
+SOC_ENUM("Middle Center Frequency", soc_enum_middle_center_freq),
+SOC_ENUM("Bass Center Frequency", soc_enum_bass_center_freq),
+SOC_SINGLE("Bass DC Mode", TDA7419_SUB_MID_BASS_REG,
+	   TDA7419_BASS_DC_MODE, 1, 1),
+SOC_SINGLE("Smoothing Filter", TDA7419_SUB_MID_BASS_REG,
+	   TDA7419_SMOOTHING_FILTER, 1, 1),
+SOC_SINGLE("Mix to LF Speaker", TDA7419_MIXING_GAIN_REG,
+	   TDA7419_MIX_LF, 1, 1),
+SOC_SINGLE("Mix to RF Speaker", TDA7419_MIXING_GAIN_REG,
+	   TDA7419_MIX_RF, 1, 1),
+SOC_SINGLE("Mix Enable", TDA7419_MIXING_GAIN_REG, TDA7419_MIX_ENABLE, 1, 1),
+SOC_SINGLE("Subwoofer Enable", TDA7419_MIXING_GAIN_REG,
+	   TDA7419_SUB_ENABLE, 1, 1),
+SOC_SINGLE_TLV("HPF Filter Playback Volume", TDA7419_MIXING_GAIN_REG,
+	       TDA7419_HPF_GAIN, 9, 0, tlv_hpf_gain),
+TDA7419_DOUBLE_R_TLV("Front Playback Volume", TDA7419_ATTENUATOR_LF_REG,
+		     TDA7419_ATTENUATOR_RF_REG, 0x7f, -80, 15, 0x10, 0,
+		     tlv_volume),
+SOC_SINGLE("Left Front Soft Step", TDA7419_ATTENUATOR_LF_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+SOC_SINGLE("Right Front Soft Step", TDA7419_ATTENUATOR_RF_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+TDA7419_DOUBLE_R_TLV("Rear Playback Volume", TDA7419_ATTENUATOR_LR_REG,
+		     TDA7419_ATTENUATOR_RR_REG, 0x7f, -80, 15, 0x10, 0,
+		     tlv_volume),
+SOC_SINGLE("Left Rear Soft Step", TDA7419_ATTENUATOR_LR_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+SOC_SINGLE("Right Rear Soft Step", TDA7419_ATTENUATOR_RR_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+TDA7419_SINGLE_TLV("Mixing Capture Volume", TDA7419_MIXING_LEVEL_REG,
+		   0x7f, -80, 15, 0x10, 0, tlv_volume),
+SOC_SINGLE("Mixing Level Soft Step", TDA7419_MIXING_LEVEL_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+TDA7419_SINGLE_TLV("Subwoofer Playback Volume", TDA7419_ATTENUATOR_SUB_REG,
+		   0x7f, -80, 15, 0x10, 0, tlv_volume),
+SOC_SINGLE("Subwoofer Soft Step", TDA7419_ATTENUATOR_SUB_REG,
+	   TDA7419_VOLUME_SOFT_STEP, 1, 1),
+SOC_ENUM("Spectrum Analyzer Q Factor", soc_enum_sa_q_factor),
+SOC_ENUM("Spectrum Analyzer Reset Mode", soc_enum_reset_mode),
+SOC_ENUM("Spectrum Analyzer Source", soc_enum_sa_src),
+SOC_SINGLE("Spectrum Analyzer Run", TDA7419_SA_CLK_AC_REG,
+	   TDA7419_SA_RUN, 1, 1),
+SOC_SINGLE("Spectrum Analyzer Reset", TDA7419_SA_CLK_AC_REG,
+	   TDA7419_RESET, 1, 1),
+SOC_ENUM("Clock Source", soc_enum_clk_src),
+SOC_ENUM("Coupling Mode", soc_enum_coupling_mode),
+};
+
+static const struct snd_soc_component_driver tda7419_component_driver = {
+	.name			= "tda7419",
+	.controls		= tda7419_controls,
+	.num_controls		= ARRAY_SIZE(tda7419_controls),
+};
+
+static int tda7419_probe(struct i2c_client *i2c,
+			 const struct i2c_device_id *id)
+{
+	struct tda7419_data *tda7419;
+	int ret;
+
+	tda7419 = devm_kzalloc(&i2c->dev,
+			       sizeof(struct tda7419_data),
+			       GFP_KERNEL);
+	if (tda7419 == NULL)
+		return -ENOMEM;
+
+	i2c_set_clientdata(i2c, tda7419);
+
+	tda7419->regmap = devm_regmap_init_i2c(i2c, &tda7419_regmap_config);
+	if (IS_ERR(tda7419->regmap)) {
+		ret = PTR_ERR(tda7419->regmap);
+		dev_err(&i2c->dev, "error initializing regmap: %d\n",
+				ret);
+		return ret;
+	}
+
+	/* Configure registers */
+	regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f);
+	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0);
+	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0);
+
+	ret = devm_snd_soc_register_component(&i2c->dev,
+		&tda7419_component_driver, NULL, 0);
+	if (ret < 0) {
+		dev_err(&i2c->dev, "error registering component: %d\n",
+				ret);
+	}
+
+	return ret;
+}
+
+static int tda7419_remove(struct i2c_client *i2c)
+{
+	int i;
+	struct tda7419_data *tda7419 = i2c_get_clientdata(i2c);
+
+	/* Reset registers to defaults */
+	for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++)
+		regmap_write(tda7419->regmap,
+			     tda7419_regmap_defaults[i].reg,
+			     tda7419_regmap_defaults[i].def);
+
+	return 0;
+}
+
+static const struct i2c_device_id tda7419_i2c_id[] = {
+	{ "tda7419", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, tda7419_i2c_id);
+
+static const struct of_device_id tda7419_of_match[] = {
+	{ .compatible = "st,tda7419" },
+	{ },
+};
+
+static struct i2c_driver tda7419_driver = {
+	.driver = {
+		.name   = "tda7419",
+		.of_match_table = tda7419_of_match,
+	},
+	.probe          = tda7419_probe,
+	.remove		= tda7419_remove,
+	.id_table       = tda7419_i2c_id,
+};
+
+module_i2c_driver(tda7419_driver);
+
+MODULE_AUTHOR("Matt Porter <mporter@konsulko.com>");
+MODULE_DESCRIPTION("TDA7419 audio processor driver");
+MODULE_LICENSE("GPL");
-- 
2.11.0

Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver

[Mark Brown]

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

On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote:

> +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return true;
> +}

This is the default behaviour, may as well omit it (but equally it does
no harm).

> +static inline int tda7419_vol_get_value(int val, unsigned int mask,
> +					int thresh, unsigned int invert)
> +{
> +	val &= mask;
> +	if (val < thresh) {
> +		if (invert)
> +			val = 0 - val;
> +	} else if (val > thresh) {

This feels like something some other device might want to use so might
warrant pulling out into a general control at some point but I'd not
insist on doing it now.

> +static struct snd_kcontrol_new tda7419_controls[] = {
> +SOC_ENUM("Main Source Select", soc_enum_main_src_sel),

Should this be a DAPM route?

> +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG,
> +	   TDA7419_MAIN_SRC_AUTOZERO, 1, 1),

There's a lot of on/off switches for various things in here - these
should all have Switch at the end of their names so that userspace can
see how it's expected to display them.  Most of the controls with a max
value of 1 probably fall into this category.

> +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG,
> +	   TDA7419_CLK_FAST_MODE, 1, 1),

What does this do - should it be in set_sysclk() or something?

> +	/* Configure registers */
> +	regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f);
> +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0);
> +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0);

This looks like it's setting default volumes - just leave those at the
chip defaults and let userspace handle setting them, what works for one
board may be totally inappropriate on another board and using the chip
default means we've got some fixed thing we don't need to discuss.

> +static int tda7419_remove(struct i2c_client *i2c)
> +{
> +	int i;
> +	struct tda7419_data *tda7419 = i2c_get_clientdata(i2c);
> +
> +	/* Reset registers to defaults */
> +	for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++)
> +		regmap_write(tda7419->regmap,
> +			     tda7419_regmap_defaults[i].reg,
> +			     tda7419_regmap_defaults[i].def);

Why are we doing this?  Other drivers don't do it...  if anything I'd
expect a reset on probe in case the bootloader or something left the
chip configured.

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

Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver

[kbuild test robot]

Hi Matt,

I love your patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on v4.16-rc3 next-20180302]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Matt-Porter/TDA7419-audio-processor-driver/20180303-042824
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next


coccinelle warnings: (new ones prefixed by >>)

>> sound/soc/codecs/tda7419.c:151:9-10: WARNING: return of 0/1 in function 'tda7419_vol_is_stereo' with return type bool

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] ASoC: fix boolreturn.cocci warnings

[kbuild test robot]

From: Fengguang Wu <fengguang.wu@intel.com>

sound/soc/codecs/tda7419.c:151:9-10: WARNING: return of 0/1 in function 'tda7419_vol_is_stereo' with return type bool

 Return statements in functions returning bool should use
 true/false instead of 1/0.
Generated by: scripts/coccinelle/misc/boolreturn.cocci

Fixes: d811df0fabec ("ASoC: add tda7419 audio processor driver")
CC: Matt Porter <mporter@konsulko.com>
Signed-off-by: Fengguang Wu <fengguang.wu@intel.com>
---

 tda7419.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/sound/soc/codecs/tda7419.c
+++ b/sound/soc/codecs/tda7419.c
@@ -148,9 +148,9 @@ struct tda7419_vol_control {
 static inline bool tda7419_vol_is_stereo(struct tda7419_vol_control *tvc)
 {
 	if (tvc->reg == tvc->rreg)
-		return 0;
+		return false;
 
-	return 1;
+	return true;
 }
 
 static int tda7419_vol_info(struct snd_kcontrol *kcontrol,

Re: [PATCH 1/2] ASoC: add tda7419 audio processor binding

[Rob Herring]

On Tue, Feb 27, 2018 at 05:51:27PM -0500, Matt Porter wrote:
> DeviceTree binding for the tda7419 audio processor.
> 
> Signed-off-by: Matt Porter <mporter@konsulko.com>
> ---
>  Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt
> new file mode 100644
> index 000000000000..919318315600
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/tda7419.txt
> @@ -0,0 +1,15 @@
> +TDA7419 audio processor
> +
> +This device supports I2C only.
> +
> +Required properties:
> +
> +- compatible : "st,tda7419"
> +- reg : the I2C address of the device.

For completeness:

st,mute-gpios for MUTE pin?

vdd-supply?

> +
> +Example:
> +
> +ap: tda7419@44 {
> +	compatible = "st,tda7419";
> +	reg = <0x44>;
> +};
> -- 
> 2.11.0
> 

Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver

[Matt Porter]

On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote:
> On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote:
> 
> > +static bool tda7419_writeable_reg(struct device *dev, unsigned int reg)
> > +{
> > +	return true;
> > +}
> 
> This is the default behaviour, may as well omit it (but equally it does
> no harm).

Ok.

> 
> > +static inline int tda7419_vol_get_value(int val, unsigned int mask,
> > +					int thresh, unsigned int invert)
> > +{
> > +	val &= mask;
> > +	if (val < thresh) {
> > +		if (invert)
> > +			val = 0 - val;
> > +	} else if (val > thresh) {
> 
> This feels like something some other device might want to use so might
> warrant pulling out into a general control at some point but I'd not
> insist on doing it now.

Ok, yeah, I was also thinking it should be moved to a general helper
when the next user shows up. The most likely case is another part in
this family may have a similar register layout.

> > +static struct snd_kcontrol_new tda7419_controls[] = {
> > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel),
> 
> Should this be a DAPM route?

Ultimately yes. I initially took the path of ignoring DAPM support in
interests of getting some clean done. Is it ok to merge DAPM support
later or do you prefer just having it in the intitial driver? For
routes, it'll include Main/Second source selects, the Rear Source
switch, and Mix enable at least.

> > +SOC_SINGLE("Main Source AutoZero", TDA7419_MAIN_SRC_REG,
> > +	   TDA7419_MAIN_SRC_AUTOZERO, 1, 1),
> 
> There's a lot of on/off switches for various things in here - these
> should all have Switch at the end of their names so that userspace can
> see how it's expected to display them.  Most of the controls with a max
> value of 1 probably fall into this category.

I see. I'll fix that.

> > +SOC_SINGLE("Clock Fast Mode", TDA7419_MUTE_CLK_REG,
> > +	   TDA7419_CLK_FAST_MODE, 1, 1),
> 
> What does this do - should it be in set_sysclk() or something?

This is where things get hazy, unfortunately. The datasheet is partially
garbage. So there's no description or guidance on clock management at
all. It's not clear what clock this is or what specifically "fast" does.
Because there's no concept of an external clock, the sysclk is clearly
internally generated. Because of the register labeling of clock
generator, it's probably sysclk but I'm not 100% sure what the relevance
is of fast mode. The working settings were divined from trial and error
as well a couple microcontroller projects scattered across the
interwebs.

On second look at this, I think I should at least remove this switch
and hard code it for now or move to set_sysclk(). I'm hesistant of the
latter because of the lack of information on this setting.

> > +	/* Configure registers */
> > +	regmap_write(tda7419->regmap, TDA7419_VOLUME_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_MIXING_GAIN_REG, 0x0f);
> > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LF_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RF_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_LR_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_RR_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_MIXING_LEVEL_REG, 0xe0);
> > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0);
> 
> This looks like it's setting default volumes - just leave those at the
> chip defaults and let userspace handle setting them, what works for one
> board may be totally inappropriate on another board and using the chip
> default means we've got some fixed thing we don't need to discuss.

This is actually setting the default/cache to the first mute value due
to the assumption in my implementation of the tda7419-specific get/set
for these registers. It simplified the code a bit to have these
initialized like this. e.g. for the attenuator group of registers,
x11xxxxx are all mute values, so 0xe0 is setting these regs to that
first mute value to simplify things. I'll take another look at
eliminating this. As it is, it does not change the fact that the actual
reset value of 0xff is also mute from a user POV.

> > +static int tda7419_remove(struct i2c_client *i2c)
> > +{
> > +	int i;
> > +	struct tda7419_data *tda7419 = i2c_get_clientdata(i2c);
> > +
> > +	/* Reset registers to defaults */
> > +	for (i = 0; i < ARRAY_SIZE(tda7419_regmap_defaults); i++)
> > +		regmap_write(tda7419->regmap,
> > +			     tda7419_regmap_defaults[i].reg,
> > +			     tda7419_regmap_defaults[i].def);
> 
> Why are we doing this?  Other drivers don't do it...  if anything I'd
> expect a reset on probe in case the bootloader or something left the
> chip configured.

Good point, I'll move this into probe. The part doesn't have a soft
reset provision so we need to do it manually like this.

-Matt

Re: [PATCH 1/2] ASoC: add tda7419 audio processor binding

[Matt Porter]

On Mon, Mar 05, 2018 at 04:29:54PM -0600, Rob Herring wrote:
> On Tue, Feb 27, 2018 at 05:51:27PM -0500, Matt Porter wrote:
> > DeviceTree binding for the tda7419 audio processor.
> > 
> > Signed-off-by: Matt Porter <mporter@konsulko.com>
> > ---
> >  Documentation/devicetree/bindings/sound/tda7419.txt | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/tda7419.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/tda7419.txt b/Documentation/devicetree/bindings/sound/tda7419.txt
> > new file mode 100644
> > index 000000000000..919318315600
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/tda7419.txt
> > @@ -0,0 +1,15 @@
> > +TDA7419 audio processor
> > +
> > +This device supports I2C only.
> > +
> > +Required properties:
> > +
> > +- compatible : "st,tda7419"
> > +- reg : the I2C address of the device.
> 
> For completeness:
> 
> st,mute-gpios for MUTE pin?

I'll add mute in as an optional property.

> 
> vdd-supply?

Yes, there's a required ~8.5V supply so I'll add the regulator reference.

Thanks,
Matt

Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver

[Mark Brown]

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

On Fri, Mar 09, 2018 at 09:35:48AM -0500, Matt Porter wrote:
> On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote:
> > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote:

> > > +static struct snd_kcontrol_new tda7419_controls[] = {
> > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel),

> > Should this be a DAPM route?

> Ultimately yes. I initially took the path of ignoring DAPM support in
> interests of getting some clean done. Is it ok to merge DAPM support
> later or do you prefer just having it in the intitial driver? For
> routes, it'll include Main/Second source selects, the Rear Source
> switch, and Mix enable at least.

You definitely shouldn't be implementing things that should be in DAPM
as non-DAPM controls.

> > > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0);

> > This looks like it's setting default volumes - just leave those at the
> > chip defaults and let userspace handle setting them, what works for one
> > board may be totally inappropriate on another board and using the chip
> > default means we've got some fixed thing we don't need to discuss.

> This is actually setting the default/cache to the first mute value due
> to the assumption in my implementation of the tda7419-specific get/set
> for these registers. It simplified the code a bit to have these
> initialized like this. e.g. for the attenuator group of registers,
> x11xxxxx are all mute values, so 0xe0 is setting these regs to that
> first mute value to simplify things. I'll take another look at
> eliminating this. As it is, it does not change the fact that the actual
> reset value of 0xff is also mute from a user POV.

If it is useful it definitely needs a comment explaining what's
happening and that there's no practical change to the configuration.  It
would be nicer to be robust against the device getting a wider range of
values in the register but that seems plausible.

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

Re: [PATCH 2/2] ASoC: add tda7419 audio processor driver

[Matt Porter]

On Fri, Mar 09, 2018 at 03:29:12PM +0000, Mark Brown wrote:
> On Fri, Mar 09, 2018 at 09:35:48AM -0500, Matt Porter wrote:
> > On Wed, Feb 28, 2018 at 11:00:38AM +0000, Mark Brown wrote:
> > > On Tue, Feb 27, 2018 at 05:51:28PM -0500, Matt Porter wrote:
> 
> > > > +static struct snd_kcontrol_new tda7419_controls[] = {
> > > > +SOC_ENUM("Main Source Select", soc_enum_main_src_sel),
> 
> > > Should this be a DAPM route?
> 
> > Ultimately yes. I initially took the path of ignoring DAPM support in
> > interests of getting some clean done. Is it ok to merge DAPM support
> > later or do you prefer just having it in the intitial driver? For
> > routes, it'll include Main/Second source selects, the Rear Source
> > switch, and Mix enable at least.
> 
> You definitely shouldn't be implementing things that should be in DAPM
> as non-DAPM controls.

Ok, I addressed this by adding DAPM support in v2.

> > > > +	regmap_write(tda7419->regmap, TDA7419_ATTENUATOR_SUB_REG, 0xe0);
> 
> > > This looks like it's setting default volumes - just leave those at the
> > > chip defaults and let userspace handle setting them, what works for one
> > > board may be totally inappropriate on another board and using the chip
> > > default means we've got some fixed thing we don't need to discuss.
> 
> > This is actually setting the default/cache to the first mute value due
> > to the assumption in my implementation of the tda7419-specific get/set
> > for these registers. It simplified the code a bit to have these
> > initialized like this. e.g. for the attenuator group of registers,
> > x11xxxxx are all mute values, so 0xe0 is setting these regs to that
> > first mute value to simplify things. I'll take another look at
> > eliminating this. As it is, it does not change the fact that the actual
> > reset value of 0xff is also mute from a user POV.
> 
> If it is useful it definitely needs a comment explaining what's
> happening and that there's no practical change to the configuration.  It
> would be nicer to be robust against the device getting a wider range of
> values in the register but that seems plausible.

I did some rework to make this unnecessary in v2.

Thanks,
Matt