[alsa-devel] [PATCH] ASoC: add RT5640 CODEC driver

[Mark Brown]

On Fri, May 31, 2013 at 03:04:59PM +0800, bardliao at realtek.com wrote:

> From: Bard Liao <bardliao at realtek.com>

This looks pretty good, a few small things below and probably one actual
bug.

> 
> * Use regmap_range_cfg to replace index read/write function.
> * Remove I2S3 related code since there is no I2S3 in ALC5640.
> * Remove Voice DSP related code since there is no Voice DSP in ALC5640.
> * Remove MICBIAS2 since there is no MICBIAS2 in ALC5640.
> * Change DMIC1/2 CLK to DMIC1/2 Power since it is for enable/disable DMIC1/2
> * Modify some texts for consistent coding style.
> * Merge sto adc l/r mux since it shares the same control bits.
> * Other minor changes.

Put stuff like this after the --- so it doesn't end up in the commit
log.

> +	switch (event) {
> +	case SND_SOC_DAPM_POST_PMU:
> +		snd_soc_update_bits(codec, RT5640_PWR_DIG1, 0x0001, 0x0001);
> +		regmap_update_bits(rt5640->regmap, 0x1c, 0xf000, 0xf000);
> +		break;

It's a bit weird to be using snd_soc_update_bits() and
regmap_update_bits()...

> +	SND_SOC_DAPM_SUPPLY("DMIC1 Power", SND_SOC_NOPM, 0, 0,
> +		rt5640_set_dmic1_event,
> +		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),
> +	SND_SOC_DAPM_SUPPLY("DMIC2 Power", SND_SOC_NOPM, 0, 0,
> +		rt5640_set_dmic2_event,
> +		SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD),

The enable bits could be listed here - this would be a bit more
idiomatic, would save the PMD event and would enable the writes to be
combined.

> +	case SND_SOC_BIAS_STANDBY:
> +		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> +				RT5640_PWR_BG | RT5640_PWR_VREF2,
> +				RT5640_PWR_VREF1 | RT5640_PWR_MB |
> +				RT5640_PWR_BG | RT5640_PWR_VREF2);
> +			mdelay(10);
> +			snd_soc_update_bits(codec, RT5640_PWR_ANLG1,
> +				RT5640_PWR_FV1 | RT5640_PWR_FV2,
> +				RT5640_PWR_FV1 | RT5640_PWR_FV2);
> +			regcache_cache_only(rt5640->regmap, false);
> +			regcache_sync(rt5640->regmap);

This looks wrong - you're writing to the device before you take it out
of cache only mode so your delay in the above sequence will have no
effect.

> +	val = snd_soc_read(codec, RT5640_VENDOR_ID2);
> +	if ((val != RT5640_DEVICE_ID)) {
> +		dev_err(codec->dev,
> +			"Device with ID register %x is not rt5640/39\n", val);
> +		return -ENODEV;
> +	}
> +
> +	rt5640_reset(codec);

Do these in the I2C probe function before you register the CODEC, that
way we don't try to start the sound card if we can't talk to the device.

> +	ret = regmap_register_patch(rt5640->regmap, init_list,
> +				    ARRAY_SIZE(init_list));
> +	if (ret != 0)
> +		dev_warn(codec->dev, "Failed to apply regmap patch: %d\n",
> +			 ret);

This should also be in the I2C probe.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20130603/588886a1/attachment.sig>