[pulseaudio-discuss] [PATCHv3 1/4] alsa: Export alsa-mixer API

[David Henningsson]

Colin asked me to do a review of these patches, so here goes. Earlier 
I've mostly looked at making sure UCM does not destroy anything else, 
but this time I've dived a bit deeper into the implementation as well. I 
have yet to take the result for a test drive though, as I don't have any 
UCM configuration files for any hardware I have here (with some work and 
training I should be able to create it though, but I'm not there yet).

I hope it doesn't sound grumpsy or anything, it's just more easy to 
comment on the things that are wrong than those who are right. (I have 
to work on that, I guess!)

On 2011-05-10 22:29, Jorge Eduardo Candelaria wrote:
> From: Margarita Olaya Cabrera<magi at slimlogic.co.uk>
>
> The UCM core needs the alsa-mixer API to be exported, so that it can
> create its UCM mappings.

Nitpick - and this goes general for all patches: public functions should 
be prefixed with pa_ whereas private shouldn't, so when you move 
something from being private (i e static) to public you should add the 
pa_ prefix.

>
> Signed-off-by: Margarita Olaya Cabrera<magi at slimlogic.co.uk>
> Signed-off-by: Jorge Eduardo Candelaria<jedu at slimlogic.co.uk>
> ---
> src/modules/alsa/alsa-mixer.c |   10 +++++-----
> src/modules/alsa/alsa-mixer.h |   12 ++++++++++++
> 2 files changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/src/modules/alsa/alsa-mixer.c b/src/modules/alsa/alsa-mixer.c
> index 3eef5f9..07670f0 100644
> --- a/src/modules/alsa/alsa-mixer.c
> +++ b/src/modules/alsa/alsa-mixer.c
> @@ -2909,7 +2909,7 @@ void pa_alsa_profile_set_free(pa_alsa_profile_set *ps) {
>      pa_xfree(ps);
> }
>
> -static pa_alsa_mapping *mapping_get(pa_alsa_profile_set *ps, const char *name) {
> +pa_alsa_mapping *mapping_get(pa_alsa_profile_set *ps, const char *name) {
>      pa_alsa_mapping *m;
>
>      if (!pa_startswith(name, "Mapping "))
> @@ -3364,7 +3364,7 @@ fail:
>      return -1;
> }
>
> -static int mapping_verify(pa_alsa_mapping *m, const pa_channel_map *bonus) {
> +int mapping_verify(pa_alsa_mapping *m, const pa_channel_map *bonus) {
>
>      static const struct description_map well_known_descriptions[] = {
>          { "analog-mono",            N_("Analog Mono") },
> @@ -3437,7 +3437,7 @@ void pa_alsa_mapping_dump(pa_alsa_mapping *m) {
>                   m->direction);
> }
>
> -static void profile_set_add_auto_pair(
> +void profile_set_add_auto_pair(
>          pa_alsa_profile_set *ps,
>          pa_alsa_mapping *m, /* output */
>          pa_alsa_mapping *n  /* input */) {
> @@ -3485,7 +3485,7 @@ static void profile_set_add_auto_pair(
>      pa_hashmap_put(ps->profiles, p->name, p);
> }
>
> -static void profile_set_add_auto(pa_alsa_profile_set *ps) {
> +void profile_set_add_auto(pa_alsa_profile_set *ps) {
>      pa_alsa_mapping *m, *n;
>      void *m_state, *n_state;
>
> @@ -3502,7 +3502,7 @@ static void profile_set_add_auto(pa_alsa_profile_set *ps) {
>          profile_set_add_auto_pair(ps, NULL, n);
> }
>
> -static int profile_verify(pa_alsa_profile *p) {
> +int profile_verify(pa_alsa_profile *p) {
>
>      static const struct description_map well_known_descriptions[] = {
>          { "output:analog-mono+input:analog-mono",     N_("Analog Mono Duplex") },
> diff --git a/src/modules/alsa/alsa-mixer.h b/src/modules/alsa/alsa-mixer.h
> index c24a896..ad8e980 100644
> --- a/src/modules/alsa/alsa-mixer.h
> +++ b/src/modules/alsa/alsa-mixer.h
> @@ -220,6 +220,11 @@ int pa_alsa_path_set_mute(pa_alsa_path *path, snd_mixer_t *m, pa_bool_t muted);
> int pa_alsa_path_select(pa_alsa_path *p, snd_mixer_t *m);
> void pa_alsa_path_set_callback(pa_alsa_path *p, snd_mixer_t *m, snd_mixer_elem_callback_t cb, void *userdata);
> void pa_alsa_path_free(pa_alsa_path *p);
> +pa_alsa_mapping *mapping_get(pa_alsa_profile_set *ps, const char *name);
> +int mapping_verify(pa_alsa_mapping *m, const pa_channel_map *bonus);
> +void profile_set_add_auto(pa_alsa_profile_set *ps);
> +void profile_set_add_auto_pair(pa_alsa_profile_set *ps, pa_alsa_mapping *m, pa_alsa_mapping *n);
> +int profile_verify(pa_alsa_profile *p);
>
> pa_alsa_path_set *pa_alsa_path_set_new(pa_alsa_mapping *m, pa_alsa_direction_t direction);
> void pa_alsa_path_set_probe(pa_alsa_path_set *s, snd_mixer_t *m, pa_bool_t ignore_dB);
> @@ -227,6 +232,13 @@ void pa_alsa_path_set_dump(pa_alsa_path_set *s);
> void pa_alsa_path_set_set_callback(pa_alsa_path_set *ps, snd_mixer_t *m, snd_mixer_elem_callback_t cb, void *userdata);
> void pa_alsa_path_set_free(pa_alsa_path_set *s);
>
> +/* Data structure that UCM uses to extract alsa profile
> + * information.
> + */
> +struct profile_data {
> +    pa_alsa_profile *profile;
> +};
> +

The profile_data struct doesn't seem to be needed to be declared here. 
It is only used in ucm_set_profile, which could take a pa_alsa_profile* 
parameter instead of a profile_data* parameter.

> struct pa_alsa_mapping {
>      pa_alsa_profile_set *profile_set;
>


-- 
David Henningsson, Canonical Ltd.
http://launchpad.net/~diwic