All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes
@ 2021-02-15 14:21 Hans de Goede
  2021-02-15 14:21 ` [PATCH 1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control Hans de Goede
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Hans de Goede @ 2021-02-15 14:21 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Oder Chiou
  Cc: Hans de Goede, alsa-devel, Takashi Iwai, Bard Liao

Hi All,

While working on adding hardware-volume control support to the UCM
profile for the rt5672 and on adding LED trigger support to the
rt5670 codec driver. I hit / noticed a couple of issues this series
fixes these issues.

Regards,

Hans


Hans de Goede (4):
  ASoC: rt5670: Remove 'OUT Channel Switch' control
  ASoC: rt5670: Remove 'HP Playback Switch' control
  ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer
    settings
  ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control

 sound/soc/codecs/rt5670.c | 110 +++++++++++++++++++++++++++++++++-----
 sound/soc/codecs/rt5670.h |   9 ++--
 2 files changed, 101 insertions(+), 18 deletions(-)

-- 
2.30.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control
  2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
@ 2021-02-15 14:21 ` Hans de Goede
  2021-02-15 14:21 ` [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback " Hans de Goede
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2021-02-15 14:21 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Oder Chiou
  Cc: Hans de Goede, alsa-devel, Takashi Iwai, Bard Liao

The "OUT Channel Switch" control is a left over from code copied from
thr rt5640 codec driver.

With the rt5640 codec driver the output volume controls have 2 pairs of
mute bits:
bit 7, 15: Mute Control for Spk/Headphone/Line Output Port
bit 6, 14: Mute Control for Spk/Headphone/Line Volume Channel

Bits 7 and 15 are normal mute bits on the rt5670/5672 which are
controlled by 2 dapm widgets:
	SND_SOC_DAPM_SWITCH("LOUT L Playback", SND_SOC_NOPM, 0, 0,
			    &lout_l_enable_control),
	SND_SOC_DAPM_SWITCH("LOUT R Playback", SND_SOC_NOPM, 0, 0,
			    &lout_r_enable_control),

But on the 5670/5672 bit 6 is always reserved, where as bit 14 is
"LOUT Differential Mode" on the 5670 and also reserved on the 5672.

So the "OUT Channel Switch" control which is controlling bits 6+14
of the "LINE Output Control" register is bogus -> remove it.

This should not cause any issues for userspace. AFAICT the rt567x codecs
are only used on x86/ACPI devices and the UCM profiles used there do not
use the "OUT Channel Switch" control.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 2 --
 sound/soc/codecs/rt5670.h | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index a0c8f58d729b..b13d9e0e0e61 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -637,8 +637,6 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 		RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
 		39, 1, out_vol_tlv),
 	/* OUTPUT Control */
-	SOC_DOUBLE("OUT Channel Switch", RT5670_LOUT1,
-		RT5670_VOL_L_SFT, RT5670_VOL_R_SFT, 1, 1),
 	SOC_DOUBLE_TLV("OUT Playback Volume", RT5670_LOUT1,
 		RT5670_L_VOL_SFT, RT5670_R_VOL_SFT, 39, 1, out_vol_tlv),
 	/* DAC Digital Volume */
diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index 56b13fe6bd3c..f9c4db156c80 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -212,12 +212,8 @@
 /* global definition */
 #define RT5670_L_MUTE				(0x1 << 15)
 #define RT5670_L_MUTE_SFT			15
-#define RT5670_VOL_L_MUTE			(0x1 << 14)
-#define RT5670_VOL_L_SFT			14
 #define RT5670_R_MUTE				(0x1 << 7)
 #define RT5670_R_MUTE_SFT			7
-#define RT5670_VOL_R_MUTE			(0x1 << 6)
-#define RT5670_VOL_R_SFT			6
 #define RT5670_L_VOL_MASK			(0x3f << 8)
 #define RT5670_L_VOL_SFT			8
 #define RT5670_R_VOL_MASK			(0x3f)
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback Switch' control
  2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
  2021-02-15 14:21 ` [PATCH 1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control Hans de Goede
@ 2021-02-15 14:21 ` Hans de Goede
  2021-02-15 18:09   ` Jaroslav Kysela
  2021-02-15 14:21 ` [PATCH 3/4] ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer settings Hans de Goede
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2021-02-15 14:21 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Oder Chiou
  Cc: Hans de Goede, alsa-devel, Takashi Iwai, Bard Liao

The RT5670_L_MUTE_SFT and RT5670_R_MUTE_SFT bits (bits 15 and 7) of the
RT5670_HP_VOL register are set / unset by the headphones deplop code
run by rt5670_hp_event() on SND_SOC_DAPM_POST_PMU / SND_SOC_DAPM_PRE_PMD.

So we should not also export a control to userspace which toggles these
same bits.

This should not cause any issues for userspace. AFAICT the rt567x codecs
are only used on x86/ACPI devices and the UCM profiles used there do not
use the "HP Playback Switch" control.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index b13d9e0e0e61..0f372a748b0e 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -631,8 +631,6 @@ static SOC_ENUM_SINGLE_DECL(rt5670_if2_adc_enum, RT5670_DIG_INF1_DATA,
 
 static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 	/* Headphone Output Volume */
-	SOC_DOUBLE("HP Playback Switch", RT5670_HP_VOL,
-		RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1),
 	SOC_DOUBLE_TLV("HP Playback Volume", RT5670_HP_VOL,
 		RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
 		39, 1, out_vol_tlv),
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/4] ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer settings
  2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
  2021-02-15 14:21 ` [PATCH 1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control Hans de Goede
  2021-02-15 14:21 ` [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback " Hans de Goede
@ 2021-02-15 14:21 ` Hans de Goede
  2021-02-15 14:21 ` [PATCH 4/4] ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control Hans de Goede
  2021-02-24 16:57 ` [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Mark Brown
  4 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2021-02-15 14:21 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Oder Chiou
  Cc: Hans de Goede, alsa-devel, Takashi Iwai, Bard Liao

The SND_SOC_DAPM_MIXER declaration for "Sto1 ADC MIXL" and "Sto1 ADC MIXR"
was using the mute bits from the RT5670_STO1_ADC_DIG_VOL control as mixer
master mute bits.

But these bits are already exposed to userspace as controls as part of the
"ADC Capture Volume" / "ADC Capture Switch" control pair:

        SOC_DOUBLE("ADC Capture Switch", RT5670_STO1_ADC_DIG_VOL,
                RT5670_L_MUTE_SFT, RT5670_R_MUTE_SFT, 1, 1),
        SOC_DOUBLE_TLV("ADC Capture Volume", RT5670_STO1_ADC_DIG_VOL,
                        RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
                        127, 0, adc_vol_tlv),

Both the fact that the mute bits belong to the same reg as the vol-ctrl
and the "Digital Mixer Path" diagram in the datasheet clearly shows that
these mute bits are not part of the mixer and having 2 separate controls
poking at the same bits is a bad idea.

Remove the master-mute bits settings from the  "Sto1 ADC MIXL" and
"Sto1 ADC MIXR" DAPM widget declarations, avoiding these bits getting
poked from 2 different places.

This should not cause any issues for userspace. AFAICT the rt567x codecs
are only used on x86/ACPI devices and the UCM profiles used there already
set the "ADC Capture Switch" as needed.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index 0f372a748b0e..4d22fa4e8ab7 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -1652,12 +1652,10 @@ static const struct snd_soc_dapm_widget rt5670_dapm_widgets[] = {
 			    RT5670_PWR_ADC_S1F_BIT, 0, NULL, 0),
 	SND_SOC_DAPM_SUPPLY("ADC Stereo2 Filter", RT5670_PWR_DIG2,
 			    RT5670_PWR_ADC_S2F_BIT, 0, NULL, 0),
-	SND_SOC_DAPM_MIXER("Sto1 ADC MIXL", RT5670_STO1_ADC_DIG_VOL,
-			   RT5670_L_MUTE_SFT, 1, rt5670_sto1_adc_l_mix,
-			   ARRAY_SIZE(rt5670_sto1_adc_l_mix)),
-	SND_SOC_DAPM_MIXER("Sto1 ADC MIXR", RT5670_STO1_ADC_DIG_VOL,
-			   RT5670_R_MUTE_SFT, 1, rt5670_sto1_adc_r_mix,
-			   ARRAY_SIZE(rt5670_sto1_adc_r_mix)),
+	SND_SOC_DAPM_MIXER("Sto1 ADC MIXL", SND_SOC_NOPM, 0, 0,
+			   rt5670_sto1_adc_l_mix, ARRAY_SIZE(rt5670_sto1_adc_l_mix)),
+	SND_SOC_DAPM_MIXER("Sto1 ADC MIXR", SND_SOC_NOPM, 0, 0,
+			   rt5670_sto1_adc_r_mix, ARRAY_SIZE(rt5670_sto1_adc_r_mix)),
 	SND_SOC_DAPM_MIXER("Sto2 ADC MIXL", SND_SOC_NOPM, 0, 0,
 			   rt5670_sto2_adc_l_mix,
 			   ARRAY_SIZE(rt5670_sto2_adc_l_mix)),
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 4/4] ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control
  2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
                   ` (2 preceding siblings ...)
  2021-02-15 14:21 ` [PATCH 3/4] ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer settings Hans de Goede
@ 2021-02-15 14:21 ` Hans de Goede
  2021-02-24 16:57 ` [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Mark Brown
  4 siblings, 0 replies; 9+ messages in thread
From: Hans de Goede @ 2021-02-15 14:21 UTC (permalink / raw)
  To: Liam Girdwood, Mark Brown, Oder Chiou
  Cc: Hans de Goede, alsa-devel, Takashi Iwai, Bard Liao

For reliable output-mute LED control we need a "DAC1 Playback Switch"
control. The "DAC Playback volume" control is the only control in the
path from the DAC1 data input to the speaker output, so the UCM profile
for the speaker output will have its PlaybackMixerElem set to "DAC1".

But userspace (pulseaudio) will set the "DAC1 Playback Volume" control to
its softest setting (which is not fully muted) while still showing the
speaker as being enabled at a low volume in the UI.

If we were to set the SNDRV_CTL_ELEM_ACCESS_SPK_LED on the "DAC1 Playback
Volume" control, this would mean then what pressing KEY_VOLUMEDOWN the
speaker-mute LED (embedded in the volume-mute toggle key) would light
while the UI is still showing the speaker as being enabled at a low
volume, meaning that the UI and the LED are out of sync.

Only after an _extra_ KEY_VOLUMEDOWN press would the UI show the
speaker as being muted.

The path from DAC1 data input to the speaker output does have
a digital mixer with DAC1's data as one of its inputs direclty after
the "DAC1 Playback Volume" control.

This commit adds an emulated "DAC1 Playback Switch" control by:

1. Declaring the enable flag for that mixers DAC1 input as well as the
"DAC1 Playback Switch" control both as SND_SOC_NOPM controls.

2. Storing the settings of both controls as driver-private data

3. Only clearing the mute flag for the DAC1 input of that mixer if the
stored values indicate both controls are enabled.

This is a preparation patch for adding "audio-mute" LED trigger support.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/codecs/rt5670.c | 96 +++++++++++++++++++++++++++++++++++++--
 sound/soc/codecs/rt5670.h |  5 ++
 2 files changed, 97 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/rt5670.c b/sound/soc/codecs/rt5670.c
index 4d22fa4e8ab7..feab15d0686a 100644
--- a/sound/soc/codecs/rt5670.c
+++ b/sound/soc/codecs/rt5670.c
@@ -629,6 +629,56 @@ static SOC_ENUM_SINGLE_DECL(rt5670_if2_dac_enum, RT5670_DIG_INF1_DATA,
 static SOC_ENUM_SINGLE_DECL(rt5670_if2_adc_enum, RT5670_DIG_INF1_DATA,
 				RT5670_IF2_ADC_SEL_SFT, rt5670_data_select);
 
+/*
+ * For reliable output-mute LED control we need a "DAC1 Playback Switch" control.
+ * We emulate this by only clearing the RT5670_M_DAC1_L/_R AD_DA_MIXER register
+ * bits when both our emulated DAC1 Playback Switch control and the DAC1 MIXL/R
+ * DAPM-mixer DAC1 input are enabled.
+ */
+static void rt5670_update_ad_da_mixer_dac1_m_bits(struct rt5670_priv *rt5670)
+{
+	int val = RT5670_M_DAC1_L | RT5670_M_DAC1_R;
+
+	if (rt5670->dac1_mixl_dac1_switch && rt5670->dac1_playback_switch_l)
+		val &= ~RT5670_M_DAC1_L;
+
+	if (rt5670->dac1_mixr_dac1_switch && rt5670->dac1_playback_switch_r)
+		val &= ~RT5670_M_DAC1_R;
+
+	regmap_update_bits(rt5670->regmap, RT5670_AD_DA_MIXER,
+			   RT5670_M_DAC1_L | RT5670_M_DAC1_R, val);
+}
+
+static int rt5670_dac1_playback_switch_get(struct snd_kcontrol *kcontrol,
+					   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct rt5670_priv *rt5670 = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.integer.value[0] = rt5670->dac1_playback_switch_l;
+	ucontrol->value.integer.value[1] = rt5670->dac1_playback_switch_r;
+
+	return 0;
+}
+
+static int rt5670_dac1_playback_switch_put(struct snd_kcontrol *kcontrol,
+					   struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_component *component = snd_soc_kcontrol_component(kcontrol);
+	struct rt5670_priv *rt5670 = snd_soc_component_get_drvdata(component);
+
+	if (rt5670->dac1_playback_switch_l == ucontrol->value.integer.value[0] &&
+	    rt5670->dac1_playback_switch_r == ucontrol->value.integer.value[1])
+		return 0;
+
+	rt5670->dac1_playback_switch_l = ucontrol->value.integer.value[0];
+	rt5670->dac1_playback_switch_r = ucontrol->value.integer.value[1];
+
+	rt5670_update_ad_da_mixer_dac1_m_bits(rt5670);
+
+	return 1;
+}
+
 static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 	/* Headphone Output Volume */
 	SOC_DOUBLE_TLV("HP Playback Volume", RT5670_HP_VOL,
@@ -640,6 +690,8 @@ static const struct snd_kcontrol_new rt5670_snd_controls[] = {
 	/* DAC Digital Volume */
 	SOC_DOUBLE("DAC2 Playback Switch", RT5670_DAC_CTRL,
 		RT5670_M_DAC_L2_VOL_SFT, RT5670_M_DAC_R2_VOL_SFT, 1, 1),
+	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
+			rt5670_dac1_playback_switch_get, rt5670_dac1_playback_switch_put),
 	SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5670_DAC1_DIG_VOL,
 			RT5670_L_VOL_SFT, RT5670_R_VOL_SFT,
 			175, 0, dac_vol_tlv),
@@ -909,18 +961,44 @@ static const struct snd_kcontrol_new rt5670_mono_adc_r_mix[] = {
 			RT5670_M_MONO_ADC_R2_SFT, 1, 1),
 };
 
+/* See comment above rt5670_update_ad_da_mixer_dac1_m_bits() */
+static int rt5670_put_dac1_mix_dac1_switch(struct snd_kcontrol *kcontrol,
+					   struct snd_ctl_elem_value *ucontrol)
+{
+	struct soc_mixer_control *mc = (struct soc_mixer_control *)kcontrol->private_value;
+	struct snd_soc_component *component = snd_soc_dapm_kcontrol_component(kcontrol);
+	struct rt5670_priv *rt5670 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	if (mc->shift == 0)
+		rt5670->dac1_mixl_dac1_switch = ucontrol->value.integer.value[0];
+	else
+		rt5670->dac1_mixr_dac1_switch = ucontrol->value.integer.value[0];
+
+	/* Apply the update (if any) */
+	ret = snd_soc_dapm_put_volsw(kcontrol, ucontrol);
+	if (ret == 0)
+		return 0;
+
+	rt5670_update_ad_da_mixer_dac1_m_bits(rt5670);
+
+	return 1;
+}
+
+#define SOC_DAPM_SINGLE_RT5670_DAC1_SW(name, shift) \
+	SOC_SINGLE_EXT(name, SND_SOC_NOPM, shift, 1, 0, \
+		       snd_soc_dapm_get_volsw, rt5670_put_dac1_mix_dac1_switch)
+
 static const struct snd_kcontrol_new rt5670_dac_l_mix[] = {
 	SOC_DAPM_SINGLE("Stereo ADC Switch", RT5670_AD_DA_MIXER,
 			RT5670_M_ADCMIX_L_SFT, 1, 1),
-	SOC_DAPM_SINGLE("DAC1 Switch", RT5670_AD_DA_MIXER,
-			RT5670_M_DAC1_L_SFT, 1, 1),
+	SOC_DAPM_SINGLE_RT5670_DAC1_SW("DAC1 Switch", 0),
 };
 
 static const struct snd_kcontrol_new rt5670_dac_r_mix[] = {
 	SOC_DAPM_SINGLE("Stereo ADC Switch", RT5670_AD_DA_MIXER,
 			RT5670_M_ADCMIX_R_SFT, 1, 1),
-	SOC_DAPM_SINGLE("DAC1 Switch", RT5670_AD_DA_MIXER,
-			RT5670_M_DAC1_R_SFT, 1, 1),
+	SOC_DAPM_SINGLE_RT5670_DAC1_SW("DAC1 Switch", 1),
 };
 
 static const struct snd_kcontrol_new rt5670_sto_dac_l_mix[] = {
@@ -2993,6 +3071,16 @@ static int rt5670_i2c_probe(struct i2c_client *i2c,
 		dev_info(&i2c->dev, "quirk JD mode 3\n");
 	}
 
+	/*
+	 * Enable the emulated "DAC1 Playback Switch" by default to avoid
+	 * muting the output with older UCM profiles.
+	 */
+	rt5670->dac1_playback_switch_l = true;
+	rt5670->dac1_playback_switch_r = true;
+	/* The Power-On-Reset values for the DAC1 mixer have the DAC1 input enabled. */
+	rt5670->dac1_mixl_dac1_switch = true;
+	rt5670->dac1_mixr_dac1_switch = true;
+
 	rt5670->regmap = devm_regmap_init_i2c(i2c, &rt5670_regmap);
 	if (IS_ERR(rt5670->regmap)) {
 		ret = PTR_ERR(rt5670->regmap);
diff --git a/sound/soc/codecs/rt5670.h b/sound/soc/codecs/rt5670.h
index f9c4db156c80..6fb3c369ee98 100644
--- a/sound/soc/codecs/rt5670.h
+++ b/sound/soc/codecs/rt5670.h
@@ -2013,6 +2013,11 @@ struct rt5670_priv {
 	int dsp_rate;
 	int jack_type;
 	int jack_type_saved;
+
+	bool dac1_mixl_dac1_switch;
+	bool dac1_mixr_dac1_switch;
+	bool dac1_playback_switch_l;
+	bool dac1_playback_switch_r;
 };
 
 void rt5670_jack_suspend(struct snd_soc_component *component);
-- 
2.30.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback Switch' control
  2021-02-15 14:21 ` [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback " Hans de Goede
@ 2021-02-15 18:09   ` Jaroslav Kysela
  2021-02-16 15:15     ` Hans de Goede
  0 siblings, 1 reply; 9+ messages in thread
From: Jaroslav Kysela @ 2021-02-15 18:09 UTC (permalink / raw)
  To: Hans de Goede, Liam Girdwood, Mark Brown, Oder Chiou
  Cc: alsa-devel, Takashi Iwai, Bard Liao

Dne 15. 02. 21 v 15:21 Hans de Goede napsal(a):
> The RT5670_L_MUTE_SFT and RT5670_R_MUTE_SFT bits (bits 15 and 7) of the
> RT5670_HP_VOL register are set / unset by the headphones deplop code
> run by rt5670_hp_event() on SND_SOC_DAPM_POST_PMU / SND_SOC_DAPM_PRE_PMD.
> 
> So we should not also export a control to userspace which toggles these
> same bits.

I think that it may be worth to preserve the full-mute functionality.
According the datasheet, the register 0x02 has volume range -46.5dB..12dB.
Even the lowest volume may produce an audible output.

I would cache the mute switch value in rt5670_priv and update the
POST_PMU/PRE_PMD code to use this settings.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback Switch' control
  2021-02-15 18:09   ` Jaroslav Kysela
@ 2021-02-16 15:15     ` Hans de Goede
  2021-02-16 16:35       ` Jaroslav Kysela
  0 siblings, 1 reply; 9+ messages in thread
From: Hans de Goede @ 2021-02-16 15:15 UTC (permalink / raw)
  To: Jaroslav Kysela, Liam Girdwood, Mark Brown, Oder Chiou
  Cc: alsa-devel, Takashi Iwai, Bard Liao

Hi,

On 2/15/21 7:09 PM, Jaroslav Kysela wrote:
> Dne 15. 02. 21 v 15:21 Hans de Goede napsal(a):
>> The RT5670_L_MUTE_SFT and RT5670_R_MUTE_SFT bits (bits 15 and 7) of the
>> RT5670_HP_VOL register are set / unset by the headphones deplop code
>> run by rt5670_hp_event() on SND_SOC_DAPM_POST_PMU / SND_SOC_DAPM_PRE_PMD.
>>
>> So we should not also export a control to userspace which toggles these
>> same bits.
> 
> I think that it may be worth to preserve the full-mute functionality.
> According the datasheet, the register 0x02 has volume range -46.5dB..12dB.
> Even the lowest volume may produce an audible output.
> 
> I would cache the mute switch value in rt5670_priv and update the
> POST_PMU/PRE_PMD code to use this settings.

Yes that should work.

Note though that patch 4/4 adds a mute for the master volume control, even
though I call it an "emulated" mute it is a full mute, it is just that
we now have 2 switches, 1 mixer switch and 1 mute switch, which must
both be true before we enable the path from the DAC through the mixer
which sits directly behind the "DAC1 Playback Volume" control.

And we need that mute anyways because the speaker output does not
have any volume control other then the master "DAC1 Playback Volume"
which has the same issue of only going very soft and not going to
a full mute.

So since we have a working mute in the master volume control, we don't
really need one for the "HP Playback Volume" control. with that all said,
your suggestion should work fine.

So the question is do we want to do as you suggest, or are we happy
with just having the master mute ?

Note I'm fine doing things either way I just wanted to ask before
spending time on implementing and testing your suggestion. Esp. the
testing which often seems to take at least as much time as actually
implementing things...

So if you can let me know how you want to proceed then I'll get
right to it.

Regards,

Hans


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback Switch' control
  2021-02-16 15:15     ` Hans de Goede
@ 2021-02-16 16:35       ` Jaroslav Kysela
  0 siblings, 0 replies; 9+ messages in thread
From: Jaroslav Kysela @ 2021-02-16 16:35 UTC (permalink / raw)
  To: Hans de Goede, Liam Girdwood, Mark Brown, Oder Chiou
  Cc: alsa-devel, Takashi Iwai, Bard Liao

Dne 16. 02. 21 v 16:15 Hans de Goede napsal(a):
> Hi,
> 
> On 2/15/21 7:09 PM, Jaroslav Kysela wrote:
>> Dne 15. 02. 21 v 15:21 Hans de Goede napsal(a):
>>> The RT5670_L_MUTE_SFT and RT5670_R_MUTE_SFT bits (bits 15 and 7) of the
>>> RT5670_HP_VOL register are set / unset by the headphones deplop code
>>> run by rt5670_hp_event() on SND_SOC_DAPM_POST_PMU / SND_SOC_DAPM_PRE_PMD.
>>>
>>> So we should not also export a control to userspace which toggles these
>>> same bits.
>>
>> I think that it may be worth to preserve the full-mute functionality.
>> According the datasheet, the register 0x02 has volume range -46.5dB..12dB.
>> Even the lowest volume may produce an audible output.
>>
>> I would cache the mute switch value in rt5670_priv and update the
>> POST_PMU/PRE_PMD code to use this settings.
> 
> Yes that should work.
> 
> Note though that patch 4/4 adds a mute for the master volume control, even
> though I call it an "emulated" mute it is a full mute, it is just that
> we now have 2 switches, 1 mixer switch and 1 mute switch, which must
> both be true before we enable the path from the DAC through the mixer
> which sits directly behind the "DAC1 Playback Volume" control.
> 
> And we need that mute anyways because the speaker output does not
> have any volume control other then the master "DAC1 Playback Volume"
> which has the same issue of only going very soft and not going to
> a full mute.
> 
> So since we have a working mute in the master volume control, we don't
> really need one for the "HP Playback Volume" control. with that all said,
> your suggestion should work fine.
> 
> So the question is do we want to do as you suggest, or are we happy
> with just having the master mute ?

One hw mute per one hw output should be sufficient. Thank you for the explanation.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes
  2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
                   ` (3 preceding siblings ...)
  2021-02-15 14:21 ` [PATCH 4/4] ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control Hans de Goede
@ 2021-02-24 16:57 ` Mark Brown
  4 siblings, 0 replies; 9+ messages in thread
From: Mark Brown @ 2021-02-24 16:57 UTC (permalink / raw)
  To: Oder Chiou, Hans de Goede, Liam Girdwood
  Cc: alsa-devel, Takashi Iwai, Bard Liao

On Mon, 15 Feb 2021 15:21:14 +0100, Hans de Goede wrote:
> While working on adding hardware-volume control support to the UCM
> profile for the rt5672 and on adding LED trigger support to the
> rt5670 codec driver. I hit / noticed a couple of issues this series
> fixes these issues.
> 
> Regards,
> 
> [...]

Applied to

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

Thanks!

[1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control
      commit: 30be2641848b2450f0f1b62e3a8aea42e14db640
[2/4] ASoC: rt5670: Remove 'HP Playback Switch' control
      commit: 8022f09883e827855d86173756caa07b891100f0
[3/4] ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer settings
      commit: 674e4ff4c2326c6e3f8ddc73c61910bf32228720
[4/4] ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control
      commit: 982042931c255e2e7f196c24f1e5d6de780e04f9

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-02-24 17:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-15 14:21 [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Hans de Goede
2021-02-15 14:21 ` [PATCH 1/4] ASoC: rt5670: Remove 'OUT Channel Switch' control Hans de Goede
2021-02-15 14:21 ` [PATCH 2/4] ASoC: rt5670: Remove 'HP Playback " Hans de Goede
2021-02-15 18:09   ` Jaroslav Kysela
2021-02-16 15:15     ` Hans de Goede
2021-02-16 16:35       ` Jaroslav Kysela
2021-02-15 14:21 ` [PATCH 3/4] ASoC: rt5670: Remove ADC vol-ctrl mute bits poking from Sto1 ADC mixer settings Hans de Goede
2021-02-15 14:21 ` [PATCH 4/4] ASoC: rt5670: Add emulated 'DAC1 Playback Switch' control Hans de Goede
2021-02-24 16:57 ` [PATCH 0/4] ASoC: rt5670: Various kcontrol fixes Mark Brown

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.