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

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

[Mark Brown]

[-- 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 --]

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

[Mark Brown]

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

On Thu, Jan 31, 2013 at 03:02:21AM +0000, Zhang Quan-B13634 wrote:

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

> [Gary-b13634] for power saving consideration. For now remove this feature to
> Implement a basic feature

I get why you're doing this at all but doing it in startup seems odd -
like I say set_bias_level() is the most idiomatic place to implement
this usually.  There's a few machine drivers doing that you should be
able to refer to for examples.

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

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