[PATCH 0/5] AsoC: rt5640/rt5651: Volume control fixes
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(-)
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 */
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 */
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;
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:
- To be able to truely fully mute the output (the softest volume setting is not fully muted).
- 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.
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:
- To be able to truely fully mute the output (the softest volume setting is not fully muted).
- 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
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:
- To be able to truely fully mute the output (the softest volume setting is not fully muted).
- 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
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.
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), };
/**
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),
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
participants (2)
-
Hans de Goede -
Mark Brown