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
