[v2,2/3] ASoC: rt715: remove kcontrols which no longer be used

Message ID 5c314f5512654aca9fff0195f77264de@realtek.com
State New
Headers show
Series
  • [v2,1/3] ASoC: rt715: add main capture switch and main capture volume
Related show

Commit Message

Jack Yu March 29, 2021, 6:54 a.m.
Using new kcontrols "Capture Switch" and "Capture Volume" instead,
remove kcontrols which no longer be used.

Signed-off-by: Jack Yu <jack.yu@realtek.com>

---
 sound/soc/codecs/rt715.c | 103 +++++++++++++--------------------------
 sound/soc/codecs/rt715.h |   1 +
 2 files changed, 36 insertions(+), 68 deletions(-)

-- 
2.29.0

Comments

Mark Brown March 30, 2021, 5:09 p.m. | #1
On Mon, Mar 29, 2021 at 06:54:00AM +0000, Jack Yu wrote:
> Using new kcontrols "Capture Switch" and "Capture Volume" instead,
> remove kcontrols which no longer be used.

Is this going to disrupt any UCM profiles?
Pierre-Louis Bossart March 30, 2021, 7:26 p.m. | #2
>>> Using new kcontrols "Capture Switch" and "Capture Volume" instead,
>>> remove kcontrols which no longer be used.
>>
>> Is this going to disrupt any UCM profiles?
> 
> Yes (the rt715 prefix is from the SOF driver):
> 
> # RT715 specific volume control settings
> 
> BootSequence [
> 	cset "name='rt715 DMIC3 Boost' 2"
> 	cset "name='rt715 DMIC4 Boost' 2"
> 	cset "name='rt715 ADC 24 Mux' 3"
> 	cset "name='rt715 ADC 25 Mux' 4"
> 	cset "name='rt715 ADC 27 Capture Switch' 1"
> 	cset "name='rt715 ADC 07 Capture Switch' 1"
> 	cset "name='rt715 ADC 07 Capture Volume' 58"
> ]

To be clearer, we wanted to change the UCM files to only try to 
configure the 'old' controls when they are present. The 'new' controls 
are aligned between RT715 and RT715-sdca.

There will be a minor inconvenience if an existing platform updates the 
kernel without updating UCM files, but it's the only solution we found 
in earlier discussions.

Distributions are typically faster with alsa-ucmconf updates than kernel 
changes so that inconvenience is likely very limited (we support 4-5 
Dell CML/TGL platforms w/ SoundWire)
Pierre-Louis Bossart April 15, 2021, 9:35 p.m. | #3
On 3/30/21 2:26 PM, Pierre-Louis Bossart wrote:
> 
> 
>>>> Using new kcontrols "Capture Switch" and "Capture Volume" instead,
>>>> remove kcontrols which no longer be used.
>>>
>>> Is this going to disrupt any UCM profiles?
>>
>> Yes (the rt715 prefix is from the SOF driver):
>>
>> # RT715 specific volume control settings
>>
>> BootSequence [
>>     cset "name='rt715 DMIC3 Boost' 2"
>>     cset "name='rt715 DMIC4 Boost' 2"
>>     cset "name='rt715 ADC 24 Mux' 3"
>>     cset "name='rt715 ADC 25 Mux' 4"
>>     cset "name='rt715 ADC 27 Capture Switch' 1"
>>     cset "name='rt715 ADC 07 Capture Switch' 1"
>>     cset "name='rt715 ADC 07 Capture Volume' 58"
>> ]
> 
> To be clearer, we wanted to change the UCM files to only try to 
> configure the 'old' controls when they are present. The 'new' controls 
> are aligned between RT715 and RT715-sdca.
> 
> There will be a minor inconvenience if an existing platform updates the 
> kernel without updating UCM files, but it's the only solution we found 
> in earlier discussions.
> 
> Distributions are typically faster with alsa-ucmconf updates than kernel 
> changes so that inconvenience is likely very limited (we support 4-5 
> Dell CML/TGL platforms w/ SoundWire)

Was there any sustained objection to this change? Mark and Jaroslav?

I would really like to fix the UCM files so that the mute and volume are 
handled at the codec driver level, and not at the SOF level as it's 
currently the case.

The existing solution really makes no sense, it was 'inspired' (in the 
copy-paste sense) by the PCH-DMIC handling for HDaudio platforms. In 
that case, it made sense to use SOF-level mute/volume because such 
controls don't exist in microphones.

With RT715 we need to use the controls in the codec driver, not the 
firmware.
Mark Brown April 16, 2021, 1:07 p.m. | #4
On Thu, Apr 15, 2021 at 04:35:14PM -0500, Pierre-Louis Bossart wrote:

> Was there any sustained objection to this change? Mark and Jaroslav?

> I would really like to fix the UCM files so that the mute and volume are
> handled at the codec driver level, and not at the SOF level as it's
> currently the case.

Well, you seemed to be objecting because it broke the UCM files and Jack
didn't reply so as far as I understand it it'll break userspace.
Jaroslav Kysela April 16, 2021, 1:40 p.m. | #5
Dne 15. 04. 21 v 23:35 Pierre-Louis Bossart napsal(a):
> 
> 
> On 3/30/21 2:26 PM, Pierre-Louis Bossart wrote:
>>
>>
>>>>> Using new kcontrols "Capture Switch" and "Capture Volume" instead,
>>>>> remove kcontrols which no longer be used.
>>>>
>>>> Is this going to disrupt any UCM profiles?
>>>
>>> Yes (the rt715 prefix is from the SOF driver):
>>>
>>> # RT715 specific volume control settings
>>>
>>> BootSequence [
>>>     cset "name='rt715 DMIC3 Boost' 2"
>>>     cset "name='rt715 DMIC4 Boost' 2"
>>>     cset "name='rt715 ADC 24 Mux' 3"
>>>     cset "name='rt715 ADC 25 Mux' 4"
>>>     cset "name='rt715 ADC 27 Capture Switch' 1"
>>>     cset "name='rt715 ADC 07 Capture Switch' 1"
>>>     cset "name='rt715 ADC 07 Capture Volume' 58"
>>> ]
>>
>> To be clearer, we wanted to change the UCM files to only try to 
>> configure the 'old' controls when they are present. The 'new' controls 
>> are aligned between RT715 and RT715-sdca.
>>
>> There will be a minor inconvenience if an existing platform updates the 
>> kernel without updating UCM files, but it's the only solution we found 
>> in earlier discussions.
>>
>> Distributions are typically faster with alsa-ucmconf updates than kernel 
>> changes so that inconvenience is likely very limited (we support 4-5 
>> Dell CML/TGL platforms w/ SoundWire)
> 
> Was there any sustained objection to this change? Mark and Jaroslav?
> 
> I would really like to fix the UCM files so that the mute and volume are 
> handled at the codec driver level, and not at the SOF level as it's 
> currently the case.
> 
> The existing solution really makes no sense, it was 'inspired' (in the 
> copy-paste sense) by the PCH-DMIC handling for HDaudio platforms. In 
> that case, it made sense to use SOF-level mute/volume because such 
> controls don't exist in microphones.
> 
> With RT715 we need to use the controls in the codec driver, not the 
> firmware.

I'm basically ok to do any change which simplifies things. The user-space
downgrade is possible but less used.

The only my concern is that RT715 as an universal codec can handle the
multiple stereo streams (current controls mapping) or the multichannel stream
(new proposed controls mapping). The ASoC codec code should be universal, so
it's a question, how to model the controls and how to detect and set the
model. I think that we're missing a communication way between the DMA /
machine driver and the codec driver. Yes, we can add DMI checks to RT715, but
usually, the specific machines are already detected in the higher layer
(soundwire bus, pci bus). It would be really nice, if the SOF driver can do a
query: "If present, I need RT715 codec in the multichannel mode.".

So my opinion is that both mappings may exist and the correct one should be
selected at runtime. So I won't delete the old mapping, it may be usable. Mark?

BTW: I already implemented the control remap plugin to alsa-lib, so you can
split / merge controls with similar parameters as you like now. I need to do
more test with the UCM integration, but it's here.

https://github.com/alsa-project/alsa-lib/blob/master/src/control/control_remap.c#L1197

					Jaroslav
Pierre-Louis Bossart April 16, 2021, 3:58 p.m. | #6
> The only my concern is that RT715 as an universal codec can handle the
> multiple stereo streams (current controls mapping) or the multichannel stream
> (new proposed controls mapping). The ASoC codec code should be universal, so
> it's a question, how to model the controls and how to detect and set the
> model. I think that we're missing a communication way between the DMA /
> machine driver and the codec driver. Yes, we can add DMI checks to RT715, but
> usually, the specific machines are already detected in the higher layer
> (soundwire bus, pci bus). It would be really nice, if the SOF driver can do a
> query: "If present, I need RT715 codec in the multichannel mode.".

The RT715 was an early SoundWire device, and it'll likely be superseded 
by the newer SDCA version. The direction for SDCA 'SmartMic' devices is 
not to manage independent stereo streams corresponding to different 
pairs of microphones, and it's not a solution we want to promote in the 
rest of the stack.

There will be multiple streams provided by the codec, but each stream 
will be tied to a given functionality (regular capture, buffered capture 
with triggers, voice, etc).

In addition, what can be configured by the user for volume/mute in 
'Feature Units' is after platform-specific channel 
remapping/mixing/processing. It could very well be that there are 4 mics 
in a platform but only 2 channels are provided to the host.

In other words, the streams generated by the codec and transmitted on 
the SoundWire bus will always be exposed with N channels, in a 
multichannel mode. We will need to report this N to UCM, currently 
that's missing, and the SOF topologies also have a limitation to 2ch 
that we will need to remove.

> So my opinion is that both mappings may exist and the correct one should be
> selected at runtime. So I won't delete the old mapping, it may be usable. Mark?

What I was suggesting is not to delete the old controls in UCM but make 
them conditional. If present you use them, otherwise you use the newer 
solution.

> BTW: I already implemented the control remap plugin to alsa-lib, so you can
> split / merge controls with similar parameters as you like now. I need to do
> more test with the UCM integration, but it's here.
> 
> https://github.com/alsa-project/alsa-lib/blob/master/src/control/control_remap.c#L1197

thanks for the pointer, I'll have to look into this.
Mark Brown April 16, 2021, 6:34 p.m. | #7
On Mon, 29 Mar 2021 06:54:00 +0000, Jack Yu wrote:
> Using new kcontrols "Capture Switch" and "Capture Volume" instead,
> remove kcontrols which no longer be used.

Applied to

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

Thanks!

[2/3] ASoC: rt715: remove kcontrols which no longer be used
      commit: fa2f98378f941786a93f8e63696f59fb4ac7538b

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

Patch

diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
index 3cb8ab452bf1..b33eabb9f740 100644
--- a/sound/soc/codecs/rt715.c
+++ b/sound/soc/codecs/rt715.c
@@ -81,12 +81,20 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
 	unsigned int addr_h, addr_l, val_h, val_ll, val_lr;
-	unsigned int read_ll, read_rl;
-	int i;
+	unsigned int read_ll, read_rl, i;
+	unsigned int k_vol_changed = 0;
+
+	for (i = 0; i < 2; i++) {
+		if (ucontrol->value.integer.value[i] != rt715->kctl_2ch_vol_ori[i]) {
+			k_vol_changed = 1;
+			break;
+		}
+	}
 
 	/* Can't use update bit function, so read the original value first */
 	addr_h = mc->reg;
 	addr_l = mc->rreg;
+
 	if (mc->shift == RT715_DIR_OUT_SFT) /* output */
 		val_h = 0x80;
 	else /* input */
@@ -94,41 +102,27 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 
 	rt715_get_gain(rt715, addr_h, addr_l, val_h, &read_rl, &read_ll);
 
+	if (dapm->bias_level <= SND_SOC_BIAS_STANDBY)
+		regmap_write(rt715->regmap,
+				RT715_SET_AUDIO_POWER_STATE, AC_PWRST_D0);
+
 	/* L Channel */
-	if (mc->invert) {
-		/* for mute */
-		val_ll = (mc->max - ucontrol->value.integer.value[0]) << 7;
-		/* keep gain */
-		read_ll = read_ll & 0x7f;
-		val_ll |= read_ll;
-	} else {
-		/* for gain */
-		val_ll = ((ucontrol->value.integer.value[0]) & 0x7f);
-		if (val_ll > mc->max)
-			val_ll = mc->max;
-		/* keep mute status */
-		read_ll = read_ll & 0x80;
-		val_ll |= read_ll;
-	}
+	rt715->kctl_2ch_vol_ori[0] = ucontrol->value.integer.value[0];
+	/* for gain */
+	val_ll = ((ucontrol->value.integer.value[0]) & 0x7f);
+	if (val_ll > mc->max)
+		val_ll = mc->max;
+	/* keep mute status */
+	val_ll |= read_ll & 0x80;
 
 	/* R Channel */
-	if (mc->invert) {
-		regmap_write(rt715->regmap,
-			     RT715_SET_AUDIO_POWER_STATE, AC_PWRST_D0);
-		/* for mute */
-		val_lr = (mc->max - ucontrol->value.integer.value[1]) << 7;
-		/* keep gain */
-		read_rl = read_rl & 0x7f;
-		val_lr |= read_rl;
-	} else {
-		/* for gain */
-		val_lr = ((ucontrol->value.integer.value[1]) & 0x7f);
-		if (val_lr > mc->max)
-			val_lr = mc->max;
-		/* keep mute status */
-		read_rl = read_rl & 0x80;
-		val_lr |= read_rl;
-	}
+	rt715->kctl_2ch_vol_ori[1] = ucontrol->value.integer.value[1];
+	/* for gain */
+	val_lr = ((ucontrol->value.integer.value[1]) & 0x7f);
+	if (val_lr > mc->max)
+		val_lr = mc->max;
+	/* keep mute status */
+	val_lr |= read_rl & 0x80;
 
 	for (i = 0; i < 3; i++) { /* retry 3 times at most */
 
@@ -136,18 +130,18 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 			/* Set both L/R channels at the same time */
 			val_h = (1 << mc->shift) | (3 << 4);
 			regmap_write(rt715->regmap, addr_h,
-				(val_h << 8 | val_ll));
+				(val_h << 8) | val_ll);
 			regmap_write(rt715->regmap, addr_l,
-				(val_h << 8 | val_ll));
+				(val_h << 8) | val_ll);
 		} else {
 			/* Lch*/
 			val_h = (1 << mc->shift) | (1 << 5);
 			regmap_write(rt715->regmap, addr_h,
-				(val_h << 8 | val_ll));
+				(val_h << 8) | val_ll);
 			/* Rch */
 			val_h = (1 << mc->shift) | (1 << 4);
 			regmap_write(rt715->regmap, addr_l,
-				(val_h << 8 | val_lr));
+				(val_h << 8) | val_lr);
 		}
 		/* check result */
 		if (mc->shift == RT715_DIR_OUT_SFT) /* output */
@@ -156,15 +150,16 @@  static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 			val_h = 0x0;
 
 		rt715_get_gain(rt715, addr_h, addr_l, val_h,
-			       &read_rl, &read_ll);
+				&read_rl, &read_ll);
 		if (read_rl == val_lr && read_ll == val_ll)
 			break;
 	}
+
 	/* D0:power on state, D3: power saving mode */
 	if (dapm->bias_level <= SND_SOC_BIAS_STANDBY)
 		regmap_write(rt715->regmap,
 				RT715_SET_AUDIO_POWER_STATE, AC_PWRST_D3);
-	return 0;
+	return k_vol_changed;
 }
 
 static int rt715_set_amp_gain_get(struct snd_kcontrol *kcontrol,
@@ -466,37 +461,9 @@  static int rt715_vol_info(struct snd_kcontrol *kcontrol,
 
 static const struct snd_kcontrol_new rt715_snd_controls[] = {
 	/* Capture switch */
-	SOC_DOUBLE_R_EXT("ADC 07 Capture Switch", RT715_SET_GAIN_MIC_ADC_H,
-			RT715_SET_GAIN_MIC_ADC_L, RT715_DIR_IN_SFT, 1, 1,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put),
-	SOC_DOUBLE_R_EXT("ADC 08 Capture Switch", RT715_SET_GAIN_LINE_ADC_H,
-			RT715_SET_GAIN_LINE_ADC_L, RT715_DIR_IN_SFT, 1, 1,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put),
-	SOC_DOUBLE_R_EXT("ADC 09 Capture Switch", RT715_SET_GAIN_MIX_ADC_H,
-			RT715_SET_GAIN_MIX_ADC_L, RT715_DIR_IN_SFT, 1, 1,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put),
-	SOC_DOUBLE_R_EXT("ADC 27 Capture Switch", RT715_SET_GAIN_MIX_ADC2_H,
-			RT715_SET_GAIN_MIX_ADC2_L, RT715_DIR_IN_SFT, 1, 1,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put),
 	RT715_MAIN_SWITCH_EXT("Capture Switch",
 			rt715_set_main_switch_get, rt715_set_main_switch_put),
 	/* Volume Control */
-	SOC_DOUBLE_R_EXT_TLV("ADC 07 Capture Volume", RT715_SET_GAIN_MIC_ADC_H,
-			RT715_SET_GAIN_MIC_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put,
-			in_vol_tlv),
-	SOC_DOUBLE_R_EXT_TLV("ADC 08 Capture Volume", RT715_SET_GAIN_LINE_ADC_H,
-			RT715_SET_GAIN_LINE_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put,
-			in_vol_tlv),
-	SOC_DOUBLE_R_EXT_TLV("ADC 09 Capture Volume", RT715_SET_GAIN_MIX_ADC_H,
-			RT715_SET_GAIN_MIX_ADC_L, RT715_DIR_IN_SFT, 0x3f, 0,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put,
-			in_vol_tlv),
-	SOC_DOUBLE_R_EXT_TLV("ADC 27 Capture Volume", RT715_SET_GAIN_MIX_ADC2_H,
-			RT715_SET_GAIN_MIX_ADC2_L, RT715_DIR_IN_SFT, 0x3f, 0,
-			rt715_set_amp_gain_get, rt715_set_amp_gain_put,
-			in_vol_tlv),
 	RT715_MAIN_VOL_EXT_TLV("Capture Volume",
 			rt715_set_main_vol_get, rt715_set_main_vol_put, in_vol_tlv),
 	/* MIC Boost Control */
diff --git a/sound/soc/codecs/rt715.h b/sound/soc/codecs/rt715.h
index 72c320bd35cb..25dba61f1760 100644
--- a/sound/soc/codecs/rt715.h
+++ b/sound/soc/codecs/rt715.h
@@ -22,6 +22,7 @@  struct rt715_priv {
 	struct sdw_bus_params params;
 	bool hw_init;
 	bool first_hw_init;
+	unsigned int kctl_2ch_vol_ori[2];
 	unsigned int kctl_8ch_switch_ori[8];
 	unsigned int kctl_8ch_vol_ori[8];
 };