Re: [pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
On Thu, May 26, 2011 at 01:07:33PM +0200, David Henningsson wrote:
On 2011-05-19 18:44, Jorge Eduardo Candelaria wrote:
+typedef enum pa_jack_event {
+PA_JACK_HEADPHONES,
+PA_JACK_HEADSET,
I'm a little hesitant to whether we should have one headset or whether
that should be emulated by one headphone and one microphone, as that
is what a headset is.
The kernel does this by having separate statuses for headphone and
microphone and reporting them both for headset.
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
I was asked by Colin to give this a quick review, so...
On 2011-05-19 18:44, Jorge Eduardo Candelaria wrote:
From: Margarita Olaya Cabreram...@slimlogic.co.uk
This patch adds support to module-alsa-card so that we can read jack insertion
and removal events using the input device subsystem. i.e. we can detect
headphone insertions and removals.
This patch adds a new module parameter called jack_id that contains the ID of
the jack input device associated with this sound card. If we receive a valid
jack_id during module init then we start a reader thread that will read the
jack input device and fire a hook on every removal and insertion event.
Jack support development was kindly sponsored by Wolfson Microelectronics PLC
Signed-off-by: Margarita Olaya Cabreram...@slimlogic.co.uk
Signed-off-by: Jorge Eduardo Candelariaj...@slimlogic.co.uk
---
src/modules/alsa/alsa-jack.h| 42
src/modules/alsa/module-alsa-card.c | 120 +++
src/pulsecore/core.h|2 +
3 files changed, 164 insertions(+), 0 deletions(-)
create mode 100644 src/modules/alsa/alsa-jack.h
diff --git a/src/modules/alsa/alsa-jack.h b/src/modules/alsa/alsa-jack.h
new file mode 100644
index 000..4fc67c6
--- /dev/null
+++ b/src/modules/alsa/alsa-jack.h
Along the lines of Tanu, if this is not alsa-specific, let's take this
header and move into pulsecore.
@@ -0,0 +1,42 @@
+#ifndef foopulsejackdetecthfoo
+#define foopulsejackdetecthfoo
+
+/***
+ This file is part of PulseAudio.
+
+ Copyright 2011 Wolfson Microelectronics PLC
+ Author Margarita Olayam...@slimlogic.co.uk
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as published
+ by the Free Software Foundation; either version 2.1 of the License,
+ or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with PulseAudio; if not, write to the Free Software
+ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+ USA.
+***/
+
+#includeinttypes.h
+
+typedef enum pa_jack_event {
+PA_JACK_HEADPHONES,
+PA_JACK_HEADSET,
I'm a little hesitant to whether we should have one headset or whether
that should be emulated by one headphone and one microphone, as that
is what a headset is.
+PA_JACK_MICROPHONE,
+PA_JACK_LINEOUT,
add PA_JACK_VIDEOOUT for HDMI/Displayport (which was just added to ALSA).
+PA_JACK_UNKNOWN,
+PA_JACK_MAX
+} pa_jack_event_t;
+
+typedef struct pa_jack_detect {
+pa_jack_event_t event;
+char *card;
Agree with Tanu about pa_card pointer here.
+} pa_jack_detect_t;
+
+#endif
diff --git a/src/modules/alsa/module-alsa-card.c
b/src/modules/alsa/module-alsa-card.c
index e60aa5e..75202fb 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -29,6 +29,9 @@
#includepulsecore/core-util.h
#includepulsecore/modargs.h
#includepulsecore/queue.h
+#includepulsecore/thread.h
+
+#includelinux/input.h
#includemodules/reserve-wrap.h
@@ -39,6 +42,7 @@
#include alsa-util.h
#include alsa-sink.h
#include alsa-source.h
+#include alsa-jack.h
#include module-alsa-card-symdef.h
PA_MODULE_AUTHOR(Lennart Poettering);
@@ -55,6 +59,7 @@ PA_MODULE_USAGE(
source_properties=properties for the source
namereg_fail=pa_namereg_register() fail parameter value
device_id=ALSA card index
+jack_id=Jack device index
Eh, a card can have more than one jack, and is further rather connected
to a port rather than a card, so this (and thus the rest of the
implementation) doesn't seem right to me.
--
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic
___
pulseaudio-discuss mailing list
pulseaudio-discuss@mail.0pointer.de
https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
Sorry for late reply... On Fri, 2011-05-20 at 09:14 -0500, Jorge Eduardo Candelaria wrote: +if (event-value) +pa_hook_fire(u-core-hooks[PA_CORE_HOOK_JACK_INSERT], jack); +else +pa_hook_fire(u-core-hooks[PA_CORE_HOOK_JACK_REMOVE], jack); Firing core hooks directly from modules looks a bit dirty to me. I'm actually not very familiar with hooks, however do we need to signal the jack insertion/removal events from jack_report(). What would be a more clean solution? Adding just a couple of trivial functions to the core would probably suffice: void pa_jack_inserted(pa_core *c, pa_jack_event *e); void pa_jack_removed(pa_core *c, pa_jack_event *e); Those functions would do nothing but call pa_hook_fire(), so this is a purely cosmetic thing... -- Tanu ___ pulseaudio-discuss mailing list pulseaudio-discuss@mail.0pointer.de https://tango.0pointer.de/mailman/listinfo/pulseaudio-discuss
Re: [pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
On May 19, 2011, at 3:11 PM, Tanu Kaskinen wrote:
I tried to apply the patch for easier review in meld, but unfortunately
it seems that some helpful software has eaten the first space on lines
that begin with one or more spaces. Because of that, git am doesn't
accept the patch.
I'll review this anyway, without meld.
Overall comments: there is some confusion about whether the jack stuff
is a core concept (hooks in pa_core) or an alsa specific thing (all jack
related type definitions are in alsa-jack.h). I think it probably makes
sense to keep alsa-jack.h as an alsa specific thing, but then the core
hooks shouldn't be tied to alsa-jack.h like they are now. In the longer
term I hope the thing how policy modules interact with jack detection
will be monitoring the (currently not existing) available status of the
port objects. However, if you don't want to start working on that now,
I'd just move alsa-jack.h under pulsecore (and of course rename it).
I think the best solution for now would be to move it to pulsecore.
The only major issue in my opinion is the jack_fd reading. It should be
made non-blocking.
--
Tanu
On Thu, 2011-05-19 at 11:44 -0500, Jorge Eduardo Candelaria wrote:
From: Margarita Olaya Cabrera m...@slimlogic.co.uk
This patch adds support to module-alsa-card so that we can read jack
insertion
and removal events using the input device subsystem. i.e. we can detect
headphone insertions and removals.
This patch adds a new module parameter called jack_id that contains the ID of
the jack input device associated with this sound card. If we receive a valid
jack_id during module init then we start a reader thread that will read the
jack input device and fire a hook on every removal and insertion event.
Jack support development was kindly sponsored by Wolfson Microelectronics PLC
Signed-off-by: Margarita Olaya Cabrera m...@slimlogic.co.uk
Signed-off-by: Jorge Eduardo Candelaria j...@slimlogic.co.uk
---
src/modules/alsa/alsa-jack.h| 42
src/modules/alsa/module-alsa-card.c | 120
+++
src/pulsecore/core.h|2 +
3 files changed, 164 insertions(+), 0 deletions(-)
create mode 100644 src/modules/alsa/alsa-jack.h
diff --git a/src/modules/alsa/alsa-jack.h b/src/modules/alsa/alsa-jack.h
new file mode 100644
index 000..4fc67c6
--- /dev/null
+++ b/src/modules/alsa/alsa-jack.h
@@ -0,0 +1,42 @@
+#ifndef foopulsejackdetecthfoo
+#define foopulsejackdetecthfoo
+
+/***
+ This file is part of PulseAudio.
+
+ Copyright 2011 Wolfson Microelectronics PLC
+ Author Margarita Olaya m...@slimlogic.co.uk
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as published
+ by the Free Software Foundation; either version 2.1 of the License,
+ or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with PulseAudio; if not, write to the Free Software
+ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+ USA.
+***/
+
+#include inttypes.h
+
+typedef enum pa_jack_event {
+PA_JACK_HEADPHONES,
+PA_JACK_HEADSET,
+PA_JACK_MICROPHONE,
+PA_JACK_LINEOUT,
+PA_JACK_UNKNOWN,
+PA_JACK_MAX
+} pa_jack_event_t;
+
+typedef struct pa_jack_detect {
+pa_jack_event_t event;
+char *card;
Instead of the card name, would it be better to store a pa_card pointer
here? It seems that you have the card object available when you
initialize this struct.
It seemed unnecessary since only the card name is being extracted from the
struct, but I get your point. I will change this.
+} pa_jack_detect_t;
Structs shouldn't have the _t suffix. (Enum typedefs should, so the
pa_jack_event_t name is correct.)
If this is alsa specific, like it seems to be, judging from the file
location and name, I recommend prefixing the types with pa_alsa_jack_,
because all other alsa stuff uses pa_alsa_ prefix too. Hmm... on the
other hand, this struct is passed to a core hook, so maybe these types
are not really alsa specific, and should be defined in a header under
src/pulsecore/?
Then these structs should defined the alsa-jack.h header, since it's going
to be moved to pulsecore.
Also, I think pa_jack_event would be a better name for what you've now
named pa_jack_detect, and the enum could be pa_jack_type_t.
Agreed, I will change this.
+#endif
diff --git a/src/modules/alsa/module-alsa-card.c
b/src/modules/alsa/module-alsa-card.c
index e60aa5e..75202fb 100644
--- a/src/modules/alsa/module-alsa-card.c
+++
Re: [pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
I tried to apply the patch for easier review in meld, but unfortunately
it seems that some helpful software has eaten the first space on lines
that begin with one or more spaces. Because of that, git am doesn't
accept the patch.
I'll review this anyway, without meld.
Overall comments: there is some confusion about whether the jack stuff
is a core concept (hooks in pa_core) or an alsa specific thing (all jack
related type definitions are in alsa-jack.h). I think it probably makes
sense to keep alsa-jack.h as an alsa specific thing, but then the core
hooks shouldn't be tied to alsa-jack.h like they are now. In the longer
term I hope the thing how policy modules interact with jack detection
will be monitoring the (currently not existing) available status of the
port objects. However, if you don't want to start working on that now,
I'd just move alsa-jack.h under pulsecore (and of course rename it).
The only major issue in my opinion is the jack_fd reading. It should be
made non-blocking.
--
Tanu
On Thu, 2011-05-19 at 11:44 -0500, Jorge Eduardo Candelaria wrote:
From: Margarita Olaya Cabrera m...@slimlogic.co.uk
This patch adds support to module-alsa-card so that we can read jack insertion
and removal events using the input device subsystem. i.e. we can detect
headphone insertions and removals.
This patch adds a new module parameter called jack_id that contains the ID of
the jack input device associated with this sound card. If we receive a valid
jack_id during module init then we start a reader thread that will read the
jack input device and fire a hook on every removal and insertion event.
Jack support development was kindly sponsored by Wolfson Microelectronics PLC
Signed-off-by: Margarita Olaya Cabrera m...@slimlogic.co.uk
Signed-off-by: Jorge Eduardo Candelaria j...@slimlogic.co.uk
---
src/modules/alsa/alsa-jack.h| 42
src/modules/alsa/module-alsa-card.c | 120 +++
src/pulsecore/core.h|2 +
3 files changed, 164 insertions(+), 0 deletions(-)
create mode 100644 src/modules/alsa/alsa-jack.h
diff --git a/src/modules/alsa/alsa-jack.h b/src/modules/alsa/alsa-jack.h
new file mode 100644
index 000..4fc67c6
--- /dev/null
+++ b/src/modules/alsa/alsa-jack.h
@@ -0,0 +1,42 @@
+#ifndef foopulsejackdetecthfoo
+#define foopulsejackdetecthfoo
+
+/***
+ This file is part of PulseAudio.
+
+ Copyright 2011 Wolfson Microelectronics PLC
+ Author Margarita Olaya m...@slimlogic.co.uk
+
+ PulseAudio is free software; you can redistribute it and/or modify
+ it under the terms of the GNU Lesser General Public License as published
+ by the Free Software Foundation; either version 2.1 of the License,
+ or (at your option) any later version.
+
+ PulseAudio is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public License
+ along with PulseAudio; if not, write to the Free Software
+ Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+ USA.
+***/
+
+#include inttypes.h
+
+typedef enum pa_jack_event {
+PA_JACK_HEADPHONES,
+PA_JACK_HEADSET,
+PA_JACK_MICROPHONE,
+PA_JACK_LINEOUT,
+PA_JACK_UNKNOWN,
+PA_JACK_MAX
+} pa_jack_event_t;
+
+typedef struct pa_jack_detect {
+pa_jack_event_t event;
+char *card;
Instead of the card name, would it be better to store a pa_card pointer
here? It seems that you have the card object available when you
initialize this struct.
+} pa_jack_detect_t;
Structs shouldn't have the _t suffix. (Enum typedefs should, so the
pa_jack_event_t name is correct.)
If this is alsa specific, like it seems to be, judging from the file
location and name, I recommend prefixing the types with pa_alsa_jack_,
because all other alsa stuff uses pa_alsa_ prefix too. Hmm... on the
other hand, this struct is passed to a core hook, so maybe these types
are not really alsa specific, and should be defined in a header under
src/pulsecore/?
Also, I think pa_jack_event would be a better name for what you've now
named pa_jack_detect, and the enum could be pa_jack_type_t.
+#endif
diff --git a/src/modules/alsa/module-alsa-card.c
b/src/modules/alsa/module-alsa-card.c
index e60aa5e..75202fb 100644
--- a/src/modules/alsa/module-alsa-card.c
+++ b/src/modules/alsa/module-alsa-card.c
@@ -29,6 +29,9 @@
#include pulsecore/core-util.h
#include pulsecore/modargs.h
#include pulsecore/queue.h
+#include pulsecore/thread.h
+
+#include linux/input.h
#include modules/reserve-wrap.h
@@ -39,6 +42,7 @@
#include alsa-util.h
#include alsa-sink.h
#include alsa-source.h
+#include alsa-jack.h
#include module-alsa-card-symdef.h
PA_MODULE_AUTHOR(Lennart Poettering);
@@ -55,6 +59,7 @@
