[alsa-devel] [PATCH v3] ASoC: Add Freescale SGTL5000 codec support

[Zeng Zhaoming]

On Wed 2011-02-16 19:53:14, Mark Brown wrote:
> 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.
>
I will add it in the next version.

> > +	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.
>
Sorry, I don't catch you means quite well, you means more comments to make it clearer?

> > +/* 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.
>
you means add it to kcontrol array directly instead of define a macro for it?

> > +	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.
>
Yes, it is clearer. thanks for the suggestion.

> > +	} 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.
>
Ok, I will try to add a fixed regulator if VDDD not provided externally.

> > +	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.

Thanks.