Re: [PATCH 1/4] ASoC: Add ADAV80x codec driver

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

On Wed, Jun 22, 2011 at 11:09:54PM +0200, Lars-Peter Clausen wrote:

> diff --git a/sound/soc/codecs/adav80x.c b/sound/soc/codecs/adav80x.c
> new file mode 100644
> index 0000000..d2f3d08

This needs adding to MAINTAINERS.

> +static const struct soc_enum adav80x_mux_enum[] = {
> +	ADAV80X_MUX_ENUM(ADAV80X_DPATH_CTRL1, 0),
> +	ADAV80X_MUX_ENUM(ADAV80X_DPATH_CTRL1, 3),
> +	ADAV80X_MUX_ENUM(ADAV80X_DPATH_CTRL2, 3),
> +};

No, don't use arrays...

> +static const struct snd_kcontrol_new adav80x_mux_ctrl[] = {
> +	SOC_DAPM_VALUE_ENUM("Route", adav80x_mux_enum[0]),
> +	SOC_DAPM_VALUE_ENUM("Route", adav80x_mux_enum[1]),
> +	SOC_DAPM_VALUE_ENUM("Route", adav80x_mux_enum[2]),
> +};

...since referencing them is so error prone...

> +#define ADAV80X_MUX(name, num) \
> +	SND_SOC_DAPM_VALUE_MUX(name, SND_SOC_NOPM, 0, 0, &adav80x_mux_ctrl[num])

...and illegible due to the magic numbers.

> +	if (adav80x->deemph) {
> +		switch (adav80x->rate) {
> +		case 0:
> +			val = ADAV80X_DAC_CTRL2_DEEMPH_NONE;
> +			break;
> +		case 32000:
> +			val = ADAV80X_DAC_CTRL2_DEEMPH_32;
> +			break;
> +		case 44100:
> +			val = ADAV80X_DAC_CTRL2_DEEMPH_44;
> +			break;
> +		case 48000:
> +		default:
> +			val = ADAV80X_DAC_CTRL2_DEEMPH_48;
> +			break;

Really?  I'd have expected a check for the closest matching rate (which
would get 32k for most low rates) or a requirement for an exact match.

> +	if (freq_out) {

> +	} else	{
> +		if (adav80x->clk_src == new_src)
> +			return 0;
> +
> +		adav80x->clk_src = new_src;
> +
> +		if (new_src == ADAV80X_CLK_XIN) {
> +			/* DAC, ADC, ICLK clock source - XIN */
> +			snd_soc_write(codec, ADAV80X_ICLK_CTRL1, 0x00);
> +			snd_soc_write(codec, ADAV80X_ICLK_CTRL2, 0x00);
> +		} else {
> +			/* DAC, ADC, ICLK clock source - MCLKI */
> +			snd_soc_write(codec, ADAV80X_ICLK_CTRL1, 0x25);
> +			snd_soc_write(codec, ADAV80X_ICLK_CTRL2, 0x01);
> +		}
> +
> +		pll_ctrl1 |= ADAV80X_PLL_CTRL1_PLL1PD;
> +		snd_soc_write(codec, ADAV80X_PLL_CTRL1, pll_ctrl1);

What's this doing?  Setting the PLL output to zero means stop the PLL.

> +/* Enforce the same sample rate on all audio interfaces */
> +static int adav80x_dai_startup(struct snd_pcm_substream *substream,
> +	struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_codec *codec = dai->codec;
> +	struct adav80x *adav80x = snd_soc_codec_get_drvdata(codec);
> +
> +	if (!codec->active || !adav80x->rate)
> +		return 0;
> +
> +	return snd_pcm_hw_constraint_minmax(substream->runtime,
> +			SNDRV_PCM_HW_PARAM_RATE, adav80x->rate, adav80x->rate);
> +}

This means playback and capture should always run at the same rate so...

> +static struct snd_soc_dai_driver adav80x_dais[] = {
> +	{

...the DAI should flag symmetric_rates, even if only for completeness.

> +static int adav80x_resume(struct snd_soc_codec *codec)
> +{
> +	return adav80x_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +}

This doesn't appear to restore the register cache, nor does
set_bias_level().