From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnaud Patard (Rtp) Subject: Re: [PATCH v3] ASoC: Add Freescale SGTL5000 codec support Date: Thu, 17 Feb 2011 09:52:52 +0100 Message-ID: <87aahvoxej.fsf@lechat.rtp-net.org> References: <1297810576-2575-1-git-send-email-zhaoming.zeng@freescale.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from lechat.rtp-net.org (lechat.rtp-net.org [88.191.19.38]) by alsa0.perex.cz (Postfix) with ESMTP id 62CBA246B4 for ; Thu, 17 Feb 2011 09:52:23 +0100 (CET) In-Reply-To: <1297810576-2575-1-git-send-email-zhaoming.zeng@freescale.com> (zhaoming zeng's message of "Wed, 16 Feb 2011 06:56:16 +0800") List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: zhaoming.zeng@freescale.com Cc: alsa-devel@alsa-project.org, s.hauer@pengutronix.de, broonie@opensource.wolfsonmicro.com, zengzm.kernel@gmail.com, lrg@slimlogic.co.uk, linuxzsc@gmail.com List-Id: alsa-devel@alsa-project.org writes: Hi, > From: Zeng Zhaoming > > Add Freescale SGTL5000 codec support > > Signed-off-by: Zeng Zhaoming > --- > changes since v2: > 1. clean up register default values > 2. rewrite codec power up code, add sgtl5000_set_power_regs() > 3. rewrite codec clock configure code sgtl5000_set_clock() > 4. reimplement PM hooks, restore register by particular order. > 5. clean up dapm code, remove dac and adc event hooks. > 6. clean up codec private structure, remove unnecessary fields. > 7. add comments for uncommon code. > > Thanks for Mark's review. [...] > +/* > + * custom put function for PCM playback volume > + * PCM volume with 0.5017 dB steps from 0 to -90 dB > + * 0x3B and less = Reserved > + * 0x3C = 0 dB > + * 0x3D = -0.5 dB > + * 0xF0 = -90 dB > + * 0xFC and greater = Muted > + */ > +static int dac_put_volsw(struct snd_kcontrol *kcontrol, > + struct snd_ctl_elem_value *ucontrol) > +{ > + struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol); > + int reg, l, r; > + > + l = ucontrol->value.integer.value[0]; > + r = ucontrol->value.integer.value[1]; > + > + l = l < 0 ? 0 : l; > + l = l > 0xfc - 0x3c ? 0xfc - 0x3c : l; > + r = r < 0 ? 0 : r; > + r = r > 0xfc - 0x3c ? 0xfc - 0x3c : r; > + l = 0xfc - l; > + r = 0xfc - r; > + > + reg = l << SGTL5000_DAC_VOL_LEFT_SHIFT | > + r << SGTL5000_DAC_VOL_RIGHT_SHIFT; > + > + snd_soc_update_bits_locked(codec, SGTL5000_CHIP_DAC_VOL, reg, reg); Why are you using update_bits_locked and not snd_soc_write ? Here snd_soc_update_bits_locked is just good at making the PCM playback mixer acting weird (I even got to a point that with a 0 value in the mixer, I was unable to raise the volume). snd_soc_write just works. [...] > + > +static int sgtl5000_probe(struct snd_soc_codec *codec) > +{ > + struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec); > + u16 reg; > + int ret; > + int rev; > + int i; > + > + /* setup i2c data ops */ > + ret = snd_soc_codec_set_cache_io(codec, 16, 16, SND_SOC_I2C); > + if (ret < 0) { > + dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret); > + return ret; > + } > + > + /* read chip information */ > + reg = snd_soc_read(codec, SGTL5000_CHIP_ID); I've a problem with doing that here. You do assume that the regulators are enabled at this point which is not always the case. This results on a timeout on i2c and then reg contains 0. Please enable regultors firsts And no, I don't see having the regulators for things like vddio set to "always on" in the machine file as a good idea. If the driver is not loaded, having the codec off is good for power consumption. Arnaud