Re: sx1 mixer support

[Takashi Iwai <tiwai@suse.de>]

At Fri, 24 Nov 2006 12:14:45 +0100,
Pavel Machek wrote:
> 
> (I think this should go in through omap tree, because sx1 support in
> mainline is nonexistent for now, but...)

Yes, it's fine with me.

> --- /dev/null
> +++ b/sound/arm/omap/omap-alsa-sx1-mixer.c
(snip)
> +static int current_volume 		= 0;	/* current volume, we cant read it */
> +static int current_fm_volume 		= 0;	/* current FM radio volume, we cant read it */

Zero-initializations can be stripped.  (And lines seem still too long
:)

> +int set_mixer_volume(int mixer_vol)

Missing static?  Also there are many global functions around here.

> +{
> +	/* FIXME: Alsa has mixer_vol in 0-100 range, while SX1 needs 0-9 range */

An incorrect information.  ALSA native mixers can use any value ranges
while OSS mixers require 0-100 range.

> +/* ---------------------------------------------------------------------------------------- */
> +static int __pcm_playback_target_info(snd_kcontrol_t *kcontrol, snd_ctl_elem_info_t *uinfo)

What is the reason to use __ prefix?

Don't use snd_xxx_t typedefs any more.  They are obsolete (and will be
removed in 2.6.20 tree).

Also, the line is too long (ditto for other callbacks).

> +static int __pcm_playback_target_put(snd_kcontrol_t *kcontrol, snd_ctl_elem_value_t *ucontrol)
> +{
> +	int ret_val = 0;
> +	int cur_val = ucontrol->value.integer.value[0];
> +
> +	if ((cur_val >= 0) &&
> +	    (cur_val < PLAYBACK_TARGET_COUNT) &&
> +	    (cur_val != current_playback_target)) {
> +		if (cur_val == PLAYBACK_TARGET_LOUDSPEAKER) {
> +			set_record_source(REC_SRC_SINGLE_ENDED_MICIN_HED);
> +			set_loudspeaker_to_playback_target();
> + 		}
> +		else if (cur_val == PLAYBACK_TARGET_HEADPHONE) {

'}' ane else should be in the same line.

> +/*
> + * Alsa mixer interface function for getting the volume read from the SX1 in a
> + * 0-100 alsa mixer format.
> + */

I don't think this comment is correct...

> +static int __pcm_playback_volume_get(snd_kcontrol_t *kcontrol, snd_ctl_elem_value_t *ucontrol)
> +{
> +	ucontrol->value.integer.value[0] = current_volume;
> +	return 0;
> +}
> +
> +static int __pcm_playback_volume_put(snd_kcontrol_t *kcontrol, snd_ctl_elem_value_t *ucontrol)
> +{
> +	return set_mixer_volume(ucontrol->value.integer.value[0]);

The put callback is supposed to return the following values:

0 = the value isn't changed
1 = the value is changed
netagive value = error

The return of 1 is not strictly needed in practice, but it's better to
behave in a correct manner.


> --- /dev/null
> +++ b/sound/arm/omap/omap-alsa-sx1.c
(snip)
> +/* Send IPC message to sound server */
> +extern int cn_sx1snd_send(unsigned int cmd, unsigned int arg1, unsigned int arg2)
> +{
> +	struct cn_msg *m;
> +	unsigned short data[3];
> +	int err;
> +
> +	m = kzalloc(sizeof(*m) + sizeof(data), GFP_ATOMIC);

Better to use gfp_any() here, too?
(Or is it really too big for stack?)


> --- /dev/null
> +++ b/sound/arm/omap/omap-alsa-sx1.h
(snip)
> +/*
> + * Defines codec specific functions pointers that can be used from the
> + * common omap-alsa base driver for all omap codecs.
> + */
> +void egold_configure(void);
> +void egold_set_samplerate(long rate);
> +void egold_clock_setup(void);
> +int egold_clock_on(void);
> +int egold_clock_off(void);
> +int egold_get_default_samplerate(void);

Do they need to be exported?  I see them only in a single file as
callbacks...


Thanks,

Takashi