Re: [PATCH v8 2/3] ASoC: add es8328 codec driver

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?