[alsa-devel] [PATCH 0/8] ASoC: sun50i-codec-analog: Cleanup and power management
This series performs some minor cleanup on the driver for the analog codec in the Allwinner A64, and hooks up the existing mute switches to DAPM widgets, in order to provide improved power management.
Samuel Holland (8): ASoC: sun50i-codec-analog: Fix duplicate use of ADC enable bits ASoC: sun50i-codec-analog: Gate the amplifier clock during suspend ASoC: sun50i-codec-analog: Group and sort mixer routes ASoC: sun50i-codec-analog: Make headphone routes stereo ASoC: sun50i-codec-analog: Enable DAPM for headphone switch ASoC: sun50i-codec-analog: Make line out routes stereo ASoC: sun50i-codec-analog: Enable DAPM for line out switch ASoC: sun50i-codec-analog: Enable DAPM for earpiece switch
sound/soc/sunxi/sun50i-codec-analog.c | 174 ++++++++++++++++---------- 1 file changed, 109 insertions(+), 65 deletions(-)
The same enable bits are currently used for both the "Left/Right ADC" and the "Left/Right ADC Mixer" widgets. This happens to work in practice because the widgets are always enabled/disabled at the same time, but each register bit should only be associated with a single widget.
To keep symmetry with the DAC widgets, keep the bits on the ADC widgets, and remove them from the ADC Mixer widgets.
Fixes: 42371f327df0 ("ASoC: sunxi: Add new driver for Allwinner A64 codec's analog path controls") Reported-by: Ondrej Jirman megous@megous.com Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index f5b7069bcca2..cbdb31c3b7bd 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -363,12 +363,10 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { SUN50I_ADDA_MIX_DAC_CTRL_RMIXEN, 0, sun50i_a64_codec_mixer_controls, ARRAY_SIZE(sun50i_a64_codec_mixer_controls)), - SND_SOC_DAPM_MIXER("Left ADC Mixer", SUN50I_ADDA_ADC_CTRL, - SUN50I_ADDA_ADC_CTRL_ADCLEN, 0, + SND_SOC_DAPM_MIXER("Left ADC Mixer", SND_SOC_NOPM, 0, 0, sun50i_codec_adc_mixer_controls, ARRAY_SIZE(sun50i_codec_adc_mixer_controls)), - SND_SOC_DAPM_MIXER("Right ADC Mixer", SUN50I_ADDA_ADC_CTRL, - SUN50I_ADDA_ADC_CTRL_ADCREN, 0, + SND_SOC_DAPM_MIXER("Right ADC Mixer", SND_SOC_NOPM, 0, 0, sun50i_codec_adc_mixer_controls, ARRAY_SIZE(sun50i_codec_adc_mixer_controls)), };
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
The same enable bits are currently used for both the "Left/Right ADC" and the "Left/Right ADC Mixer" widgets. This happens to work in practice because the widgets are always enabled/disabled at the same time, but each register bit should only be associated with a single widget.
To keep symmetry with the DAC widgets, keep the bits on the ADC widgets, and remove them from the ADC Mixer widgets.
Fixes: 42371f327df0 ("ASoC: sunxi: Add new driver for Allwinner A64 codec's analog path controls") Reported-by: Ondrej Jirman megous@megous.com Signed-off-by: Samuel Holland samuel@sholland.org
Acked-by: Chen-Yu Tsai wens@csie.org
The clock must be running for the zero-crossing mute functionality. However, it must be gated for VDD-SYS to be turned off during system suspend. Disable it in the suspend callback, after everything has already been muted, to avoid pops when muting/unmuting outputs.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index cbdb31c3b7bd..4ad262c2e59b 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -438,6 +438,19 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "EARPIECE", NULL, "Earpiece Amp" }, };
+static int sun50i_a64_codec_suspend(struct snd_soc_component *component) +{ + return regmap_update_bits(component->regmap, SUN50I_ADDA_HP_CTRL, + BIT(SUN50I_ADDA_HP_CTRL_PA_CLK_GATE), + BIT(SUN50I_ADDA_HP_CTRL_PA_CLK_GATE)); +} + +static int sun50i_a64_codec_resume(struct snd_soc_component *component) +{ + return regmap_update_bits(component->regmap, SUN50I_ADDA_HP_CTRL, + BIT(SUN50I_ADDA_HP_CTRL_PA_CLK_GATE), 0); +} + static const struct snd_soc_component_driver sun50i_codec_analog_cmpnt_drv = { .controls = sun50i_a64_codec_controls, .num_controls = ARRAY_SIZE(sun50i_a64_codec_controls), @@ -445,6 +458,8 @@ static const struct snd_soc_component_driver sun50i_codec_analog_cmpnt_drv = { .num_dapm_widgets = ARRAY_SIZE(sun50i_a64_codec_widgets), .dapm_routes = sun50i_a64_codec_routes, .num_dapm_routes = ARRAY_SIZE(sun50i_a64_codec_routes), + .suspend = sun50i_a64_codec_suspend, + .resume = sun50i_a64_codec_resume, };
static const struct of_device_id sun50i_codec_analog_of_match[] = {
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
The clock must be running for the zero-crossing mute functionality. However, it must be gated for VDD-SYS to be turned off during system suspend. Disable it in the suspend callback, after everything has already been muted, to avoid pops when muting/unmuting outputs.
Signed-off-by: Samuel Holland samuel@sholland.org
Acked-by: Chen-Yu Tsai wens@csie.org
Sort the controls in the same order as the bits in the register. Then group the routes by sink, and sort them in the same order as the controls. This makes it much easier to verify that all mixer inputs are accounted for.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 58 +++++++++++++-------------- 1 file changed, 28 insertions(+), 30 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 4ad262c2e59b..17165f1ddb63 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -121,50 +121,50 @@
/* mixer controls */ static const struct snd_kcontrol_new sun50i_a64_codec_mixer_controls[] = { - SOC_DAPM_DOUBLE_R("DAC Playback Switch", + SOC_DAPM_DOUBLE_R("Mic1 Playback Switch", SUN50I_ADDA_OL_MIX_CTRL, SUN50I_ADDA_OR_MIX_CTRL, - SUN50I_ADDA_OL_MIX_CTRL_DACL, 1, 0), - SOC_DAPM_DOUBLE_R("DAC Reversed Playback Switch", + SUN50I_ADDA_OL_MIX_CTRL_MIC1, 1, 0), + SOC_DAPM_DOUBLE_R("Mic2 Playback Switch", SUN50I_ADDA_OL_MIX_CTRL, SUN50I_ADDA_OR_MIX_CTRL, - SUN50I_ADDA_OL_MIX_CTRL_DACR, 1, 0), + SUN50I_ADDA_OL_MIX_CTRL_MIC2, 1, 0), SOC_DAPM_DOUBLE_R("Line In Playback Switch", SUN50I_ADDA_OL_MIX_CTRL, SUN50I_ADDA_OR_MIX_CTRL, SUN50I_ADDA_OL_MIX_CTRL_LINEINL, 1, 0), - SOC_DAPM_DOUBLE_R("Mic1 Playback Switch", + SOC_DAPM_DOUBLE_R("DAC Playback Switch", SUN50I_ADDA_OL_MIX_CTRL, SUN50I_ADDA_OR_MIX_CTRL, - SUN50I_ADDA_OL_MIX_CTRL_MIC1, 1, 0), - SOC_DAPM_DOUBLE_R("Mic2 Playback Switch", + SUN50I_ADDA_OL_MIX_CTRL_DACL, 1, 0), + SOC_DAPM_DOUBLE_R("DAC Reversed Playback Switch", SUN50I_ADDA_OL_MIX_CTRL, SUN50I_ADDA_OR_MIX_CTRL, - SUN50I_ADDA_OL_MIX_CTRL_MIC2, 1, 0), + SUN50I_ADDA_OL_MIX_CTRL_DACR, 1, 0), };
/* ADC mixer controls */ static const struct snd_kcontrol_new sun50i_codec_adc_mixer_controls[] = { - SOC_DAPM_DOUBLE_R("Mixer Capture Switch", + SOC_DAPM_DOUBLE_R("Mic1 Capture Switch", SUN50I_ADDA_L_ADCMIX_SRC, SUN50I_ADDA_R_ADCMIX_SRC, - SUN50I_ADDA_L_ADCMIX_SRC_OMIXRL, 1, 0), - SOC_DAPM_DOUBLE_R("Mixer Reversed Capture Switch", + SUN50I_ADDA_L_ADCMIX_SRC_MIC1, 1, 0), + SOC_DAPM_DOUBLE_R("Mic2 Capture Switch", SUN50I_ADDA_L_ADCMIX_SRC, SUN50I_ADDA_R_ADCMIX_SRC, - SUN50I_ADDA_L_ADCMIX_SRC_OMIXRR, 1, 0), + SUN50I_ADDA_L_ADCMIX_SRC_MIC2, 1, 0), SOC_DAPM_DOUBLE_R("Line In Capture Switch", SUN50I_ADDA_L_ADCMIX_SRC, SUN50I_ADDA_R_ADCMIX_SRC, SUN50I_ADDA_L_ADCMIX_SRC_LINEINL, 1, 0), - SOC_DAPM_DOUBLE_R("Mic1 Capture Switch", + SOC_DAPM_DOUBLE_R("Mixer Capture Switch", SUN50I_ADDA_L_ADCMIX_SRC, SUN50I_ADDA_R_ADCMIX_SRC, - SUN50I_ADDA_L_ADCMIX_SRC_MIC1, 1, 0), - SOC_DAPM_DOUBLE_R("Mic2 Capture Switch", + SUN50I_ADDA_L_ADCMIX_SRC_OMIXRL, 1, 0), + SOC_DAPM_DOUBLE_R("Mixer Reversed Capture Switch", SUN50I_ADDA_L_ADCMIX_SRC, SUN50I_ADDA_R_ADCMIX_SRC, - SUN50I_ADDA_L_ADCMIX_SRC_MIC2, 1, 0), + SUN50I_ADDA_L_ADCMIX_SRC_OMIXRR, 1, 0), };
static const DECLARE_TLV_DB_SCALE(sun50i_codec_out_mixer_pregain_scale, @@ -373,24 +373,32 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = {
static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { /* Left Mixer Routes */ + { "Left Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" }, + { "Left Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" }, + { "Left Mixer", "Line In Playback Switch", "LINEIN" }, { "Left Mixer", "DAC Playback Switch", "Left DAC" }, { "Left Mixer", "DAC Reversed Playback Switch", "Right DAC" }, - { "Left Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" },
/* Right Mixer Routes */ + { "Right Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" }, + { "Right Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" }, + { "Right Mixer", "Line In Playback Switch", "LINEIN" }, { "Right Mixer", "DAC Playback Switch", "Right DAC" }, { "Right Mixer", "DAC Reversed Playback Switch", "Left DAC" }, - { "Right Mixer", "Mic1 Playback Switch", "Mic1 Amplifier" },
/* Left ADC Mixer Routes */ + { "Left ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" }, + { "Left ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" }, + { "Left ADC Mixer", "Line In Capture Switch", "LINEIN" }, { "Left ADC Mixer", "Mixer Capture Switch", "Left Mixer" }, { "Left ADC Mixer", "Mixer Reversed Capture Switch", "Right Mixer" }, - { "Left ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" },
/* Right ADC Mixer Routes */ + { "Right ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" }, + { "Right ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" }, + { "Right ADC Mixer", "Line In Capture Switch", "LINEIN" }, { "Right ADC Mixer", "Mixer Capture Switch", "Right Mixer" }, { "Right ADC Mixer", "Mixer Reversed Capture Switch", "Left Mixer" }, - { "Right ADC Mixer", "Mic1 Capture Switch", "Mic1 Amplifier" },
/* ADC Routes */ { "Left ADC", NULL, "Left ADC Mixer" }, @@ -410,16 +418,6 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = {
/* Microphone Routes */ { "Mic2 Amplifier", NULL, "MIC2"}, - { "Left Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" }, - { "Right Mixer", "Mic2 Playback Switch", "Mic2 Amplifier" }, - { "Left ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" }, - { "Right ADC Mixer", "Mic2 Capture Switch", "Mic2 Amplifier" }, - - /* Line-in Routes */ - { "Left Mixer", "Line In Playback Switch", "LINEIN" }, - { "Right Mixer", "Line In Playback Switch", "LINEIN" }, - { "Left ADC Mixer", "Line In Capture Switch", "LINEIN" }, - { "Right ADC Mixer", "Line In Capture Switch", "LINEIN" },
/* Line-out Routes */ { "Line Out Source Playback Route", "Stereo", "Left Mixer" },
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
Sort the controls in the same order as the bits in the register. Then group the routes by sink, and sort them in the same order as the controls. This makes it much easier to verify that all mixer inputs are accounted for.
Signed-off-by: Samuel Holland samuel@sholland.org
Acked-by: Chen-Yu Tsai wens@csie.org
This matches the hardware more accurately, and is necessary for including the (stereo) headphone mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 17165f1ddb63..f98851067f97 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -311,9 +311,15 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { */
SND_SOC_DAPM_REGULATOR_SUPPLY("cpvdd", 0, 0), - SND_SOC_DAPM_MUX("Headphone Source Playback Route", + SND_SOC_DAPM_MUX("Left Headphone Source", SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src), - SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN50I_ADDA_HP_CTRL, + SND_SOC_DAPM_MUX("Right Headphone Source", + SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src), + SND_SOC_DAPM_OUT_DRV("Left Headphone Amp", + SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_OUT_DRV("Right Headphone Amp", + SND_SOC_NOPM, 0, 0, NULL, 0), + SND_SOC_DAPM_SUPPLY("Headphone Amp", SUN50I_ADDA_HP_CTRL, SUN50I_ADDA_HP_CTRL_HPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("HP"),
@@ -405,13 +411,19 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Right ADC", NULL, "Right ADC Mixer" },
/* Headphone Routes */ - { "Headphone Source Playback Route", "DAC", "Left DAC" }, - { "Headphone Source Playback Route", "DAC", "Right DAC" }, - { "Headphone Source Playback Route", "Mixer", "Left Mixer" }, - { "Headphone Source Playback Route", "Mixer", "Right Mixer" }, - { "Headphone Amp", NULL, "Headphone Source Playback Route" }, + { "Left Headphone Source", "DAC", "Left DAC" }, + { "Left Headphone Source", "Mixer", "Left Mixer" }, + { "Left Headphone Amp", NULL, "Left Headphone Source" }, + { "Left Headphone Amp", NULL, "Headphone Amp" }, + { "HP", NULL, "Left Headphone Amp" }, + + { "Right Headphone Source", "DAC", "Right DAC" }, + { "Right Headphone Source", "Mixer", "Right Mixer" }, + { "Right Headphone Amp", NULL, "Right Headphone Source" }, + { "Right Headphone Amp", NULL, "Headphone Amp" }, + { "HP", NULL, "Right Headphone Amp" }, + { "Headphone Amp", NULL, "cpvdd" }, - { "HP", NULL, "Headphone Amp" },
/* Microphone Routes */ { "Mic1 Amplifier", NULL, "MIC1"},
Hi,
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
This matches the hardware more accurately, and is necessary for including the (stereo) headphone mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org
sound/soc/sunxi/sun50i-codec-analog.c | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 17165f1ddb63..f98851067f97 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -311,9 +311,15 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { */
SND_SOC_DAPM_REGULATOR_SUPPLY("cpvdd", 0, 0),
SND_SOC_DAPM_MUX("Headphone Source Playback Route",
SND_SOC_DAPM_MUX("Left Headphone Source", SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src),
SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN50I_ADDA_HP_CTRL,
SND_SOC_DAPM_MUX("Right Headphone Source",
Please don't remove the widget suffix. The suffix was chosen to work with alsa-lib's control parsing code. The term "Playback Route" is appropriate because it is playback only, and it is a routing switch, not a source or sink.
Also, what's wrong with just having a single "stereo" widget instead of two "mono" widgets? I added stereo (2-channel) support to DAPM quite some time ago. You just have to have two routes in and out.
ChenYu
SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src),SND_SOC_DAPM_OUT_DRV("Left Headphone Amp",SND_SOC_NOPM, 0, 0, NULL, 0),SND_SOC_DAPM_OUT_DRV("Right Headphone Amp",SND_SOC_NOPM, 0, 0, NULL, 0),SND_SOC_DAPM_SUPPLY("Headphone Amp", SUN50I_ADDA_HP_CTRL, SUN50I_ADDA_HP_CTRL_HPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("HP"),@@ -405,13 +411,19 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Right ADC", NULL, "Right ADC Mixer" },
/* Headphone Routes */
{ "Headphone Source Playback Route", "DAC", "Left DAC" },{ "Headphone Source Playback Route", "DAC", "Right DAC" },{ "Headphone Source Playback Route", "Mixer", "Left Mixer" },{ "Headphone Source Playback Route", "Mixer", "Right Mixer" },{ "Headphone Amp", NULL, "Headphone Source Playback Route" },
{ "Left Headphone Source", "DAC", "Left DAC" },{ "Left Headphone Source", "Mixer", "Left Mixer" },{ "Left Headphone Amp", NULL, "Left Headphone Source" },{ "Left Headphone Amp", NULL, "Headphone Amp" },{ "HP", NULL, "Left Headphone Amp" },{ "Right Headphone Source", "DAC", "Right DAC" },{ "Right Headphone Source", "Mixer", "Right Mixer" },{ "Right Headphone Amp", NULL, "Right Headphone Source" },{ "Right Headphone Amp", NULL, "Headphone Amp" },{ "HP", NULL, "Right Headphone Amp" },{ "Headphone Amp", NULL, "cpvdd" },
{ "HP", NULL, "Headphone Amp" }, /* Microphone Routes */ { "Mic1 Amplifier", NULL, "MIC1"},-- 2.24.1
Hello,
On 2/16/20 9:48 PM, Chen-Yu Tsai wrote:
Hi,
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
This matches the hardware more accurately, and is necessary for including the (stereo) headphone mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org
sound/soc/sunxi/sun50i-codec-analog.c | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 17165f1ddb63..f98851067f97 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -311,9 +311,15 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { */
SND_SOC_DAPM_REGULATOR_SUPPLY("cpvdd", 0, 0),
SND_SOC_DAPM_MUX("Headphone Source Playback Route",
SND_SOC_DAPM_MUX("Left Headphone Source", SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src),
SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN50I_ADDA_HP_CTRL,
SND_SOC_DAPM_MUX("Right Headphone Source",Please don't remove the widget suffix. The suffix was chosen to work with alsa-lib's control parsing code. The term "Playback Route" is appropriate because it is playback only, and it is a routing switch, not a source or sink.
Removing the suffix here doesn't affect the control name as seen by userspace, because the control is shared between multiple widgets (Left and Right).
Also, what's wrong with just having a single "stereo" widget instead of two "mono" widgets? I added stereo (2-channel) support to DAPM quite some time ago. You just have to have two routes in and out.
If you have any completed path through a widget, the widget is turned on. A stereo mute switch is logically two separate paths. So if I break one path by muting one channel, that lets me turn off everything before and after in the path (e.g. turning off the right side of the DAC lets DAPM turn off the right mixer, the right side of the output amp, even the right side of the AIF or the ADC if that was the only input. That only works if there are separate Left/Right widgets.
ChenYu
Samuel
SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src),SND_SOC_DAPM_OUT_DRV("Left Headphone Amp",SND_SOC_NOPM, 0, 0, NULL, 0),SND_SOC_DAPM_OUT_DRV("Right Headphone Amp",SND_SOC_NOPM, 0, 0, NULL, 0),SND_SOC_DAPM_SUPPLY("Headphone Amp", SUN50I_ADDA_HP_CTRL, SUN50I_ADDA_HP_CTRL_HPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("HP"),@@ -405,13 +411,19 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Right ADC", NULL, "Right ADC Mixer" },
/* Headphone Routes */
{ "Headphone Source Playback Route", "DAC", "Left DAC" },{ "Headphone Source Playback Route", "DAC", "Right DAC" },{ "Headphone Source Playback Route", "Mixer", "Left Mixer" },{ "Headphone Source Playback Route", "Mixer", "Right Mixer" },{ "Headphone Amp", NULL, "Headphone Source Playback Route" },
{ "Left Headphone Source", "DAC", "Left DAC" },{ "Left Headphone Source", "Mixer", "Left Mixer" },{ "Left Headphone Amp", NULL, "Left Headphone Source" },{ "Left Headphone Amp", NULL, "Headphone Amp" },{ "HP", NULL, "Left Headphone Amp" },{ "Right Headphone Source", "DAC", "Right DAC" },{ "Right Headphone Source", "Mixer", "Right Mixer" },{ "Right Headphone Amp", NULL, "Right Headphone Source" },{ "Right Headphone Amp", NULL, "Headphone Amp" },{ "HP", NULL, "Right Headphone Amp" },{ "Headphone Amp", NULL, "cpvdd" },
{ "HP", NULL, "Headphone Amp" }, /* Microphone Routes */ { "Mic1 Amplifier", NULL, "MIC1"},-- 2.24.1
On Mon, Feb 17, 2020 at 11:57 AM Samuel Holland samuel@sholland.org wrote:
Hello,
On 2/16/20 9:48 PM, Chen-Yu Tsai wrote:
Hi,
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
This matches the hardware more accurately, and is necessary for including the (stereo) headphone mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org
sound/soc/sunxi/sun50i-codec-analog.c | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 17165f1ddb63..f98851067f97 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -311,9 +311,15 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { */
SND_SOC_DAPM_REGULATOR_SUPPLY("cpvdd", 0, 0),
SND_SOC_DAPM_MUX("Headphone Source Playback Route",
SND_SOC_DAPM_MUX("Left Headphone Source", SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src),
SND_SOC_DAPM_OUT_DRV("Headphone Amp", SUN50I_ADDA_HP_CTRL,
SND_SOC_DAPM_MUX("Right Headphone Source",Please don't remove the widget suffix. The suffix was chosen to work with alsa-lib's control parsing code. The term "Playback Route" is appropriate because it is playback only, and it is a routing switch, not a source or sink.
Removing the suffix here doesn't affect the control name as seen by userspace, because the control is shared between multiple widgets (Left and Right).
You're right.
Also, what's wrong with just having a single "stereo" widget instead of two "mono" widgets? I added stereo (2-channel) support to DAPM quite some time ago. You just have to have two routes in and out.
If you have any completed path through a widget, the widget is turned on. A stereo mute switch is logically two separate paths. So if I break one path by muting one channel, that lets me turn off everything before and after in the path (e.g. turning off the right side of the DAC lets DAPM turn off the right mixer, the right side of the output amp, even the right side of the AIF or the ADC if that was the only input. That only works if there are separate Left/Right widgets.
Looks like that's the case indeed. Might be worth revisiting the core DAPM code later on if I can find the time.
Since the widgets and routes changed are internal to the codec, there won't be any issue if we rework this stuff later on. So for now,
Reviewed-by: Chen-Yu Tsai wens@csie.org
By including the headphone mute switch to the DAPM graph, both the headphone amplifier and the Mixer/DAC inputs can be powered off when the headphones are muted.
The mute switch is between the source selection and the amplifier, as per the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index f98851067f97..176d6658d099 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -193,11 +193,6 @@ static const struct snd_kcontrol_new sun50i_a64_codec_controls[] = { SUN50I_ADDA_HP_CTRL_HPVOL, 0x3f, 0, sun50i_codec_hp_vol_scale),
- SOC_DOUBLE("Headphone Playback Switch", - SUN50I_ADDA_MIX_DAC_CTRL, - SUN50I_ADDA_MIX_DAC_CTRL_LHPPAMUTE, - SUN50I_ADDA_MIX_DAC_CTRL_RHPPAMUTE, 1, 0), - /* Mixer pre-gain */ SOC_SINGLE_TLV("Mic1 Playback Volume", SUN50I_ADDA_MIC1_CTRL, SUN50I_ADDA_MIC1_CTRL_MIC1G, @@ -264,6 +259,12 @@ static const struct snd_kcontrol_new sun50i_codec_hp_src[] = { sun50i_codec_hp_src_enum), };
+static const struct snd_kcontrol_new sun50i_codec_hp_switch = + SOC_DAPM_DOUBLE("Headphone Playback Switch", + SUN50I_ADDA_MIX_DAC_CTRL, + SUN50I_ADDA_MIX_DAC_CTRL_LHPPAMUTE, + SUN50I_ADDA_MIX_DAC_CTRL_RHPPAMUTE, 1, 0); + static const char * const sun50i_codec_lineout_src_enum_text[] = { "Stereo", "Mono Differential", }; @@ -315,6 +316,10 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src), SND_SOC_DAPM_MUX("Right Headphone Source", SND_SOC_NOPM, 0, 0, sun50i_codec_hp_src), + SND_SOC_DAPM_SWITCH("Left Headphone Switch", + SND_SOC_NOPM, 0, 0, &sun50i_codec_hp_switch), + SND_SOC_DAPM_SWITCH("Right Headphone Switch", + SND_SOC_NOPM, 0, 0, &sun50i_codec_hp_switch), SND_SOC_DAPM_OUT_DRV("Left Headphone Amp", SND_SOC_NOPM, 0, 0, NULL, 0), SND_SOC_DAPM_OUT_DRV("Right Headphone Amp", @@ -413,13 +418,15 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { /* Headphone Routes */ { "Left Headphone Source", "DAC", "Left DAC" }, { "Left Headphone Source", "Mixer", "Left Mixer" }, - { "Left Headphone Amp", NULL, "Left Headphone Source" }, + { "Left Headphone Switch", "Headphone Playback Switch", "Left Headphone Source" }, + { "Left Headphone Amp", NULL, "Left Headphone Switch" }, { "Left Headphone Amp", NULL, "Headphone Amp" }, { "HP", NULL, "Left Headphone Amp" },
{ "Right Headphone Source", "DAC", "Right DAC" }, { "Right Headphone Source", "Mixer", "Right Mixer" }, - { "Right Headphone Amp", NULL, "Right Headphone Source" }, + { "Right Headphone Switch", "Headphone Playback Switch", "Right Headphone Source" }, + { "Right Headphone Amp", NULL, "Right Headphone Switch" }, { "Right Headphone Amp", NULL, "Headphone Amp" }, { "HP", NULL, "Right Headphone Amp" },
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
By including the headphone mute switch to the DAPM graph, both the headphone amplifier and the Mixer/DAC inputs can be powered off when the headphones are muted.
The mute switch is between the source selection and the amplifier, as per the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Chen-Yu Tsai wens@csie.org
BTW, have you also considered tying in the headphone volume control? It also has a mute setting.
ChenYu
On 2/17/20 1:17 AM, Chen-Yu Tsai wrote:
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
By including the headphone mute switch to the DAPM graph, both the headphone amplifier and the Mixer/DAC inputs can be powered off when the headphones are muted.
The mute switch is between the source selection and the amplifier, as per the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Chen-Yu Tsai wens@csie.org
BTW, have you also considered tying in the headphone volume control? It also has a mute setting.
As far as I can tell, setting a volume control to its "mute" level has no effect on the DAPM power state. So I didn't add PGA widgets for the volume controls on either codec. I can add them if there's some benefit to doing so.
ChenYu
Regards, Samuel
This matches the hardware more accurately, and is necessary for including the (stereo) line out mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 176d6658d099..df39f6ffe25a 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -328,7 +328,9 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { SUN50I_ADDA_HP_CTRL_HPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("HP"),
- SND_SOC_DAPM_MUX("Line Out Source Playback Route", + SND_SOC_DAPM_MUX("Left Line Out Source", + SND_SOC_NOPM, 0, 0, sun50i_codec_lineout_src), + SND_SOC_DAPM_MUX("Right Line Out Source", SND_SOC_NOPM, 0, 0, sun50i_codec_lineout_src), SND_SOC_DAPM_OUTPUT("LINEOUT"),
@@ -439,12 +441,14 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Mic2 Amplifier", NULL, "MIC2"},
/* Line-out Routes */ - { "Line Out Source Playback Route", "Stereo", "Left Mixer" }, - { "Line Out Source Playback Route", "Stereo", "Right Mixer" }, - { "Line Out Source Playback Route", "Mono Differential", "Left Mixer" }, - { "Line Out Source Playback Route", "Mono Differential", - "Right Mixer" }, - { "LINEOUT", NULL, "Line Out Source Playback Route" }, + { "Left Line Out Source", "Stereo", "Left Mixer" }, + { "Left Line Out Source", "Mono Differential", "Left Mixer" }, + { "Left Line Out Source", "Mono Differential", "Right Mixer" }, + { "LINEOUT", NULL, "Left Line Out Source" }, + + { "Right Line Out Source", "Stereo", "Right Mixer" }, + { "Right Line Out Source", "Mono Differential", "Left Line Out Source" }, + { "LINEOUT", NULL, "Right Line Out Source" },
/* Earpiece Routes */ { "Earpiece Source Playback Route", "DACL", "Left DAC" },
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
This matches the hardware more accurately, and is necessary for including the (stereo) line out mute switch in the DAPM graph.
Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Chen-Yu Tsai wens@csie.org
By including the line out mute switch in the DAPM graph, the Mixer/DAC inputs can be powered off when the line output is muted.
The line outputs have an unusual routing scheme. The left side mute switch is between the source selection and the amplifier, as usual. The right side source selection comes *after* its amplifier (and after the left side amplifier), and its mute switch controls whichever source is currently selected. This matches the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index df39f6ffe25a..84bb76cad74f 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -228,11 +228,6 @@ static const struct snd_kcontrol_new sun50i_a64_codec_controls[] = { SUN50I_ADDA_LINEOUT_CTRL1_VOL, 0x1f, 0, sun50i_codec_lineout_vol_scale),
- SOC_DOUBLE("Line Out Playback Switch", - SUN50I_ADDA_LINEOUT_CTRL0, - SUN50I_ADDA_LINEOUT_CTRL0_LEN, - SUN50I_ADDA_LINEOUT_CTRL0_REN, 1, 0), - SOC_SINGLE_TLV("Earpiece Playback Volume", SUN50I_ADDA_EARPIECE_CTRL1, SUN50I_ADDA_EARPIECE_CTRL1_ESP_VOL, 0x1f, 0, @@ -280,6 +275,12 @@ static const struct snd_kcontrol_new sun50i_codec_lineout_src[] = { sun50i_codec_lineout_src_enum), };
+static const struct snd_kcontrol_new sun50i_codec_lineout_switch = + SOC_DAPM_DOUBLE("Line Out Playback Switch", + SUN50I_ADDA_LINEOUT_CTRL0, + SUN50I_ADDA_LINEOUT_CTRL0_LEN, + SUN50I_ADDA_LINEOUT_CTRL0_REN, 1, 0); + static const char * const sun50i_codec_earpiece_src_enum_text[] = { "DACR", "DACL", "Right Mixer", "Left Mixer", }; @@ -332,6 +333,10 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { SND_SOC_NOPM, 0, 0, sun50i_codec_lineout_src), SND_SOC_DAPM_MUX("Right Line Out Source", SND_SOC_NOPM, 0, 0, sun50i_codec_lineout_src), + SND_SOC_DAPM_SWITCH("Left Line Out Switch", + SND_SOC_NOPM, 0, 0, &sun50i_codec_lineout_switch), + SND_SOC_DAPM_SWITCH("Right Line Out Switch", + SND_SOC_NOPM, 0, 0, &sun50i_codec_lineout_switch), SND_SOC_DAPM_OUTPUT("LINEOUT"),
SND_SOC_DAPM_MUX("Earpiece Source Playback Route", @@ -444,10 +449,12 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Left Line Out Source", "Stereo", "Left Mixer" }, { "Left Line Out Source", "Mono Differential", "Left Mixer" }, { "Left Line Out Source", "Mono Differential", "Right Mixer" }, - { "LINEOUT", NULL, "Left Line Out Source" }, + { "Left Line Out Switch", "Line Out Playback Switch", "Left Line Out Source" }, + { "LINEOUT", NULL, "Left Line Out Switch" },
- { "Right Line Out Source", "Stereo", "Right Mixer" }, - { "Right Line Out Source", "Mono Differential", "Left Line Out Source" }, + { "Right Line Out Switch", "Line Out Playback Switch", "Right Mixer" }, + { "Right Line Out Source", "Stereo", "Right Line Out Switch" }, + { "Right Line Out Source", "Mono Differential", "Left Line Out Switch" }, { "LINEOUT", NULL, "Right Line Out Source" },
/* Earpiece Routes */
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
By including the line out mute switch in the DAPM graph, the Mixer/DAC inputs can be powered off when the line output is muted.
The line outputs have an unusual routing scheme. The left side mute switch is between the source selection and the amplifier, as usual. The right side source selection comes *after* its amplifier (and after the left side amplifier), and its mute switch controls whichever source is currently selected. This matches the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org
Reviewed-by: Chen-Yu Tsai wens@csie.org
By including the earpiece mute switch in the DAPM graph, both the earpiece amplifier and the Mixer/DAC inputs can be powered off when the earpiece is muted.
The mute switch is between the source selection and the amplifier, as per the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org --- sound/soc/sunxi/sun50i-codec-analog.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 84bb76cad74f..6c89b0716bbd 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -232,11 +232,6 @@ static const struct snd_kcontrol_new sun50i_a64_codec_controls[] = { SUN50I_ADDA_EARPIECE_CTRL1, SUN50I_ADDA_EARPIECE_CTRL1_ESP_VOL, 0x1f, 0, sun50i_codec_earpiece_vol_scale), - - SOC_SINGLE("Earpiece Playback Switch", - SUN50I_ADDA_EARPIECE_CTRL1, - SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_MUTE, 1, 0), - };
static const char * const sun50i_codec_hp_src_enum_text[] = { @@ -295,6 +290,11 @@ static const struct snd_kcontrol_new sun50i_codec_earpiece_src[] = { sun50i_codec_earpiece_src_enum), };
+static const struct snd_kcontrol_new sun50i_codec_earpiece_switch = + SOC_DAPM_SINGLE("Playback Switch", + SUN50I_ADDA_EARPIECE_CTRL1, + SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_MUTE, 1, 0); + static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { /* DAC */ SND_SOC_DAPM_DAC("Left DAC", NULL, SUN50I_ADDA_MIX_DAC_CTRL, @@ -341,6 +341,8 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = {
SND_SOC_DAPM_MUX("Earpiece Source Playback Route", SND_SOC_NOPM, 0, 0, sun50i_codec_earpiece_src), + SND_SOC_DAPM_SWITCH("Earpiece", + SND_SOC_NOPM, 0, 0, &sun50i_codec_earpiece_switch), SND_SOC_DAPM_OUT_DRV("Earpiece Amp", SUN50I_ADDA_EARPIECE_CTRL1, SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("EARPIECE"), @@ -462,7 +464,8 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Earpiece Source Playback Route", "DACR", "Right DAC" }, { "Earpiece Source Playback Route", "Left Mixer", "Left Mixer" }, { "Earpiece Source Playback Route", "Right Mixer", "Right Mixer" }, - { "Earpiece Amp", NULL, "Earpiece Source Playback Route" }, + { "Earpiece", "Playback Switch", "Earpiece Source Playback Route" }, + { "Earpiece Amp", NULL, "Earpiece" }, { "EARPIECE", NULL, "Earpiece Amp" }, };
Hi,
On Mon, Feb 17, 2020 at 10:18 AM Samuel Holland samuel@sholland.org wrote:
By including the earpiece mute switch in the DAPM graph, both the earpiece amplifier and the Mixer/DAC inputs can be powered off when the earpiece is muted.
The mute switch is between the source selection and the amplifier, as per the diagram in the SoC manual.
Signed-off-by: Samuel Holland samuel@sholland.org
sound/soc/sunxi/sun50i-codec-analog.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sunxi/sun50i-codec-analog.c b/sound/soc/sunxi/sun50i-codec-analog.c index 84bb76cad74f..6c89b0716bbd 100644 --- a/sound/soc/sunxi/sun50i-codec-analog.c +++ b/sound/soc/sunxi/sun50i-codec-analog.c @@ -232,11 +232,6 @@ static const struct snd_kcontrol_new sun50i_a64_codec_controls[] = { SUN50I_ADDA_EARPIECE_CTRL1, SUN50I_ADDA_EARPIECE_CTRL1_ESP_VOL, 0x1f, 0, sun50i_codec_earpiece_vol_scale),
SOC_SINGLE("Earpiece Playback Switch",SUN50I_ADDA_EARPIECE_CTRL1,SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_MUTE, 1, 0),};
static const char * const sun50i_codec_hp_src_enum_text[] = { @@ -295,6 +290,11 @@ static const struct snd_kcontrol_new sun50i_codec_earpiece_src[] = { sun50i_codec_earpiece_src_enum), };
+static const struct snd_kcontrol_new sun50i_codec_earpiece_switch =
SOC_DAPM_SINGLE("Playback Switch",SUN50I_ADDA_EARPIECE_CTRL1,SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_MUTE, 1, 0);static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = { /* DAC */ SND_SOC_DAPM_DAC("Left DAC", NULL, SUN50I_ADDA_MIX_DAC_CTRL, @@ -341,6 +341,8 @@ static const struct snd_soc_dapm_widget sun50i_a64_codec_widgets[] = {
SND_SOC_DAPM_MUX("Earpiece Source Playback Route", SND_SOC_NOPM, 0, 0, sun50i_codec_earpiece_src),
SND_SOC_DAPM_SWITCH("Earpiece",SND_SOC_NOPM, 0, 0, &sun50i_codec_earpiece_switch),
I would suggest naming this something a bit more specific, in case someone uses "Earpiece" as a widget name at the board level. Also, having the "Earpiece" come before the "Earpiece Amp" in the route doesn't make much sense. However, this creates an issue with the name of the created kcontrol... Any other ideas?
ChenYu
SND_SOC_DAPM_OUT_DRV("Earpiece Amp", SUN50I_ADDA_EARPIECE_CTRL1, SUN50I_ADDA_EARPIECE_CTRL1_ESPPA_EN, 0, NULL, 0), SND_SOC_DAPM_OUTPUT("EARPIECE"),@@ -462,7 +464,8 @@ static const struct snd_soc_dapm_route sun50i_a64_codec_routes[] = { { "Earpiece Source Playback Route", "DACR", "Right DAC" }, { "Earpiece Source Playback Route", "Left Mixer", "Left Mixer" }, { "Earpiece Source Playback Route", "Right Mixer", "Right Mixer" },
{ "Earpiece Amp", NULL, "Earpiece Source Playback Route" },
{ "Earpiece", "Playback Switch", "Earpiece Source Playback Route" },{ "Earpiece Amp", NULL, "Earpiece" }, { "EARPIECE", NULL, "Earpiece Amp" },};
-- 2.24.1
participants (2)
-
Chen-Yu Tsai -
Samuel Holland