Re: [PATCH v2] ASoC: imx-wm8958: add imx-wm8958 machine driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1324 bytes --]

On Fri, Feb 26, 2016 at 11:42:42AM +0800, Zidan Wang wrote:

> +	if (id == HIFI_DAI) {
> +		/*
> +		 * Set GPIO1 pin function to reserve, so that DAC1 and ADC1
> +		 * using shared LRCLK from DACLRCK1.
> +		 */
> +		snd_soc_update_bits(codec, WM8994_GPIO_1, 0x1f, 0x2);

No, this is broken - if you're writing directly into the register map of
a device without the driver that's just asking for breakage.

> +	} else if (id == VOICE_DAI) {
> +		/*

This looks like you're writing a switch statement...

> +		if (card->dapm.bias_level == SND_SOC_BIAS_OFF) {
> +			/* need to enable mclk to write/read wm8958 register */
> +			for (i = 0; i < WM8958_MCLK_MAX; i++) {
> +				if (!IS_ERR(data->mclk[i])) {
> +					ret = clk_prepare_enable(data->mclk[i]);

The CODEC needs to look after its own clocks.

> +	[VOICE_DAI] =   {
> +			.name = "Voice",
> +			.stream_name = "Voice",
> +			.cpu_dai_name = "snd-soc-dummy-dai",
> +			.codec_name = "wm8994-codec",
> +			.codec_dai_name = "wm8994-aif2",
> +			.platform_name = "snd-soc-dummy",

Why are you mapping in dummy DAIs?  If these devices aren't connected
then they're not connected and you shouldn't represent them.  If they
are connected to something then describe those connections, possibly in
followup patches if you have other devices you need to support upstream
first.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]