On Thu, 2011-05-19 at 11:44 -0500, Jorge Eduardo Candelaria wrote:
> From: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
>
> This patch adds support for udev detection of sound card jack event devices.
>
> When a new card is detected, the code now also checks for the presence of any
> associated input device via udev, and report the device to module-alsa-card
> via its jack_id parameter.
>
> Jack support development was kindly sponsored by Wolfson Microelectronics PLC
>
> Signed-off-by: Margarita Olaya Cabrera <magi at slimlogic.co.uk>
> Signed-off-by: Jorge Eduardo Candelaria <jedu at slimlogic.co.uk>
> ---
> src/modules/module-udev-detect.c | 68 ++++++++++++++++++++++++++++++++++---
> 1 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/src/modules/module-udev-detect.c b/src/modules/module-udev-detect.c
> index 63ad195..4321b32 100644
> --- a/src/modules/module-udev-detect.c
> +++ b/src/modules/module-udev-detect.c
> @@ -71,6 +71,8 @@ struct userdata {
>
> int inotify_fd;
> pa_io_event *inotify_io;
> +
> + char *card_name;
This doesn't really seem to be used for anything.
> };
>
> static const char* const valid_modargs[] = {
> @@ -163,6 +165,21 @@ static pa_bool_t pcm_is_modem(const char *card_idx, const char *pcm) {
> return is_modem;
> }
>
> +static pa_bool_t input_node_belongs_to_device(
> + struct device *d,
> + const char *node) {
> +
> + char *cd;
> + pa_bool_t b;
> +
> + cd = pa_sprintf_malloc("/sys%s", d->path);
> +
> + b = pa_startswith(node, cd);
> + pa_xfree(cd);
> +
> + return b;
> +}
> +
> static pa_bool_t is_card_busy(const char *id) {
> char *card_path = NULL, *pcm_path = NULL, *sub_status = NULL;
> DIR *card_dir = NULL, *pcm_dir = NULL;
> @@ -352,11 +369,26 @@ static void verify_access(struct userdata *u, struct device *d) {
> }
> }
>
> +static const char *jack_get_input_id(const char *path) {
> + int i;
> +
> + for( i = 0; i < strlen(path); i++) {
strlen() is called in each iteration - it would be better to save its
return value outside the loop.
> + if (pa_startswith(path + i, "/input"))
> + return path + i + 6;
> + }
> +
> + return NULL;
> +}
> +
> static void card_changed(struct userdata *u, struct udev_device *dev) {
> struct device *d;
> + struct udev_enumerate *enumerate;
> + struct udev_list_entry *item, *first;
> const char *path;
> const char *t;
> char *n;
> + char *jack_path;
> + char *jack_input;
>
> pa_assert(u);
> pa_assert(dev);
> @@ -383,6 +415,28 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
>
> n = pa_namereg_make_valid_name(t);
> d->card_name = pa_sprintf_malloc("alsa_card.%s", n);
> +
> + if (!(enumerate = udev_enumerate_new(u->udev)))
> + pa_log("Failed to initialize udev enumerator");
> + else {
> + if (!udev_enumerate_add_match_subsystem(enumerate, "input"))
> + pa_log_debug("Failed to match to subsystem input");
> +
> + if (udev_enumerate_scan_devices(enumerate))
> + pa_log_debug("Failed to scan for devices");
Should these two errors abort the enumeration, or is a debug message
really good enough error handling here?
> +
> + first = udev_enumerate_get_list_entry(enumerate);
> + udev_list_entry_foreach(item, first) {
> + jack_path = udev_list_entry_get_name(item);
> + if (input_node_belongs_to_device(d, jack_path)) {
> + jack_input = pa_xstrdup(jack_get_input_id(jack_path));
> + pa_log_debug("jack is %s\n", jack_input);
jack_input can, at least in theory, be null (or if it can't, you should
add an assertion to jack_get_input_id() instead of returning NULL). I'm
not sure what happens if you pass a null pointer as a string to the log
functions - does it crash, or does it work? There's the pa_strnull()
function that returns the string "(null)" if the argument is null, and
the function is being used in similar contexts as this one. Either you
should call pa_strnull() here, or the other uses of pa_strnull() are
redundant.
> + break;
> + }
> + }
> + udev_enumerate_unref(enumerate);
> + }
> +
> d->args = pa_sprintf_malloc("device_id=\"%s\" "
> "name=\"%s\" "
> "card_name=\"%s\" "
> @@ -390,15 +444,19 @@ static void card_changed(struct userdata *u, struct udev_device *dev) {
> "tsched=%s "
> "ignore_dB=%s "
> "sync_volume=%s "
> + "jack_id=\"%s\" "
> "card_properties=\"module-udev-detect.discovered=1\"",
> path_get_card_id(path),
> n,
> d->card_name,
> pa_yes_no(u->use_tsched),
> pa_yes_no(u->ignore_dB),
> - pa_yes_no(u->sync_volume));
> - pa_xfree(n);
> + pa_yes_no(u->sync_volume),
> + jack_input);
The comment about pa_strnull() applies here too.
>
> + pa_xfree(n);
> + pa_xfree(jack_input);
> + u->card_name = d->card_name;
> pa_hashmap_put(u->devices, d->path, d);
>
> verify_access(u, d);
> @@ -481,13 +539,11 @@ static void monitor_cb(
> goto fail;
> }
>
> - if (!path_get_card_id(udev_device_get_devpath(dev))) {
> + if (path_get_card_id(udev_device_get_devpath(dev))) {
> + process_device(u, dev);
> udev_device_unref(dev);
> - return;
> }
>
> - process_device(u, dev);
> - udev_device_unref(dev);
If path_get_card_id() returns zero, dev doesn't get unreffed. Is this
intentional - did you fix a bug, or introduce one?
> return;
>
> fail:
--
Tanu
