Re: [PATCH v3] ASoC: Add Freescale SGTL5000 codec support

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Feb 16, 2011 at 06:56:16AM +0800, zhaoming.zeng@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.