On Mon, Nov 28, 2011 at 18:52, Stephen Warren swarren@nvidia.com wrote:
Leon Romanovsky wrote at Sunday, November 27, 2011 1:48 AM:
On Wed, Nov 23, 2011 at 23:50, Stephen Warren swarren@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 ALC5632codec"
depends on SND_SOC_TEGRA && I2Cselect SND_SOC_TEGRA_I2Sselect SND_SOC_ALC5632helpSay Y or M here if you want to add support for SoC audio onthe
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.
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, };
Thanks, I previously missed the difference: static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpios[] -----> static struct snd_soc_jack_gpio tegra_alc5632_hs_jack_gpio
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
Thanks for the review, I'll post updated patch in the next few days.