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

Mark Brown
2011-06-23T01:21:23Z

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().