[4/8] ASoC: sun50i-codec-analog: Make headphone routes stereo
diff mbox series

Message ID 20200217021813.53266-5-samuel@sholland.org
State New
Headers show
Series
  • ASoC: sun50i-codec-analog: Cleanup and power management
Related show

Commit Message

Samuel Holland Feb. 17, 2020, 2:18 a.m. UTC
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(-)

Comments

Chen-Yu Tsai (Moxa) Feb. 17, 2020, 3:48 a.m. UTC | #1
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
>
Samuel Holland Feb. 17, 2020, 3:57 a.m. UTC | #2
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
>>
Chen-Yu Tsai (Moxa) Feb. 17, 2020, 6:56 a.m. UTC | #3
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>

Patch
diff mbox series

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"},