[alsa-devel] [PATCH v2] ASoC: Add FreeScale SGTL5000 codec suppor

[Mark Brown]

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.html
> 
> 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 at 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?).

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

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

> +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_DAC_POWERUP,
> +		SGTL5000_DAC_POWERUP);
> +

Again, this is really unclear.

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

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;
> +		else
> +			reg |= 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?

> +	if ((clk_ctl & SGTL5000_MCLK_FREQ_MASK) == SGTL5000_MCLK_FREQ_PLL) {
> +		snd_soc_write(codec, SGTL5000_CHIP_PLL_CTRL, pll_ctl);

This looks very odd - why has pll_ctrl been precomputed if it's only
going to be used here?

> +/*
> + * set dac bias
> + * common state changes:
> + * startup:
> + * off --> standby --> prepare --> on
> + * standby --> prepare --> on
> + *
> + * stop:
> + * on --> prepare --> standby
> + */
> +static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
> +				   enum snd_soc_bias_level level)
> +{
> +	codec->bias_level = level;
> +	return 0;
> +}

May as well remove this, it does nothing.

> +#define SGTL5000_FORMATS (SNDRV_PCM_FMTBIT_S16_LE |\
> +			SNDRV_PCM_FMTBIT_S20_3LE |\
> +			SNDRV_PCM_FMTBIT_S24_LE)

Your hw_params() claimed 32 bit support too.

> +	/* 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;
> +	}

Your resume code might get a bit clearer if you were able to use the
standard cache sync functionality that the generic register cache code
has.

> +	/* get and enable all regulators */
> +	for (i = 0; i < SGTL5000_SUPPLY_NUM; i++) {
> +		struct regulator *reg;
> +		reg = regulator_get(codec->dev, supply_names[i]);
> +
> +		if (IS_ERR(reg))
> +			continue;
> +
> +		regulator_enable(reg);
> +		sgtl5000->supplies[i] = reg;
> +	}
> +
> +	/* vdda and vddio regulator must configed */
> +	if (!sgtl5000->supplies[VDDA] || !sgtl5000->supplies[VDDIO]) {
> +		dev_err(codec->dev,
> +			"Not set vdda or vddio regulator correctly\n");
> +		/* FIXME, no such platform regulator configed */
> +		return -ENODEV;
> +	}

This looks rather wrong; you're not clearing up any regulators you did
get, you're not logging any errors if you fail to get regulators and
it's *very* unusual to see any supplies being optional.  You certainly
need comments here.

> +	vdda  = regulator_get_voltage(sgtl5000->supplies[VDDA]) / 1000;
> +	vddio = regulator_get_voltage(sgtl5000->supplies[VDDIO]) / 1000;
> +
> +	if (regulator_get_voltage(sgtl5000->supplies[VDDD]))
> +		vddd = regulator_get_voltage(sgtl5000->supplies[VDDD]) / 1000;
> +	else
> +		vddd = 0;
> +
> +	/* workaround for rev 0x11: use vddd linear regulator */
> +	if (!vddd || (rev >= 0x11)) {
> +		/* set VDDD to 1.2v */
> +		lreg_ctrl |= 0x8 << SGTL5000_LINREG_VDDD_SHIFT;
> +		/* power internal linear regulator */
> +		ana_pwr |= SGTL5000_LINEREG_D_POWERUP;
> +	} else {
> +		/* turn of startup power */
> +		ana_pwr &= ~SGTL5000_STARTUP_POWERUP;
> +		ana_pwr &= ~SGTL5000_LINREG_SIMPLE_POWERUP;
> +	}

If you have an internal LDO that can optinally be used it'd probably be
better to have it visible via the regulator API.  Also, if the internal
regulator is in use surely an external supply should be left off?

> +	/* set ADC/DAC ref voltage to vdda / 2 */
> +	vag = vdda / 2;
> +	if (vag <= SGTL5000_ANA_GND_BASE)
> +		vag = 0;
> +	else if (vag >= SGTL5000_ANA_GND_BASE + SGTL5000_ANA_GND_STP *
> +		 (SGTL5000_ANA_GND_MASK >> SGTL5000_ANA_GND_SHIFT))
> +		vag = SGTL5000_ANA_GND_MASK >> SGTL5000_ANA_GND_SHIFT;
> +	else
> +		vag = (vag - SGTL5000_ANA_GND_BASE) / SGTL5000_ANA_GND_STP;
> +
> +	/* set line out ref voltage to vddio / 2 */
> +	vag = vddio / 2;

The above block of code calculates a value for vag but does nothing with
it before it's overwritten here...

> +	if (vag <= SGTL5000_LINE_OUT_GND_BASE)
> +		vag = 0;
> +	else if (vag >= SGTL5000_LINE_OUT_GND_BASE + SGTL5000_LINE_OUT_GND_STP *
> +		 SGTL5000_LINE_OUT_GND_MAX)
> +		vag = SGTL5000_LINE_OUT_GND_MAX;
> +	else
> +		vag = (vag - SGTL5000_LINE_OUT_GND_BASE) /
> +		    SGTL5000_LINE_OUT_GND_STP;

...then there's no immediate use of this calculation either, instead we
start doing...

> +
> +	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL, lreg_ctrl);
> +	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, ana_pwr);
> +	msleep(10);
> +

...this.  It'd most likely be *much* clearer if the calculations and
register writes were kept together.

> +static const struct i2c_device_id sgtl5000_id[] = {
> +	{"sgtl5000-codec", 0},
> +	{},
> +};

Remove the -codec, it's not at all idiomatic.

> +	.driver = {
> +		   .name = "sgtl5000-codec",
> +		   .owner = THIS_MODULE,

Please remove it here too.