[alsa-devel] [PATCH v2] ASoC: tlv320aic3x: Add support for tlv320aic3104

[Benoît Thébaudeau]

Dear Jyri Sarha,

Sorry to come late.

On Mon, Feb 2, 2015 at 3:48 PM, Jyri Sarha <jsarha at ti.com> wrote:
[...]
> diff --git a/sound/soc/codecs/tlv320aic3x.c b/sound/soc/codecs/tlv320aic3x.c
> index b7ebce0..cb92cdb 100644
> --- a/sound/soc/codecs/tlv320aic3x.c
> +++ b/sound/soc/codecs/tlv320aic3x.c
[...]
> @@ -550,6 +575,22 @@ static const struct snd_kcontrol_new aic3x_right_pga_mixer_controls[] = {
>         SOC_DAPM_SINGLE_AIC3X("Mic3R Switch", MIC3LR_2_RADC_CTRL, 0, 1, 1),
>  };
>
> +/* Left PGA Mixer for tlv320aic3104 */
> +static const struct snd_kcontrol_new aic3104_left_pga_mixer_controls[] = {
> +       SOC_DAPM_SINGLE_AIC3X("Line1L Switch", LINE1L_2_LADC_CTRL, 3, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Line1R Switch", LINE1R_2_LADC_CTRL, 3, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Mic2L Switch", MIC3LR_2_LADC_CTRL, 4, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Mic2R Switch", MIC3LR_2_LADC_CTRL, 0, 1, 1),
> +};
> +
> +/* Right PGA Mixer for tlv320aic3104 */
> +static const struct snd_kcontrol_new aic3104_right_pga_mixer_controls[] = {
> +       SOC_DAPM_SINGLE_AIC3X("Line1R Switch", LINE1R_2_RADC_CTRL, 3, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Line1L Switch", LINE1L_2_RADC_CTRL, 3, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Mic2L Switch", MIC3LR_2_RADC_CTRL, 4, 1, 1),
> +       SOC_DAPM_SINGLE_AIC3X("Mic2R Switch", MIC3LR_2_RADC_CTRL, 0, 1, 1),
> +};
> +

This part is wrong. All these controls should be turned into TLVs.
Otherwise, because of the reset values of these registers, the
reserved value 0b1110 is used for the corresponding bit-fields if
their lsb is cleared using the controls above.

Generally speaking, you have tracked reserved registers, but you
should also track reserved values.

>  /* Left Line1 Mux */
>  static const struct snd_kcontrol_new aic3x_left_line1l_mux_controls =
>  SOC_DAPM_ENUM("Route", aic3x_line1l_2_l_enum);
[...]
> @@ -839,6 +890,72 @@ static const struct snd_soc_dapm_route intercon[] = {
[...]
> +/* For other than tlv320aic3104 */

Typo above: not "other than" here.

> +static const struct snd_soc_dapm_route intercon_extra_3104[] = {
> +       /* Left Input */
> +       {"Left PGA Mixer", "Mic2L Switch", "MIC2L"},
> +       {"Left PGA Mixer", "Mic2R Switch", "MIC2R"},
> +
> +       /* Right Input */
> +       {"Right PGA Mixer", "Mic2L Switch", "MIC2L"},
> +       {"Right PGA Mixer", "Mic2R Switch", "MIC2R"},
> +};
> +
>  static const struct snd_soc_dapm_route intercon_mono[] = {
>         /* Mono Output */
>         {"Mono Mixer", "Line2L Bypass Switch", "Left Line2L Mux"},
[...]

The rest is good.

Best regards,
Benoît