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?