Re: [PATCH 3/4] ASOC: imx: add machine driver using wm8962 codec

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

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

On Tue, Jan 29, 2013 at 03:49:50PM +0800, Gary Zhang wrote:

> +static struct imx_priv card_priv;

Global data bad...

> +static int imx_wm8962_startup(struct snd_pcm_substream *substream)
> +{
> +	struct snd_soc_pcm_runtime *rtd = substream->private_data;
> +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
> +	struct imx_priv *priv = &card_priv;
> +	struct imx_wm8962_data *data = platform_get_drvdata(priv->pdev);
> +
> +	if (!codec_dai->active)
> +		clk_enable(data->codec_clk);

Should be clk_prepare_enable() but it's not clear to me why you need
this...   Also the clock API does refcounting so there should be no need
to check for active, you should get matching startups and shutdowns.

This will also fail for analogue bypass paths, set_bias_level() would be
a good place to cover those.

> +	int_port--;
> +	ext_port--;
> +	ret = imx_audmux_v2_configure_port(int_port,
> +			IMX_AUDMUX_V2_PTCR_SYN |
> +			IMX_AUDMUX_V2_PTCR_TFSEL(ext_port) |
> +			IMX_AUDMUX_V2_PTCR_TCSEL(ext_port) |
> +			IMX_AUDMUX_V2_PTCR_TFSDIR |
> +			IMX_AUDMUX_V2_PTCR_TCLKDIR,
> +			IMX_AUDMUX_V2_PDCR_RXDSEL(ext_port));
> +	if (ret) {
> +		dev_err(&pdev->dev, "audmux internal port setup failed\n");
> +		return ret;
> +	}
> +	imx_audmux_v2_configure_port(ext_port,
> +			IMX_AUDMUX_V2_PTCR_SYN,
> +			IMX_AUDMUX_V2_PDCR_RXDSEL(int_port));
> +	if (ret) {
> +		dev_err(&pdev->dev, "audmux external port setup failed\n");
> +		return ret;
> +	}

This stuff is pretty common for i.MX drivers - can we factor it out into
a helper library?

> +	ssi_pdev = of_find_device_by_node(ssi_np);
> +	if (!ssi_pdev) {
> +		dev_err(&pdev->dev, "failed to find SSI platform device\n");
> +		ret = -EINVAL;
> +		goto fail;
> +	}

Please remember to include binding documentation for all new device tree
bindings.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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