Re: [alsa-devel] [PATCH] ASoC: add RT286 CODEC driver

On Tue, Nov 26, 2013 at 05:11:31PM +0800, bardliao@realtek.com wrote:

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 983d087a..56748d4 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -313,6 +313,9 @@ config SND_SOC_PCM1792A config SND_SOC_PCM3008 tristate

+config SND_SOC_RT286

• tristate

This needs to be added to ALL_CODECS too.

+#define DEBUG

No, not in mainline.

+static int hp_amp_time = 100; +module_param(hp_amp_time, int, 0644);

No module parameters. Make a sysfs tunable for this if it's required, or make it platform data.

+#define RT_SUPPORT_COMBO_JACK 1

No compile time configuration please, platform data.

+#define VERSION "0.0.1-beta3 alsa 1.0.25"

The kernel is already versioned, don't try to maintian an individual driver version.

+static unsigned long rt286_read(struct snd_soc_codec *codec,

• 			   unsigned int vid,
  

• 			   unsigned int nid,
  

• 			   unsigned int data)
  

You should be implementing this in regmap using the reg_read and reg_write operations.

+static int rt286_index_write(struct snd_soc_codec *codec,

• unsigned int WidgetID, unsigned int index, unsigned int data)

Coding style - no CamelCase.

+static const struct reg_default rt286_reg[TOTAL_NODE_ID + 1] = {

• { NODE_ID_DAC_OUT1, 0x7f7f },
• { NODE_ID_DAC_OUT2, 0x7f7f },
• { NODE_ID_SPDIF, 0x0000 },
• { NODE_ID_ADC_IN1, 0x4343 },
• { NODE_ID_ADC_IN2, 0x4343 },
• { NODE_ID_MIC1, 0x0002 },
• { NODE_ID_MIXER_IN, 0x000b },
• { NODE_ID_MIXER_OUT1, 0x0002 },
• { NODE_ID_MIXER_OUT2, 0x0000 },
• { NODE_ID_SPK_OUT, 0x0000 },
• { NODE_ID_HP_OUT, 0x0000 },
• { NODE_ID_MIXER_IN1, 0x0005 },
• { NODE_ID_MIXER_IN2, 0x0005 },

+};

Why is there a size specified for this array?

+static bool rt286_volatile_register(struct device *dev, unsigned int reg) +{

• return 0;

+}

This is the default when caching, no need to have this function.

+static bool rt286_readable_register(struct device *dev, unsigned int reg) +{

• case NODE_ID_MIXER_IN2:

• return 1;
  

• default:

• return 0;
  

Return true and false for booleans.

+int rt286_jack_detect(struct snd_soc_codec *codec, bool *bHP, bool *bMIC) +{

Again, coding style. This should also be a static function (which presumably will be used by some jack detection code further down)?

+static int rt286_playback_vol_get(struct snd_kcontrol *kcontrol,

• 		  struct snd_ctl_elem_value *ucontrol)
  

+static int rt286_playback_vol_put(struct snd_kcontrol *kcontrol,

• 		  struct snd_ctl_elem_value *ucontrol)
  

+static int rt286_capture_vol_get(struct snd_kcontrol *kcontrol,

• 		  struct snd_ctl_elem_value *ucontrol)
  

+static int rt286_capture_vol_put(struct snd_kcontrol *kcontrol,

• 		  struct snd_ctl_elem_value *ucontrol)
  

There should be some code reuse between all these things, they seem to just differ in bits of data from a quick scan through.

• SOC_SINGLE_EXT_TLV("AMIC Gain", NODE_ID_MIC1, 0, 0x3, 0,

• 	   rt286_mic_gain_get, rt286_mic_gain_put,
  

• 	   mic_vol_tlv),
  

Volume.

+static void hwCodecConfigure(struct snd_soc_codec *codec)

Coding style yet again.

• rt286_write(codec, AC_VERB_SET_POWER_STATE,

• NODE_ID_AUDIO_FUNCTION_GROUP, AC_PWRST_D3);
  

• for (i = 0; i < RT286_POWER_REG_LEN; i++) {

• rt286_write(codec, AC_VERB_SET_POWER_STATE,
  

• rt286_support_power_controls[i], AC_PWRST_D3);
  

• }
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x4F, 0xb029);
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x09, 0xc400);
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x0A, 0x0120);
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x33, 0x020A);

+#if !RT_SUPPORT_COMBO_JACK

• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x50, 0x0000);
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x19, 0x0816);
• rt286_index_write(codec, NODE_ID_VENDOR_REGISTERS, 0x20, 0x0000);

+#endif

This is writing a bunch of apparently configuration dependant magic numbers into the device. The configuration should come from platform data or be done at runtime and we should have some visibility of what the magic numbers are up to.

• /* Sync volume */
• regmap_read(rt286->regmap, NODE_ID_ADC_IN1, &val);
• val = (val >> 8) & 0x7f;
• rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,

• 	NODE_ID_ADC_IN1, 0x6000 | val);
  

It's not clear what any of this is doing but the general policy is to use the hardware defaults.

+/* HP-OUT source */ +static const char * const rt286_hpo_src[] = {

• "Front", "Surr"

+};

Spell out Surround fully to help readabiity.

• rt286_write(codec, AC_VERB_SET_AMP_GAIN_MUTE,
  

• 	NODE_ID_SPK_OUT, 0xb000);
  

Wasn't there a separate speaker mute control?

+static int rt286_hp_pow_event(struct snd_soc_dapm_widget *w,

• 	   struct snd_kcontrol *kcontrol, int event)
  

It looks like a lot of these events should be sharing code as well.

• switch (params_rate(params)) {
• /* bit 14 0:48K 1:44.1K */
• case 48000:

• val &= ~0x4000;
  

• break;
  

• case 44100:

• val |= 0x4000;
  

• break;
  

Use snd_soc_update_bits() when writing out.

• default:

• pr_err("unsportted sample rate %d\n", params_rate(params));
  

• return -EINVAL;
  

dev_err().

• } else {

• pr_err("unsportted channels %d\n", params_channels(params));
  

Unsupported.

• /* bit 15 Stream Type 0:PCM 1:Non-PCM */
• val_dac &= ~0x8000;
• val_adc &= ~0x8000;
• rt286_write(codec, AC_VERB_SET_STREAM_FORMAT,

• NODE_ID_DAC_OUT1, val_dac);
  

• rt286_write(codec, AC_VERB_SET_STREAM_FORMAT,

• NODE_ID_ADC_IN1, val_adc);
  

update_bits here too.

• switch (freq) {
• case 19200000:
• case 24000000:

• rt286_index_update_bits(codec, NODE_ID_VENDOR_REGISTERS,
  

• 	0x63, 0x4, 0x4);
  

• rt286_index_update_bits(codec, NODE_ID_VENDOR_REGISTERS,
  

• 	0x49, 0x20, 0x0);
  

• break;
  

This doesn't look right - you've got multiple clock rates but are writing the same value out (and similarly for everything else). Do you need to add a constraint limiting what sample rates can be used depending on the system clock?

Too many magic numbers too as with the rest of the code.

+static int rt286_set_dai_dfs(struct snd_soc_dai *dai, unsigned int fs) +{

• struct snd_soc_codec *codec = dai->codec;
• pr_debug("%s fs=%d\n", __func__, fs);
• if (50 == fs)

• rt286_index_update_bits(codec,
  

• 	NODE_ID_VENDOR_REGISTERS, 0x09, 0x1000, 0x1000);
  

• return 0;

+}

This looks buggy, if the fs value happens to be 50 you write something otherwise you write nothing - what happens if we move from 50 to another value?

+static ssize_t rt286_codec_show(struct device *dev,

• 		 struct device_attribute *attr, char *buf)
  

Use the framework support for this.

+static int rt286_set_bias_level(struct snd_soc_codec *codec,

• 		 enum snd_soc_bias_level level)
  

+{

• switch (level) {
• case SND_SOC_BIAS_ON:

• pr_debug("In case SND_SOC_BIAS_ON:\n");
  

Remove noisy stuff like this, you're duplicating the core.

+#ifdef CONFIG_PM +static int rt286_suspend(struct snd_soc_codec *codec) +{

• return 0;

+}

Remove empty functions.

• regcache_cache_only(rt286->regmap, true);

Nothing ever clears the cache_only flag in the driver so register writes won't be happening. Has this code been tested?

• ret = snd_soc_register_codec(&i2c->dev, &soc_codec_dev_rt286,

• 		     rt286_dai, ARRAY_SIZE(rt286_dai));
  

devm_snd_soc_register_codec().

+static void rt286_i2c_shutdown(struct i2c_client *client) +{

• struct rt286_priv *rt286 = i2c_get_clientdata(client);
• struct snd_soc_codec *codec = rt286->codec;
• if (codec != NULL)

• rt286_set_bias_level(codec, SND_SOC_BIAS_OFF);
  

+}

The core should be dealing with this.