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

[Stephen Warren]

Leon Romanovsky wrote at Sunday, November 27, 2011 1:48 AM:
> 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?

Well, I suppose we can always add this later if we need.

Thinking about this more though, with the moved to device-tree, having
such a variable isn't that useful, since the arch/arm/mach-tegra side
won't have individual machine Kconfig options that'll allow selecting
this driver.

So, it's fine the way it is.

> >> 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.

Mark pointed out that this might be correct anyway. Still, I'd ask that
you check the codec's datasheet to be sure.

> >> +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?

I don't quite understand your response. I was simply pointing out that
The code above would be simpler as:

static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio = {
     .name = "Headset Detect",
     .report = SND_JACK_HEADSET,
     .debounce_time = 150,
};

This was review feedback from Mark on other Tegra ASoC machine drivers.

It's plausible there are machines which don't format it that way. Still,
when adding new ones, we may as well follow best practices if possible.

> >> +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.

My comment was re: the design of the Harmony board, which is difficult
to fully support in HW. It's good that there are two mic bias pins. I
see no issue with your code.

-- 
nvpublic