On Wed, Dec 11, 2013 at 09:58:10AM +0800, bardliao@realtek.com wrote:
This is an improvement but there are still quite a few problems here.
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile index bc12676..b31615c 100644 --- a/sound/soc/codecs/Makefile +++ b/sound/soc/codecs/Makefile @@ -48,6 +48,7 @@ snd-soc-pcm1792a-codec-objs := pcm1792a.o snd-soc-pcm3008-objs := pcm3008.o snd-soc-rt5631-objs := rt5631.o snd-soc-rt5640-objs := rt5640.o +snd-soc-rt286-objs := rt286.o snd-soc-sgtl5000-objs := sgtl5000.o snd-soc-alc5623-objs := alc5623.o snd-soc-alc5632-objs := alc5632.o
Keep this file sorted please.
+static int rt286_hw_read(void *context, unsigned int reg, unsigned int *value)
To repeat what I said when reviewing the first version of this you should be using regmap not open coding your register I/O. This looks totally standard...
+int rt286_jack_detect(struct snd_soc_codec *codec, bool *hp, bool *mic) +{
- struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
- unsigned int val;
- int i;
+EXPORT_SYMBOL_GPL(rt286_jack_detect);
Use the standard jack detection APIs.
- if (rt286->pdata.irq_en)
rt286_index_update_bits(codec,NODE_ID_VENDOR_REGISTERS, 0x33, 0x0002, 0x0002);
Why would the system ever want to supply an interrupt and not use it?
- /* Set configuration default */
- rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
NODE_ID_MIC1, 0x00);- rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
NODE_ID_DMIC1, 0x00);- rt286_write(codec, AC_VERB_SET_CONFIG_DEFAULT_BYTES_3,
NODE_ID_DMIC2, 0x00);
Why are the silicon defaults not adequate?
+static int rt286_hp_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *kcontrol, int event)+{
- struct snd_soc_codec *codec = w->codec;
- unsigned int val;
- switch (event) {
- case SND_SOC_DAPM_POST_PMU:
val = snd_soc_read(codec, NODE_ID_HP_OUT) & 0x8080;switch (val) {case 0x0:rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,NODE_ID_HP_OUT, 0xb000);break;case 0x8000:rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,NODE_ID_HP_OUT, 0x9000);break;case 0x0080:rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,NODE_ID_HP_OUT, 0xa000);break;}
This is rather unclear. What is it doing?
+static int rt286_set_dai_sysclk(struct snd_soc_dai *dai,
int clk_id, unsigned int freq, int dir)+{
- struct snd_soc_codec *codec = dai->codec;
- struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec);
- pr_debug("%s freq=%d\n", __func__, freq);
dev_dbg().
+static int rt286_set_bias_level(struct snd_soc_codec *codec,
enum snd_soc_bias_level level)+{
- switch (level) {
- case SND_SOC_BIAS_PREPARE:
if (SND_SOC_BIAS_STANDBY == codec->dapm.bias_level)rt286_write(codec, AC_VERB_SET_POWER_STATE,NODE_ID_AUDIO_FUNCTION_GROUP, AC_PWRST_D0);break;- case SND_SOC_BIAS_OFF:
rt286_write(codec, AC_VERB_SET_POWER_STATE,NODE_ID_AUDIO_FUNCTION_GROUP, AC_PWRST_D3);break;
These aren't matched - you power up when leaving standby but only power down when you go to off. That's a bit odd and probably won't do the right thing and will leave the CODEC powered up all the time since you don't set idle_bias_off (though perhaps you should).
+static int rt286_probe(struct snd_soc_codec *codec) +{
- hw_configure(codec);
Just inline hw_configure(), there's no point having a separate function with only one use in a tiny function.
+static int rt286_remove(struct snd_soc_codec *codec) +{
- rt286_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
The framework should power down the device for you.
+struct snd_soc_dai_driver rt286_dai[] = {
- {
.name = "rt286-aif1",.id = RT286_AIF1,.playback = {.stream_name = "AIF1 Playback",.channels_min = 1,.channels_max = 2,.rates = RT286_STEREO_RATES,.formats = RT286_FORMATS,},.capture = {.stream_name = "AIF1 Capture",.channels_min = 1,.channels_max = 2,.rates = RT286_STEREO_RATES,.formats = RT286_FORMATS,},.ops = &rt286_aif_dai_ops,},
Your code handles up to 16 channels, and looking at the code I suspect you need symmetric_rates.
- ret = devm_snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt286,
rt286_dai, ARRAY_SIZE(rt286_dai));- if (ret < 0)
goto err;- return 0;
+err:
- return ret;
+}
+static int rt286_i2c_remove(struct i2c_client *i2c) +{
- snd_soc_unregister_codec(&i2c->dev);
Remove this, the whole point of devm_ is that it cleans up for you.
I'm also not seeing any code for interrupts even though there appears to be some code to initialise them in the driver.
+static int __init rt286_modinit(void) +{
- return i2c_add_driver(&rt286_i2c_driver);
+}
+module_init(rt286_modinit);
module_i2c_driver().
+#define NODE_ID_AUDIO_FUNCTION_GROUP 0x01 +#define NODE_ID_DAC_OUT1 0x02 +#define NODE_ID_DAC_OUT2 0x03 +#define NODE_ID_ADC_IN1 0x09 +#define NODE_ID_ADC_IN2 0x08 +#define NODE_ID_MIXER_IN 0x0b +#define NODE_ID_MIXER_OUT1 0x0c +#define NODE_ID_MIXER_OUT2 0x0d +#define NODE_ID_DMIC1 0x12 +#define NODE_ID_DMIC2 0x13 +#define NODE_ID_SPK_OUT 0x14 +#define NODE_ID_MIC1 0x18 +#define NODE_ID_LINE1 0x1a +#define NODE_ID_BEEP 0x1d +#define NODE_ID_SPDIF 0x1e +#define NODE_ID_VENDOR_REGISTERS 0x20 +#define NODE_ID_HP_OUT 0x21 +#define NODE_ID_MIXER_IN1 0x22 +#define NODE_ID_MIXER_IN2 0x23 +#define TOTAL_NODE_ID 0x23
+#define CONNECTION_INDEX_MIC1 0X0 +#define CONNECTION_INDEX_DMIC 0X5
These need namespacing if they're device specific.