From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933874AbWKXLlz (ORCPT ); Fri, 24 Nov 2006 06:41:55 -0500 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S933879AbWKXLlz (ORCPT ); Fri, 24 Nov 2006 06:41:55 -0500 Received: from ns1.suse.de ([195.135.220.2]:3800 "EHLO mx1.suse.de") by vger.kernel.org with ESMTP id S933874AbWKXLly (ORCPT ); Fri, 24 Nov 2006 06:41:54 -0500 Date: Fri, 24 Nov 2006 12:41:48 +0100 Message-ID: From: Takashi Iwai To: Pavel Machek Cc: Tony Lindgren , kernel list , Vladimir Ananiev , linux-omap Subject: Re: sx1 mixer support In-Reply-To: <20061124111445.GA5940@elf.ucw.cz> References: <20061124111445.GA5940@elf.ucw.cz> User-Agent: Wanderlust/2.12.0 (Your Wildest Dreams) SEMI/1.14.6 (Maruoka) FLIM/1.14.7 (=?ISO-8859-4?Q?Sanj=F2?=) APEL/10.6 MULE XEmacs/21.5 (beta27) (fiddleheads) (+CVS-20060704) (i386-suse-linux) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org 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