Re: [alsa-devel] [PATCH 3/5] ASoC: omap: Add HA (HEAD acoustics) DSP add-on card audio driver for TAO3530

On Mon, Apr 28, 2014 at 02:17:54PM +0200, Stefan Roese wrote:

• switch (params_channels(params)) {
• case 2:
• case 4:
• case 8:
• case 16:

• fmt = SND_SOC_DAIFMT_DSP_B | SND_SOC_DAIFMT_IB_NF;
  

• break;
  

• default:

• return -EINVAL;
  

• }

Why is this "conditional" on the number of channels (though the same value is always chosen)?

• /*
  

•  * Calculate McBSP SRG divisor in McBSP master mode
  

•  */
  

• div = 96000000 / params_rate(params) / params_channels(params);
  

• switch (params_format(params)) {
  

• case SNDRV_PCM_FORMAT_S16_LE:
  

• 	div /= 16;
  

• 	break;
  

• case SNDRV_PCM_FORMAT_S24_LE:
  

• case SNDRV_PCM_FORMAT_S32_LE:
  

• 	div /= 32;
  

• 	break;
  

• };
  

params_width().

• ret = snd_soc_dai_set_clkdiv(cpu_dai, OMAP_MCBSP_CLKGDV, div);
  

• if (ret < 0) {
  

• 	pr_err("can't set SRG clock divider\n");
  

• 	return ret;
  

• }
  

Why not put this code into the DAI driver for the SoC - what's board specific about the calcuation? I'd expect this to be handlable by set_sysclk().

• if (!node) {

• dev_err(&pdev->dev, "No DT node provided\n");
  

• return -ENODEV;
  

• }

You've got something with a DT binding here but no binding document, documentation is mandatory for new DT bindings.

• priv->slave = of_property_read_bool(node, "ha_dsp_codec_slave");
• if (priv->slave)

• dev_info(&pdev->dev, "Using slave mode for testing!\n");
  

This seems like something that shouldn't be in production code, or perhaps a build time option if the testing is valuable?

• if (snd_soc_of_parse_card_name(card, "ti,model")) {

• dev_err(&pdev->dev, "Card name is not provided\n");
  

• ret = -ENODEV;
  

• goto err;
  

• }

Why not have a default?

• ret = snd_soc_register_card(card);
• if (ret) {

• dev_err(&pdev->dev, "snd_soc_register_card() failed: %d\n",
  

• 	ret);
  

• goto err;
  

• }

devm_snd_soc_register_card().