| Message ID | 1481609526-21933-1-git-send-email-benzh@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Dec 14, 2016 at 04:40:15PM +0800, John Hsu wrote: > On 12/13/2016 2:12 PM, Ben Zhang wrote: > > +static SOC_ENUM_SINGLE_DECL( > > + nau8825_class_g_enum, SND_SOC_NOPM, 0, nau8825_class_g_src); > > + > The register SND_SOC_NOPM can't use at the enum put and get function > in the older kernel. Ex. kernel 3.14. The driver has to modify the > DAPM in the platform with older kernel. Older kernels are not relevant upstream. If you need to backport that should be handled separately.
On Mon, Dec 12, 2016 at 10:12:06PM -0800, Ben Zhang wrote: > Add a DAPM_MUX for userspace to control the Class G function: > "Switching": Use +-1.8V or +-0.9V supply, based on signal amplitude. > "1.8V": Always use +-1.8V. This doesn't affect audio routing so why is it being done with a DAPM mux?
On Wed, Dec 14, 2016 at 6:57 AM, Mark Brown <broonie@kernel.org> wrote: > On Mon, Dec 12, 2016 at 10:12:06PM -0800, Ben Zhang wrote: >> Add a DAPM_MUX for userspace to control the Class G function: >> "Switching": Use +-1.8V or +-0.9V supply, based on signal amplitude. >> "1.8V": Always use +-1.8V. > > This doesn't affect audio routing so why is it being done with a DAPM > mux? When user wants the switching voltages, the Class G register bit needs to be toggled at a specific time in the widget power up/down sequences on the playback route. It helps reduce pop and is consistent with existing behavior. DAPM_MUX achieves this by taking DAPM_PGA_S in and out of the playback route based on user selection. If the register bit is directly controlled by a separate kcontrol in snd_soc_codec_driver.controls unrelated to widgets, the bit will be set immediately by user even when there is no active playback. Then when playback starts, Class G is already on before the DACs/charge pumps/output drivers/pulldowns are enabled. The existing/desired behavior is to turn on Class G last and turn it off first. John has sent me a patch which uses a separate SOC_ENUM_EXT kcontrol in snd_soc_codec_driver.controls to set a flag, then has DAPM_PGA_S PM event callbacks selectively setting the register bit based on the flag. But it feels cleaner to implement this with a DAPM_MUX: The DAPM_PGA_S widget power status correctly reflects the actual register bit. User can experiment with the setting during playback. John, to make SND_SOC_NOPM work with DAPM_MUX on older kernels, commit 236aaa686358 ("ASoC: dapm: Consolidate MUXs and virtual MUXs") can be backported, which landed in v3.15. chromeos-3.14 has backported it already. Another solution is to backport my patch by using SND_SOC_DAPM_VIRT_MUX on older kernels.
On Thu, Dec 15, 2016 at 09:05:52PM -0800, Ben Zhang wrote: > On Wed, Dec 14, 2016 at 6:57 AM, Mark Brown <broonie@kernel.org> wrote: > > This doesn't affect audio routing so why is it being done with a DAPM > > mux? > When user wants the switching voltages, the Class G register bit needs > to be toggled at a specific time in the widget power up/down sequences > on the playback route. It helps reduce pop and is consistent with > existing behavior. DAPM_MUX achieves this by taking DAPM_PGA_S in and Sure, but a more normal way of doing that would be to use an event on the widget for the amplifier that looks at the value a regular control has set in a variable rather than having a mux... > John has sent me a patch which uses a separate SOC_ENUM_EXT kcontrol > in snd_soc_codec_driver.controls to set a flag, then has DAPM_PGA_S PM > event callbacks selectively setting the register bit based on the > flag. But it feels cleaner to implement this with a DAPM_MUX: The > DAPM_PGA_S widget power status correctly reflects the actual register > bit. User can experiment with the setting during playback. ...which is what it sounds like John's patch did. It is more idiomatic, it ensures that the DAPM routing represents the actual audio routing in the chip which means we keep everything in the abstractions that the system is using and avoid the potential for breakage resulting from things getting confused later on (eg, if we start exporting the routing to userspace).
diff --git a/sound/soc/codecs/nau8825.c b/sound/soc/codecs/nau8825.c index efe3a44..ddd15a0 100644 --- a/sound/soc/codecs/nau8825.c +++ b/sound/soc/codecs/nau8825.c @@ -1062,6 +1062,10 @@ static const char * const nau8825_dac_src[] = { "DACL", "DACR", }; +static const char * const nau8825_class_g_src[] = { + "Switching", "1.8V" +}; + static SOC_ENUM_SINGLE_DECL( nau8825_dacl_enum, NAU8825_REG_DACL_CTRL, NAU8825_DACL_CH_SEL_SFT, nau8825_dac_src); @@ -1070,12 +1074,17 @@ static SOC_ENUM_SINGLE_DECL( nau8825_dacr_enum, NAU8825_REG_DACR_CTRL, NAU8825_DACR_CH_SEL_SFT, nau8825_dac_src); +static SOC_ENUM_SINGLE_DECL( + nau8825_class_g_enum, SND_SOC_NOPM, 0, nau8825_class_g_src); + static const struct snd_kcontrol_new nau8825_dacl_mux = SOC_DAPM_ENUM("DACL Source", nau8825_dacl_enum); static const struct snd_kcontrol_new nau8825_dacr_mux = SOC_DAPM_ENUM("DACR Source", nau8825_dacr_enum); +static const struct snd_kcontrol_new nau8825_class_g_mux = + SOC_DAPM_ENUM("Class G Source", nau8825_class_g_enum); static const struct snd_soc_dapm_widget nau8825_dapm_widgets[] = { SND_SOC_DAPM_AIF_OUT("AIFTX", "Capture", 0, NAU8825_REG_I2S_PCM_CTRL2, @@ -1154,9 +1163,16 @@ static const struct snd_soc_dapm_widget nau8825_dapm_widgets[] = { SND_SOC_DAPM_PGA_S("HP Boost Driver", 9, NAU8825_REG_BOOST, 9, 1, NULL, 0), - /* Class G operation control*/ - SND_SOC_DAPM_PGA_S("Class G", 10, + /* Class G operation control. "Class G" MUX options: + * "Switching": Use +-1.8V or +-0.9V supply, based on signal amplitude. + * "1.8V": Always use +-1.8V. + * The PGA_S widget is used to ensure the Class G control bit is set at + * the correct time w.r.t the other widgets. + */ + SND_SOC_DAPM_PGA_S("Class G Driver", 10, NAU8825_REG_CLASSG_CTRL, 0, 0, NULL, 0), + SND_SOC_DAPM_MUX("Class G", SND_SOC_NOPM, 0, 0, + &nau8825_class_g_mux), SND_SOC_DAPM_OUTPUT("HPOL"), SND_SOC_DAPM_OUTPUT("HPOR"), @@ -1197,7 +1213,9 @@ static const struct snd_soc_dapm_route nau8825_dapm_routes[] = { {"HPOR Pulldown", NULL, "Output DACR"}, {"HP Boost Driver", NULL, "HPOL Pulldown"}, {"HP Boost Driver", NULL, "HPOR Pulldown"}, - {"Class G", NULL, "HP Boost Driver"}, + {"Class G Driver", NULL, "HP Boost Driver"}, + {"Class G", "1.8V", "HP Boost Driver"}, + {"Class G", "Switching", "Class G Driver"}, {"HPOL", NULL, "Class G"}, {"HPOR", NULL, "Class G"}, };
Add a DAPM_MUX for userspace to control the Class G function: "Switching": Use +-1.8V or +-0.9V supply, based on signal amplitude. "1.8V": Always use +-1.8V. The default is "Switching" which is the existing behavior without this patch, so no effect on current users. On some devices, the switching may cause a clicking noise at low volume. "1.8V" eliminates the noise at the expense of a few additional mW. Signed-off-by: Ben Zhang <benzh@chromium.org> --- sound/soc/codecs/nau8825.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)