On Tue, Jul 15, 2014 at 09:41:33AM +0800, Sean Cross wrote:
> Add a codec driver for the Everest ES8328. It supports two separate audio
> outputs and two separate audio inputs.
This looks mostly fine so I've applied it, there are some issues below -
please send followups fixing them.
> + if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> + reg = ES8328_DACCONTROL2;
> +
> + else
> + reg = ES8328_ADCCONTROL5;
Coding style, extra blank line in the middle of the if ().
> + switch (params_rate(params)) {
> + case 8000:
> + val = es8328_sample_ratios[ES8328_RATE_8019];
> + break;
> + case 11025:
> + val = es8328_sample_ratios[ES8328_RATE_11025];
> + break;
> + case 22050:
> + val = es8328_sample_ratios[ES8328_RATE_22050];
> + break;
> + case 44100:
> + val = es8328_sample_ratios[ES8328_RATE_44100];
> + break;
> + default:
> + dev_err(codec->dev, "%s: unknown rate %d\n",
> + __func__, params_rate(params));
> + return -EINVAL;
> + }
The array serves no function given that you're never iterating over it
or anything - either have the array be a lookup table or just put the
constants in the switch statement directly. You're also calling the
array "_sample_ratios" but it actually appears to be sample rates.
> + snd_soc_write(codec, ES8328_MASTERMODE, ES8328_MASTERMODE_MSC);
> +
> + clk_rate = clk_get_rate(es8328->clk);
> + if (clk_rate == ES8328_SYSCLK_RATE_1X)
> + snd_soc_write(codec, ES8328_MASTERMODE,
> + ES8328_MASTERMODE_MSC);
> + else
> + snd_soc_write(codec, ES8328_MASTERMODE,
> + ES8328_MASTERMODE_MCLKDIV2 |
> + ES8328_MASTERMODE_MSC);
update_bits().
> + snd_soc_write(codec, ES8328_CHIPPOWER,
> + ES8328_CHIPPOWER_ADCDIG_OFF |
> + ES8328_CHIPPOWER_DACDIG_OFF |
> + ES8328_CHIPPOWER_ADCSTM_RESET |
> + ES8328_CHIPPOWER_DACSTM_RESET |
> + ES8328_CHIPPOWER_ADCPLL_OFF |
> + ES8328_CHIPPOWER_DACPLL_OFF |
> + ES8328_CHIPPOWER_ADCVREF_OFF |
> + ES8328_CHIPPOWER_DACVREF_OFF);
> + break;
This looks like most if not all of it should be in DAPM, especially the
DAC and ADC power - the device is wasting power on the ADC during
playback for example. Looking at the code I suspect that some of these
should be done as supply widgets.
> +static int es8328_suspend(struct snd_soc_codec *codec)
> +{
> + es8328_set_bias_level(codec, SND_SOC_BIAS_OFF);
> + return 0;
> +}
> +
> +static int es8328_resume(struct snd_soc_codec *codec)
> +{
> + struct regmap *regmap = dev_get_regmap(codec->dev, NULL);
> +
> + regcache_mark_dirty(regmap);
> + regcache_sync(regmap);
> +
> + es8328_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> + return 0;
> +}
But leave the regulators and clocks enabled?
Attachment:
signature.asc
Description: Digital signature
