On Fri, Jun 06, 2014 at 12:15:38PM +0800, bardliao@realtek.com wrote:
This patch adds a minimum support of Realtek ALC5670 codec.
This looks good overall, there are a few issues I've identified below but they're relatively small.
- /* IN1/IN2 Control */
- SOC_SINGLE_TLV("IN1 Boost", RT5670_CJ_CTRL1,
RT5670_BST_SFT1, 8, 0, bst_tlv),- SOC_SINGLE_TLV("IN2 Boost", RT5670_IN2,
RT5670_BST_SFT1, 8, 0, bst_tlv),
Boost Volume.
- /* ADC Boost Volume Control */
- SOC_DOUBLE_TLV("STO1 ADC Boost Gain", RT5670_ADC_BST_VOL1,
RT5670_STO1_ADC_L_BST_SFT, RT5670_STO1_ADC_R_BST_SFT,3, 0, adc_bst_tlv),- SOC_DOUBLE_TLV("STO2 ADC Boost Gain", RT5670_ADC_BST_VOL1,
RT5670_STO2_ADC_L_BST_SFT, RT5670_STO2_ADC_R_BST_SFT,3, 0, adc_bst_tlv),
s/Gain/Volume/
+static int check_dac_monor_asrc_source(struct snd_soc_dapm_widget *source,
struct snd_soc_dapm_widget *sink)+{
- struct snd_soc_codec *codec = source->codec;
- return is_using_asrc(codec, RT5670_ASRC_2, 4);
+}
I have to say I'm not convinced all these wrapper functions are really adding much - just making is_using_asrc() take a widget and passing the arguments directly seems as simple.
- /* ASRC */
- SND_SOC_DAPM_SUPPLY_S("I2S1 ASRC", 1, RT5670_ASRC_1,
11, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("I2S2 ASRC", 1, RT5670_ASRC_1,
12, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("DAC STO ASRC", 1, RT5670_ASRC_1,
10, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("DAC MONO L ASRC", 1, RT5670_ASRC_1,
9, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("DAC MONO R ASRC", 1, RT5670_ASRC_1,
8, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("ADC STO1 ASRC", 1, RT5670_ASRC_1,
3, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("ADC STO2 ASRC", 1, RT5670_ASRC_1,
2, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("ADC MONO L ASRC", 1, RT5670_ASRC_1,
1, 0, NULL, 0),- SND_SOC_DAPM_SUPPLY_S("ADC MONO R ASRC", 1, RT5670_ASRC_1,
0, 0, NULL, 0),
These are all _S but they seem not to have different sequence numbers - do they really need to be _S.
- /* Input Side */
- /* micbias */
- SND_SOC_DAPM_MICBIAS("MICBIAS1", RT5670_PWR_ANLG2,
RT5670_PWR_MB1_BIT, 0),
Modern drivers should use supply widgets for MICBIAS.
+static const struct snd_soc_dapm_route rt5670_dapm_routes[] = {
- { "adc stereo1 filter", NULL, "ADC STO1 ASRC",
check_adc_sto1_asrc_source },- { "adc stereo2 filter", NULL, "ADC STO2 ASRC",
check_adc_sto2_asrc_source },- { "adc mono left filter", NULL, "ADC MONO L ASRC",
check_adc_monol_asrc_source },- { "adc mono right filter", NULL, "ADC MONO R ASRC",
check_adc_monor_asrc_source },- { "dac mono left filter", NULL, "DAC MONO L ASRC",
check_dac_monol_asrc_source },- { "dac mono right filter", NULL, "DAC MONO R ASRC",
check_dac_monor_asrc_source },- { "dac stereo1 filter", NULL, "DAC STO ASRC",
check_dac_sto_asrc_source },- {"I2S1", NULL, "I2S1 ASRC"},
Coding style here is a bit inconsistent, and normally widget names are capitalised.
- switch (dai->id) {
- case RT5670_AIF1:
snd_soc_update_bits(codec, RT5670_I2S1_SDP,RT5670_I2S_MS_MASK | RT5670_I2S_BP_MASK |RT5670_I2S_DF_MASK, reg_val);break;- case RT5670_AIF2:
Extra space.
- switch (slots) {
- case 4:
val |= (1 << 12);break;- case 6:
val |= (2 << 12);break;- case 8:
val |= (3 << 12);break;- case 2:
- default:
break;- }
This seems wrong - the code will map all unknown slot counts to stereo which probably won't do what was expected.
- switch (slot_width) {
- case 20:
val |= (1 << 10);break;- case 24:
val |= (2 << 10);break;- case 32:
val |= (3 << 10);break;- case 16:
- default:
break;- }
Similarly here for 16 bits.
+static int rt5670_probe(struct snd_soc_codec *codec) +{
- struct rt5670_priv *rt5670 = snd_soc_codec_get_drvdata(codec);
- rt5670->codec = codec;
- snd_soc_update_bits(codec, RT5670_GEN_CTRL3, 0x4, 0x4);
- rt5670_set_bias_level(codec, SND_SOC_BIAS_OFF);
- return 0;
+}
I would expect this setup to be done in the bus level probe() rather than at ASoC level.