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
