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

[Arnaud Patard (Rtp) <arnaud.patard@rtp-net.org>]

<zhaoming.zeng@freescale.com> writes:

Hi,


> From: Zeng Zhaoming <zhaoming.zeng@freescale.com>
>
> Add Freescale SGTL5000 codec support
>
> Signed-off-by: Zeng Zhaoming <zhaoming.zeng@freescale.com>
> ---
> changes since v2:
> 1. clean up register default values
> 2. rewrite codec power up code, add sgtl5000_set_power_regs()
> 3. rewrite codec clock configure code sgtl5000_set_clock()
> 4. reimplement PM hooks, restore register by particular order.
> 5. clean up dapm code, remove dac and adc event hooks.
> 6. clean up codec private structure, remove unnecessary fields.
> 7. add comments for uncommon code.
>
> Thanks for Mark's review.

[...]

> +/*
> + * custom put function for PCM playback volume
> + * PCM 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
> + */
> +static int dac_put_volsw(struct snd_kcontrol *kcontrol,
> +			 struct snd_ctl_elem_value *ucontrol)
> +{
> +	struct snd_soc_codec *codec = snd_kcontrol_chip(kcontrol);
> +	int reg, l, r;
> +
> +	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;
> +
> +	reg = l << SGTL5000_DAC_VOL_LEFT_SHIFT |
> +	    r << SGTL5000_DAC_VOL_RIGHT_SHIFT;
> +
> +	snd_soc_update_bits_locked(codec, SGTL5000_CHIP_DAC_VOL, reg, reg);

Why are you using update_bits_locked and not snd_soc_write ?
Here snd_soc_update_bits_locked is just good at making the PCM playback
mixer acting weird (I even got to a point that with a 0 value in the
mixer, I was unable to raise the volume). snd_soc_write just works.

[...]

> +
> +static int sgtl5000_probe(struct snd_soc_codec *codec)
> +{
> +	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
> +	u16 reg;
> +	int ret;
> +	int rev;
> +	int i;
> +
> +	/* 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;
> +	}
> +
> +	/* read chip information */
> +	reg = snd_soc_read(codec, SGTL5000_CHIP_ID);

I've a problem with doing that here. You do assume that the regulators
are enabled at this point which is not always the case. This results on
a timeout on i2c and then reg contains 0. Please enable regultors firsts
And no, I don't see having the regulators for things like vddio set to
"always on" in the machine file as a good idea. If the driver is not
loaded, having the codec off is good for power consumption.

Arnaud