[PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Jarkko Nikula]

Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
external widgets doesn't alter the output state.

Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
---
I noticed this with a widget and audio map below where state of
"Headphone Jack" wasn't changing the codec output.

SND_SOC_DAPM_HP("Headphone Jack", NULL),

{"Headphone Jack", NULL, "TPA6130A2 Headphone Left"},
{"Headphone Jack", NULL, "TPA6130A2 Headphone Right"},
{"TPA6130A2 Left", NULL, "LLOUT"},
{"TPA6130A2 Right", NULL, "RLOUT"}
---
 sound/soc/codecs/tpa6130a2.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
index 100a747..99b70e5 100644
--- a/sound/soc/codecs/tpa6130a2.c
+++ b/sound/soc/codecs/tpa6130a2.c
@@ -359,8 +359,8 @@ static const struct snd_soc_dapm_widget tpa6130a2_dapm_widgets[] = {
 			0, 0, tpa6130a2_supply_event,
 			SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
 	/* Outputs */
-	SND_SOC_DAPM_HP("TPA6130A2 Headphone Left", NULL),
-	SND_SOC_DAPM_HP("TPA6130A2 Headphone Right", NULL),
+	SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Left"),
+	SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Right"),
 };
 
 static const struct snd_soc_dapm_route audio_map[] = {
-- 
1.7.1

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Peter Ujfalusi]

On Wednesday 19 May 2010 13:55:26 ext Jarkko Nikula wrote:
> Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
> external widgets doesn't alter the output state.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> ---
> I noticed this with a widget and audio map below where state of
> "Headphone Jack" wasn't changing the codec output.

This is intentional.
TPA is _not_ a codec, it is amplifier.

> SND_SOC_DAPM_HP("Headphone Jack", NULL),
> 
> {"Headphone Jack", NULL, "TPA6130A2 Headphone Left"},
> {"Headphone Jack", NULL, "TPA6130A2 Headphone Right"},
> {"TPA6130A2 Left", NULL, "LLOUT"},
> {"TPA6130A2 Right", NULL, "RLOUT"}

And this is how you should use it in a machine driver:
Connect the codec's outputs to TPA. The TPA code adds the HPs for you already, 
so you don't need to care about it...

 {"TPA6130A2 Left", NULL, "LLOUT"},
 {"TPA6130A2 Right", NULL, "RLOUT"}

If you want to turn off the headset path, which I suppose you want to do here, 
than within the same kcontrol you will have for HS mute you can do this:

	if (jack_state) {
		snd_soc_dapm_enable_pin(codec, "TPA6130A2 Headphone Left");
		snd_soc_dapm_enable_pin(codec, "TPA6130A2 Headphone Right");
	} else {
		snd_soc_dapm_disable_pin(codec, "TPA6130A2 Headphone Left");
		snd_soc_dapm_disable_pin(codec, "TPA6130A2 Headphone Right");
	}

> ---
>  sound/soc/codecs/tpa6130a2.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/codecs/tpa6130a2.c b/sound/soc/codecs/tpa6130a2.c
> index 100a747..99b70e5 100644
> --- a/sound/soc/codecs/tpa6130a2.c
> +++ b/sound/soc/codecs/tpa6130a2.c
> @@ -359,8 +359,8 @@ static const struct snd_soc_dapm_widget
> tpa6130a2_dapm_widgets[] = { 0, 0, tpa6130a2_supply_event,
>  			SND_SOC_DAPM_POST_PMU|SND_SOC_DAPM_POST_PMD),
>  	/* Outputs */
> -	SND_SOC_DAPM_HP("TPA6130A2 Headphone Left", NULL),
> -	SND_SOC_DAPM_HP("TPA6130A2 Headphone Right", NULL),
> +	SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Left"),
> +	SND_SOC_DAPM_OUTPUT("TPA6130A2 Headphone Right"),
>  };
> 
>  static const struct snd_soc_dapm_route audio_map[] = {

For now I'm not acking this.

-- 
Péter

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Jarkko Nikula]

On Wed, 19 May 2010 15:00:14 +0300
Peter Ujfalusi <peter.ujfalusi@nokia.com> wrote:

> On Wednesday 19 May 2010 13:55:26 ext Jarkko Nikula wrote:
> > Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
> > external widgets doesn't alter the output state.
> > 
> > Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> > Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> > ---
> > I noticed this with a widget and audio map below where state of
> > "Headphone Jack" wasn't changing the codec output.
> 
> This is intentional.
> TPA is _not_ a codec, it is amplifier.
> 
> > SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > 
> > {"Headphone Jack", NULL, "TPA6130A2 Headphone Left"},
> > {"Headphone Jack", NULL, "TPA6130A2 Headphone Right"},
> > {"TPA6130A2 Left", NULL, "LLOUT"},
> > {"TPA6130A2 Right", NULL, "RLOUT"}
> 
> And this is how you should use it in a machine driver:
> Connect the codec's outputs to TPA. The TPA code adds the HPs for you already, 
> so you don't need to care about it...
> 
>  {"TPA6130A2 Left", NULL, "LLOUT"},
>  {"TPA6130A2 Right", NULL, "RLOUT"}
> 
> If you want to turn off the headset path, which I suppose you want to do here, 
> than within the same kcontrol you will have for HS mute you can do this:
> 
> 	if (jack_state) {
> 		snd_soc_dapm_enable_pin(codec, "TPA6130A2 Headphone Left");
> 		snd_soc_dapm_enable_pin(codec, "TPA6130A2 Headphone Right");
> 	} else {
> 		snd_soc_dapm_disable_pin(codec, "TPA6130A2 Headphone Left");
> 		snd_soc_dapm_disable_pin(codec, "TPA6130A2 Headphone Right");
> 	}
> 
Thanks for update, this also works, I was unclear are there bug or not
so wanted to try with a patch first :-)


-- 
Jarkko

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Mark Brown]

On Wed, 2010-05-19 at 13:55 +0300, Jarkko Nikula wrote:
> Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
> external widgets doesn't alter the output state.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Mark Brown]

On Wed, 2010-05-19 at 15:00 +0300, Peter Ujfalusi wrote:
> On Wednesday 19 May 2010 13:55:26 ext Jarkko Nikula wrote:
> > Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
> > external widgets doesn't alter the output state.
> > 
> > Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> > Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> > ---
> > I noticed this with a widget and audio map below where state of
> > "Headphone Jack" wasn't changing the codec output.

> This is intentional.
> TPA is _not_ a codec, it is amplifier.

It seems reasonable enough for a user to want to do this - the only
difference with regard to the headphone amplifiers integrated onto
CODECs is that it happens to be on a separate piece of silicon.

> > SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > 
> > {"Headphone Jack", NULL, "TPA6130A2 Headphone Left"},
> > {"Headphone Jack", NULL, "TPA6130A2 Headphone Right"},
> > {"TPA6130A2 Left", NULL, "LLOUT"},
> > {"TPA6130A2 Right", NULL, "RLOUT"}

> And this is how you should use it in a machine driver:
> Connect the codec's outputs to TPA. The TPA code adds the HPs for you already, 
> so you don't need to care about it...

It's not unreasonable for a user to want to wire the device through to a
board-defined jack. For example, they may find this helps with
documenting which physical jack is used or they may have a jack that's
got multiple output types on it such as a dock connector. They may also
wish to use the event that is available on the jack widget to perform
some board specific action when the headphone is activated.

There's no actual requirement to define headphone widgets if they don't
have any other code associated with them - if the outputs are left
unconnected the effect will be the same as if a noop and unused
headphone widget were there.

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Liam Girdwood]

On Wed, 2010-05-19 at 13:55 +0300, Jarkko Nikula wrote:
> Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as otherwise
> external widgets doesn't alter the output state.
> 
> Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> ---
Applied.

Thanks

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Peter Ujfalusi]

Hi,

this is now just educational discussion - for me ;)
Since Liam already taken the patch, Takashi pulled it, and it is on the train to 
2.6.35...
But still

On Wednesday 19 May 2010 18:05:12 ext Mark Brown wrote:
> On Wed, 2010-05-19 at 15:00 +0300, Peter Ujfalusi wrote:
> > On Wednesday 19 May 2010 13:55:26 ext Jarkko Nikula wrote:
> > > Codec output pin should be defined with SND_SOC_DAPM_OUTPUT as
> > > otherwise external widgets doesn't alter the output state.
> > > 
> > > Signed-off-by: Jarkko Nikula <jhnikula@gmail.com>
> > > Cc: Peter Ujfalusi <peter.ujfalusi@nokia.com>
> > > ---
> > > I noticed this with a widget and audio map below where state of
> > > "Headphone Jack" wasn't changing the codec output.
> > 
> > This is intentional.
> > TPA is _not_ a codec, it is amplifier.
> 
> It seems reasonable enough for a user to want to do this

There is no way to argue against this sentence ;)

> - the only difference with regard to the headphone amplifiers integrated onto
> CODECs is that it happens to be on a separate piece of silicon.

This driver had the _HP widgets since it has been introduced, and even the 
initial commit message explained, that the amp driver provides the _HP, and 
machines are only need to connect the amp to their codecs, and all is taken care 
of DAPM.

I guess my reasoning was to avoid the following DAPM route, which for me seamed 
weird:
soc codec                        |      tap6130a2        |   machine driver
codec_DAC -> .. -> codec_OUTPUT -> tpa_PGA -> tpa_OUTPUT -> machine_HP

It seamed wrong to have codec_OUTPUT and tpa_OUTPUT in the same DAPM route.

> 
> > > SND_SOC_DAPM_HP("Headphone Jack", NULL),
> > > 
> > > {"Headphone Jack", NULL, "TPA6130A2 Headphone Left"},
> > > {"Headphone Jack", NULL, "TPA6130A2 Headphone Right"},
> > > {"TPA6130A2 Left", NULL, "LLOUT"},
> > > {"TPA6130A2 Right", NULL, "RLOUT"}
> > 
> > And this is how you should use it in a machine driver:
> > Connect the codec's outputs to TPA. The TPA code adds the HPs for you
> > already, so you don't need to care about it...
> 
> It's not unreasonable for a user to want to wire the device through to a
> board-defined jack.

Again, hard to argue with this ;)

> For example, they may find this helps with documenting which physical jack is
> used

Not that fatal, a /* what is this for */ on above the TPA and codec DAPM route 
connection might be enough.

> or they may have a jack that's got multiple output types on it such as a dock
> connector.

Can be done with the way it was (with the _HP in the tpa6130a2 driver)

> They may also wish to use the event that is available on the jack widget to
> perform some board specific action when the headphone is activated.

The event is _usually_ needed to toggle the power for the amp (enable/disable).
The power for the tpa is managed within the driver, so no additional enable is 
needed.

> There's no actual requirement to define headphone widgets if they don't
> have any other code associated with them - if the outputs are left
> unconnected the effect will be the same as if a noop and unused
> headphone widget were there.

That is true, but in that case why would anyone add the tpa amp to their DAPM 
route in the first place (when it is not connected)?

Anyways, I still think that the original driver was covering 99% of use cases 
for a Headphone amplifier like the tpa6130a2/tpa6140a2.
But it is true, the there might be some really exotic (brain dead?) HW 
implementation, where this would not fit...

-- 
Péter

Re: [PATCH] ASoC: tpa6130a2: Define output pins with SND_SOC_DAPM_OUTPUT

[Mark Brown]

On Thu, May 20, 2010 at 09:03:53AM +0300, Peter Ujfalusi wrote:
> On Wednesday 19 May 2010 18:05:12 ext Mark Brown wrote:

> I guess my reasoning was to avoid the following DAPM route, which for me seamed 
> weird:
> soc codec                        |      tap6130a2        |   machine driver
> codec_DAC -> .. -> codec_OUTPUT -> tpa_PGA -> tpa_OUTPUT -> machine_HP

> It seamed wrong to have codec_OUTPUT and tpa_OUTPUT in the same DAPM route.

That seems absolutely fine to me - it's something I'd expect to happen
as we move into a multi-chip ASoC world, you'll have signals which go
out of one chip and into another before going to the edge of the system.
The output wigdget is the output of one chip but there's no reason you
can't use it as an input for another chip.

> > They may also wish to use the event that is available on the jack widget to
> > perform some board specific action when the headphone is activated.

> The event is _usually_ needed to toggle the power for the amp (enable/disable).
> The power for the tpa is managed within the driver, so no additional enable is 
> needed.

Well, usually the event is completely unused and the headphone widget is
entirely ornamental.

> > There's no actual requirement to define headphone widgets if they don't
> > have any other code associated with them - if the outputs are left
> > unconnected the effect will be the same as if a noop and unused
> > headphone widget were there.

> That is true, but in that case why would anyone add the tpa amp to their DAPM 
> route in the first place (when it is not connected)?

They wouldn't, I'm just saying that having the extra headphone widget on
the end is not a requirement for anything to function.