On Thu, Feb 05, 2015 at 12:53:43PM -0800, Kenneth Westfield wrote:
- irqreturn_t ret = IRQ_NONE;
- interrupts = ioread32(drvdata->lpaif + status_offset) & chan_bitmask;
Elsewhere we're using regmap... why not here and how does this play with regmap?
- if (likely(interrupts & period_bitmask)) {
In general it's better not to use likely() and unlikely() annotations unless you've got some evidence that they actually improve things (which should be explained somewhere). They make the code that bit noisier and there's some potential for them to cause the compiler to do unhelpful things.
+static int lpass_platform_alloc_buffer(struct snd_pcm_substream *substream,
struct snd_soc_pcm_runtime *soc_runtime)+{
- struct snd_dma_buffer *buf = &substream->dma_buffer;
- struct lpass_data *drvdata =
snd_soc_platform_get_drvdata(soc_runtime->platform);- if (atomic_cmpxchg(&drvdata->lpm_lock, 0, 1)) {
cmpxchg() and atomics are both among the more error prone synchronization primitives the kernel has. In cases where it is appropriate to use them it is very important that the code be entirely clear about what is going on so that not only are we clear that the concurrency handling is safe but we can also modify the code safely in the future.
This driver has no documentation at all for this variable so we've at least got a problem with the clarity side here. Looking at the code it's not entirely clear to me wat exactly is being protected or why we're not using a simpler mechanism like variable protected by a mutex.
- return devm_snd_soc_register_platform(&pdev->dev,
&lpass_platform_driver);+} +EXPORT_SYMBOL(asoc_qcom_lpass_platform_register);
ASoC APIs are all exported EXPORT_SYMBOL_GPL, their users should be too.