Hi Nicolas, Some misc other comments inline. On Mon, Aug 19, 2013 at 08:08:48PM +0800, Nicolin Chen wrote: > This patch implements a device-tree-only CPU DAI driver for Freescale > + > +struct fsl_spdif_priv { > + struct spdif_mixer_control fsl_spdif_control; > + struct snd_soc_dai_driver cpu_dai_drv; > + struct platform_device *pdev; > + struct regmap *regmap; > + atomic_t dpll_locked; You don't need an atomic_t to track a bool variable. Use a plain bool or int instead. > + u8 txclk_div[SPDIF_TXRATE_MAX]; > + u8 txclk_src[SPDIF_TXRATE_MAX]; > + u8 rxclk_src; > + struct clk *txclk[SPDIF_TXRATE_MAX]; > + struct clk *rxclk; > + struct snd_dmaengine_dai_dma_data dma_params_tx; > + struct snd_dmaengine_dai_dma_data dma_params_rx; > + > + /* The name space will be allocated dynamically */ > + char name[0]; > +}; > + > + > +#ifdef DEBUG > +static void dumpregs(struct fsl_spdif_priv *spdif_priv) > +{ > + struct regmap *regmap = spdif_priv->regmap; > + struct platform_device *pdev = spdif_priv->pdev; > + u32 val, i; > + int ret; > + > + /* Valid address set of SPDIF is {[0x0-0x38], 0x44, 0x50} */ > + for (i = 0 ; i <= REG_SPDIF_STC; i += 4) { > + ret = regmap_read(regmap, REG_SPDIF_SCR + i, &val); > + if (!ret) > + dev_dbg(&pdev->dev, "REG 0x%02x = 0x%06x\n", i, val); > + } > +} > +#else > +static void dumpregs(struct fsl_spdif_priv *spdif_priv) {} > +#endif Is this needed? regmap provides a register dump in debugfs. > + > +static int spdif_clk_set_rate(struct clk *clk, unsigned long rate) > +{ > + unsigned long rate_actual; > + > + rate_actual = clk_round_rate(clk, rate); > + return clk_set_rate(clk, rate_actual); > +} clk_round_rate returns the rate which clk_set_rate would set if called with the same rate. The clk_round_rate() is unnecessary. > + > + dev_dbg(&pdev->dev, "expected clock rate = %d\n", > + (int)(64 * sample_rate * div)); > + dev_dbg(&pdev->dev, "acutal clock rate = %d\n", > + (int)clk_get_rate(spdif_priv->txclk[rate])); s/acutal/actual/ Also please use %ld instead of casting the unsigned long to int. > + > + dev_dbg(&pdev->dev, "FreqMeas: %d\n", (int)freqmeas); > + dev_dbg(&pdev->dev, "BusclkFreq: %d\n", (int)busclk_freq); > + dev_dbg(&pdev->dev, "RxRate: %d\n", (int)tmpval64); Get rid of the casts > + > + spdif_priv = devm_kzalloc(&pdev->dev, > + sizeof(struct fsl_spdif_priv) + strlen(np->name) + 1, GFP_KERNEL); > + if (!spdif_priv) { > + dev_err(&pdev->dev, "could not allocate DAI object\n"); Please drop this message. You'll never see it. > + > + for (i = 0; i < SPDIF_TXRATE_MAX; i++) { > + ret = fsl_spdif_probe_txclk(spdif_priv, i); > + if (ret) > + return ret; > + } > + > + /* Prepare rx/tx clock */ > + clk_prepare(spdif_priv->rxclk); > + for (i = 0; i < SPDIF_TXRATE_MAX; i++) > + clk_prepare(spdif_priv->txclk[i]); Why do you prepare all clocks instead of the one you actually use? Also, no need to do this here. You can use clk_prepare_enable instead where you have clk_enable now. > + > +/* SPDIF rx clock source */ > +enum spdif_rxclk_src { > + SRPC_CLKSRC_0 = 0, > + SRPC_CLKSRC_1, > + SRPC_CLKSRC_2, > + SRPC_CLKSRC_3, > + SRPC_CLKSRC_4, > + SRPC_CLKSRC_5, > + SRPC_CLKSRC_6, > + SRPC_CLKSRC_7, > + SRPC_CLKSRC_8, > + SRPC_CLKSRC_9, > + SRPC_CLKSRC_10, > + SRPC_CLKSRC_11, > + SRPC_CLKSRC_12, > + SRPC_CLKSRC_13, > + SRPC_CLKSRC_14, > + SRPC_CLKSRC_15, > +}; These are unused and look unnecessary. > + > +/* SPDIF tx clksrc */ > +enum spdif_txclk_src { > + STC_TXCLK_SRC_0 = 0, > + STC_TXCLK_SRC_1, > + STC_TXCLK_SRC_2, > + STC_TXCLK_SRC_3, > + STC_TXCLK_SRC_4, > + STC_TXCLK_SRC_5, > + STC_TXCLK_SRC_6, > + STC_TXCLK_SRC_7, > +}; Also unused. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html