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

[Mark Brown]

On Mon, Mar 24, 2014 at 10:16:37AM +0800, oder_chiou at realtek.com wrote:
> From: Oder Chiou <oder_chiou at realtek.com>
> 
> This patch adds the RT5639 codec driver.

Looking at the level of code similiarity between this and the rt5640
driver I can't help but think that these should be supported from a
single driver - a few quick spot checks of the register map suggests
that there's at least some overlap.  There will need to be some device
specific handling but it looks like there's more shared than not shared.
What are the issues that prevent the code being shared, there may be
something I've missed?

There's also a few issues below that should be fixed, most of them
should be fixed in rt5640 too.

> +	switch (params_format(params)) {
> +	case SNDRV_PCM_FORMAT_S16_LE:
> +		break;
> +	case SNDRV_PCM_FORMAT_S20_3LE:
> +		val_len |= RT5639_I2S_DL_20;
> +		break;
> +	case SNDRV_PCM_FORMAT_S24_LE:
> +		val_len |= RT5639_I2S_DL_24;
> +		break;
> +	case SNDRV_PCM_FORMAT_S8:
> +		val_len |= RT5639_I2S_DL_8;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}

This should be converted to use params_width().

> +static int rt5639_set_bias_level(struct snd_soc_codec *codec,
> +			enum snd_soc_bias_level level)
> +{
> +	struct rt5639_priv *rt5639 = snd_soc_codec_get_drvdata(codec);
> +	switch (level) {
> +	case SND_SOC_BIAS_STANDBY:
> +		if (SND_SOC_BIAS_OFF == codec->dapm.bias_level) {
> +			regcache_cache_only(rt5639->regmap, false);

...

> +	case SND_SOC_BIAS_OFF:
> +		snd_soc_write(codec, RT5639_DEPOP_M1, 0x0004);
> +		snd_soc_write(codec, RT5639_DEPOP_M2, 0x1100);
> +		snd_soc_update_bits(codec, RT5639_DUMMY1, 0x1, 0);
> +		snd_soc_write(codec, RT5639_PWR_DIG1, 0x0000);
> +		snd_soc_write(codec, RT5639_PWR_DIG2, 0x0000);
> +		snd_soc_write(codec, RT5639_PWR_VOL, 0x0000);
> +		snd_soc_write(codec, RT5639_PWR_MIXER, 0x0000);
> +		snd_soc_write(codec, RT5639_PWR_ANLG1, 0x0000);
> +		snd_soc_write(codec, RT5639_PWR_ANLG2, 0x0000);
> +		break;

I'd expect to see the regmap being made cache_only in _OFF given that it
is resynced in _STANDBY when exiting _OFF, or alternatively to see the
cache restore moved to the resume to mirror where it's currently made
cache only.

> +static int rt5639_probe(struct snd_soc_codec *codec)
> +{
> +	struct rt5639_priv *rt5639 = snd_soc_codec_get_drvdata(codec);
> +	int ret;
> +
> +	rt5639->codec = codec;
> +	codec->control_data = rt5639->regmap;
> +
> +	ret = snd_soc_codec_set_cache_io(codec, 8, 16, SND_SOC_REGMAP);
> +	if (ret != 0) {
> +		dev_err(codec->dev, "Failed to set cache I/O: %d\n", ret);
> +		return ret;
> +	}

There have been some API changes here this release cycle, you should be
able to just remove the cache_io() and regmap setting code here.

> +	if (rt5639->pdata.in1_diff)
> +		regmap_update_bits(rt5639->regmap, RT5639_IN1_IN2,
> +					RT5639_IN_DF1, RT5639_IN_DF1);
> +
> +	if (rt5639->pdata.in2_diff)
> +		regmap_update_bits(rt5639->regmap, RT5639_IN3_IN4,
> +					RT5639_IN_DF2, RT5639_IN_DF2);

Why not have platform data to register if the DMICs are in use rather
than twiddling the GPIOs in an event?  It'd be more idiomatic.
-------------- 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/20140325/1a5222f0/attachment.sig>