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

On Wed, Jun 18, 2014 at 11:47:20AM +0800, Sean Cross wrote:

+Required properties:
+
+  - compatible : "es8328"

This needs the prefix, as does the example.

+static int es8328_i2c_probe(struct i2c_client *i2c,
+			    const struct i2c_device_id *id)
+{
+	struct regmap_config config;
+
+	config = es8328_regmap_config;
+	return es8328_probe(&i2c->dev, devm_regmap_init_i2c(i2c, &config));
+}

config appears to serve no purpose here.

+/* Run the codec at 22.5792 MHz to support these rates */
+enum es8328_rate {
+	ES8328_RATE_8019,
+	ES8328_RATE_11025,
+	ES8328_RATE_22050,
+	ES8328_RATE_44100,
+};

The driver should be enforcing constraints based on the clock it's got
then.  Not everything does though so it's a bit optional.

+static int es8328_put_deemph(struct snd_kcontrol *kcontrol,
+			     struct snd_ctl_elem_value *ucontrol)
+{
+	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
+	struct es8328_priv *es8328 = snd_soc_codec_get_drvdata(codec);
+	int deemph = ucontrol->value.enumerated.item[0];
+
+	if (deemph > 1)
+		return -EINVAL;
+
+	es8328->deemph = deemph;
+
+	return es8328_set_deemph(codec);
+}

This will unconditionally store the new deemphasis setting even if we
failed to set it.  That should never happen but still...

+	SOC_DOUBLE_TLV("Mic PGA Gain", ES8328_ADCCONTROL1, 4, 0, 8, 0, mic_tlv),

Mic PGA Volume.

+	SND_SOC_DAPM_MICBIAS("Mic Bias", ES8328_ADCPOWER,
+			ES8328_ADCPOWER_MIC_BIAS_OFF, 1),

Represent this as a suply, MICBIAS widgets are legacy.

+	case SND_SOC_BIAS_PREPARE:
+		/* VREF, VMID=2x50k, digital enabled */
+		snd_soc_write(codec, ES8328_CHIPPOWER, pwr_reg);
+		snd_soc_write(codec, ES8328_CONTROL1,
+				cc1_reg |
+				ES8328_CONTROL1_VMIDSEL_50k |
+				ES8328_CONTROL1_ENREF);

snd_soc_update_bits().

+const struct regmap_config es8328_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = ES8328_REG_MAX,
+	.cache_type = REGCACHE_NONE,
+};
+EXPORT_SYMBOL_GPL(es8328_regmap_config);

No need to specify no cache, it's the default - though a cache is
better, it's faster and allows for suspend/resume.