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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 1893 bytes --]

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 #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]