Thank you Sascha, I'll revise them all in v9
On Mon, Aug 19, 2013 at 02:34:49PM +0200, Sascha Hauer wrote:
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 |