diff mbox series

[v1,3/5] ASoC: codecs: ES8326: Adding new volume kcontrols

Message ID 20240120101240.12496-4-zhuning0077@gmail.com (mailing list archive)
State New, archived
Headers show
Series ASoC: codecs: fix ES8326 performance and pop noise | expand

Commit Message

Zhu Ning Jan. 20, 2024, 10:12 a.m. UTC 
ES8326 features a headphone volume control register and four DAC
volume control registers.
We add new volume Kcontrols for these registers to enhance the
configurability of the volume settings, providing users with
greater flexibility.

Signed-off-by: Zhu Ning <zhuning0077@gmail.com>
---
 sound/soc/codecs/es8326.c | 88 ++++++++++++++++++++++++++++++++++++++-
 sound/soc/codecs/es8326.h |  5 ++-
 2 files changed, 91 insertions(+), 2 deletions(-)

Comments

Mark Brown Jan. 22, 2024, 5:54 p.m. UTC | #1 
On Sat, Jan 20, 2024 at 06:12:38PM +0800, Zhu Ning wrote:
> ES8326 features a headphone volume control register and four DAC
> volume control registers.
> We add new volume Kcontrols for these registers to enhance the
> configurability of the volume settings, providing users with
> greater flexibility.

This is much better integrated than the earlier version, but there's
still a few issues.

> +static int es8326_hplvol_set(struct snd_kcontrol *kcontrol,
> +		struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
> +	unsigned int hp_vol;
> +
> +	if (ucontrol->value.integer.value[0] == 3) {
> +		dev_dbg(component->dev, "HPL_VOL does not change");
> +		return 0;
> +	}

This will silently ignore attempts to write the invalid value which
isn't great any might confuse some userspace code, it would be better to
do something like

	val = ucontrol->value.integer.value[0];
	if (val >= 3)
		val++;

(with corresponding changes to the other functions) so that we just skip
over the invalid value without userspace being aware of it.  We should
also be validating that out of range values are rejected or at least
constained to the relevant bitfield, other than 3 any invalid value will
just be written straight into the register (rejecting should be more
robust).

> +	regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
> +	hp_vol &= 0x8F;
> +	hp_vol |= ucontrol->value.integer.value[0] << 4;
> +	regmap_write(es8326->regmap, ES8326_HP_VOL, hp_vol);

regmap_update_bits().

> +
> +	return 0;
> +}

Also please run mixer-test on your driver - for this control it'll tell
you that this function isn't returning 1 when the value changes, meaning
that events won't be generated when the value changes.

> +	SOC_SINGLE_TLV("HPL Playback Volume", ES8326_DACL_VOL, 0, 0xbf, 0, dac_vol_tlv),
> +	SOC_SINGLE_TLV("HPR Playback Volume", ES8326_DACR_VOL, 0, 0xbf, 0, dac_vol_tlv),
> +	SOC_SINGLE_TLV("SPKL Playback Volume", ES8326_SPKL_VOL, 0, 0xbf, 0, dac_vol_tlv),
> +	SOC_SINGLE_TLV("SPKR Playback Volume", ES8326_SPKR_VOL, 0, 0xbf, 0, dac_vol_tlv),

It would be *better* if these were stereo controls, but it's not
essential.

> +
> +	SOC_ENUM("HPVol SPKVol switch", hpvol_spkvol_switch),

Switch should have a capital letter (mixer-test should catch this as
well).
diff mbox series

Patch

diff --git a/sound/soc/codecs/es8326.c b/sound/soc/codecs/es8326.c
index 10157a4bd500..51dc8081e475 100755
--- a/sound/soc/codecs/es8326.c
+++ b/sound/soc/codecs/es8326.c
@@ -121,6 +121,72 @@  static int es8326_crosstalk2_set(struct snd_kcontrol *kcontrol,
 	return 0;
 }
 
+static int es8326_hplvol_get(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
+	unsigned int hp_vol;
+
+	regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
+	hp_vol &= 0x70;
+	ucontrol->value.integer.value[0] = hp_vol >> 4;
+
+	return 0;
+}
+
+static int es8326_hplvol_set(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
+	unsigned int hp_vol;
+
+	if (ucontrol->value.integer.value[0] == 3) {
+		dev_dbg(component->dev, "HPL_VOL does not change");
+		return 0;
+	}
+	regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
+	hp_vol &= 0x8F;
+	hp_vol |= ucontrol->value.integer.value[0] << 4;
+	regmap_write(es8326->regmap, ES8326_HP_VOL, hp_vol);
+
+	return 0;
+}
+
+static int es8326_hprvol_get(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
+	unsigned int hp_vol;
+
+	regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
+	hp_vol &= 0x07;
+	ucontrol->value.integer.value[0] = hp_vol;
+
+	return 0;
+}
+
+static int es8326_hprvol_set(struct snd_kcontrol *kcontrol,
+		struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
+	struct es8326_priv *es8326 = snd_soc_component_get_drvdata(component);
+	unsigned int hp_vol;
+
+	if (ucontrol->value.integer.value[0] == 3) {
+		dev_dbg(component->dev, "HPR_VOL does not change");
+		return 0;
+	}
+	regmap_read(es8326->regmap, ES8326_HP_VOL, &hp_vol);
+	hp_vol &= 0xF8;
+	hp_vol |= ucontrol->value.integer.value[0];
+	regmap_write(es8326->regmap, ES8326_HP_VOL, hp_vol);
+
+	return 0;
+}
+
 static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(dac_vol_tlv, -9550, 50, 0);
 static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(adc_vol_tlv, -9550, 50, 0);
 static const SNDRV_CTL_TLVD_DECLARE_DB_SCALE(adc_analog_pga_tlv, 0, 300, 0);
@@ -151,15 +217,24 @@  static const char *const winsize[] = {
 static const char *const dacpol_txt[] =	{
 	"Normal", "R Invert", "L Invert", "L + R Invert" };
 
+static const char *const hp_spkvol_switch[] = {
+	"HPVOL: HPL+HPL, SPKVOL: HPL+HPL",
+	"HPVOL: HPL+HPR, SPKVOL: HPL+HPR",
+	"HPVOL: HPL+HPL, SPKVOL: SPKL+SPKR",
+	"HPVOL: HPL+HPR, SPKVOL: SPKL+SPKR",
+};
+
 static const struct soc_enum dacpol =
 	SOC_ENUM_SINGLE(ES8326_DAC_DSM, 4, 4, dacpol_txt);
 static const struct soc_enum alc_winsize =
 	SOC_ENUM_SINGLE(ES8326_ADC_RAMPRATE, 4, 16, winsize);
 static const struct soc_enum drc_winsize =
 	SOC_ENUM_SINGLE(ES8326_DRC_WINSIZE, 4, 16, winsize);
+static const struct soc_enum hpvol_spkvol_switch =
+	SOC_ENUM_SINGLE(ES8326_HP_MISC, 6, 4, hp_spkvol_switch);
 
 static const struct snd_kcontrol_new es8326_snd_controls[] = {
-	SOC_SINGLE_TLV("DAC Playback Volume", ES8326_DAC_VOL, 0, 0xbf, 0, dac_vol_tlv),
+	SOC_SINGLE_TLV("DAC Playback Volume", ES8326_DACL_VOL, 0, 0xbf, 0, dac_vol_tlv),
 	SOC_ENUM("Playback Polarity", dacpol),
 	SOC_SINGLE_TLV("DAC Ramp Rate", ES8326_DAC_RAMPRATE, 0, 0x0f, 0, softramp_rate),
 	SOC_SINGLE_TLV("DRC Recovery Level", ES8326_DRC_RECOVERY, 0, 4, 0, drc_recovery_tlv),
@@ -182,6 +257,17 @@  static const struct snd_kcontrol_new es8326_snd_controls[] = {
 			es8326_crosstalk1_get, es8326_crosstalk1_set),
 	SOC_SINGLE_EXT("CROSSTALK2", SND_SOC_NOPM, 0, 31, 0,
 			es8326_crosstalk2_get, es8326_crosstalk2_set),
+	SOC_SINGLE_EXT("HPL Volume", SND_SOC_NOPM, 0, 6, 0,
+			es8326_hplvol_get, es8326_hplvol_set),
+	SOC_SINGLE_EXT("HPR Volume", SND_SOC_NOPM, 0, 6, 0,
+			es8326_hprvol_get, es8326_hprvol_set),
+
+	SOC_SINGLE_TLV("HPL Playback Volume", ES8326_DACL_VOL, 0, 0xbf, 0, dac_vol_tlv),
+	SOC_SINGLE_TLV("HPR Playback Volume", ES8326_DACR_VOL, 0, 0xbf, 0, dac_vol_tlv),
+	SOC_SINGLE_TLV("SPKL Playback Volume", ES8326_SPKL_VOL, 0, 0xbf, 0, dac_vol_tlv),
+	SOC_SINGLE_TLV("SPKR Playback Volume", ES8326_SPKR_VOL, 0, 0xbf, 0, dac_vol_tlv),
+
+	SOC_ENUM("HPVol SPKVol switch", hpvol_spkvol_switch),
 };
 
 static const struct snd_soc_dapm_widget es8326_dapm_widgets[] = {
diff --git a/sound/soc/codecs/es8326.h b/sound/soc/codecs/es8326.h
index 4234bbb900c4..ee12caef8105 100644
--- a/sound/soc/codecs/es8326.h
+++ b/sound/soc/codecs/es8326.h
@@ -69,7 +69,7 @@ 
 #define ES8326_DAC_DSM		0x4D
 #define ES8326_DAC_RAMPRATE	0x4E
 #define ES8326_DAC_VPPSCALE	0x4F
-#define ES8326_DAC_VOL		0x50
+#define ES8326_DACL_VOL	        0x50
 #define ES8326_DRC_RECOVERY	0x53
 #define ES8326_DRC_WINSIZE	0x54
 #define ES8326_DAC_CROSSTALK	0x55
@@ -81,6 +81,9 @@ 
 #define ES8326_SDINOUT23_IO	0x5B
 #define ES8326_JACK_PULSE	0x5C
 
+#define ES8326_DACR_VOL		0xF4
+#define ES8326_SPKL_VOL		0xF5
+#define ES8326_SPKR_VOL		0xF6
 #define ES8326_HP_MISC		0xF7
 #define ES8326_CTIA_OMTP_STA	0xF8
 #define ES8326_PULLUP_CTL	0xF9