Re: [alsa-devel][PATCH] ASoC: fsl: add imx-wm8960 machine driver

[Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]

On Wed, Jun 10, 2015 at 04:26:27PM +0800, Zidan Wang wrote:

> +  - hp-det		: ADCLRC/GPIO1, LINPUT3/JD2 and RINPUT3/JD3 pins can
> +			  be selected as headphone jack detect inputs to
> +			  automatically disable the speaker output and enable
> +			  the headphone output.
> +			  hp-det = <hp-det-pin hp-det-polarity>;
> +			  hp-det-pin = 1: ADCLRC/GPIO1 used as detect pin
> +			  hp-det-pin = 2: LINPUT3/JD2 used as detect pin
> +			  hp-det-pin = 3: RINPUT3/JD3 used as detect pin
> +			  hp-det-polarity = 0: hp detect high for headphone
> +			  hp-det-polarity = 1: hp detect high for speaker

This looks like something that should be in the DT binding for the
CODEC, not the machine driver.

> +static int hp_set_status_check(void *data)
> +{

Why is this in the driver?

> +	hp_status = gpio_get_value(priv->hp_set_gpio) ? 1 : 0;

gpio_get_value() already returns a boolean.

> +	if (hp_status != priv->hp_active_low) {
> +		snprintf(buf, 32, "STATE=%d", 2);
> +		snd_soc_dapm_disable_pin(&priv->codec->dapm, "Ext Spk");
> +		snd_soc_dapm_disable_pin(&priv->codec->dapm, "Main MIC");
> +		ret = imx_hp_set_gpio.report;

The generic jack code already has support for disabling and enabling
pins, though disabling the speaker looks like a policy decision which
probably doesn't belong here.

> +		/*
> +		 *  As the hp MIC only connect the input for left channel, we
> +		 *  need to route it for right channel.
> +		 */
> +		snd_soc_update_bits(priv->codec, WM8960_ADDCTL1, 3<<2, 1<<2);

This looks like routing which we'd expect userspace to be doing.

> +		snd_kctl_jack_report(priv->snd_card, priv->headset_kctl, 1);

Use the ASoC level helpers.

> +	envp[0] = "NAME=headset";
> +	envp[1] = buf;
> +	envp[2] = NULL;
> +	kobject_uevent_env(&pdev->dev.kobj, KOBJ_CHANGE, envp);
> +	kfree(buf);

Let the core deal with notifying userspace - it looks like you want to
implement extcon integration there.

> +	/* set cpu DAI configuration */
> +	ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
> +	if (ret) {
> +		dev_err(dev, "failed to set cpu dai fmt: %d\n", ret);
> +		return ret;
> +	}
> +	/* set codec DAI configuration */
> +	ret = snd_soc_dai_set_fmt(codec_dai, fmt);
> +	if (ret) {
> +		dev_err(dev, "failed to set codec dai fmt: %d\n", ret);
> +		return ret;
> +	}

Initialise these in the dai_link structure.

> +	for (i = 0; i < ARRAY_SIZE(sysclk_divs); ++i) {
> +		if (sysclk_divs[i] == -1)
> +			continue;
> +		sysclk /= sysclk_divs[i];
> +		for (j = 0; j < ARRAY_SIZE(dac_divs); ++j) {
> +			if (sysclk == sample_rate * dac_divs[j]) {
> +				for (k = 0; k < ARRAY_SIZE(bclk_divs); ++k)
> +					if (sysclk == bclk * bclk_divs[k] / 10)
> +						break;
> +				if (k != ARRAY_SIZE(bclk_divs))
> +					break;
> +			}
> +		}
> +		if (j != ARRAY_SIZE(dac_divs))
> +			break;
> +	}
> +	if (i != ARRAY_SIZE(sysclk_divs)) {
> +		/* Set codec sysclk */
> +		snd_soc_dai_set_sysclk(codec_dai,
> +				       WM8960_SYSCLK_MCLK, sysclk, 0);
> +		snd_soc_dai_set_clkdiv(codec_dai, WM8960_SYSCLKDIV, i << 1);
> +		return 0;
> +	}

Better, upgrade the CODEC driver to do this.

> +	/* codec mclk should be enabled early to avoid jack detect error */
> +	ret = clk_prepare_enable(data->codec_clk);
> +	if (ret) {
> +		dev_err(card->dev, "Failed to enable MCLK: %d\n", ret);
> +		return ret;
> +	}

Similarly integrating clock API support into the CODEC is better - it
makes all the DT stuff work a lot more smoothly and avoids things having
to get duplicated.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]