Re: [PATCH v8 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver

[Nicolin Chen <b42378@freescale.com>]

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