[PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[David Henningsson]

Signed-off-by: David Henningsson <david.henningsson@canonical.com>
---

Hi Takashi,

A patch for your jack branch - it seems you forgot to add kctls for conexant?
Also it would be nice to see this merged, soon it might be too late for 3.3?

 sound/pci/hda/patch_conexant.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
index ae9c028..99942e6 100644
--- a/sound/pci/hda/patch_conexant.c
+++ b/sound/pci/hda/patch_conexant.c
@@ -4325,6 +4325,7 @@ static int cx_auto_build_input_controls(struct hda_codec *codec)
 
 static int cx_auto_build_controls(struct hda_codec *codec)
 {
+	struct conexant_spec *spec = codec->spec;
 	int err;
 
 	err = cx_auto_build_output_controls(codec);
@@ -4333,6 +4334,9 @@ static int cx_auto_build_controls(struct hda_codec *codec)
 	err = cx_auto_build_input_controls(codec);
 	if (err < 0)
 		return err;
+	err = snd_hda_jack_add_kctls(codec, &spec->autocfg);
+	if (err < 0)
+		return err;
 	return conexant_build_controls(codec);
 }
 
-- 
1.7.5.4

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Takashi Iwai]

At Tue, 20 Dec 2011 15:23:05 +0100,
David Henningsson wrote:
> 
> Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> ---
> 
> Hi Takashi,
> 
> A patch for your jack branch - it seems you forgot to add kctls for conexant?

Obviously yes.  Applied now.

> Also it would be nice to see this merged, soon it might be too late for 3.3?

OK, let's merge it.  The things are still local to hda-intel, so we
can fix pending issues and rewrite with a more generic framework later
for unified jack-handling stuff.


thanks,

Takashi

>  sound/pci/hda/patch_conexant.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> index ae9c028..99942e6 100644
> --- a/sound/pci/hda/patch_conexant.c
> +++ b/sound/pci/hda/patch_conexant.c
> @@ -4325,6 +4325,7 @@ static int cx_auto_build_input_controls(struct hda_codec *codec)
>  
>  static int cx_auto_build_controls(struct hda_codec *codec)
>  {
> +	struct conexant_spec *spec = codec->spec;
>  	int err;
>  
>  	err = cx_auto_build_output_controls(codec);
> @@ -4333,6 +4334,9 @@ static int cx_auto_build_controls(struct hda_codec *codec)
>  	err = cx_auto_build_input_controls(codec);
>  	if (err < 0)
>  		return err;
> +	err = snd_hda_jack_add_kctls(codec, &spec->autocfg);
> +	if (err < 0)
> +		return err;
>  	return conexant_build_controls(codec);
>  }
>  
> -- 
> 1.7.5.4
> 

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Takashi Iwai]

At Tue, 20 Dec 2011 15:32:21 +0100,
Takashi Iwai wrote:
> 
> At Tue, 20 Dec 2011 15:23:05 +0100,
> David Henningsson wrote:
> > 
> > Signed-off-by: David Henningsson <david.henningsson@canonical.com>
> > ---
> > 
> > Hi Takashi,
> > 
> > A patch for your jack branch - it seems you forgot to add kctls for conexant?
> 
> Obviously yes.  Applied now.

Ah, I forgot that this was fixed in the branch in sound-unstable tree
but I didn't merge back to sound tree.  Sorry for confusion.

Now all merged and pushed out.


Takashi


> > Also it would be nice to see this merged, soon it might be too late for 3.3?
> 
> OK, let's merge it.  The things are still local to hda-intel, so we
> can fix pending issues and rewrite with a more generic framework later
> for unified jack-handling stuff.
> 
> 
> thanks,
> 
> Takashi
> 
> >  sound/pci/hda/patch_conexant.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/sound/pci/hda/patch_conexant.c b/sound/pci/hda/patch_conexant.c
> > index ae9c028..99942e6 100644
> > --- a/sound/pci/hda/patch_conexant.c
> > +++ b/sound/pci/hda/patch_conexant.c
> > @@ -4325,6 +4325,7 @@ static int cx_auto_build_input_controls(struct hda_codec *codec)
> >  
> >  static int cx_auto_build_controls(struct hda_codec *codec)
> >  {
> > +	struct conexant_spec *spec = codec->spec;
> >  	int err;
> >  
> >  	err = cx_auto_build_output_controls(codec);
> > @@ -4333,6 +4334,9 @@ static int cx_auto_build_controls(struct hda_codec *codec)
> >  	err = cx_auto_build_input_controls(codec);
> >  	if (err < 0)
> >  		return err;
> > +	err = snd_hda_jack_add_kctls(codec, &spec->autocfg);
> > +	if (err < 0)
> > +		return err;
> >  	return conexant_build_controls(codec);
> >  }
> >  
> > -- 
> > 1.7.5.4
> > 

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Mark Brown]

On Tue, Dec 20, 2011 at 03:32:21PM +0100, Takashi Iwai wrote:
> David Henningsson wrote:

> > Also it would be nice to see this merged, soon it might be too late for 3.3?

> OK, let's merge it.  The things are still local to hda-intel, so we
> can fix pending issues and rewrite with a more generic framework later
> for unified jack-handling stuff.

I was rather expecting that the patches would be posted to the list for
review at some point (though I may have missed them as not having
generic support is really disappointing).  We should at least verify
that the userspace interface isn't going to cause problems for non-HDA
systems.

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Takashi Iwai]

At Tue, 20 Dec 2011 23:58:13 +0000,
Mark Brown wrote:
> 
> On Tue, Dec 20, 2011 at 03:32:21PM +0100, Takashi Iwai wrote:
> > David Henningsson wrote:
> 
> > > Also it would be nice to see this merged, soon it might be too late for 3.3?
> 
> > OK, let's merge it.  The things are still local to hda-intel, so we
> > can fix pending issues and rewrite with a more generic framework later
> > for unified jack-handling stuff.
> 
> I was rather expecting that the patches would be posted to the list for
> review at some point (though I may have missed them as not having
> generic support is really disappointing).

Err, sorry, I thought I have posted some in RFC, but wasn't there.
The essential patch for the common helper is attached below.

>  We should at least verify
> that the userspace interface isn't going to cause problems for non-HDA
> systems.

Well, the kctl-jack part itself can't break anything right now since
it's used only in HD-audio.  If others will use, this will be a pure
addition, so it won't break except for possibly different ctl numids.

The question toward the integration with the input-jack stuff is still
open.  In the HD-audio case, input-jack functions are merged into the
kctl-jack helpers.  But for others, it doesn't have to be so.

One remaining obstacle for this integration is that the input-jack
provides multiple key bits per jack entry while this doesn't exist in
the kctl notification.  With kctl, the notifier says just that the 
value was changed.  Then the user-space can query the current value
(not necessarily a boolean but can be an integer or else), but the
value isn't passed in the notification itself.  So, it's a different
design. 


thanks,

Takashi

---
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: Introduce common helper functions for jack-detection
 control

Now move the helper function for creating and reporting the jack-detection
to the common place.  The driver that needs this functionality should
select CONFIG_SND_KCTL_JACK kconfig.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
diff --git a/include/sound/control.h b/include/sound/control.h
index 1a94a21..b2796e8 100644
--- a/include/sound/control.h
+++ b/include/sound/control.h
@@ -227,4 +227,12 @@ snd_ctl_add_slave_uncached(struct snd_kcontrol *master,
 	return _snd_ctl_add_slave(master, slave, SND_CTL_SLAVE_NEED_UPDATE);
 }
 
+/*
+ * Helper functions for jack-detection controls
+ */
+struct snd_kcontrol *
+snd_kctl_jack_new(const char *name, int idx, void *private_data);
+void snd_kctl_jack_report(struct snd_card *card,
+			  struct snd_kcontrol *kctl, bool status);
+
 #endif	/* __SOUND_CONTROL_H */
diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index 475455c..66f287f 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -207,6 +207,9 @@ config SND_PCM_XRUN_DEBUG
 config SND_VMASTER
 	bool
 
+config SND_KCTL_JACK
+	bool
+
 config SND_DMA_SGBUF
 	def_bool y
 	depends on X86
diff --git a/sound/core/Makefile b/sound/core/Makefile
index 350a08d..b4637c3 100644
--- a/sound/core/Makefile
+++ b/sound/core/Makefile
@@ -7,6 +7,7 @@ snd-y     := sound.o init.o memory.o info.o control.o misc.o device.o
 snd-$(CONFIG_ISA_DMA_API) += isadma.o
 snd-$(CONFIG_SND_OSSEMUL) += sound_oss.o info_oss.o
 snd-$(CONFIG_SND_VMASTER) += vmaster.o
+snd-$(CONFIG_SND_KCTL_JACK) += ctljack.o
 snd-$(CONFIG_SND_JACK)	  += jack.o
 
 snd-pcm-objs := pcm.o pcm_native.o pcm_lib.o pcm_timer.o pcm_misc.o \
diff --git a/sound/core/ctljack.c b/sound/core/ctljack.c
new file mode 100644
index 0000000..af0e78a
--- /dev/null
+++ b/sound/core/ctljack.c
@@ -0,0 +1,55 @@
+/*
+ * Helper functions for jack-detection kcontrols
+ *
+ * Copyright (c) 2011 Takashi Iwai <tiwai@suse.de>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+#include <linux/kernel.h>
+#include <sound/core.h>
+#include <sound/control.h>
+
+#define jack_detect_kctl_info	snd_ctl_boolean_mono_info
+
+static int jack_detect_kctl_get(struct snd_kcontrol *kcontrol,
+				struct snd_ctl_elem_value *ucontrol)
+{
+	ucontrol->value.integer.value[0] = kcontrol->private_value;
+	return 0;
+}
+
+static struct snd_kcontrol_new jack_detect_kctl = {
+	/* name is filled later */
+	.iface = SNDRV_CTL_ELEM_IFACE_CARD,
+	.access = SNDRV_CTL_ELEM_ACCESS_READ,
+	.info = jack_detect_kctl_info,
+	.get = jack_detect_kctl_get,
+};
+
+struct snd_kcontrol *
+snd_kctl_jack_new(const char *name, int idx, void *private_data)
+{
+	struct snd_kcontrol *kctl;
+	kctl = snd_ctl_new1(&jack_detect_kctl, private_data);
+	if (!kctl)
+		return NULL;
+	snprintf(kctl->id.name, sizeof(kctl->id.name), "%s Jack", name);
+	kctl->id.index = idx;
+	kctl->private_value = 0;
+	return kctl;
+}
+EXPORT_SYMBOL_GPL(snd_kctl_jack_new);
+
+void snd_kctl_jack_report(struct snd_card *card,
+			  struct snd_kcontrol *kctl, bool status)
+{
+	if (kctl->private_value == status)
+		return;
+	kctl->private_value = status;
+	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
+}
+EXPORT_SYMBOL_GPL(snd_kctl_jack_report);

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Mark Brown]

On Wed, Dec 21, 2011 at 03:42:17PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > I was rather expecting that the patches would be posted to the list for
> > review at some point (though I may have missed them as not having
> > generic support is really disappointing).

> Err, sorry, I thought I have posted some in RFC, but wasn't there.
> The essential patch for the common helper is attached below.

Thanks.  It'll be tomorrow at the earliest before I can read it properly.

> >  We should at least verify
> > that the userspace interface isn't going to cause problems for non-HDA
> > systems.

> Well, the kctl-jack part itself can't break anything right now since
> it's used only in HD-audio.  If others will use, this will be a pure
> addition, so it won't break except for possibly different ctl numids.

I'm more worried about the userspace interface than the in kernel one,
we can rewrite the kernel if it sucks but we're stuck with userspace.

> The question toward the integration with the input-jack stuff is still
> open.  In the HD-audio case, input-jack functions are merged into the
> kctl-jack helpers.  But for others, it doesn't have to be so.

I really don't see any reason why you'd want to have to worry about the
specific mechanisms in drivers (and there's also the extcon/switch stuff
that's looking likely to hit soon).  Indeed the extcon stuff might make
this needless anyway.

> One remaining obstacle for this integration is that the input-jack
> provides multiple key bits per jack entry while this doesn't exist in
> the kctl notification.  With kctl, the notifier says just that the 
> value was changed.  Then the user-space can query the current value
> (not necessarily a boolean but can be an integer or else), but the
> value isn't passed in the notification itself.  So, it's a different
> design. 

The detail in the notification seems completely trivial to deal with -
the input layer also provides similar readback support, it just happens
to have a more verbose report (and it needs the cache for readback
anyway as it also uses it to eliminate noop reports).

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Mark Brown]

On Wed, Dec 21, 2011 at 03:42:17PM +0100, Takashi Iwai wrote:

> Well, the kctl-jack part itself can't break anything right now since
> it's used only in HD-audio.  If others will use, this will be a pure
> addition, so it won't break except for possibly different ctl numids.

A pure addition can still cause problems as we roll out the same
interface into other drivers, for example if there's assumptions that
aren't generally true.

> +void snd_kctl_jack_report(struct snd_card *card,
> +			  struct snd_kcontrol *kctl, bool status)
> +{
> +	if (kctl->private_value == status)
> +		return;
> +	kctl->private_value = status;
> +	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
> +}
> +EXPORT_SYMBOL_GPL(snd_kctl_jack_report);

So, this looks good in so far as it goes but obviously there's no
semantics being defined here for the controls or their names.  That's
more the bit that worries me as we'll get applications starting to use
the new interface and they should have something consistent to work
with.

I'm guessing that as this is just a simple boolean each jack will have a
series of controls like "Headset Jack Headphone" and "Headset Jack
Microphone" or whatever and userspace should match them all together in
the same way that it does for Volume and Switch controls?  That sounds
like it'll work well, we just need to define some strings for standard
jacks and conections.

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Takashi Iwai]

At Thu, 22 Dec 2011 12:21:55 +0000,
Mark Brown wrote:
> 
> On Wed, Dec 21, 2011 at 03:42:17PM +0100, Takashi Iwai wrote:
> 
> > Well, the kctl-jack part itself can't break anything right now since
> > it's used only in HD-audio.  If others will use, this will be a pure
> > addition, so it won't break except for possibly different ctl numids.
> 
> A pure addition can still cause problems as we roll out the same
> interface into other drivers, for example if there's assumptions that
> aren't generally true.

In later future, yes.  But I was talking about 3.3 kernel.

> > +void snd_kctl_jack_report(struct snd_card *card,
> > +			  struct snd_kcontrol *kctl, bool status)
> > +{
> > +	if (kctl->private_value == status)
> > +		return;
> > +	kctl->private_value = status;
> > +	snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_VALUE, &kctl->id);
> > +}
> > +EXPORT_SYMBOL_GPL(snd_kctl_jack_report);
> 
> So, this looks good in so far as it goes but obviously there's no
> semantics being defined here for the controls or their names.  That's
> more the bit that worries me as we'll get applications starting to use
> the new interface and they should have something consistent to work
> with.

Right.  We'll need a closer look back with the realistic use-cases.

> I'm guessing that as this is just a simple boolean each jack will have a
> series of controls like "Headset Jack Headphone" and "Headset Jack
> Microphone" or whatever and userspace should match them all together in
> the same way that it does for Volume and Switch controls?

My current assumption is so, too.  OTOH, if a single jack must provide
several values inevitably, it may take an integer or an enum, in
theory.  But I think (and hope) this wouldn't happen.

>  That sounds
> like it'll work well, we just need to define some strings for standard
> jacks and conections.

Yeah.  Unfortunately this standardization isn't always trivial,
especially when multiple jacks are present with the same type...

My idea is to give some association between this jack kctl and another
kctl, in a simple way like TLV.  But it's likely a new year's dream.


thanks,

Takashi

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Mark Brown]

On Thu, Dec 22, 2011 at 02:55:57PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > A pure addition can still cause problems as we roll out the same
> > interface into other drivers, for example if there's assumptions that
> > aren't generally true.

> In later future, yes.  But I was talking about 3.3 kernel.

Once we've got a userspace interface we're pretty much stuck with it...

> > I'm guessing that as this is just a simple boolean each jack will have a
> > series of controls like "Headset Jack Headphone" and "Headset Jack
> > Microphone" or whatever and userspace should match them all together in
> > the same way that it does for Volume and Switch controls?

> My current assumption is so, too.  OTOH, if a single jack must provide
> several values inevitably, it may take an integer or an enum, in
> theory.  But I think (and hope) this wouldn't happen.

Supporting multiple objects on a single jack is a very basic requirement
in order to support headsets (which exist on PCs as well, the MacBooks
for example) - we can usually distinguish between at least headset and
headphone.  I think if we're going to do this via a single multi-state
control rather than a series of booleans it'd need to be an enum for
usability ("an object of type 2!"), Android uses integers and it's
pretty miserable for usability.

> >  That sounds
> > like it'll work well, we just need to define some strings for standard
> > jacks and conections.

> Yeah.  Unfortunately this standardization isn't always trivial,
> especially when multiple jacks are present with the same type...

I was thinking more about the things you might detect rather than the
jacks, that's a much more tractable problem.

> My idea is to give some association between this jack kctl and another
> kctl, in a simple way like TLV.  But it's likely a new year's dream.

Associating the jacks with the audio routes is definitely a separate
problem.

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Takashi Iwai]

At Thu, 22 Dec 2011 14:12:51 +0000,
Mark Brown wrote:
> 
> On Thu, Dec 22, 2011 at 02:55:57PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > A pure addition can still cause problems as we roll out the same
> > > interface into other drivers, for example if there's assumptions that
> > > aren't generally true.
> 
> > In later future, yes.  But I was talking about 3.3 kernel.
> 
> Once we've got a userspace interface we're pretty much stuck with it...

Yes, but this is no new interface.  It uses the existing control API.

> > > I'm guessing that as this is just a simple boolean each jack will have a
> > > series of controls like "Headset Jack Headphone" and "Headset Jack
> > > Microphone" or whatever and userspace should match them all together in
> > > the same way that it does for Volume and Switch controls?
> 
> > My current assumption is so, too.  OTOH, if a single jack must provide
> > several values inevitably, it may take an integer or an enum, in
> > theory.  But I think (and hope) this wouldn't happen.
> 
> Supporting multiple objects on a single jack is a very basic requirement
> in order to support headsets (which exist on PCs as well, the MacBooks
> for example) - we can usually distinguish between at least headset and
> headphone.

Interesting.  This can be a single jack connected to multiple pins, so
the codec sees two individual detection points.  So, if we want to
expose pins, it'd become different from the jack representation.
Hmm...

>  I think if we're going to do this via a single multi-state
> control rather than a series of booleans it'd need to be an enum for
> usability ("an object of type 2!"), Android uses integers and it's
> pretty miserable for usability.

I see, good to know.


Takashi

> > >  That sounds
> > > like it'll work well, we just need to define some strings for standard
> > > jacks and conections.
> 
> > Yeah.  Unfortunately this standardization isn't always trivial,
> > especially when multiple jacks are present with the same type...
> 
> I was thinking more about the things you might detect rather than the
> jacks, that's a much more tractable problem.
> 
> > My idea is to give some association between this jack kctl and another
> > kctl, in a simple way like TLV.  But it's likely a new year's dream.
> 
> Associating the jacks with the audio routes is definitely a separate
> problem.

Re: [PATCH] ALSA: hda/jack - Also add jack kctls for Conexant codecs

[Mark Brown]

On Thu, Dec 22, 2011 at 03:19:40PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > Once we've got a userspace interface we're pretty much stuck with it...

> Yes, but this is no new interface.  It uses the existing control API.

In terms of system calls it's not new but once you start saying "jacks
will appear as controls called X with type Y" or whatever that's also an
interface built on top of the raw interface which applications can start
relying on.  I'd call the entire ALSA control naming standard an
interface for example.

> > Supporting multiple objects on a single jack is a very basic requirement
> > in order to support headsets (which exist on PCs as well, the MacBooks
> > for example) - we can usually distinguish between at least headset and
> > headphone.

> Interesting.  This can be a single jack connected to multiple pins, so
> the codec sees two individual detection points.  So, if we want to
> expose pins, it'd become different from the jack representation.
> Hmm...

At the hardware level this is often a single detection point with
multiple values - a common technique for distinguishing a headphone is
that it's a microphone that's had the button shorted ever since it was
plugged in.  With all the different techniques for this stuff you end up
with a N:M mapping between jacks and detection methods and the same
again for things reported.