Re: [Patch V4 06/10] ASoC: ipq806x: Add LPASS CPU DAI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux