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 +++
[pulseaudio-discuss] [PATCH 1/2] alsa: add jack detection support
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; +} 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 @@ #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 @@ 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 format=sample format rate=sample rate fragments=number of fragments @@ -90,6 +95,7 @@ static const char* const valid_modargs[] = { ignore_dB, sync_volume, profile_set, +jack_id, NULL }; @@ -106,6 +112,14 @@ struct userdata { pa_modargs *modargs; pa_alsa_profile_set *profile_set; + + /*userdata needed for jack detection */ +int jack_fd; +char *jack_id; +pa_thread *thread; +pa_thread_mq thread_mq; +pa_rtpoll *rtpoll; +pa_rtpoll_item *rtpoll_item; }; struct profile_data { @@ -284,11 +298,89 @@ static void set_card_name(pa_card_new_data *data, pa_modargs *ma, const char *de pa_xfree(t); } +/* Jack detection API */ +static void jack_report(struct userdata *u, struct input_event *event) +{ +pa_jack_detect_t jack; + +jack.card = u-card-name ; + +pa_log_debug(jack: card %s event type %x code %x value %x, u-card-name, event-type, event-code, event-value); + +/* only process switch events */ +if (event-type != EV_SW) { +pa_log_debug(jack: card %s ignored event type %x, u-card-name, event-type); +return; +} + +switch (event-code) { +case SW_HEADPHONE_INSERT: +jack.event = PA_JACK_HEADPHONES; +break; +case SW_MICROPHONE_INSERT: +jack.event = PA_JACK_MICROPHONE; +break; +case SW_LINEOUT_INSERT: +jack.event = PA_JACK_LINEOUT; +break; +case SW_JACK_PHYSICAL_INSERT: +jack.event = PA_JACK_UNKNOWN; +break; +default: +pa_log_debug(jack: card %s ignored event code %x, u-card-name, event-code); +break; +} + +if (event-value) +pa_hook_fire(u-core-hooks[PA_CORE_HOOK_JACK_INSERT],
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 @@