On Mon, Mar 24, 2014 at 10:16:37AM +0800, oder_chiou@realtek.com wrote:
From: Oder Chiou oder_chiou@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.