| Message ID | 20201214064903.2746-1-jack.yu@realtek.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: rt715: add main capture switch and main capture volume control | expand |
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
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?
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
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
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.
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
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
+ 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.
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,