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

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.