Messages in this thread | | | Date | Thu, 23 Jun 2011 02:21:19 +0100 | From | Mark Brown <> | Subject | Re: [PATCH 1/4] ASoC: Add ADAV80x codec driver |
| |
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().
| |