[alsa-devel] [PATCH] ASoC: Tegra: Add support of tegra boards based on ALC5632 codec

[Leon Romanovsky]

On Wed, Nov 23, 2011 at 23:50, Stephen Warren <swarren at nvidia.com> wrote:
> Leon Romanovsky wrote at Monday, November 21, 2011 1:08 PM:
>> At this stage only Toshiba AC100/Dynabook supported.
> ...
>> diff --git a/sound/soc/tegra/Kconfig b/sound/soc/tegra/Kconfig
> ...
>> +config SND_SOC_TEGRA_ALC5632
>> +     tristate "SoC Audio support for Tegra boards using an ALC5632 codec"
>> +     depends on SND_SOC_TEGRA && I2C
>> +     select SND_SOC_TEGRA_I2S
>> +     select SND_SOC_ALC5632
>> +     help
>> +       Say Y or M here if you want to add support for SoC audio on the
>> +       Toshiba AC100 netbook.
>
> If this is going to be named tegra_acl5632 rather than something board-
> specific such as tegra_paz00, then perhaps you should do Kconfig the
> same way as the tegra_wm8903 driver; have a MACH_HAS_SND_SOC_TEGRA_ALC5632
> variable that boards need to select, and make this depend on that?
I saw in the Kconfing MACH_HAS_SND_SOC_TEGRA_WM8903, but don't know if
it is necessary, because no one use it.
Do I need to do the same in our case too?

>
>> diff --git a/sound/soc/tegra/tegra_alc5632.c b/sound/soc/tegra/tegra_alc5632.c
> ...
>> +static int tegra_alc5632_asoc_hw_params(struct snd_pcm_substream *substream,
>> +                                     struct snd_pcm_hw_params *params)
> ...
>> +     srate = params_rate(params);
>> +     switch (srate) {
>> +     case 64000:
>> +     case 88200:
>> +     case 96000:
>> +             mclk = 128 * srate;
>> +             break;
>> +     default:
>> +             mclk = 512 * srate;
>> +             break;
>> +     }
>> +     /* FIXME: Codec only requires >= 3MHz if OSR==0 */
>> +     while (mclk < 6000000)
>> +             mclk *= 2;
>
> That is identical to the code in tegra_wm8903. Are you sure it's correct
> for the ALC5632? At the very least, I think the comment is wrong; OSR is
> a WM8903 register field, which I doubt exists on the ALC5632.
The code started from the harmony board file, there are possibilities
it is not necessary.


>> +static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] = {
>> +     {
>> +             .name = "Headset Detect",
>> +             .report = SND_JACK_HEADSET,
>> +             .debounce_time = 150,
>> +     }
>> +};
>
> You probably don't want to make this an array, since that'll make the
> code that fills in tegra_alc5632_hs_jack_gpio [0].gpio have to hard-code
> the "0" array index; better to make this just:
This is something, I never understand. There are plenty
implementations which use SND_JACK_HEADSET and plenty which prefer to
separate it.
What is the guideline?

>> +static const struct snd_soc_dapm_route tegra_alc5632_audio_map[] = {
>> +     /* Internal Mic */
>> +     {"MIC2", NULL, "Mic Bias 2"},
>> +     {"Mic Bias 2", NULL, "Int Mic"},
>
> Yay, a separate mic bias for each microphone:-)
Is it wrong? Our codec support two microphones, and two different biases.

-- 
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon at leon.nu