[PATCH 0/3] ASoC: tlv320aic32x4: Add support for TAS2505

[PATCH 0/3] ASoC: tlv320aic32x4: Add support for TAS2505

[Claudius Heine]

Hi,

this is an early patchset that provides support for the TAS2505 by the
tlv320aic32x4 driver.

Playback with aplay and the right mixer settings seems to be working, but some
mixer settings either cause distortions or popping. Any input on how to prevent
that is welcome.

Also with this change TAS2505 is compiled in unconditionally whenever
SND_SOC_TLV320AIC32X4_I2C is enabled. Maybe an additional option for the TAS2505
should be added, but then we probably also need a setting to provide only
support for the TAS2505 and not the TLV320AIC32X4. Not sure how to set that up
in the best way.

This is my first time working with the sound subsystem, so expect stupidity and
obvious mistakes.

regards,
Claudius

Claudius Heine (3):
  ASoC: tlv320aic32x4: prepare driver for different device variants
  ASoC: tlv320aic32x4: add support for TAS2505
  ASoC: tlv320aic32x4: dt-bindings: add TAS2505 to compatible

 .../bindings/sound/tlv320aic32x4.txt          |   1 +
 sound/soc/codecs/tlv320aic32x4-i2c.c          |  22 ++-
 sound/soc/codecs/tlv320aic32x4-spi.c          |  23 ++-
 sound/soc/codecs/tlv320aic32x4.c              | 173 +++++++++++++++++-
 sound/soc/codecs/tlv320aic32x4.h              |  10 +
 5 files changed, 219 insertions(+), 10 deletions(-)


base-commit: 009c9aa5be652675a06d5211e1640e02bbb1c33d
-- 
2.32.0

[PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Claudius Heine]

With this change it will be possible to add different code paths for
similar devices.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 sound/soc/codecs/tlv320aic32x4-i2c.c | 20 ++++++++++++++++----
 sound/soc/codecs/tlv320aic32x4-spi.c | 23 +++++++++++++++++++----
 sound/soc/codecs/tlv320aic32x4.c     |  3 +++
 sound/soc/codecs/tlv320aic32x4.h     |  5 +++++
 4 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4-i2c.c b/sound/soc/codecs/tlv320aic32x4-i2c.c
index 6d54cbf70a0b..247fb1e13674 100644
--- a/sound/soc/codecs/tlv320aic32x4-i2c.c
+++ b/sound/soc/codecs/tlv320aic32x4-i2c.c
@@ -16,6 +16,8 @@
 
 #include "tlv320aic32x4.h"
 
+static const struct of_device_id aic32x4_of_id[];
+
 static int aic32x4_i2c_probe(struct i2c_client *i2c,
 			     const struct i2c_device_id *id)
 {
@@ -27,6 +29,16 @@ static int aic32x4_i2c_probe(struct i2c_client *i2c,
 	config.val_bits = 8;
 
 	regmap = devm_regmap_init_i2c(i2c, &config);
+
+	if (i2c->dev.of_node) {
+		const struct of_device_id *oid;
+
+		oid = of_match_node(aic32x4_of_id, i2c->dev.of_node);
+		dev_set_drvdata(&i2c->dev, (void *)oid->data);
+	} else if (id) {
+		dev_set_drvdata(&i2c->dev, (void *)id->driver_data);
+	}
+
 	return aic32x4_probe(&i2c->dev, regmap);
 }
 
@@ -36,15 +48,15 @@ static int aic32x4_i2c_remove(struct i2c_client *i2c)
 }
 
 static const struct i2c_device_id aic32x4_i2c_id[] = {
-	{ "tlv320aic32x4", 0 },
-	{ "tlv320aic32x6", 1 },
+	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
+	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, aic32x4_i2c_id);
 
 static const struct of_device_id aic32x4_of_id[] = {
-	{ .compatible = "ti,tlv320aic32x4", },
-	{ .compatible = "ti,tlv320aic32x6", },
+	{ .compatible = "ti,tlv320aic32x4", .data = (void *)AIC32X4_TYPE_AIC32X4 },
+	{ .compatible = "ti,tlv320aic32x6", .data = (void *)AIC32X4_TYPE_AIC32X6 },
 	{ /* senitel */ }
 };
 MODULE_DEVICE_TABLE(of, aic32x4_of_id);
diff --git a/sound/soc/codecs/tlv320aic32x4-spi.c b/sound/soc/codecs/tlv320aic32x4-spi.c
index a22e7700bfc8..e81c72958a82 100644
--- a/sound/soc/codecs/tlv320aic32x4-spi.c
+++ b/sound/soc/codecs/tlv320aic32x4-spi.c
@@ -16,6 +16,8 @@
 
 #include "tlv320aic32x4.h"
 
+static const struct of_device_id aic32x4_of_id[];
+
 static int aic32x4_spi_probe(struct spi_device *spi)
 {
 	struct regmap *regmap;
@@ -28,6 +30,19 @@ static int aic32x4_spi_probe(struct spi_device *spi)
 	config.read_flag_mask = 0x01;
 
 	regmap = devm_regmap_init_spi(spi, &config);
+
+	if (spi->dev.of_node) {
+		const struct of_device_id *oid;
+
+		oid = of_match_node(aic32x4_of_id, spi->dev.of_node);
+		dev_set_drvdata(&spi->dev, (void *)oid->data);
+	} else {
+		const struct spi_device_id *id_entry;
+
+		id_entry = spi_get_device_id(spi);
+		dev_set_drvdata(&spi->dev, (void *)id_entry->driver_data);
+	}
+
 	return aic32x4_probe(&spi->dev, regmap);
 }
 
@@ -37,15 +52,15 @@ static int aic32x4_spi_remove(struct spi_device *spi)
 }
 
 static const struct spi_device_id aic32x4_spi_id[] = {
-	{ "tlv320aic32x4", 0 },
-	{ "tlv320aic32x6", 1 },
+	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
+	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(spi, aic32x4_spi_id);
 
 static const struct of_device_id aic32x4_of_id[] = {
-	{ .compatible = "ti,tlv320aic32x4", },
-	{ .compatible = "ti,tlv320aic32x6", },
+	{ .compatible = "ti,tlv320aic32x4", .data = (void *)AIC32X4_TYPE_AIC32X4 },
+	{ .compatible = "ti,tlv320aic32x6", .data = (void *)AIC32X4_TYPE_AIC32X6 },
 	{ /* senitel */ }
 };
 MODULE_DEVICE_TABLE(of, aic32x4_of_id);
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index b689f26fc4be..70a1574fb72a 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -48,6 +48,7 @@ struct aic32x4_priv {
 
 	struct aic32x4_setup_data *setup;
 	struct device *dev;
+	enum aic32x4_type type;
 };
 
 static int aic32x4_reset_adc(struct snd_soc_dapm_widget *w,
@@ -1198,6 +1199,8 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap)
 		return -ENOMEM;
 
 	aic32x4->dev = dev;
+	aic32x4->type = (enum aic32x4_type)dev_get_drvdata(dev);
+
 	dev_set_drvdata(dev, aic32x4);
 
 	if (pdata) {
diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h
index 7550122e9f8a..8a18dbec76a6 100644
--- a/sound/soc/codecs/tlv320aic32x4.h
+++ b/sound/soc/codecs/tlv320aic32x4.h
@@ -10,6 +10,11 @@
 struct device;
 struct regmap_config;
 
+enum aic32x4_type {
+	AIC32X4_TYPE_AIC32X4 = 0,
+	AIC32X4_TYPE_AIC32X6,
+};
+
 extern const struct regmap_config aic32x4_regmap_config;
 int aic32x4_probe(struct device *dev, struct regmap *regmap);
 int aic32x4_remove(struct device *dev);
-- 
2.32.0

[PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Claudius Heine]

This adds support for TAS2505 and TAS2521 to the tlv320aic32x4 driver.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 sound/soc/codecs/tlv320aic32x4-i2c.c |   2 +
 sound/soc/codecs/tlv320aic32x4.c     | 170 ++++++++++++++++++++++++++-
 sound/soc/codecs/tlv320aic32x4.h     |   5 +
 3 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic32x4-i2c.c b/sound/soc/codecs/tlv320aic32x4-i2c.c
index 247fb1e13674..04ad38311360 100644
--- a/sound/soc/codecs/tlv320aic32x4-i2c.c
+++ b/sound/soc/codecs/tlv320aic32x4-i2c.c
@@ -50,6 +50,7 @@ static int aic32x4_i2c_remove(struct i2c_client *i2c)
 static const struct i2c_device_id aic32x4_i2c_id[] = {
 	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
 	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
+	{ "tas2505", (kernel_ulong_t)AIC32X4_TYPE_TAS2505 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, aic32x4_i2c_id);
@@ -57,6 +58,7 @@ MODULE_DEVICE_TABLE(i2c, aic32x4_i2c_id);
 static const struct of_device_id aic32x4_of_id[] = {
 	{ .compatible = "ti,tlv320aic32x4", .data = (void *)AIC32X4_TYPE_AIC32X4 },
 	{ .compatible = "ti,tlv320aic32x6", .data = (void *)AIC32X4_TYPE_AIC32X6 },
+	{ .compatible = "ti,tas2505", .data = (void *)AIC32X4_TYPE_TAS2505 },
 	{ /* senitel */ }
 };
 MODULE_DEVICE_TABLE(of, aic32x4_of_id);
diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
index 70a1574fb72a..58e773d682d2 100644
--- a/sound/soc/codecs/tlv320aic32x4.c
+++ b/sound/soc/codecs/tlv320aic32x4.c
@@ -221,6 +221,38 @@ static int aic32x4_set_mfp5_gpio(struct snd_kcontrol *kcontrol,
 	return 0;
 };
 
+static int aic32x4_tas2505_spkdrv_getvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+	unsigned int val;
+
+	val = snd_soc_component_read(component, TAS2505_SPKVOL1);
+	val = (val > mc->max) ? mc->max : val;
+	val = mc->invert ? mc->max - val : val;
+	ucontrol->value.integer.value[0] = val;
+
+	return 0;
+}
+
+static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
+	struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct soc_mixer_control *mc =
+		(struct soc_mixer_control *)kcontrol->private_value;
+	u8 val;
+
+	val = (ucontrol->value.integer.value[0] & 0x7f);
+	val = mc->invert ? mc->max - val : val;
+	val = (val < 0) ? 0 : val;
+	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
+
+	return 0;
+}
+
 static const struct snd_kcontrol_new aic32x4_mfp1[] = {
 	SOC_SINGLE_BOOL_EXT("MFP1 GPIO", 0, aic32x4_get_mfp1_gpio, NULL),
 };
@@ -251,6 +283,9 @@ static DECLARE_TLV_DB_SCALE(tlv_driver_gain, -600, 100, 0);
 /* -12dB min, 0.5dB steps */
 static DECLARE_TLV_DB_SCALE(tlv_adc_vol, -1200, 50, 0);
 
+static DECLARE_TLV_DB_LINEAR(tlv_spk_vol, TLV_DB_GAIN_MUTE, 0);
+static DECLARE_TLV_DB_SCALE(tlv_amp_vol, 0, 600, 1);
+
 static const char * const lo_cm_text[] = {
 	"Full Chip", "1.65V",
 };
@@ -1059,6 +1094,130 @@ static const struct snd_soc_component_driver soc_component_dev_aic32x4 = {
 	.non_legacy_dai_naming	= 1,
 };
 
+static const struct snd_kcontrol_new aic32x4_tas2505_snd_controls[] = {
+	SOC_DOUBLE_R_S_TLV("PCM Playback Volume", AIC32X4_LDACVOL,
+			AIC32X4_LDACVOL, 0, -0x7f, 0x30, 7, 0, tlv_pcm),
+	SOC_ENUM("DAC Playback PowerTune Switch", l_ptm_enum),
+	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
+			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
+			tlv_driver_gain),
+	SOC_DOUBLE_R("HP DAC Playback Switch", AIC32X4_HPLGAIN,
+			AIC32X4_HPLGAIN, 6, 0x01, 1),
+
+	SOC_SINGLE("Auto-mute Switch", AIC32X4_DACMUTE, 4, 7, 0),
+
+	SOC_SINGLE_RANGE_EXT_TLV("Speaker Driver Playback Volume", TAS2505_SPKVOL1,
+			0, 0, 117, 1, aic32x4_tas2505_spkdrv_getvol,
+			aic32x4_tas2505_spkdrv_putvol, tlv_spk_vol),
+	SOC_SINGLE_TLV("Speaker Amplifier Playback Volume", TAS2505_SPKVOL2,
+			4, 5, 0, tlv_amp_vol),
+};
+
+static const struct snd_kcontrol_new hp_output_mixer_controls[] = {
+	SOC_DAPM_SINGLE("DAC Switch", AIC32X4_HPLROUTE, 3, 1, 0),
+};
+
+static const struct snd_soc_dapm_widget aic32x4_tas2505_dapm_widgets[] = {
+	SND_SOC_DAPM_DAC("DAC", "Playback", AIC32X4_DACSETUP, 7, 0),
+	SND_SOC_DAPM_MIXER("HP Output Mixer", SND_SOC_NOPM, 0, 0,
+			   &hp_output_mixer_controls[0],
+			   ARRAY_SIZE(hp_output_mixer_controls)),
+	SND_SOC_DAPM_PGA("HP Power", AIC32X4_OUTPWRCTL, 5, 0, NULL, 0),
+
+	SND_SOC_DAPM_PGA("Speaker Driver", TAS2505_SPK, 1, 0, NULL, 0),
+
+	SND_SOC_DAPM_OUTPUT("HP"),
+	SND_SOC_DAPM_OUTPUT("Speaker"),
+};
+
+static const struct snd_soc_dapm_route aic32x4_tas2505_dapm_routes[] = {
+	/* Left Output */
+	{"HP Output Mixer", "DAC Switch", "DAC"},
+
+	{"HP Power", NULL, "HP Output Mixer"},
+	{"HP", NULL, "HP Power"},
+
+	{"Speaker Driver", NULL, "DAC"},
+	{"Speaker", NULL, "Speaker Driver"},
+};
+
+static struct snd_soc_dai_driver aic32x4_tas2505_dai = {
+	.name = "tas2505-hifi",
+	.playback = {
+			 .stream_name = "Playback",
+			 .channels_min = 1,
+			 .channels_max = 1,
+			 .rates = SNDRV_PCM_RATE_8000_96000,
+			 .formats = AIC32X4_FORMATS,},
+	.ops = &aic32x4_ops,
+	.symmetric_rates = 1,
+};
+
+static int aic32x4_tas2505_component_probe(struct snd_soc_component *component)
+{
+	struct aic32x4_priv *aic32x4 = snd_soc_component_get_drvdata(component);
+	u32 tmp_reg;
+	int ret;
+
+	struct clk_bulk_data clocks[] = {
+		{ .id = "codec_clkin" },
+		{ .id = "pll" },
+		{ .id = "bdiv" },
+		{ .id = "mdac" },
+	};
+
+	ret = devm_clk_bulk_get(component->dev, ARRAY_SIZE(clocks), clocks);
+	if (ret)
+		return ret;
+
+	if (aic32x4->setup)
+		aic32x4_setup_gpios(component);
+
+	clk_set_parent(clocks[0].clk, clocks[1].clk);
+	clk_set_parent(clocks[2].clk, clocks[3].clk);
+
+	/* Power platform configuration */
+	if (aic32x4->power_cfg & AIC32X4_PWR_AVDD_DVDD_WEAK_DISABLE)
+		snd_soc_component_write(component, AIC32X4_PWRCFG, AIC32X4_AVDDWEAKDISABLE);
+
+	tmp_reg = (aic32x4->power_cfg & AIC32X4_PWR_AIC32X4_LDO_ENABLE) ?
+			AIC32X4_LDOCTLEN : 0;
+	snd_soc_component_write(component, AIC32X4_LDOCTL, tmp_reg);
+
+	tmp_reg = snd_soc_component_read(component, AIC32X4_CMMODE);
+	if (aic32x4->power_cfg & AIC32X4_PWR_CMMODE_LDOIN_RANGE_18_36)
+		tmp_reg |= AIC32X4_LDOIN_18_36;
+	if (aic32x4->power_cfg & AIC32X4_PWR_CMMODE_HP_LDOIN_POWERED)
+		tmp_reg |= AIC32X4_LDOIN2HP;
+	snd_soc_component_write(component, AIC32X4_CMMODE, tmp_reg);
+
+	/*
+	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
+	 * before using the analog circuits.
+	 */
+	snd_soc_component_write(component, TAS2505_REFPOWERUP,
+				AIC32X4_REFPOWERUP_40MS);
+	msleep(40);
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver soc_component_dev_aic32x4_tas2505 = {
+	.probe			= aic32x4_tas2505_component_probe,
+	.set_bias_level		= aic32x4_set_bias_level,
+	.controls		= aic32x4_tas2505_snd_controls,
+	.num_controls		= ARRAY_SIZE(aic32x4_tas2505_snd_controls),
+	.dapm_widgets		= aic32x4_tas2505_dapm_widgets,
+	.num_dapm_widgets	= ARRAY_SIZE(aic32x4_tas2505_dapm_widgets),
+	.dapm_routes		= aic32x4_tas2505_dapm_routes,
+	.num_dapm_routes	= ARRAY_SIZE(aic32x4_tas2505_dapm_routes),
+	.suspend_bias_off	= 1,
+	.idle_bias_on		= 1,
+	.use_pmdown_time	= 1,
+	.endianness		= 1,
+	.non_legacy_dai_naming	= 1,
+};
+
 static int aic32x4_parse_dt(struct aic32x4_priv *aic32x4,
 		struct device_node *np)
 {
@@ -1250,8 +1409,15 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap)
 	if (ret)
 		goto err_disable_regulators;
 
-	ret = devm_snd_soc_register_component(dev,
-			&soc_component_dev_aic32x4, &aic32x4_dai, 1);
+	switch (aic32x4->type) {
+	case AIC32X4_TYPE_TAS2505:
+		ret = devm_snd_soc_register_component(dev,
+			&soc_component_dev_aic32x4_tas2505, &aic32x4_tas2505_dai, 1);
+		break;
+	default:
+		ret = devm_snd_soc_register_component(dev,
+	}
+
 	if (ret) {
 		dev_err(dev, "Failed to register component\n");
 		goto err_disable_regulators;
diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h
index 8a18dbec76a6..e9fd2e55d6c3 100644
--- a/sound/soc/codecs/tlv320aic32x4.h
+++ b/sound/soc/codecs/tlv320aic32x4.h
@@ -13,6 +13,7 @@ struct regmap_config;
 enum aic32x4_type {
 	AIC32X4_TYPE_AIC32X4 = 0,
 	AIC32X4_TYPE_AIC32X6,
+	AIC32X4_TYPE_TAS2505,
 };
 
 extern const struct regmap_config aic32x4_regmap_config;
@@ -93,6 +94,9 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
 #define	AIC32X4_LOLGAIN		AIC32X4_REG(1, 18)
 #define	AIC32X4_LORGAIN		AIC32X4_REG(1, 19)
 #define AIC32X4_HEADSTART	AIC32X4_REG(1, 20)
+#define TAS2505_SPK		AIC32X4_REG(1, 45)
+#define TAS2505_SPKVOL1		AIC32X4_REG(1, 46)
+#define TAS2505_SPKVOL2		AIC32X4_REG(1, 48)
 #define AIC32X4_MICBIAS		AIC32X4_REG(1, 51)
 #define AIC32X4_LMICPGAPIN	AIC32X4_REG(1, 52)
 #define AIC32X4_LMICPGANIN	AIC32X4_REG(1, 54)
@@ -101,6 +105,7 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
 #define AIC32X4_FLOATINGINPUT	AIC32X4_REG(1, 58)
 #define AIC32X4_LMICPGAVOL	AIC32X4_REG(1, 59)
 #define AIC32X4_RMICPGAVOL	AIC32X4_REG(1, 60)
+#define TAS2505_REFPOWERUP	AIC32X4_REG(1, 122)
 #define AIC32X4_REFPOWERUP	AIC32X4_REG(1, 123)
 
 /* Bits, masks, and shifts */
-- 
2.32.0

[PATCH 3/3] ASoC: tlv320aic32x4: dt-bindings: add TAS2505 to compatible

[Claudius Heine]

This adds 'ti,tas2505' for TAS2505 to the list of allowed compatible
strings.

Signed-off-by: Claudius Heine <ch@denx.de>
---
 Documentation/devicetree/bindings/sound/tlv320aic32x4.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/sound/tlv320aic32x4.txt b/Documentation/devicetree/bindings/sound/tlv320aic32x4.txt
index ca75890f0d07..f59125bc79d1 100644
--- a/Documentation/devicetree/bindings/sound/tlv320aic32x4.txt
+++ b/Documentation/devicetree/bindings/sound/tlv320aic32x4.txt
@@ -6,6 +6,7 @@ Required properties:
  - compatible - "string" - One of:
 	"ti,tlv320aic32x4" TLV320AIC3204
 	"ti,tlv320aic32x6" TLV320AIC3206, TLV320AIC3256
+	"ti,tas2505" TAS2505, TAS2521
  - reg: I2C slave address
  - supply-*: Required supply regulators are:
     "iov" - digital IO power supply
-- 
2.32.0

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Mark Brown]

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

On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:

> With this change it will be possible to add different code paths for
> similar devices.

>  static const struct i2c_device_id aic32x4_i2c_id[] = {
> -	{ "tlv320aic32x4", 0 },
> -	{ "tlv320aic32x6", 1 },
> +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
> +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
>  	{ /* sentinel */ }

It appears that the device already supports multiple variants?

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

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Mark Brown]

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

On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:

> With this change it will be possible to add different code paths for
> similar devices.

>  static const struct i2c_device_id aic32x4_i2c_id[] = {
> -	{ "tlv320aic32x4", 0 },
> -	{ "tlv320aic32x6", 1 },
> +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
> +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
>  	{ /* sentinel */ }

It appears that the device already supports multiple variants?

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

Re: [PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Claudius Heine]

On 2021-06-15 11:49, Claudius Heine wrote:
> This adds support for TAS2505 and TAS2521 to the tlv320aic32x4 driver.

There are some rebase issues here, I will have to fix up.

regards,
Claudius

> 
> Signed-off-by: Claudius Heine <ch@denx.de>
> ---
>   sound/soc/codecs/tlv320aic32x4-i2c.c |   2 +
>   sound/soc/codecs/tlv320aic32x4.c     | 170 ++++++++++++++++++++++++++-
>   sound/soc/codecs/tlv320aic32x4.h     |   5 +
>   3 files changed, 175 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/tlv320aic32x4-i2c.c b/sound/soc/codecs/tlv320aic32x4-i2c.c
> index 247fb1e13674..04ad38311360 100644
> --- a/sound/soc/codecs/tlv320aic32x4-i2c.c
> +++ b/sound/soc/codecs/tlv320aic32x4-i2c.c
> @@ -50,6 +50,7 @@ static int aic32x4_i2c_remove(struct i2c_client *i2c)
>   static const struct i2c_device_id aic32x4_i2c_id[] = {
>   	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
>   	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
> +	{ "tas2505", (kernel_ulong_t)AIC32X4_TYPE_TAS2505 },
>   	{ /* sentinel */ }
>   };
>   MODULE_DEVICE_TABLE(i2c, aic32x4_i2c_id);
> @@ -57,6 +58,7 @@ MODULE_DEVICE_TABLE(i2c, aic32x4_i2c_id);
>   static const struct of_device_id aic32x4_of_id[] = {
>   	{ .compatible = "ti,tlv320aic32x4", .data = (void *)AIC32X4_TYPE_AIC32X4 },
>   	{ .compatible = "ti,tlv320aic32x6", .data = (void *)AIC32X4_TYPE_AIC32X6 },
> +	{ .compatible = "ti,tas2505", .data = (void *)AIC32X4_TYPE_TAS2505 },
>   	{ /* senitel */ }
>   };
>   MODULE_DEVICE_TABLE(of, aic32x4_of_id);
> diff --git a/sound/soc/codecs/tlv320aic32x4.c b/sound/soc/codecs/tlv320aic32x4.c
> index 70a1574fb72a..58e773d682d2 100644
> --- a/sound/soc/codecs/tlv320aic32x4.c
> +++ b/sound/soc/codecs/tlv320aic32x4.c
> @@ -221,6 +221,38 @@ static int aic32x4_set_mfp5_gpio(struct snd_kcontrol *kcontrol,
>   	return 0;
>   };
>   
> +static int aic32x4_tas2505_spkdrv_getvol(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	unsigned int val;
> +
> +	val = snd_soc_component_read(component, TAS2505_SPKVOL1);
> +	val = (val > mc->max) ? mc->max : val;
> +	val = mc->invert ? mc->max - val : val;
> +	ucontrol->value.integer.value[0] = val;
> +
> +	return 0;
> +}
> +
> +static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	u8 val;
> +
> +	val = (ucontrol->value.integer.value[0] & 0x7f);
> +	val = mc->invert ? mc->max - val : val;
> +	val = (val < 0) ? 0 : val;
> +	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
> +
> +	return 0;
> +}
> +
>   static const struct snd_kcontrol_new aic32x4_mfp1[] = {
>   	SOC_SINGLE_BOOL_EXT("MFP1 GPIO", 0, aic32x4_get_mfp1_gpio, NULL),
>   };
> @@ -251,6 +283,9 @@ static DECLARE_TLV_DB_SCALE(tlv_driver_gain, -600, 100, 0);
>   /* -12dB min, 0.5dB steps */
>   static DECLARE_TLV_DB_SCALE(tlv_adc_vol, -1200, 50, 0);
>   
> +static DECLARE_TLV_DB_LINEAR(tlv_spk_vol, TLV_DB_GAIN_MUTE, 0);
> +static DECLARE_TLV_DB_SCALE(tlv_amp_vol, 0, 600, 1);
> +
>   static const char * const lo_cm_text[] = {
>   	"Full Chip", "1.65V",
>   };
> @@ -1059,6 +1094,130 @@ static const struct snd_soc_component_driver soc_component_dev_aic32x4 = {
>   	.non_legacy_dai_naming	= 1,
>   };
>   
> +static const struct snd_kcontrol_new aic32x4_tas2505_snd_controls[] = {
> +	SOC_DOUBLE_R_S_TLV("PCM Playback Volume", AIC32X4_LDACVOL,
> +			AIC32X4_LDACVOL, 0, -0x7f, 0x30, 7, 0, tlv_pcm),
> +	SOC_ENUM("DAC Playback PowerTune Switch", l_ptm_enum),
> +	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
> +			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
> +			tlv_driver_gain),
> +	SOC_DOUBLE_R("HP DAC Playback Switch", AIC32X4_HPLGAIN,
> +			AIC32X4_HPLGAIN, 6, 0x01, 1),
> +
> +	SOC_SINGLE("Auto-mute Switch", AIC32X4_DACMUTE, 4, 7, 0),
> +
> +	SOC_SINGLE_RANGE_EXT_TLV("Speaker Driver Playback Volume", TAS2505_SPKVOL1,
> +			0, 0, 117, 1, aic32x4_tas2505_spkdrv_getvol,
> +			aic32x4_tas2505_spkdrv_putvol, tlv_spk_vol),
> +	SOC_SINGLE_TLV("Speaker Amplifier Playback Volume", TAS2505_SPKVOL2,
> +			4, 5, 0, tlv_amp_vol),
> +};
> +
> +static const struct snd_kcontrol_new hp_output_mixer_controls[] = {
> +	SOC_DAPM_SINGLE("DAC Switch", AIC32X4_HPLROUTE, 3, 1, 0),
> +};
> +
> +static const struct snd_soc_dapm_widget aic32x4_tas2505_dapm_widgets[] = {
> +	SND_SOC_DAPM_DAC("DAC", "Playback", AIC32X4_DACSETUP, 7, 0),
> +	SND_SOC_DAPM_MIXER("HP Output Mixer", SND_SOC_NOPM, 0, 0,
> +			   &hp_output_mixer_controls[0],
> +			   ARRAY_SIZE(hp_output_mixer_controls)),
> +	SND_SOC_DAPM_PGA("HP Power", AIC32X4_OUTPWRCTL, 5, 0, NULL, 0),
> +
> +	SND_SOC_DAPM_PGA("Speaker Driver", TAS2505_SPK, 1, 0, NULL, 0),
> +
> +	SND_SOC_DAPM_OUTPUT("HP"),
> +	SND_SOC_DAPM_OUTPUT("Speaker"),
> +};
> +
> +static const struct snd_soc_dapm_route aic32x4_tas2505_dapm_routes[] = {
> +	/* Left Output */
> +	{"HP Output Mixer", "DAC Switch", "DAC"},
> +
> +	{"HP Power", NULL, "HP Output Mixer"},
> +	{"HP", NULL, "HP Power"},
> +
> +	{"Speaker Driver", NULL, "DAC"},
> +	{"Speaker", NULL, "Speaker Driver"},
> +};
> +
> +static struct snd_soc_dai_driver aic32x4_tas2505_dai = {
> +	.name = "tas2505-hifi",
> +	.playback = {
> +			 .stream_name = "Playback",
> +			 .channels_min = 1,
> +			 .channels_max = 1,
> +			 .rates = SNDRV_PCM_RATE_8000_96000,
> +			 .formats = AIC32X4_FORMATS,},
> +	.ops = &aic32x4_ops,
> +	.symmetric_rates = 1,
> +};
> +
> +static int aic32x4_tas2505_component_probe(struct snd_soc_component *component)
> +{
> +	struct aic32x4_priv *aic32x4 = snd_soc_component_get_drvdata(component);
> +	u32 tmp_reg;
> +	int ret;
> +
> +	struct clk_bulk_data clocks[] = {
> +		{ .id = "codec_clkin" },
> +		{ .id = "pll" },
> +		{ .id = "bdiv" },
> +		{ .id = "mdac" },
> +	};
> +
> +	ret = devm_clk_bulk_get(component->dev, ARRAY_SIZE(clocks), clocks);
> +	if (ret)
> +		return ret;
> +
> +	if (aic32x4->setup)
> +		aic32x4_setup_gpios(component);
> +
> +	clk_set_parent(clocks[0].clk, clocks[1].clk);
> +	clk_set_parent(clocks[2].clk, clocks[3].clk);
> +
> +	/* Power platform configuration */
> +	if (aic32x4->power_cfg & AIC32X4_PWR_AVDD_DVDD_WEAK_DISABLE)
> +		snd_soc_component_write(component, AIC32X4_PWRCFG, AIC32X4_AVDDWEAKDISABLE);
> +
> +	tmp_reg = (aic32x4->power_cfg & AIC32X4_PWR_AIC32X4_LDO_ENABLE) ?
> +			AIC32X4_LDOCTLEN : 0;
> +	snd_soc_component_write(component, AIC32X4_LDOCTL, tmp_reg);
> +
> +	tmp_reg = snd_soc_component_read(component, AIC32X4_CMMODE);
> +	if (aic32x4->power_cfg & AIC32X4_PWR_CMMODE_LDOIN_RANGE_18_36)
> +		tmp_reg |= AIC32X4_LDOIN_18_36;
> +	if (aic32x4->power_cfg & AIC32X4_PWR_CMMODE_HP_LDOIN_POWERED)
> +		tmp_reg |= AIC32X4_LDOIN2HP;
> +	snd_soc_component_write(component, AIC32X4_CMMODE, tmp_reg);
> +
> +	/*
> +	 * Enable the fast charging feature and ensure the needed 40ms ellapsed
> +	 * before using the analog circuits.
> +	 */
> +	snd_soc_component_write(component, TAS2505_REFPOWERUP,
> +				AIC32X4_REFPOWERUP_40MS);
> +	msleep(40);
> +
> +	return 0;
> +}
> +
> +static const struct snd_soc_component_driver soc_component_dev_aic32x4_tas2505 = {
> +	.probe			= aic32x4_tas2505_component_probe,
> +	.set_bias_level		= aic32x4_set_bias_level,
> +	.controls		= aic32x4_tas2505_snd_controls,
> +	.num_controls		= ARRAY_SIZE(aic32x4_tas2505_snd_controls),
> +	.dapm_widgets		= aic32x4_tas2505_dapm_widgets,
> +	.num_dapm_widgets	= ARRAY_SIZE(aic32x4_tas2505_dapm_widgets),
> +	.dapm_routes		= aic32x4_tas2505_dapm_routes,
> +	.num_dapm_routes	= ARRAY_SIZE(aic32x4_tas2505_dapm_routes),
> +	.suspend_bias_off	= 1,
> +	.idle_bias_on		= 1,
> +	.use_pmdown_time	= 1,
> +	.endianness		= 1,
> +	.non_legacy_dai_naming	= 1,
> +};
> +
>   static int aic32x4_parse_dt(struct aic32x4_priv *aic32x4,
>   		struct device_node *np)
>   {
> @@ -1250,8 +1409,15 @@ int aic32x4_probe(struct device *dev, struct regmap *regmap)
>   	if (ret)
>   		goto err_disable_regulators;
>   
> -	ret = devm_snd_soc_register_component(dev,
> -			&soc_component_dev_aic32x4, &aic32x4_dai, 1);
> +	switch (aic32x4->type) {
> +	case AIC32X4_TYPE_TAS2505:
> +		ret = devm_snd_soc_register_component(dev,
> +			&soc_component_dev_aic32x4_tas2505, &aic32x4_tas2505_dai, 1);
> +		break;
> +	default:
> +		ret = devm_snd_soc_register_component(dev,
> +	}
> +
>   	if (ret) {
>   		dev_err(dev, "Failed to register component\n");
>   		goto err_disable_regulators;
> diff --git a/sound/soc/codecs/tlv320aic32x4.h b/sound/soc/codecs/tlv320aic32x4.h
> index 8a18dbec76a6..e9fd2e55d6c3 100644
> --- a/sound/soc/codecs/tlv320aic32x4.h
> +++ b/sound/soc/codecs/tlv320aic32x4.h
> @@ -13,6 +13,7 @@ struct regmap_config;
>   enum aic32x4_type {
>   	AIC32X4_TYPE_AIC32X4 = 0,
>   	AIC32X4_TYPE_AIC32X6,
> +	AIC32X4_TYPE_TAS2505,
>   };
>   
>   extern const struct regmap_config aic32x4_regmap_config;
> @@ -93,6 +94,9 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
>   #define	AIC32X4_LOLGAIN		AIC32X4_REG(1, 18)
>   #define	AIC32X4_LORGAIN		AIC32X4_REG(1, 19)
>   #define AIC32X4_HEADSTART	AIC32X4_REG(1, 20)
> +#define TAS2505_SPK		AIC32X4_REG(1, 45)
> +#define TAS2505_SPKVOL1		AIC32X4_REG(1, 46)
> +#define TAS2505_SPKVOL2		AIC32X4_REG(1, 48)
>   #define AIC32X4_MICBIAS		AIC32X4_REG(1, 51)
>   #define AIC32X4_LMICPGAPIN	AIC32X4_REG(1, 52)
>   #define AIC32X4_LMICPGANIN	AIC32X4_REG(1, 54)
> @@ -101,6 +105,7 @@ int aic32x4_register_clocks(struct device *dev, const char *mclk_name);
>   #define AIC32X4_FLOATINGINPUT	AIC32X4_REG(1, 58)
>   #define AIC32X4_LMICPGAVOL	AIC32X4_REG(1, 59)
>   #define AIC32X4_RMICPGAVOL	AIC32X4_REG(1, 60)
> +#define TAS2505_REFPOWERUP	AIC32X4_REG(1, 122)
>   #define AIC32X4_REFPOWERUP	AIC32X4_REG(1, 123)
>   
>   /* Bits, masks, and shifts */
> 

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de

            PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153
                              Keyserver: hkp://pool.sks-keyservers.net

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Claudius Heine]

Hi Mark,

On 2021-06-15 13:54, Mark Brown wrote:
> On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:
> 
>> With this change it will be possible to add different code paths for
>> similar devices.
> 
>>   static const struct i2c_device_id aic32x4_i2c_id[] = {
>> -	{ "tlv320aic32x4", 0 },
>> -	{ "tlv320aic32x6", 1 },
>> +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
>> +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
>>   	{ /* sentinel */ }
> 
> It appears that the device already supports multiple variants?

Those values aren't used anywhere as far as I can see.

regards,
Claudius

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Claudius Heine]

Hi Mark,

On 2021-06-15 13:54, Mark Brown wrote:
> On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:
> 
>> With this change it will be possible to add different code paths for
>> similar devices.
> 
>>   static const struct i2c_device_id aic32x4_i2c_id[] = {
>> -	{ "tlv320aic32x4", 0 },
>> -	{ "tlv320aic32x6", 1 },
>> +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
>> +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
>>   	{ /* sentinel */ }
> 
> It appears that the device already supports multiple variants?

Those values aren't used anywhere as far as I can see.

regards,
Claudius

Re: [PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Mark Brown]

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

On Tue, Jun 15, 2021 at 11:49:31AM +0200, Claudius Heine wrote:

> +static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	u8 val;
> +
> +	val = (ucontrol->value.integer.value[0] & 0x7f);
> +	val = mc->invert ? mc->max - val : val;
> +	val = (val < 0) ? 0 : val;
> +	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
> +
> +	return 0;
> +}

Controls should return a boolean indicating if they changed their value
when written.  Other than the hard coded register what's device specific
here?  It looks like a normal control with a maximum value, it is
unclear why this is being open coded.

> +	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
> +			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
> +			tlv_driver_gain),

Drop the Gain.  See control-names.rst.

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

Re: [PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Mark Brown]

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

On Tue, Jun 15, 2021 at 11:49:31AM +0200, Claudius Heine wrote:

> +static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
> +	struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct soc_mixer_control *mc =
> +		(struct soc_mixer_control *)kcontrol->private_value;
> +	u8 val;
> +
> +	val = (ucontrol->value.integer.value[0] & 0x7f);
> +	val = mc->invert ? mc->max - val : val;
> +	val = (val < 0) ? 0 : val;
> +	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
> +
> +	return 0;
> +}

Controls should return a boolean indicating if they changed their value
when written.  Other than the hard coded register what's device specific
here?  It looks like a normal control with a maximum value, it is
unclear why this is being open coded.

> +	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
> +			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
> +			tlv_driver_gain),

Drop the Gain.  See control-names.rst.

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

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Mark Brown]

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

On Tue, Jun 15, 2021 at 02:06:30PM +0200, Claudius Heine wrote:
> On 2021-06-15 13:54, Mark Brown wrote:
> > On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:

> > > With this change it will be possible to add different code paths for
> > > similar devices.

> > > -	{ "tlv320aic32x4", 0 },
> > > -	{ "tlv320aic32x6", 1 },
> > > +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
> > > +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
> > >   	{ /* sentinel */ }

> > It appears that the device already supports multiple variants?

> Those values aren't used anywhere as far as I can see.

The point here is that you need a better changelog, the driver clearly
already supports multiple devices so we need a few more words to explain
what this is doing.

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

Re: [PATCH 1/3] ASoC: tlv320aic32x4: prepare driver for different device variants

[Mark Brown]

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

On Tue, Jun 15, 2021 at 02:06:30PM +0200, Claudius Heine wrote:
> On 2021-06-15 13:54, Mark Brown wrote:
> > On Tue, Jun 15, 2021 at 11:49:30AM +0200, Claudius Heine wrote:

> > > With this change it will be possible to add different code paths for
> > > similar devices.

> > > -	{ "tlv320aic32x4", 0 },
> > > -	{ "tlv320aic32x6", 1 },
> > > +	{ "tlv320aic32x4", (kernel_ulong_t)AIC32X4_TYPE_AIC32X4 },
> > > +	{ "tlv320aic32x6", (kernel_ulong_t)AIC32X4_TYPE_AIC32X6 },
> > >   	{ /* sentinel */ }

> > It appears that the device already supports multiple variants?

> Those values aren't used anywhere as far as I can see.

The point here is that you need a better changelog, the driver clearly
already supports multiple devices so we need a few more words to explain
what this is doing.

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

Re: [PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Claudius Heine]

On 2021-06-15 14:22, Mark Brown wrote:
> On Tue, Jun 15, 2021 at 11:49:31AM +0200, Claudius Heine wrote:
> 
>> +static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct soc_mixer_control *mc =
>> +		(struct soc_mixer_control *)kcontrol->private_value;
>> +	u8 val;
>> +
>> +	val = (ucontrol->value.integer.value[0] & 0x7f);
>> +	val = mc->invert ? mc->max - val : val;
>> +	val = (val < 0) ? 0 : val;
>> +	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
>> +
>> +	return 0;
>> +}
> 
> Controls should return a boolean indicating if they changed their value
> when written.  Other than the hard coded register what's device specific
> here?  It looks like a normal control with a maximum value, it is
> unclear why this is being open coded.

Well probably because I didn't knew any better. Will look into it. Thx!

> 
>> +	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
>> +			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
>> +			tlv_driver_gain),
> 
> Drop the Gain.  See control-names.rst.
> 

Ok.

Thanks!
Claudius

Re: [PATCH 2/3] ASoC: tlv320aic32x4: add support for TAS2505

[Claudius Heine]

On 2021-06-15 14:22, Mark Brown wrote:
> On Tue, Jun 15, 2021 at 11:49:31AM +0200, Claudius Heine wrote:
> 
>> +static int aic32x4_tas2505_spkdrv_putvol(struct snd_kcontrol *kcontrol,
>> +	struct snd_ctl_elem_value *ucontrol)
>> +{
>> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
>> +	struct soc_mixer_control *mc =
>> +		(struct soc_mixer_control *)kcontrol->private_value;
>> +	u8 val;
>> +
>> +	val = (ucontrol->value.integer.value[0] & 0x7f);
>> +	val = mc->invert ? mc->max - val : val;
>> +	val = (val < 0) ? 0 : val;
>> +	snd_soc_component_write(component, TAS2505_SPKVOL1, val);
>> +
>> +	return 0;
>> +}
> 
> Controls should return a boolean indicating if they changed their value
> when written.  Other than the hard coded register what's device specific
> here?  It looks like a normal control with a maximum value, it is
> unclear why this is being open coded.

Well probably because I didn't knew any better. Will look into it. Thx!

> 
>> +	SOC_DOUBLE_R_S_TLV("HP Driver Gain Playback Volume", AIC32X4_HPLGAIN,
>> +			AIC32X4_HPLGAIN, 0, -0x6, 0x1d, 5, 0,
>> +			tlv_driver_gain),
> 
> Drop the Gain.  See control-names.rst.
> 

Ok.

Thanks!
Claudius