[alsa-devel] [PATCH v3] ASoC: Add Freescale SGTL5000 codec support
Mark Brown
broonie at opensource.wolfsonmicro.com
Wed Feb 16 20:53:14 CET 2011
On Wed, Feb 16, 2011 at 06:56:16AM +0800, zhaoming.zeng at freescale.com wrote:
> +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_update_bits(w->codec, SGTL5000_CHIP_MIC_CTRL,
> + SGTL5000_BIAS_R_4k, SGTL5000_BIAS_R_4k);
> +
> + return 0;
> +}
I'd expect something to power off the mic bias when there's a power off
event.
> + 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;
My previous comments about the lebility of this still stand.
> +/* need SOC_DOUBLE_S8_TLV with invert */
> +#define SGTL5000_PCM_PLAYBACK_CONTROL \
> + { \
> + .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, \
> + }
Since there's no macro arguments this could just be defined directly in
line.
> + SOC_SINGLE("Capture Attenuate Switch (-6db)",
> + SGTL5000_CHIP_ANA_ADC_CTRL,
> + 8, 1, 0),
This should be a TLV control (-6 to 0 in steps of 6 dB) with Volume
rather than Switch.
> + /* set devided factor of frame clock */
> + switch (sys_fs / frame_rate) {
> + 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;
> + /*
> + * this implict case 1:
> + * because default: sys_fs = lrclk;
> + */
> + default:
> + clk_ctl |= SGTL5000_RATE_MODE_DIV_1 << SGTL5000_RATE_MODE_SHIFT;
> + break;
> + }
This would be much better off as case 1: with a default returning an
error in case the user has misclocked the device. For example, if the
ratio were to wind up as 8 then we'd accept that.
> + /*
> + * calculate the divider of mclk/sample_freq,
> + * factor of freq =96k can only be 256, since mclk in range (12m,27m)
> + */
> + if (sys_fs * 256 == sgtl5000->sysclk)
> + clk_ctl |= SGTL5000_MCLK_FREQ_256FS <<
> + SGTL5000_MCLK_FREQ_SHIFT;
> + else if (sys_fs * 384 == sgtl5000->sysclk)
> + clk_ctl |= SGTL5000_MCLK_FREQ_384FS <<
> + SGTL5000_MCLK_FREQ_SHIFT;
> + else if (sys_fs * 512 == sgtl5000->sysclk)
> + clk_ctl |= SGTL5000_MCLK_FREQ_512FS <<
> + SGTL5000_MCLK_FREQ_SHIFT;
This'd be clearer as a switch on sysfs / sysclk with this:
> + else {
> + /* if mclk not satisify the divider, using pll */
> + if (sgtl5000->master)
> + clk_ctl |= SGTL5000_MCLK_FREQ_PLL <<
> + SGTL5000_MCLK_FREQ_SHIFT;
> + else {
> + pr_err("%s: PLL not supported in slave mode\n",
> + __func__);
> + return -EINVAL;
> + }
as the default.
> + } else
> + /* power down pll */
> + snd_soc_update_bits(codec, SGTL5000_CHIP_ANA_POWER,
> + SGTL5000_PLL_POWERUP | SGTL5000_VCOAMP_POWERUP,
> + 0);
Use { } for the else case if the main case uses them.
> + case SND_SOC_BIAS_STANDBY:
> + if (codec->bias_level == SND_SOC_BIAS_OFF) {
> + for (i = 0; i < SGTL5000_SUPPLY_NUM; i++) {
> + if (!sgtl5000->supplies[i])
> + continue;
> + regulator_enable(sgtl5000->supplies[i]);
Why are the supplies optional?
> + vdda = regulator_get_voltage(sgtl5000->supplies[VDDA]) / 1000;
> + vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO]) / 1000;
You're not checking these for errors.
> + if (sgtl5000->supplies[VDDD])
> + vddd = regulator_get_voltage(sgtl5000->supplies[VDDD]) / 1000;
> + else
> + vddd = 0;
If VDD is optional it'd seem easier to substitute in a fixed voltage
regulator when it's not detected. It'd make the code much more
idiomatic and save having to special case missing regulators everywhere.
> + if (!vddd) {
> + /* set VDDD to 1.2v */
> + lreg_ctrl |= 0x8 << SGTL5000_LINREG_VDDD_SHIFT;
> + /* power up internal linear regulator */
> + ana_pwr |= SGTL5000_LINEREG_D_POWERUP |
> + SGTL5000_LINREG_SIMPLE_POWERUP |
> + SGTL5000_STARTUP_POWERUP;
> + }
Or even implement an actual regulator driver for it.
> + /* set default volume of adc */
> + reg = snd_soc_read(codec, SGTL5000_CHIP_ANA_ADC_CTRL);
> + reg &= ~SGTL5000_ADC_VOL_M6DB;
> + reg &= ~(SGTL5000_ADC_VOL_LEFT_MASK | SGTL5000_ADC_VOL_RIGHT_MASK);
> + reg |= (0xf << SGTL5000_ADC_VOL_LEFT_SHIFT)
> + | (0xf << SGTL5000_ADC_VOL_RIGHT_SHIFT);
> + snd_soc_write(codec, SGTL5000_CHIP_ANA_ADC_CTRL, reg);
Leave this up to userspace.
More information about the Alsa-devel
mailing list