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

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2185 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]