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.