On Wed, 2013-09-18 at 16:01 +0200, David Henningsson wrote:
> This is for headphone/headset only, so far. It currently works for
> Android switches, but should be easy to convert to extcon when there
> is a need, hence the name.
>
> Meanwhile, if you're the 99% that does not run an Android kernel with
> this switch, this patch should be harmless - it would just notice there
> is no switch and don't do anything else.
> ---
> src/Makefile.am | 1 +
> src/modules/alsa/alsa-extcon.c | 272 +++++++++++++++++++++++++++++++++++
> src/modules/alsa/alsa-extcon.h | 33 +++++
> src/modules/alsa/alsa-ucm.c | 4 +-
> src/modules/alsa/alsa-ucm.h | 1 +
> src/modules/alsa/module-alsa-card.c | 5 +
> 6 files changed, 314 insertions(+), 2 deletions(-)
> create mode 100644 src/modules/alsa/alsa-extcon.c
> create mode 100644 src/modules/alsa/alsa-extcon.h
>
> diff --git a/src/Makefile.am b/src/Makefile.am
> index 8392953..7caa1db 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -1729,6 +1729,7 @@ module_coreaudio_device_la_LIBADD = $(MODULE_LIBADD)
> libalsa_util_la_SOURCES = \
> modules/alsa/alsa-util.c modules/alsa/alsa-util.h \
> modules/alsa/alsa-ucm.c modules/alsa/alsa-ucm.h \
> + modules/alsa/alsa-extcon.c modules/alsa/alsa-extcon.h \
Apparently the code depends on UCM and udev. I think these files should
be added to SOURCES only if both UCM and udev are available. The two
calls in alsa-card.c need to be then protected with #ifdefs too.
Another alternative is to do the ifdeffery in alsa-extcon.h and
alsa-extcon.c only, similar to how alsa-ucm.h and .c do it. I'd be fine
with that too.
> modules/alsa/alsa-mixer.c modules/alsa/alsa-mixer.h \
> modules/alsa/alsa-sink.c modules/alsa/alsa-sink.h \
> modules/alsa/alsa-source.c modules/alsa/alsa-source.h \
> diff --git a/src/modules/alsa/alsa-extcon.c b/src/modules/alsa/alsa-extcon.c
> new file mode 100644
> index 0000000..fb21fdb
> --- /dev/null
> +++ b/src/modules/alsa/alsa-extcon.c
> @@ -0,0 +1,272 @@
> +/***
> + This file is part of PulseAudio.
> +
> + Copyright 2013 David Henningsson, Canonical Ltd.
> +
> + 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.
> +***/
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#include <pulsecore/core-util.h>
> +#include <pulsecore/device-port.h>
> +#include <pulsecore/i18n.h>
> +#include <libudev.h>
> +
> +#include "alsa-util.h"
> +#include "alsa-extcon.h"
> +
> +/* IFDEF HAVE_UCM ? */
> +#include <use-case.h>
> +#include "alsa-ucm.h"
> +/* ENDIF */
> +
> +/* For android */
> +#define EXTCON_NAME "switch"
> +/* For extcon */
> +/* #define EXTCON_NAME "extcon" */
> +
> +static pa_available_t hp_avail(int state)
> +{
> + return ((state & 3) != 0) ? PA_AVAILABLE_YES : PA_AVAILABLE_NO;
Some commenting would be nice: what do the first and second bits mean
that you check here?
> +}
> +
> +static pa_available_t hsmic_avail(int state)
> +{
> + return (state & 1) ? PA_AVAILABLE_YES : PA_AVAILABLE_NO;
> +}
> +
> +struct android_switch {
> + char *name;
> + uint32_t current_value;
I suggest storing the decoded pa_available_t value here instead of the
raw integer.
> +};
> +
> +static void android_switch_free(struct android_switch *as) {
> + if (!as)
> + return;
Style: don't accept NULL in free().
> + pa_xfree(as->name);
> + pa_xfree(as);
> +}
> +
> +static struct android_switch *android_switch_new(const char *name) {
> +
> + struct android_switch *as = NULL;
> + char *filename = pa_sprintf_malloc("/sys/class/%s/%s/state", EXTCON_NAME, name);
> + char *state = pa_read_line_from_file(filename);
> +
> + if (state == NULL) {
> + pa_log_debug("Cannot open '%s'. Skipping.", filename);
> + pa_xfree(filename);
> + return NULL;
> + }
> + pa_xfree(filename);
> +
> + as = pa_xnew0(struct android_switch, 1);
> + as->name = pa_xstrdup(name);
> +
> + if (pa_atou(state, &as->current_value) < 0) {
> + pa_log_warn("Switch '%s' has invalid value '%s'", name, state);
> + pa_xfree(state);
> + android_switch_free(as);
> + return NULL;
> + }
> +
> + return as;
> +}
> +
> +struct udev_data {
> + struct udev *udev;
> + struct udev_monitor *monitor;
> + pa_io_event *event;
> +};
Minor bike-shedding, feel free to ignore:
The type "struct udev_data" is only used as a member of pa_alsa_extcon,
so having a stand-alone type is unnecessary. I think it one of these
would be neater:
struct pa_alsa_extcon {
pa_card *card;
struct android_switch *h2w;
struct {
struct udev *udev;
struct udev_monitor *monitor;
pa_io_event *event;
} udev;
};
or
struct pa_alsa_extcon {
pa_card *card;
struct android_switch *h2w;
struct udev *udev;
struct udev_monitor *udev_monitor;
pa_io_event *udev_event;
};
> +
> +struct pa_alsa_extcon {
> + pa_card *card;
> + struct android_switch *h2w;
> + struct udev_data udev;
> +};
> +
> +static struct android_switch *find_matching_switch(pa_alsa_extcon *u,
> + const char *devpath) {
> +
> + if (pa_streq(devpath, "/devices/virtual/" EXTCON_NAME "/h2w"))
> + return u->h2w; /* To be extended if we ever support more switches */
> + return NULL;
> +}
> +
> +static void notify_ports(pa_alsa_extcon *u, struct android_switch *as) {
> +
> + pa_device_port *p;
> + void *state;
> +
> + pa_assert(as == u->h2w); /* To be extended if we ever support more switches */
> +
> + pa_log_debug("Value of switch %s is now %d.", as->name, as->current_value);
> +
> + PA_HASHMAP_FOREACH(p, u->card->ports, state) {
> + if (p->direction == PA_DIRECTION_OUTPUT) {
> + if (!strcmp(p->name, "analog-output-headphones"))
Use pa_streq().
> + pa_device_port_set_available(p, hp_avail(as->current_value));
> +/* IFDEF HAVE_UCM ? */
> + else if (pa_alsa_ucm_port_contains(p->name, SND_USE_CASE_DEV_HEADSET, true) ||
> + pa_alsa_ucm_port_contains(p->name, SND_USE_CASE_DEV_HEADPHONES, true))
pa_alsa_ucm_port_contains() assumes that the port name follows the
format that alsa-ucm.c uses when generating port names. I think it would
be good to check if the card is actually using UCM before calling
pa_alsa_ucm_port_contains(). This is not a big issue in practice (the
only "problem" is accidental match in a non-UCM port name), but still
I'd prefer not feeding "garbage" to pa_alsa_ucm_port_contains().
> + pa_device_port_set_available(p, hp_avail(as->current_value));
> +/* ENDIF */
> + }
> + else if (p->direction == PA_DIRECTION_INPUT) {
> + if (!strcmp(p->name, "analog-input-headset-mic"))
Use pa_streq().
> + pa_device_port_set_available(p, hsmic_avail(as->current_value));
> +/* IFDEF HAVE_UCM ? */
> + else if (pa_alsa_ucm_port_contains(p->name, SND_USE_CASE_DEV_HEADSET, false))
> + pa_device_port_set_available(p, hsmic_avail(as->current_value));
> +/* ENDIF */
> + }
> + }
> +}
> +
> +static void udev_cb(pa_mainloop_api *a, pa_io_event *e, int fd,
> + pa_io_event_flags_t events, void *userdata) {
> +
> + pa_alsa_extcon *u = userdata;
> + struct udev_device *d = udev_monitor_receive_device(u->udev.monitor);
> + struct udev_list_entry *entry;
> + struct android_switch *as;
> + const char *devpath, *state;
> +
> + if (!d) {
> + pa_log("udev_monitor_receive_device failed.");
> + pa_assert(a);
> + a->io_free(u->udev.event);
> + u->udev.event = NULL;
> + return;
> + }
> +
> + devpath = udev_device_get_devpath(d);
> + if (!devpath) {
> + pa_log("udev_device_get_devpath failed.");
> + goto out;
> + }
> + pa_log_debug("Got uevent with devpath=%s", devpath);
> +
> + as = find_matching_switch(u, devpath);
> + if (!as)
> + goto out;
> +
> + entry = udev_list_entry_get_by_name(
> + udev_device_get_properties_list_entry(d), "SWITCH_STATE");
> + if (!entry) {
> + pa_log("udev_list_entry_get_by_name failed to find 'SWITCH_STATE' entry.");
> + goto out;
> + }
> +
> + state = udev_list_entry_get_value(entry);
> + if (!state) {
> + pa_log("udev_list_entry_get_by_name failed.");
Copy-paste error in log message.
> + goto out;
> + }
> +
> + if (pa_atou(state, &as->current_value) < 0) {
> + pa_log_warn("Switch '%s' has invalid value '%s'", as->name, state);
> + goto out;
> + }
> +
> + notify_ports(u, as);
> +
> +out:
> + udev_device_unref(d);
> +}
> +
> +static bool init_udev(pa_alsa_extcon *u, pa_core *core) {
Use int for success/failure reporting.
> +
> + int fd;
> +
> + u->udev.udev = udev_new();
> + if (!u->udev.udev) {
> + pa_log("udev_new failed.");
> + return false;
> + }
> +
> + u->udev.monitor = udev_monitor_new_from_netlink(u->udev.udev, "udev");
> + if (!u->udev.monitor) {
> + pa_log("udev_monitor_new_from_netlink failed.");
> + return false;
> + }
> +/* FIXME: Research why this filter did not work */
Do you plan to do the research?
--
Tanu
