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

[Lars-Peter Clausen <lars@metafoo.de>]

On 06/23/2011 03:21 AM, Mark Brown wrote:
> 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.

Done in patch 3 of this series, but I can merge it into this one.

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

Since the codec supports 32k, 44.1k, 48k, 64k, 48.2k and 96k this will select
the closest 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.

That's exactly what's it doing. Switching to an external clock and powering the
PLL down. Or what do you mean?

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

I had it there first, but removed it since it would mean doing the same work twice.

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

The register contents is not lost unless we'd cut external power, but I could
add restoring for completeness.