[alsa-devel] [Patch V4 06/10] ASoC: ipq806x: Add LPASS CPU DAI driver

[Mark Brown]

On Thu, Feb 05, 2015 at 12:53:42PM -0800, Kenneth Westfield wrote:

> +	int bitwidth, ret;

> +	bitwidth = snd_pcm_format_width(format);
> +	if (bitwidth < 0) {
> +		dev_err(dai->dev, "%s() invalid bit width given\n", __func__);

Print the error code.

> +	regval = 0;
> +	regval |= LPAIF_I2SCTL_LOOPBACK_DISABLE;
> +	regval |= LPAIF_I2SCTL_WSSRC_INTERNAL;

Why not just write a single assignment statement?

> +	default:
> +		dev_err(dai->dev, "%s() invalid bitwidth given: %u\n",
> +				__func__, bitwidth);

bitwidth is a signed type but you are using an unsigned format specifier
here.

> +	reg = LPAIF_I2SCTL_REG(LPAIF_I2S_PORT_MI2S);
> +	mask = LPAIF_I2SCTL_SPKEN_MASK;
> +	val = LPAIF_I2SCTL_SPKEN_ENABLE;
> +	ret = regmap_update_bits(drvdata->lpaif_map, reg, mask, val);

None of these intermediate variables seem to be doing a lot, why not
just specify the constants directly as arguments (that's the more normal
style)?  A similar thing applies in several other places in this file.

> +#ifdef CONFIG_OF
> +static const struct of_device_id lpass_cpu_device_id[] = {
> +	{.compatible = "qcom," DRV_NAME},
> +	{}
> +};
> +#endif

Using DRV_NAME in the compatible like this makes it impossible to grep
for the driver which isn't helpful.  In general I prefer not to use
DRV_NAME at all (exactly how often do we change the driver name?) but in
this case it's actively harmful.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20150207/68db6dac/attachment-0002.sig>