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.
Attachment:
signature.asc
Description: Digital signature
