[PATCH 0/5] AsoC: rt5640/rt5651: Volume control fixes

[PATCH 0/5] AsoC: rt5640/rt5651: Volume control fixes

[Hans de Goede]

Hi All,

Here is a series of rt5640/rt5651 volume-control fixes which I wrote
while working on a bytcr-rt5640 UCM profile patch-series adding
hardware-volume control to devices using this UCM profile.

The UCM series will also work on older kernels, but it works best on
kernels with this series applied, giving e.g. finer grained volume
control and support for hardware muting the outputs.

Regards,

Hans


Hans de Goede (5):
  ASoC: rt5640: Fix dac- and adc- vol-tlv values being off by a factor
    of 10
  ASoC: rt5651: Fix dac- and adc- vol-tlv values being off by a factor
    of 10
  ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control
  ASoC: rt5640: Rename 'Mono DAC Playback Volume' to 'DAC2 Playback
    Volume'
  ASoC: Intel: bytcr_rt5640: Add used AIF to the components string

 sound/soc/codecs/rt5640.c             | 106 +++++++++++++++++++++++---
 sound/soc/codecs/rt5640.h             |   4 +
 sound/soc/codecs/rt5651.c             |   4 +-
 sound/soc/intel/boards/bytcr_rt5640.c |  11 ++-
 4 files changed, 111 insertions(+), 14 deletions(-)

-- 
2.30.1

[PATCH 1/5] ASoC: rt5640: Fix dac- and adc- vol-tlv values being off by a factor of 10

[Hans de Goede]

The adc_vol_tlv volume-control has a range from -17.625 dB to +30 dB,
not -176.25 dB to + 300 dB. This wrong scale is esp. a problem in userspace
apps which translate the dB scale to a linear scale. With the logarithmic
dB scale being of by a factor of 10 we loose all precision in the lower
area of the range when apps translate things to a linear scale.

E.g. the 0 dB default, which corresponds with a value of 47 of the
0 - 127 range for the control, would be shown as 0/100 in alsa-mixer.

Since the centi-dB values used in the TLV struct cannot represent the
0.375 dB step size used by these controls, change the TLV definition
for them to specify a min and max value instead of min + stepsize.

Note this mirrors commit 3f31f7d9b540 ("ASoC: rt5670: Fix dac- and adc-
vol-tlv values being off by a factor of 10") which made the exact same
change to the rt5670 codec driver.

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

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index 1414ad15d01c..a5674c227b3a 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -339,9 +339,9 @@ static bool rt5640_readable_register(struct device *dev, unsigned int reg)
 }
 
 static const DECLARE_TLV_DB_SCALE(out_vol_tlv, -4650, 150, 0);
-static const DECLARE_TLV_DB_SCALE(dac_vol_tlv, -65625, 375, 0);
+static const DECLARE_TLV_DB_MINMAX(dac_vol_tlv, -6562, 0);
 static const DECLARE_TLV_DB_SCALE(in_vol_tlv, -3450, 150, 0);
-static const DECLARE_TLV_DB_SCALE(adc_vol_tlv, -17625, 375, 0);
+static const DECLARE_TLV_DB_MINMAX(adc_vol_tlv, -1762, 3000);
 static const DECLARE_TLV_DB_SCALE(adc_bst_tlv, 0, 1200, 0);
 
 /* {0, +20, +24, +30, +35, +40, +44, +50, +52} dB */
-- 
2.30.1

[PATCH 2/5] ASoC: rt5651: Fix dac- and adc- vol-tlv values being off by a factor of 10

[Hans de Goede]

The adc_vol_tlv volume-control has a range from -17.625 dB to +30 dB,
not -176.25 dB to + 300 dB. This wrong scale is esp. a problem in userspace
apps which translate the dB scale to a linear scale. With the logarithmic
dB scale being of by a factor of 10 we loose all precision in the lower
area of the range when apps translate things to a linear scale.

E.g. the 0 dB default, which corresponds with a value of 47 of the
0 - 127 range for the control, would be shown as 0/100 in alsa-mixer.

Since the centi-dB values used in the TLV struct cannot represent the
0.375 dB step size used by these controls, change the TLV definition
for them to specify a min and max value instead of min + stepsize.

Note this mirrors commit 3f31f7d9b540 ("ASoC: rt5670: Fix dac- and adc-
vol-tlv values being off by a factor of 10") which made the exact same
change to the rt5670 codec driver.

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

diff --git a/sound/soc/codecs/rt5651.c b/sound/soc/codecs/rt5651.c
index d198e191fb0c..e59fdc81dbd4 100644
--- a/sound/soc/codecs/rt5651.c
+++ b/sound/soc/codecs/rt5651.c
@@ -285,9 +285,9 @@ static bool rt5651_readable_register(struct device *dev, unsigned int reg)
 }
 
 static const DECLARE_TLV_DB_SCALE(out_vol_tlv, -4650, 150, 0);
-static const DECLARE_TLV_DB_SCALE(dac_vol_tlv, -65625, 375, 0);
+static const DECLARE_TLV_DB_MINMAX(dac_vol_tlv, -6562, 0);
 static const DECLARE_TLV_DB_SCALE(in_vol_tlv, -3450, 150, 0);
-static const DECLARE_TLV_DB_SCALE(adc_vol_tlv, -17625, 375, 0);
+static const DECLARE_TLV_DB_MINMAX(adc_vol_tlv, -1762, 3000);
 static const DECLARE_TLV_DB_SCALE(adc_bst_tlv, 0, 1200, 0);
 
 /* {0, +20, +24, +30, +35, +40, +44, +50, +52} dB */
-- 
2.30.1

[PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

[Hans de Goede]

When using AIF1 the 'DAC1 Playback Volume' control will be used as the
PlaybackMasterElem in UCM.

We need a matching 'DAC1 Playback Switch' for 2 reasons:

1. To be able to truely fully mute the output (the softest volume setting
   is not fully muted).
2. For reliable output-mute LED control.

The path from the IF1_DAC data input to the 'Stereo DAC MIXL' /
'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital
mixer with IF1_DAC 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 the mixers IF1_DAC 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 IF1_DAC input of that mixer if the
stored values indicate both controls are enabled.

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

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index a5674c227b3a..c143ca174921 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -378,6 +378,56 @@ static const char * const rt5640_clsd_spk_ratio[] = {"1.66x", "1.83x", "1.94x",
 static SOC_ENUM_SINGLE_DECL(rt5640_clsd_spk_ratio_enum, RT5640_CLS_D_OUT,
 			    RT5640_CLSD_RATIO_SFT, rt5640_clsd_spk_ratio);
 
+/*
+ * For reliable output-mute LED control we need a "DAC1 Playback Switch" control.
+ * We emulate this by only clearing the RT5640_M_IF1_DAC_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 rt5640_update_ad_da_mixer_if1_dac_m_bits(struct rt5640_priv *rt5640)
+{
+	int val = RT5640_M_IF1_DAC_L | RT5640_M_IF1_DAC_R;
+
+	if (rt5640->dac1_mixl_if1_switch && rt5640->dac1_playback_switch_l)
+		val &= ~RT5640_M_IF1_DAC_L;
+
+	if (rt5640->dac1_mixr_if1_switch && rt5640->dac1_playback_switch_r)
+		val &= ~RT5640_M_IF1_DAC_R;
+
+	regmap_update_bits(rt5640->regmap, RT5640_AD_DA_MIXER,
+			   RT5640_M_IF1_DAC_L | RT5640_M_IF1_DAC_R, val);
+}
+
+static int rt5640_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 rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
+
+	ucontrol->value.integer.value[0] = rt5640->dac1_playback_switch_l;
+	ucontrol->value.integer.value[1] = rt5640->dac1_playback_switch_r;
+
+	return 0;
+}
+
+static int rt5640_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 rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
+
+	if (rt5640->dac1_playback_switch_l == ucontrol->value.integer.value[0] &&
+	    rt5640->dac1_playback_switch_r == ucontrol->value.integer.value[1])
+		return 0;
+
+	rt5640->dac1_playback_switch_l = ucontrol->value.integer.value[0];
+	rt5640->dac1_playback_switch_r = ucontrol->value.integer.value[1];
+
+	rt5640_update_ad_da_mixer_if1_dac_m_bits(rt5640);
+
+	return 1;
+}
+
 static const struct snd_kcontrol_new rt5640_snd_controls[] = {
 	/* Speaker Output Volume */
 	SOC_DOUBLE("Speaker Channel Switch", RT5640_SPK_VOL,
@@ -400,6 +450,8 @@ static const struct snd_kcontrol_new rt5640_snd_controls[] = {
 	/* DAC Digital Volume */
 	SOC_DOUBLE("DAC2 Playback Switch", RT5640_DAC2_CTRL,
 		RT5640_M_DAC_L2_VOL_SFT, RT5640_M_DAC_R2_VOL_SFT, 1, 1),
+	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
+			rt5640_dac1_playback_switch_get, rt5640_dac1_playback_switch_put),
 	SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5640_DAC1_DIG_VOL,
 			RT5640_L_VOL_SFT, RT5640_R_VOL_SFT,
 			175, 0, dac_vol_tlv),
@@ -515,18 +567,44 @@ static const struct snd_kcontrol_new rt5640_mono_adc_r_mix[] = {
 			RT5640_M_MONO_ADC_R2_SFT, 1, 1),
 };
 
+/* See comment above rt5640_update_ad_da_mixer_if1_dac_m_bits() */
+static int rt5640_put_dac1_mix_if1_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 rt5640_priv *rt5640 = snd_soc_component_get_drvdata(component);
+	int ret;
+
+	if (mc->shift == 0)
+		rt5640->dac1_mixl_if1_switch = ucontrol->value.integer.value[0];
+	else
+		rt5640->dac1_mixr_if1_switch = ucontrol->value.integer.value[0];
+
+	/* Apply the update (if any) */
+	ret = snd_soc_dapm_put_volsw(kcontrol, ucontrol);
+	if (ret == 0)
+		return 0;
+
+	rt5640_update_ad_da_mixer_if1_dac_m_bits(rt5640);
+
+	return 1;
+}
+
+#define SOC_DAPM_SINGLE_RT5640_IF1_SW(name, shift) \
+	SOC_SINGLE_EXT(name, SND_SOC_NOPM, shift, 1, 0, \
+		       snd_soc_dapm_get_volsw, rt5640_put_dac1_mix_if1_switch)
+
 static const struct snd_kcontrol_new rt5640_dac_l_mix[] = {
 	SOC_DAPM_SINGLE("Stereo ADC Switch", RT5640_AD_DA_MIXER,
 			RT5640_M_ADCMIX_L_SFT, 1, 1),
-	SOC_DAPM_SINGLE("INF1 Switch", RT5640_AD_DA_MIXER,
-			RT5640_M_IF1_DAC_L_SFT, 1, 1),
+	SOC_DAPM_SINGLE_RT5640_IF1_SW("INF1 Switch", 0),
 };
 
 static const struct snd_kcontrol_new rt5640_dac_r_mix[] = {
 	SOC_DAPM_SINGLE("Stereo ADC Switch", RT5640_AD_DA_MIXER,
 			RT5640_M_ADCMIX_R_SFT, 1, 1),
-	SOC_DAPM_SINGLE("INF1 Switch", RT5640_AD_DA_MIXER,
-			RT5640_M_IF1_DAC_R_SFT, 1, 1),
+	SOC_DAPM_SINGLE_RT5640_IF1_SW("INF1 Switch", 1),
 };
 
 static const struct snd_kcontrol_new rt5640_sto_dac_l_mix[] = {
@@ -2831,6 +2909,16 @@ static int rt5640_i2c_probe(struct i2c_client *i2c,
 	INIT_DELAYED_WORK(&rt5640->bp_work, rt5640_button_press_work);
 	INIT_WORK(&rt5640->jack_work, rt5640_jack_work);
 
+	/*
+	 * Enable the emulated "DAC1 Playback Switch" by default to avoid
+	 * muting the output with older UCM profiles.
+	 */
+	rt5640->dac1_playback_switch_l = true;
+	rt5640->dac1_playback_switch_r = true;
+	/* The Power-On-Reset values for the DAC1 mixer have the INF1 input enabled. */
+	rt5640->dac1_mixl_if1_switch = true;
+	rt5640->dac1_mixr_if1_switch = true;
+
 	/* Make sure work is stopped on probe-error / remove */
 	ret = devm_add_action_or_reset(&i2c->dev, rt5640_cancel_work, rt5640);
 	if (ret)
diff --git a/sound/soc/codecs/rt5640.h b/sound/soc/codecs/rt5640.h
index 4fd47f2b936b..0d029f5dbb61 100644
--- a/sound/soc/codecs/rt5640.h
+++ b/sound/soc/codecs/rt5640.h
@@ -2135,6 +2135,10 @@ struct rt5640_priv {
 
 	bool hp_mute;
 	bool asrc_en;
+	bool dac1_mixl_if1_switch;
+	bool dac1_mixr_if1_switch;
+	bool dac1_playback_switch_l;
+	bool dac1_playback_switch_r;
 
 	/* Jack and button detect data */
 	bool ovcd_irq_enabled;
-- 
2.30.1

[PATCH 4/5] ASoC: rt5640: Rename 'Mono DAC Playback Volume' to 'DAC2 Playback Volume'

[Hans de Goede]

Rename 'Mono DAC Playback Volume' to 'DAC2 Playback Volume' and move it
from rt5640_specific_snd_controls[] to rt5640_snd_controls[].

The RT5640_DAC2_DIG_VOL register controlled by this mixer-element has
nothing to do with the Mono (Amplified) output which is only available
on the ALC5640 chip and not on the ALC5642 chip.

The RT5640_DAC2_DIG_VOL volume-control is the main volume control for
audio coming from the I2S2 / AIF2 input of the chip and as such is also
available on the ALC5642.

This commit results in the following userspace visible changes:

1. On devices with an ACL5640 codec, the 'Mono DAC Playback Volume'
control is renamed to 'DAC2 Playback Volume' allowing the alsa-lib
mixer code to properly group it with the 'DAC2 Playback Switch' which
is controlling the mute bits in the RT5640_DAC2_DIG_VOL register.

Note the removal of the 'Mono DAC Playback Volume' is not an issue for
userspace because the UCM profiles do not use it (the UCM profiles are
shared betweent the 5640 and 5642 and only the 5640 had this control).

2. On devices with an ACL5642 codec, there now will be a new
'DAC2 Playback Volume', grouped with the 'DAC2 Playback Switch'

Having a complete 'DAC2 Playback Volume' / 'DAC2 Playback Switch' pair
on both variants will allow enabling hardware-volume control by
setting the UCM PlaybackMasterElem to "DAC2" on devices where the
I2S2/AIF2 interface of the codec is used.

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

diff --git a/sound/soc/codecs/rt5640.c b/sound/soc/codecs/rt5640.c
index c143ca174921..38cc3155bf25 100644
--- a/sound/soc/codecs/rt5640.c
+++ b/sound/soc/codecs/rt5640.c
@@ -450,6 +450,9 @@ static const struct snd_kcontrol_new rt5640_snd_controls[] = {
 	/* DAC Digital Volume */
 	SOC_DOUBLE("DAC2 Playback Switch", RT5640_DAC2_CTRL,
 		RT5640_M_DAC_L2_VOL_SFT, RT5640_M_DAC_R2_VOL_SFT, 1, 1),
+	SOC_DOUBLE_TLV("DAC2 Playback Volume", RT5640_DAC2_DIG_VOL,
+			RT5640_L_VOL_SFT, RT5640_R_VOL_SFT,
+			175, 0, dac_vol_tlv),
 	SOC_DOUBLE_EXT("DAC1 Playback Switch", SND_SOC_NOPM, 0, 1, 1, 0,
 			rt5640_dac1_playback_switch_get, rt5640_dac1_playback_switch_put),
 	SOC_DOUBLE_TLV("DAC1 Playback Volume", RT5640_DAC1_DIG_VOL,
@@ -495,9 +498,6 @@ static const struct snd_kcontrol_new rt5640_specific_snd_controls[] = {
 	/* MONO Output Control */
 	SOC_SINGLE("Mono Playback Switch", RT5640_MONO_OUT, RT5640_L_MUTE_SFT,
 		1, 1),
-
-	SOC_DOUBLE_TLV("Mono DAC Playback Volume", RT5640_DAC2_DIG_VOL,
-		RT5640_L_VOL_SFT, RT5640_R_VOL_SFT, 175, 0, dac_vol_tlv),
 };
 
 /**
-- 
2.30.1

[PATCH 5/5] ASoC: Intel: bytcr_rt5640: Add used AIF to the components string

[Hans de Goede]

Depending on which AIF is used the UCM profile needs to setup
a different path through the rt5640's "Digital Mixer Path" graph.

ATM the UCM profiles solve this by just enabling paths to the outputs /
from the input from both AIF1 and AIF2 and then relying on the DAPM
framework to power-down the parts of the graph connected to the
unused AIF.

But in order to be able to use hardware-volumecontrol and to use
the hardware mute controls, which are necessary for mute LED control,
the UCM profiles need to know which AIF is actually being used.

Add a new "aif:1" or "aif:2" part to the component string to provide
info about the used AIF to userspace / to the UCM profiles.

Note the size of byt_rt5640_components is not increased because the
size of 32 chars already is big enough.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 sound/soc/intel/boards/bytcr_rt5640.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/sound/soc/intel/boards/bytcr_rt5640.c b/sound/soc/intel/boards/bytcr_rt5640.c
index 5d48cc359c3d..1f6a636571c2 100644
--- a/sound/soc/intel/boards/bytcr_rt5640.c
+++ b/sound/soc/intel/boards/bytcr_rt5640.c
@@ -1252,6 +1252,7 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	int ret_val = 0;
 	int dai_index = 0;
 	int i, cfg_spk;
+	int aif;
 
 	is_bytcr = false;
 	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
@@ -1363,8 +1364,12 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	log_quirks(&pdev->dev);
 
 	if ((byt_rt5640_quirk & BYT_RT5640_SSP2_AIF2) ||
-	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
+	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2)) {
 		byt_rt5640_dais[dai_index].codecs->dai_name = "rt5640-aif2";
+		aif = 2;
+	} else {
+		aif = 1;
+	}
 
 	if ((byt_rt5640_quirk & BYT_RT5640_SSP0_AIF1) ||
 	    (byt_rt5640_quirk & BYT_RT5640_SSP0_AIF2))
@@ -1402,8 +1407,8 @@ static int snd_byt_rt5640_mc_probe(struct platform_device *pdev)
 	}
 
 	snprintf(byt_rt5640_components, sizeof(byt_rt5640_components),
-		 "cfg-spk:%d cfg-mic:%s", cfg_spk,
-		 map_name[BYT_RT5640_MAP(byt_rt5640_quirk)]);
+		 "cfg-spk:%d cfg-mic:%s aif:%d", cfg_spk,
+		 map_name[BYT_RT5640_MAP(byt_rt5640_quirk)], aif);
 	byt_rt5640_card.components = byt_rt5640_components;
 #if !IS_ENABLED(CONFIG_SND_SOC_INTEL_USER_FRIENDLY_LONG_NAMES)
 	snprintf(byt_rt5640_long_name, sizeof(byt_rt5640_long_name),
-- 
2.30.1

Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

[Mark Brown]

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

On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote:
> When using AIF1 the 'DAC1 Playback Volume' control will be used as the
> PlaybackMasterElem in UCM.
> 
> We need a matching 'DAC1 Playback Switch' for 2 reasons:
> 
> 1. To be able to truely fully mute the output (the softest volume setting
>    is not fully muted).
> 2. For reliable output-mute LED control.
> 
> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' /
> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital
> mixer with IF1_DAC data as one of its inputs direclty after the
> 'DAC1 Playback Volume' control.
> 
> This commit adds an emulated "DAC1 Playback Switch" control by:

This feels icky, it seems like if userspace needs to stitch together a
stereo mute control that doesn't already exist in the hardware from
existing mono controls then UCM ought to have support for figuring that
out anyway or if we *must* bodge this in the kernel there should be some
generic way of doing it rather than open coding in drivers.

It also makes the whole mute LED thing feel a lot messier even for
existing systems than you seemed to be suggesting in the other thread.
This device has two I2S interfaces, two DACs (only one of which seems to
be affected by this control), and it appears that there's bypass path
from the ADC to DAC1 which won't be muted by the newly added mute switch
here so this reliable mute control won't be entirely reliable.  There
look to also be some analogue bypass paths, I didn't fully check.  One
could equally argue that a software defined mute control should be
muting all the analogue outputs, it'd certainly seem more robust.

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

Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

[Hans de Goede]

Hi,

On 3/1/21 7:55 PM, Mark Brown wrote:
> On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote:
>> When using AIF1 the 'DAC1 Playback Volume' control will be used as the
>> PlaybackMasterElem in UCM.
>>
>> We need a matching 'DAC1 Playback Switch' for 2 reasons:
>>
>> 1. To be able to truely fully mute the output (the softest volume setting
>>    is not fully muted).
>> 2. For reliable output-mute LED control.
>>
>> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' /
>> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital
>> mixer with IF1_DAC data as one of its inputs direclty after the
>> 'DAC1 Playback Volume' control.
>>
>> This commit adds an emulated "DAC1 Playback Switch" control by:
> 
> This feels icky, it seems like if userspace needs to stitch together a
> stereo mute control that doesn't already exist in the hardware

But it does exist in the hardware the digital mixer bits around DAC1
have some more functionality then those around DAc2, including a mixer
directly behind the DAC1 volume-control which allows digital loopback.

The inputs to those mixer are all 4 (2 pairs of l/r) controlled by
mute bits. The codec designer has left out the mute switches normally
directly following the volume control since then there would be 2 mute
switches in series, one as part of the volume-control/mute combo which
is usually used and 1 directly behind that to mute/unmute the mixer
input.

This is a nice hw optimization, but annoying to deal with in software,
esp. since userspace generally expects a "Foo Playback Volume",
"Foo Playback Switch" pair. By for the easiest way to deal with this
is to undo the hw optimization in software and add the expected
"Foo Playback Switch"

> from
> existing mono controls then UCM ought to have support for figuring that
> out anyway or if we *must* bodge this in the kernel there should be some
> generic way of doing it rather than open coding in drivers.

This is highly codec specific. So far this has not really been an
issue because so far on asoc based systems regular Linux userspace has
always been using software volume-control. But now that we are starting
to use hardware volume-control it really is desirable to also have
a hardware mute switch available.

Also problematic here is that things like volume-controls and their
accompanying mute switches (often bit 15 + 8 of the same register)
are modules as "normal" mixer elements which are not seen as part of
the DAPM graph, where as the mixer in this case is part of the DAPM graph.

> It also makes the whole mute LED thing feel a lot messier even for
> existing systems than you seemed to be suggesting in the other thread.
> This device has two I2S interfaces, two DACs (only one of which seems to
> be affected by this control), and it appears that there's bypass path
> from the ADC to DAC1 which won't be muted by the newly added mute switch
> here so this reliable mute control won't be entirely reliable.  There
> look to also be some analogue bypass paths, I didn't fully check.

I don't believe that it is necessary to take bypass / loopback paths into
account for the playback mute LED. These are normally always off and they
don't involve the normal playback paths. So even if they are on any
audio played from within the OS is still muted.

> One
> could equally argue that a software defined mute control should be
> muting all the analogue outputs, it'd certainly seem more robust.

The mute switches in the analog output paths are part of the DAPM
graph, which means that they will get turned off automatically to
save power when the audio device is not playing audio (is not kept
open by userspace). AFAIK this makes them unsuitable to be used as a
source for the mute-led trigger, we want the mute LED to turn on
when the volume control is set to mute, not when the last app
closes the audio device.

I honestly don't understand your objections against the current
set of patches for dealing with the mute-led trigger. Your main
worry seems to be that this is not flexible enough, but it currently
is all handled inside the kernel. So if more complex cases come up
then we can easily adjust the code to deal with this, since there
is no userspace API part to worry about. And if these more complex
cases do require more userspace involvement then we can worry about
that then (and not now) when we actually have a concrete example of
what such a more complex setup could look like and thus also have
something to actually design any userspace api for this around.

Regards,

Hans

Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

[Hans de Goede]

Hi,

On 3/1/21 8:21 PM, Hans de Goede wrote:
> Hi,
> 
> On 3/1/21 7:55 PM, Mark Brown wrote:
>> On Fri, Feb 26, 2021 at 03:38:15PM +0100, Hans de Goede wrote:
>>> When using AIF1 the 'DAC1 Playback Volume' control will be used as the
>>> PlaybackMasterElem in UCM.
>>>
>>> We need a matching 'DAC1 Playback Switch' for 2 reasons:
>>>
>>> 1. To be able to truely fully mute the output (the softest volume setting
>>>    is not fully muted).
>>> 2. For reliable output-mute LED control.
>>>
>>> The path from the IF1_DAC data input to the 'Stereo DAC MIXL' /
>>> 'Stereo DAC MIXR' digital mixer has a 'DAC MIXL' / 'DAC MIXR' digital
>>> mixer with IF1_DAC data as one of its inputs direclty after the
>>> 'DAC1 Playback Volume' control.
>>>
>>> This commit adds an emulated "DAC1 Playback Switch" control by:
>>
>> This feels icky, it seems like if userspace needs to stitch together a
>> stereo mute control that doesn't already exist in the hardware
> 
> But it does exist in the hardware the digital mixer bits around DAC1
> have some more functionality then those around DAc2, including a mixer
> directly behind the DAC1 volume-control which allows digital loopback.
> 
> The inputs to those mixer are all 4 (2 pairs of l/r) controlled by
> mute bits. The codec designer has left out the mute switches normally
> directly following the volume control since then there would be 2 mute
> switches in series, one as part of the volume-control/mute combo which
> is usually used and 1 directly behind that to mute/unmute the mixer
> input.
> 
> This is a nice hw optimization, but annoying to deal with in software,
> esp. since userspace generally expects a "Foo Playback Volume",
> "Foo Playback Switch" pair. By for the easiest way to deal with this
> is to undo the hw optimization in software and add the expected
> "Foo Playback Switch"
> 
>> from
>> existing mono controls then UCM ought to have support for figuring that
>> out anyway or if we *must* bodge this in the kernel there should be some
>> generic way of doing it rather than open coding in drivers.
> 
> This is highly codec specific. So far this has not really been an
> issue because so far on asoc based systems regular Linux userspace has
> always been using software volume-control. But now that we are starting
> to use hardware volume-control it really is desirable to also have
> a hardware mute switch available.
> 
> Also problematic here is that things like volume-controls and their
> accompanying mute switches (often bit 15 + 8 of the same register)
> are modules as "normal" mixer elements which are not seen as part of
> the DAPM graph, where as the mixer in this case is part of the DAPM graph.
> 
>> It also makes the whole mute LED thing feel a lot messier even for
>> existing systems than you seemed to be suggesting in the other thread.
>> This device has two I2S interfaces, two DACs (only one of which seems to
>> be affected by this control), and it appears that there's bypass path
>> from the ADC to DAC1 which won't be muted by the newly added mute switch
>> here so this reliable mute control won't be entirely reliable.  There
>> look to also be some analogue bypass paths, I didn't fully check.
> 
> I don't believe that it is necessary to take bypass / loopback paths into
> account for the playback mute LED. These are normally always off and they
> don't involve the normal playback paths. So even if they are on any
> audio played from within the OS is still muted.
> 
>> One
>> could equally argue that a software defined mute control should be
>> muting all the analogue outputs, it'd certainly seem more robust.
> 
> The mute switches in the analog output paths are part of the DAPM
> graph, which means that they will get turned off automatically to
> save power when the audio device is not playing audio (is not kept
> open by userspace). AFAIK this makes them unsuitable to be used as a
> source for the mute-led trigger, we want the mute LED to turn on
> when the volume control is set to mute, not when the last app
> closes the audio device.
> 
> I honestly don't understand your objections against the current
> set of patches for dealing with the mute-led trigger. Your main
> worry seems to be that this is not flexible enough, but it currently
> is all handled inside the kernel. So if more complex cases come up
> then we can easily adjust the code to deal with this, since there
> is no userspace API part to worry about. And if these more complex
> cases do require more userspace involvement then we can worry about
> that then (and not now) when we actually have a concrete example of
> what such a more complex setup could look like and thus also have
> something to actually design any userspace api for this around.

I think we might be conversion on a solution for this in the other
thread (see the email which I am about to send but have not sent
yet), so lets continue this discussion there.

Regards,

Hans

Re: [PATCH 3/5] ASoC: rt5640: Add emulated 'DAC1 Playback Switch' control

[Mark Brown]

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

On Mon, Mar 01, 2021 at 08:21:56PM +0100, Hans de Goede wrote:

> > This feels icky, it seems like if userspace needs to stitch together a
> > stereo mute control that doesn't already exist in the hardware

> But it does exist in the hardware the digital mixer bits around DAC1
> have some more functionality then those around DAc2, including a mixer
> directly behind the DAC1 volume-control which allows digital loopback.

Given that there's other inputs to the mixer (what with it being a mixer
and all) I'm not convinced about that?

> This is a nice hw optimization, but annoying to deal with in software,
> esp. since userspace generally expects a "Foo Playback Volume",
> "Foo Playback Switch" pair. By for the easiest way to deal with this
> is to undo the hw optimization in software and add the expected
> "Foo Playback Switch"

Eh, some userspace might have that expectation but it doesn't really map
onto general hardware designs.

> > from
> > existing mono controls then UCM ought to have support for figuring that
> > out anyway or if we *must* bodge this in the kernel there should be some
> > generic way of doing it rather than open coding in drivers.

> This is highly codec specific. So far this has not really been an
> issue because so far on asoc based systems regular Linux userspace has
> always been using software volume-control. But now that we are starting
> to use hardware volume-control it really is desirable to also have
> a hardware mute switch available.

There's a lot of things that would be desirable but aren't really
realistic...  there's a bunch of hardware that this model just plain
doesn't map onto.  I mentioned the wm5012 based systems in the other
thread - that's a fairly clear example where a singular DAC mute control
just doesn't correspond to the hardware design at all, it's got any to
any routing throughout the device with DACs at the outputs.

> > It also makes the whole mute LED thing feel a lot messier even for
> > existing systems than you seemed to be suggesting in the other thread.
> > This device has two I2S interfaces, two DACs (only one of which seems to
> > be affected by this control), and it appears that there's bypass path
> > from the ADC to DAC1 which won't be muted by the newly added mute switch
> > here so this reliable mute control won't be entirely reliable.  There
> > look to also be some analogue bypass paths, I didn't fully check.

> I don't believe that it is necessary to take bypass / loopback paths into
> account for the playback mute LED. These are normally always off and they
> don't involve the normal playback paths. So even if they are on any
> audio played from within the OS is still muted.

For me I would say that if the mute LED is on and I can hear audio
coming out of the system then there is a bug, probably also if I can
manage to record audio possibly depending on labelling.  This all very
clearly seems to be pointing towards this being a policy decision which
probably belongs in userspace.

> > One
> > could equally argue that a software defined mute control should be
> > muting all the analogue outputs, it'd certainly seem more robust.

> The mute switches in the analog output paths are part of the DAPM
> graph, which means that they will get turned off automatically to
> save power when the audio device is not playing audio (is not kept

So there's not actually any mute switches on the outputs for this
device then and it only has power controls?  In general device will
often have separate power and mute controls, especially with older VMID
based devices but that's often carried through to ground referenced
stuff.  This is yet another example of how devices may not conform to
random policy decisions userspace might want them to have.

> I honestly don't understand your objections against the current
> set of patches for dealing with the mute-led trigger. Your main
> worry seems to be that this is not flexible enough, but it currently
> is all handled inside the kernel. So if more complex cases come up
> then we can easily adjust the code to deal with this, since there
> is no userspace API part to worry about. And if these more complex
> cases do require more userspace involvement then we can worry about
> that then (and not now) when we actually have a concrete example of
> what such a more complex setup could look like and thus also have
> something to actually design any userspace api for this around.

All I've seen of this is the ASoC bits of this, including this series
and it's all fitting patterns that look like forcing policy decisions
into the kernel in ways that are hard to review - look at this patch as
an example of this, there's stuff like the handling of bypass paths
which you're dismissing.  You say "when we actually have concrete
examples of what such a more complex setup could look like" but this
very patch seems to be for a device that causes issues, and I keep
pointing at the wm5102 and similar devices which just break this model. 

You have a clear model of what you want to do on your systems and are
trying to implement that but that model looks to have policy in it which
a resonable system integrator could want to decide differently even when
running on the same hardware.  If it is all handled inside the kernel
then it's a lot more painful to do anything about that than when it's
handled in userspace.

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

Re: (subset) [PATCH 0/5] AsoC: rt5640/rt5651: Volume control fixes

[Mark Brown]

On Fri, 26 Feb 2021 15:38:12 +0100, Hans de Goede wrote:
> Here is a series of rt5640/rt5651 volume-control fixes which I wrote
> while working on a bytcr-rt5640 UCM profile patch-series adding
> hardware-volume control to devices using this UCM profile.
> 
> The UCM series will also work on older kernels, but it works best on
> kernels with this series applied, giving e.g. finer grained volume
> control and support for hardware muting the outputs.
> 
> [...]

Applied to

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

Thanks!

[1/5] ASoC: rt5640: Fix dac- and adc- vol-tlv values being off by a factor of 10
      commit: 24a7b77daed8f973bf8a5ed2f83344f44f9f6396
[2/5] ASoC: rt5651: Fix dac- and adc- vol-tlv values being off by a factor of 10
      commit: e4ffab875d32bf4ffa37b5cd725ace9e15d1707d

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