[PATCH] ASoC: rt715: add main capture switch and main capture volume control

[PATCH] ASoC: rt715: add main capture switch and main capture volume control

[jack.yu]

From: Jack Yu <jack.yu@realtek.com>

Add main capture switch and main capture volume for callback to be in
single operation.

Signed-off-by: Jack Yu <jack.yu@realtek.com>
---
 sound/soc/codecs/rt715.c | 174 +++++++++++++++++++++++----------------
 1 file changed, 105 insertions(+), 69 deletions(-)

diff --git a/sound/soc/codecs/rt715.c b/sound/soc/codecs/rt715.c
index 9a7d393d424a..cdcba70146da 100644
--- a/sound/soc/codecs/rt715.c
+++ b/sound/soc/codecs/rt715.c
@@ -80,85 +80,106 @@ static int rt715_set_amp_gain_put(struct snd_kcontrol *kcontrol,
 	struct soc_mixer_control *mc =
 		(struct soc_mixer_control *)kcontrol->private_value;
 	struct rt715_priv *rt715 = snd_soc_component_get_drvdata(component);
+	unsigned int capture_reg_H[] = {RT715_SET_GAIN_MIC_ADC_H,
+		RT715_SET_GAIN_LINE_ADC_H, RT715_SET_GAIN_MIX_ADC_H,
+		RT715_SET_GAIN_MIX_ADC2_H};
+	unsigned int capture_reg_L[] = {RT715_SET_GAIN_MIC_ADC_L,
+		RT715_SET_GAIN_LINE_ADC_L, RT715_SET_GAIN_MIX_ADC_L,
+		RT715_SET_GAIN_MIX_ADC2_L};
 	unsigned int addr_h, addr_l, val_h, val_ll, val_lr;
-	unsigned int read_ll, read_rl;
-	int i;
-
-	/* 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 */
-		val_h = 0x0;
-
-	rt715_get_gain(rt715, addr_h, addr_l, val_h, &read_rl, &read_ll);
+	unsigned int read_ll, read_rl, i, j, loop_cnt;
 
-	/* 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;
-	}
-
-	/* 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;
-	}
-
-	for (i = 0; i < 3; i++) { /* retry 3 times at most */
+	if (strstr(ucontrol->id.name, "Main Capture Switch") ||
+		strstr(ucontrol->id.name, "Main Capture Volume"))
+		loop_cnt = 4;
+	else
+		loop_cnt = 1;
 
-		if (val_ll == val_lr) {
-			/* 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));
-			regmap_write(rt715->regmap, addr_l,
-				(val_h << 8 | val_ll));
+	for (j = 0; j < loop_cnt; j++) {
+		/* Can't use update bit function, so read the original value first */
+		if (loop_cnt == 1) {
+			addr_h = mc->reg;
+			addr_l = mc->rreg;
 		} else {
-			/* Lch*/
-			val_h = (1 << mc->shift) | (1 << 5);
-			regmap_write(rt715->regmap, addr_h,
-				(val_h << 8 | val_ll));
-			/* Rch */
-			val_h = (1 << mc->shift) | (1 << 4);
-			regmap_write(rt715->regmap, addr_l,
-				(val_h << 8 | val_lr));
+			addr_h = capture_reg_H[j];
+			addr_l = capture_reg_L[j];
 		}
-		/* check result */
+
 		if (mc->shift == RT715_DIR_OUT_SFT) /* output */
 			val_h = 0x80;
 		else /* input */
 			val_h = 0x0;
 
-		rt715_get_gain(rt715, addr_h, addr_l, val_h,
-			       &read_rl, &read_ll);
-		if (read_rl == val_lr && read_ll == val_ll)
-			break;
+		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;
+		}
+
+		/* R Channel */
+		if (mc->invert) {
+			/* 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;
+		}
+
+		for (i = 0; i < 3; i++) { /* retry 3 times at most */
+
+			if (val_ll == val_lr) {
+				/* 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));
+				regmap_write(rt715->regmap, addr_l,
+					(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));
+				/* Rch */
+				val_h = (1 << mc->shift) | (1 << 4);
+				regmap_write(rt715->regmap, addr_l,
+					(val_h << 8 | val_lr));
+			}
+			/* check result */
+			if (mc->shift == RT715_DIR_OUT_SFT) /* output */
+				val_h = 0x80;
+			else /* input */
+				val_h = 0x0;
+
+			rt715_get_gain(rt715, addr_h, addr_l, val_h,
+					&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)
@@ -226,6 +247,13 @@ static const struct snd_kcontrol_new rt715_snd_controls[] = {
 	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),
+	/*
+	 * "Main Capture Switch" looks the same as "ADC 07 Capture Switch",
+	 * but the callback has different action internally.
+	 */
+	SOC_DOUBLE_R_EXT("Main 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),
 	/* 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,
@@ -243,6 +271,14 @@ static const struct snd_kcontrol_new rt715_snd_controls[] = {
 			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),
+	/*
+	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",
+	 * but the callback has different action internally.
+	 */
+	SOC_DOUBLE_R_EXT_TLV("Main 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),
 	/* MIC Boost Control */
 	SOC_DOUBLE_R_EXT_TLV("DMIC1 Boost", RT715_SET_GAIN_DMIC1_H,
 			RT715_SET_GAIN_DMIC1_L, RT715_DIR_IN_SFT, 3, 0,
-- 
2.29.0

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jaroslav Kysela]

Dne 14. 12. 20 v 7:49 jack.yu@realtek.com napsal(a):
> From: Jack Yu <jack.yu@realtek.com>
> 
> Add main capture switch and main capture volume for callback to be in
> single operation.

Could you be more verbose, what you're trying to do in the patch description?

> +	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",

I would just use "Capture Volume" and "Capture Switch" here without the Main
prefix to follow other drivers.

					Jaroslav

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

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Pierre-Louis Bossart]

On 12/14/20 1:35 AM, Jaroslav Kysela wrote:
> Dne 14. 12. 20 v 7:49 jack.yu@realtek.com napsal(a):
>> From: Jack Yu <jack.yu@realtek.com>
>>
>> Add main capture switch and main capture volume for callback to be in
>> single operation.
> 
> Could you be more verbose, what you're trying to do in the patch description?
> 
>> +	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",
> 
> I would just use "Capture Volume" and "Capture Switch" here without the Main
> prefix to follow other drivers.

It's similar to the 'Master Capture Switch' used in HDaudio, what other 
drivers were you referring to?

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jaroslav Kysela]

Dne 14. 12. 20 v 16:07 Pierre-Louis Bossart napsal(a):
> 
> 
> On 12/14/20 1:35 AM, Jaroslav Kysela wrote:
>> Dne 14. 12. 20 v 7:49 jack.yu@realtek.com napsal(a):
>>> From: Jack Yu <jack.yu@realtek.com>
>>>
>>> Add main capture switch and main capture volume for callback to be in
>>> single operation.
>>
>> Could you be more verbose, what you're trying to do in the patch description?

I see that it's just additional volume coupling functionality (one control,
set all four output volume/switch registers, right?).

Some points:

1) the separate volume controls don't send change events back to the user
space when the coupled capture settings is applied and versa vice - no sync
2) we have already virtual master API - sound/core/vmaster.c which should
cover this requirement
3) I don't see the purpose for this coupling (the capture direction)

>>> +	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",
>>
>> I would just use "Capture Volume" and "Capture Switch" here without the Main
>> prefix to follow other drivers.
> 
> It's similar to the 'Master Capture Switch' used in HDaudio, what other 
> drivers were you referring to?

HDAudio is using just 'Capture Switch' and 'Capture Volume' for the root
capture controls plus the input selector (enum). The Master prefix is used
only for the playback direction. And the word master is not prohibited for the
audio context, is it?

					Jaroslav

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

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Takashi Iwai]

On Mon, 14 Dec 2020 17:44:27 +0100,
Jaroslav Kysela wrote:
> 
> >>> +	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",
> >>
> >> I would just use "Capture Volume" and "Capture Switch" here without the Main
> >> prefix to follow other drivers.
> > 
> > It's similar to the 'Master Capture Switch' used in HDaudio, what other 
> > drivers were you referring to?
> 
> HDAudio is using just 'Capture Switch' and 'Capture Volume' for the root
> capture controls plus the input selector (enum). The Master prefix is used
> only for the playback direction. And the word master is not prohibited for the
> audio context, is it?

Right, we keep the word Master in the sound API because it's deeply
bound with the actual use case.


Takashi

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Pierre-Louis Bossart]

Hi Jaroslav,

>>>> Add main capture switch and main capture volume for callback to be in
>>>> single operation.
>>>
>>> Could you be more verbose, what you're trying to do in the patch description?
> 
> I see that it's just additional volume coupling functionality (one control,
> set all four output volume/switch registers, right?).
> 
> Some points:
> 
> 1) the separate volume controls don't send change events back to the user
> space when the coupled capture settings is applied and versa vice - no sync
> 2) we have already virtual master API - sound/core/vmaster.c which should
> cover this requirement
> 3) I don't see the purpose for this coupling (the capture direction)

That was for UCM integration - agree the context was not well captured 
in the commit message

We have 'Capture Switch' and 'Capture Volume' statements that are 
required, and currently mistakenly set to SOF controls when they should 
be codec controls.

So when we have several possible inputs controls for the codec (ADC 07 
or ADC 27) depending on the microphone settings, which one should be 
used in UCM?

We thought this would simplify the UCM integration by adding one generic 
control. If you have a better suggestion we are all ears.

>>>> +	 * "Main Capture Volume" looks the same as "ADC 07 Capture Volume",
>>>
>>> I would just use "Capture Volume" and "Capture Switch" here without the Main
>>> prefix to follow other drivers.
>>
>> It's similar to the 'Master Capture Switch' used in HDaudio, what other
>> drivers were you referring to?
> 
> HDAudio is using just 'Capture Switch' and 'Capture Volume' for the root
> capture controls plus the input selector (enum). The Master prefix is used
> only for the playback direction. And the word master is not prohibited for the
> audio context, is it?

The naming is not the problem, we can remove the 'main' if needed, the 
point is how to go about UCM integration.

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jaroslav Kysela]

Dne 14. 12. 20 v 18:12 Pierre-Louis Bossart napsal(a):
> Hi Jaroslav,
> 
>>>>> Add main capture switch and main capture volume for callback to be in
>>>>> single operation.
>>>>
>>>> Could you be more verbose, what you're trying to do in the patch description?
>>
>> I see that it's just additional volume coupling functionality (one control,
>> set all four output volume/switch registers, right?).
>>
>> Some points:
>>
>> 1) the separate volume controls don't send change events back to the user
>> space when the coupled capture settings is applied and versa vice - no sync
>> 2) we have already virtual master API - sound/core/vmaster.c which should
>> cover this requirement
>> 3) I don't see the purpose for this coupling (the capture direction)
> 
> That was for UCM integration - agree the context was not well captured 
> in the commit message
> 
> We have 'Capture Switch' and 'Capture Volume' statements that are 
> required, and currently mistakenly set to SOF controls when they should 
> be codec controls.
> 
> So when we have several possible inputs controls for the codec (ADC 07 
> or ADC 27) depending on the microphone settings, which one should be 
> used in UCM?
> 
> We thought this would simplify the UCM integration by adding one generic 
> control. If you have a better suggestion we are all ears.

So, it's for the microphone array with four microphones, right? Ideally there
should be one multi-channel control exported to the user space with the exact
number of the connected links in hardware. But I know, SoC has universal codec
drivers which exports the functionality without the detailed knowledge of the
controlled hardware.

For UCM (without one control for volume and switch), the situation is more
difficult, because we need to export only one volume/switch control to the
applications (abstract layer). I see the requirement to describe the more
complex control mechanism. I have an idea to extend the alsa-lib (mixer
interface) to allow this description via the alsa-lib configuration files
which may be eventually part of the UCM configuration. The nice thing is that
we can specify the custom mixer and control devices for PA already (my latest
UCM updates in PA 14.0), so there's a room to improve things in the user space.

My suggestions are (pick one):

1) create one multichannel control and remove the stereo controls when the
hardware is detected (no functionality dup)
2) create proper vmaster control like for HDA playback
3) wait until UCM can describe this hardware and set the DAC values manually
to a sensible value via sequences (the specific hardware levels can be set
using the conditions in UCM)

BTW: I see lack of analog volume level controls also for other SDW hardware
(with the RT1308 amplifier). It's a bit pitty that the audio basics are
ignored here. Everyone wants to control the analog levels at first for audio
I/O and then to modify the samples in the digital stream.

					Jaroslav

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

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Pierre-Louis Bossart]

> My suggestions are (pick one):
> 
> 1) create one multichannel control and remove the stereo controls when the
> hardware is detected (no functionality dup)

we can't remove controls that existed before, this might break userspace 
with older UCM files that touch those ADC07 and ADC27. That's why we 
added a new one, to be backwards compatible with a user updates their 
kernel.

> 2) create proper vmaster control like for HDA playback

That might be the better option. What was suggested in this patch is 
essentially to introduce a layer that drives the actual controls, but it 
was done 'manually' and may not follow the proper rules.

That said I know absolutely nothing about 'vmaster controls', pointers 
to a typical example would be greatly appreciated.

> 3) wait until UCM can describe this hardware and set the DAC values manually
> to a sensible value via sequences (the specific hardware levels can be set
> using the conditions in UCM)

Not an option, there are products that need to ship soon.

> BTW: I see lack of analog volume level controls also for other SDW hardware
> (with the RT1308 amplifier). It's a bit pitty that the audio basics are
> ignored here. Everyone wants to control the analog levels at first for audio
> I/O and then to modify the samples in the digital stream.

I agree, there was an oversight in initial UCM files where the SOF 
controls modifying the digital parts were used across the board. This 
was mandatory for DMICs controlled by SOF but all other streams should 
have been controlled by controls exposed by external devices.

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jaroslav Kysela]

Dne 15. 12. 20 v 17:00 Pierre-Louis Bossart napsal(a):
> 
> 
> 
>> My suggestions are (pick one):
>>
>> 1) create one multichannel control and remove the stereo controls when the
>> hardware is detected (no functionality dup)
> 
> we can't remove controls that existed before, this might break userspace 

It's not widely used, so it would be better to break things now than later.
But I see that others are a bit conservative.

> with older UCM files that touch those ADC07 and ADC27. That's why we 

The upstream UCM files don't refer to those controls.

> added a new one, to be backwards compatible with a user updates their 
> kernel.

Even if you don't remove the duplicate controls, the right abstraction is more
appropriate in my eyes (better than vmaster extension). The double stereo -> 4
channel array mapping is not fully correct (vmaster, proposed patch).

>> 2) create proper vmaster control like for HDA playback
> 
> That might be the better option. What was suggested in this patch is 
> essentially to introduce a layer that drives the actual controls, but it 
> was done 'manually' and may not follow the proper rules.
> 
> That said I know absolutely nothing about 'vmaster controls', pointers 
> to a typical example would be greatly appreciated.

sound/core/vmaster.c ; The ASoC core will probably require another layer to
support this?

>> 3) wait until UCM can describe this hardware and set the DAC values manually
>> to a sensible value via sequences (the specific hardware levels can be set
>> using the conditions in UCM)
> 
> Not an option, there are products that need to ship soon.

It's the easiest method for now. It's just about to change the UCM files
without any other changes in the kernel / user space. It's heavily used for
SST drivers, isn't?

The current UCM upstream modifies only SOF volume levels (PGA Master Capture).

						Jaroslav

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

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Pierre-Louis Bossart]

>>> My suggestions are (pick one):
>>>
>>> 1) create one multichannel control and remove the stereo controls when the
>>> hardware is detected (no functionality dup)
>>
>> we can't remove controls that existed before, this might break userspace
> 
> It's not widely used, so it would be better to break things now than later.

rt715 has been used on CometLake-based devices for a while (1.5 years?).

> But I see that others are a bit conservative.
> 
>> with older UCM files that touch those ADC07 and ADC27. That's why we
> 
> The upstream UCM files don't refer to those controls.

they do, unfortunately, see ucm2/codecs/rt715/init.conf

cset "name='rt715 ADC 27 Capture Switch' 1"
cset "name='rt715 ADC 07 Capture Switch' 1"		
cset "name='rt715 ADC 07 Capture Volume' 58"

>> added a new one, to be backwards compatible with a user updates their
>> kernel.
> 
> Even if you don't remove the duplicate controls, the right abstraction is more
> appropriate in my eyes (better than vmaster extension). The double stereo -> 4
> channel array mapping is not fully correct (vmaster, proposed patch).

The hardware exposes registers to deal with two inputs separately, they 
are not duplicates. The point here is that we need a mapping to a 
simpler view where those two inputs are merged logically.

>>> 2) create proper vmaster control like for HDA playback
>>
>> That might be the better option. What was suggested in this patch is
>> essentially to introduce a layer that drives the actual controls, but it
>> was done 'manually' and may not follow the proper rules.
>>
>> That said I know absolutely nothing about 'vmaster controls', pointers
>> to a typical example would be greatly appreciated.
> 
> sound/core/vmaster.c ; The ASoC core will probably require another layer to
> support this?

I'll look into it.

>>> 3) wait until UCM can describe this hardware and set the DAC values manually
>>> to a sensible value via sequences (the specific hardware levels can be set
>>> using the conditions in UCM)
>>
>> Not an option, there are products that need to ship soon.
> 
> It's the easiest method for now. It's just about to change the UCM files
> without any other changes in the kernel / user space. It's heavily used for
> SST drivers, isn't?
> 
> The current UCM upstream modifies only SOF volume levels (PGA Master Capture).

that's not right, see above.

I may have misunderstood your point for 3). I assumed you'd need a 
description coming from the kernel, as we did before for the components 
(cfg-mics, etc). How would UCM know which of the controls to use without 
any change to the kernel?

Re: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jaroslav Kysela]

Dne 15. 12. 20 v 18:05 Pierre-Louis Bossart napsal(a):
> 
>>>> My suggestions are (pick one):
>>>>
>>>> 1) create one multichannel control and remove the stereo controls when the
>>>> hardware is detected (no functionality dup)
>>>
>>> we can't remove controls that existed before, this might break userspace
>>
>> It's not widely used, so it would be better to break things now than later.
> 
> rt715 has been used on CometLake-based devices for a while (1.5 years?).

But SDW is supported recently in the upstream Linux kernel. So there are no users.

>> But I see that others are a bit conservative.
>>
>>> with older UCM files that touch those ADC07 and ADC27. That's why we
>>
>> The upstream UCM files don't refer to those controls.
> 
> they do, unfortunately, see ucm2/codecs/rt715/init.conf
> 
> cset "name='rt715 ADC 27 Capture Switch' 1"
> cset "name='rt715 ADC 07 Capture Switch' 1"		
> cset "name='rt715 ADC 07 Capture Volume' 58"
> 
>>> added a new one, to be backwards compatible with a user updates their
>>> kernel.
>>
>> Even if you don't remove the duplicate controls, the right abstraction is more
>> appropriate in my eyes (better than vmaster extension). The double stereo -> 4
>> channel array mapping is not fully correct (vmaster, proposed patch).
> 
> The hardware exposes registers to deal with two inputs separately, they 
> are not duplicates. The point here is that we need a mapping to a 
> simpler view where those two inputs are merged logically.

Yes, but why to force stereo grouping when you need to control 4 independent
channels from the user space POV? I'm speaking about the forced 'stereo -> 4
channels volume / switch' mapping.

>>>> 3) wait until UCM can describe this hardware and set the DAC values manually
>>>> to a sensible value via sequences (the specific hardware levels can be set
>>>> using the conditions in UCM)
>>>
>>> Not an option, there are products that need to ship soon.
>>
>> It's the easiest method for now. It's just about to change the UCM files
>> without any other changes in the kernel / user space. It's heavily used for
>> SST drivers, isn't?
>>
>> The current UCM upstream modifies only SOF volume levels (PGA Master Capture).
> 
> that's not right, see above.
> 
> I may have misunderstood your point for 3). I assumed you'd need a 
> description coming from the kernel, as we did before for the components 
> (cfg-mics, etc). How would UCM know which of the controls to use without 
> any change to the kernel?

Ideally, yes - it will help to reduce the configuration and the driver already
knows more about the hardware. But we can do DMI matching in UCM for now, too.

Example of the sysfs substitution:

  ${sys:class/dmi/id/sys_vendor}
  ${sys:class/dmi/id/product_version}

						Jaroslav

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

RE: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Jack Yu]

+ Intel Libin

Hi Jaroslav and Intel team,

> >
> >>>> My suggestions are (pick one):
> >>>>
> >>>> 1) create one multichannel control and remove the stereo controls
> >>>> when the hardware is detected (no functionality dup)
> >>>
> >>> we can't remove controls that existed before, this might break
> >>> userspace
> >>

"create one multichannel control and remove the stereo controls" 
This might be the easiest way for implementation on driver, but it might affect current ucm structure.

Besides, per previous discussion (on DEC 15,2020) ,
remove "Main" from "Main Capture Switch/Volume" and keep the rest controls is also an option, is it?

For me, both above suggestions are doable, what's intel team's opinion? 

> >> It's not widely used, so it would be better to break things now than later.
> >
> > rt715 has been used on CometLake-based devices for a while (1.5 years?).
> 
> But SDW is supported recently in the upstream Linux kernel. So there are no
> users.
> 
> >> But I see that others are a bit conservative.
> >>
> >>> with older UCM files that touch those ADC07 and ADC27. That's why we
> >>
> >> The upstream UCM files don't refer to those controls.
> >
> > they do, unfortunately, see ucm2/codecs/rt715/init.conf
> >
> > cset "name='rt715 ADC 27 Capture Switch' 1"
> > cset "name='rt715 ADC 07 Capture Switch' 1"
> > cset "name='rt715 ADC 07 Capture Volume' 58"
> >
> >>> added a new one, to be backwards compatible with a user updates
> >>> their kernel.
> >>
> >> Even if you don't remove the duplicate controls, the right
> >> abstraction is more appropriate in my eyes (better than vmaster
> >> extension). The double stereo -> 4 channel array mapping is not fully
> correct (vmaster, proposed patch).
> >
> > The hardware exposes registers to deal with two inputs separately,
> > they are not duplicates. The point here is that we need a mapping to a
> > simpler view where those two inputs are merged logically.
> 
> Yes, but why to force stereo grouping when you need to control 4 independent
> channels from the user space POV? I'm speaking about the forced 'stereo -> 4
> channels volume / switch' mapping.
> 
> >>>> 3) wait until UCM can describe this hardware and set the DAC values
> >>>> manually to a sensible value via sequences (the specific hardware
> >>>> levels can be set using the conditions in UCM)
> >>>
> >>> Not an option, there are products that need to ship soon.
> >>
> >> It's the easiest method for now. It's just about to change the UCM
> >> files without any other changes in the kernel / user space. It's
> >> heavily used for SST drivers, isn't?
> >>
> >> The current UCM upstream modifies only SOF volume levels (PGA Master
> Capture).
> >
> > that's not right, see above.
> >
> > I may have misunderstood your point for 3). I assumed you'd need a
> > description coming from the kernel, as we did before for the
> > components (cfg-mics, etc). How would UCM know which of the controls
> > to use without any change to the kernel?
> 
> Ideally, yes - it will help to reduce the configuration and the driver already
> knows more about the hardware. But we can do DMI matching in UCM for now,
> too.
> 

@Libin  Is this modification workable for you? I'd like to know your opinion about it. Thanks.

> Example of the sysfs substitution:
> 
>   ${sys:class/dmi/id/sys_vendor}
>   ${sys:class/dmi/id/product_version}
> 
> 						Jaroslav
> 
> --
> Jaroslav Kysela <perex@perex.cz>
> Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> 
> ------Please consider the environment before printing this e-mail.

RE: [PATCH] ASoC: rt715: add main capture switch and main capture volume control

[Yang, Libin]

> >
> > >>>> 3) wait until UCM can describe this hardware and set the DAC
> > >>>> values manually to a sensible value via sequences (the specific
> > >>>> hardware levels can be set using the conditions in UCM)
> > >>>
> > >>> Not an option, there are products that need to ship soon.
> > >>
> > >> It's the easiest method for now. It's just about to change the UCM
> > >> files without any other changes in the kernel / user space. It's
> > >> heavily used for SST drivers, isn't?
> > >>
> > >> The current UCM upstream modifies only SOF volume levels (PGA
> > >> Master
> > Capture).
> > >
> > > that's not right, see above.
> > >
> > > I may have misunderstood your point for 3). I assumed you'd need a
> > > description coming from the kernel, as we did before for the
> > > components (cfg-mics, etc). How would UCM know which of the controls
> > > to use without any change to the kernel?
> >
> > Ideally, yes - it will help to reduce the configuration and the driver
> > already knows more about the hardware. But we can do DMI matching in
> > UCM for now, too.
> >
> 
> @Libin  Is this modification workable for you? I'd like to know your opinion
> about it. Thanks.

I think Jaroslav's suggestion of matching DMI is reasonable. 
Only special platforms should use this feature. Non-dell or 
some old dell platforms should not use this feature if I understand 
correctly based on 
https://www.spinics.net/lists/alsa-devel/msg118123.html
In the above link, Mario said the MIC LED is controlled by HW, not the SW.

So the codec driver won't control MIC LED. It means on other platforms 
which doesn't support the feature of HW controlling LED, using the new
kcontrol in UCM will cause some issues. When user press the MIC
mute key, the MIC LED won't change the status.

Regards,
Libin

> 
> > Example of the sysfs substitution:
> >
> >   ${sys:class/dmi/id/sys_vendor}
> >   ${sys:class/dmi/id/product_version}
> >
> > 						Jaroslav
> >
> > --
> > Jaroslav Kysela <perex@perex.cz>
> > Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
> >
> > ------Please consider the environment before printing this e-mail.