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().
Attachment:
signature.asc
Description: Digital signature
