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

[Stephen Warren]

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?

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

> +	err = snd_soc_dai_set_fmt(cpu_dai,
> +					SND_SOC_DAIFMT_I2S |
> +					SND_SOC_DAIFMT_NB_NF |
> +					SND_SOC_DAIFMT_CBS_CFS);
> +	if (err < 0) {
> +		dev_err(card->dev, "cpu_dai fmt not set\n");
> +		return err;
> +	}
> +
> +	err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
> +					SND_SOC_CLOCK_IN);
> +	if (err < 0) {
> +		dev_err(card->dev, "codec_dai clock not set\n");
> +		return err;
> +	}

Mark Brown wrote here:
> These should use the .dai_fmt member in the dai_link struct.

Mark, I guess you mean that we should remove those two snd_soc_dai_set_fmt
calls, and just set .dai_fmt to a constant value? If so, that should be
applied to the other two Tegra machine drivers too.

That said, while these values are constant right now, I think they won't
always be; I think we'll need to switch between DSP mode for mono and
I2S mode for stereo here; at least I /think/ that's what I remember our
downstream drivers doing...

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

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

And then you can assign tegra_alc5632_hs_jack_gpio.gpio.

> +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:-)

> +static int tegra_alc5632_asoc_init(struct snd_soc_pcm_runtime *rtd)
...
> +	snd_soc_jack_new(codec, "Headset Jack", SND_JACK_HEADSET,
> +			 &tegra_alc5632_hs_jack);
> +	snd_soc_jack_add_pins(&tegra_alc5632_hs_jack,
> +			ARRAY_SIZE(tegra_alc5632_hs_jack_pins),
> +			tegra_alc5632_hs_jack_pins);
> +	snd_soc_jack_add_gpios(&tegra_alc5632_hs_jack,
> +			ARRAY_SIZE(tegra_alc5632_hs_jack_gpios),
> +			tegra_alc5632_hs_jack_gpios);

Do you need to call something in the ALC5632 codec driver here to start
mic detection? The tegra_wm8903 driver calls wm8903_mic_detect() here.

> +	snd_soc_dapm_nc_pin(dapm, "AUXOUT");
> +	snd_soc_dapm_nc_pin(dapm, "LINEINL");
> +	snd_soc_dapm_nc_pin(dapm, "LINEINR");
> +	snd_soc_dapm_nc_pin(dapm, "PHONEP");
> +	snd_soc_dapm_nc_pin(dapm, "PHONEN");
> +	snd_soc_dapm_nc_pin(dapm, "MIC2");

----------==========----------==========----------==========----------==========
You may be able to remove that if you set snd_soc_tegra_alc5632.fully_routed.
The patch for that was just merged into Mark's ASoC tree.

> +static __devinit int tegra_alc5632_probe(struct platform_device *pdev)
...
> +	alc5632 = kzalloc(sizeof(struct tegra_alc5632), GFP_KERNEL);

If you use devm_kzalloc there, it'll remove the need to manually free
it; see the very latest tegra_wm8903 driver.

> +err_clear_drvdata:
> +	snd_soc_card_set_drvdata(card, NULL);
> +	platform_set_drvdata(pdev, NULL);
> +	card->dev = NULL;

There's no need for those last 3 lines.

> +	tegra_asoc_utils_fini(&alc5632->util_data);
> +err_free_tegra_alc5632:
> +	kfree(alc5632);

That kfree is what you could get rid of using devm_kzalloc.

> +static int __devexit tegra_alc5632_remove(struct platform_device *pdev)
...
> +	snd_soc_card_set_drvdata(card, NULL);
> +	platform_set_drvdata(pdev, NULL);
> +	card->dev = NULL;

Same here; no need for those last 3 lines.

> +	tegra_asoc_utils_fini(&alc5632->util_data);
> +
> +	kfree(alc5632);

That's another thing you could remove if using devm_kzalloc.

...
> +MODULE_LICENSE("GPL");

The comment at the top would imply "GPL v2", since no "or later" is
mentioned there. Yes, the existing Tegra code is probably wrong here too.

-- 
nvpublic