On Mon 2011-01-17 15:32:11, Mark Brown wrote:
On Mon, Jan 17, 2011 at 02:48:54PM +0800, Zeng Zhaoming wrote:
v1 url: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-December/034615.ht...
Changes since v1:
- Add DAPM widges as Mark and Arnaud point out.
- cleanup regulator code
- new implement dai_ops functions
- sort configs in Kconfig and Makefile
From 1704fd5e7cf2540e247aa8a4f69d81ae5e6cbd4e Mon Sep 17 00:00:00 2001 From: Zeng Zhaoming zengzm.kernel@gmail.com Date: Mon, 17 Jan 2011 10:34:42 +0800
Please submit patches in the form documented in SubmittingPatches. In particular please don't send as an attachement, particularly not with extraneous stuff before the changelog.
+struct sgtl5000_priv {
int sysclk; /* sysclk rate */int master; /* i2s master or not? */int fmt; /* i2s data format */int lrclk; /* frame clock rate */int capture_channels; /* the num of channels for capture. */int small_pop; /* hw assistant to eliminate pop */It's not clear why you're storing some of this stuff in the private data
- for example, capture_channels is only referred to in the place where
it is set (so you may as well use the original value) and small_pop is set unconditionally (it looks like the intention was to have platform data for it?).
yes, you are right. lrclk and capture_channels fields can be remove. small_pop is a configurable option of platform data.
+static int mic_bias_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
/* change mic bias resistor to 4Kohm */snd_soc_write(w->codec, SGTL5000_CHIP_MIC_CTRL, SGTL5000_BIAS_R_4k);return 0;+}
This seems really odd - this does the write unconditionally regardless of event and never turns anything off. If this can't just be set once at startup then a comment explaining why an event is needed would be good.
the odd code is cuased by: If this is set to zero the micbias block is powered off, and 4Kohm is common case for enabled micbias. 0x0 = Powered off 0x1 = 2Kohm 0x2 = 4Kohm 0x3 = 8Kohm when turn on micbias, it will set to 2Kohm, so we need to change it to 4Kohm after widget powerup.
+static int dac_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
switch (event) {case SND_SOC_DAPM_PRE_PMU:snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP,SGTL5000_VAG_POWERUP|SGTL5000_DAC_POWERUP);break;case SND_SOC_DAPM_POST_PMD:snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,SGTL5000_DAC_POWERUP, 0);break;Why does the powerdown not disable VAG_POWERUP? My first thought was that VAG_POWERUP ought to be a supply widget...
My first version, VAG_POWERUP is a supply widget, but spec says: The headphone (and/or lineout) powerup should be set BEFORE clearing this bit.
But the powerdown sequence go against it. So I turn to powerup VAG with DAC, and powerdown it before HP and Line_out.
+static int adc_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
snd_soc_update_bits(w->codec, SGTL5000_CHIP_ANA_POWER,SGTL5000_ADC_POWERUP,SGTL5000_ADC_POWERUP);Again, this is really unclear.
ADC powerup be controlled by both CHIP_ANA_POWER and CHIP_DIG_POWER. It seems we need to set both of them.
+/* custom function to get of PCM playback volume */ +static int dac_get_volsw(struct snd_kcontrol *kcontrol,
struct snd_ctl_elem_value *ucontrol)+{
struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);int reg, l, r;reg = snd_soc_read(codec, SGTL5000_CHIP_DAC_VOL);l = (reg & SGTL5000_DAC_VOL_LEFT_MASK) >> SGTL5000_DAC_VOL_LEFT_SHIFT;r = (reg & SGTL5000_DAC_VOL_RIGHT_MASK) >> SGTL5000_DAC_VOL_RIGHT_SHIFT;l = l < 0x3c ? 0x3c : l;l = l > 0xfc ? 0xfc : l;r = r < 0x3c ? 0x3c : r;r = r > 0xfc ? 0xfc : r;/* revert it */l = 0xfc - l;r = 0xfc - r;This is really unclear. The lack of any comments and use of the ternery operator isn't helping... I'd suggest describing the register semantics in a comment.
Ok, I will add more useful comment for this. spec says: Set the Right channel DAC 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
It also looks like you're missing some locking.
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;Likewise.
/* we need SOC_DOUBLE_S8_TLV with invert */{.iface = SNDRV_CTL_ELEM_IFACE_MIXER,.name = "PCM Playback Volume",.access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |SNDRV_CTL_ELEM_ACCESS_READWRITE,.info = dac_info_volsw,.get = dac_get_volsw,.put = dac_put_volsw,},Perhaps implementing the abstract type would help with the clarity issues?
case SND_SOC_DAIFMT_CBM_CFS:case SND_SOC_DAIFMT_CBS_CFM:return -EINVAL;break;default:return -EINVAL;}Could just have the default: case.
if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {sgtl5000->capture_channels = channels;reg = snd_soc_read(codec, SGTL5000_CHIP_ANA_POWER);reg |= SGTL5000_ADC_POWERUP;if (sgtl5000->capture_channels == 1)reg &= ~SGTL5000_ADC_STEREO;elsereg |= SGTL5000_ADC_STEREO;snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, reg);}snd_soc_update_bits().
/* get devided factor of frame clock */switch (sys_fs / sgtl5000->lrclk) {case 4:clk_ctl |= SGTL5000_RATE_MODE_DIV_4 << SGTL5000_RATE_MODE_SHIFT;break;case 2:clk_ctl |= SGTL5000_RATE_MODE_DIV_2 << SGTL5000_RATE_MODE_SHIFT;break;default:break;}I'd expect to see this error out if it can't find an appropriate divider?
the default means sys_fs / lrclk = 1, not a error.