On Wed, Dec 24, 2014 at 08:42:07AM -0800, Kenneth Westfield wrote:
+static inline u32 lpass_lpaif_int_retrieve(struct snd_soc_platform *platform) +{
- struct lpass_mi2s_data *drvdata =
snd_soc_platform_get_drvdata(platform);- u32 status_offset = LPAIF_DMAIRQ_STAT(LPAIF_IRQ_RECV_HOST);
- u32 clear_offset = LPAIF_DMAIRQ_CLEAR(LPAIF_IRQ_RECV_HOST);
- u32 chan_bitmask = LPAIF_DMAIRQ_ALL(LPAIF_DMA_RD_CH_MI2S);
- u32 value;
- value = readl(drvdata->base + status_offset);
- value &= chan_bitmask;
- writel(value, drvdata->base + clear_offset);
- return value;
+}
More of these tiny inline functions in this driver, and in this case there's a definite readability problem since this is not just "retrieve", it's reading and acking the interrupts which means that...
+static irqreturn_t lpass_pcm_mi2s_irq(int irq, void *data) +{
- struct snd_pcm_substream *substream = data;
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
- struct lpass_mi2s_data *drvdata =
snd_soc_platform_get_drvdata(soc_runtime->platform);- u32 xrun_bitmask = LPAIF_DMAIRQ_XRUN(LPAIF_DMA_RD_CH_MI2S);
- u32 period_bitmask = LPAIF_DMAIRQ_PER(LPAIF_DMA_RD_CH_MI2S);
- u32 error_bitmask = LPAIF_DMAIRQ_ERR(LPAIF_DMA_RD_CH_MI2S);
- u32 pending;
- irqreturn_t ret = IRQ_NONE;
- pending = lpass_lpaif_int_retrieve(soc_runtime->platform);
- if (unlikely(pending & xrun_bitmask) && snd_pcm_running(substream)) {
dev_warn(soc_runtime->dev, "%s: xrun warning\n", __func__);snd_pcm_stop(substream, SNDRV_PCM_STATE_XRUN);pending &= ~xrun_bitmask;ret = IRQ_HANDLED;- }
...all this code working out if we actually handled an interrupt might be lying.
- if (pending & period_bitmask) {
if (++drvdata->period_index >= runtime->periods)drvdata->period_index = 0;snd_pcm_period_elapsed(substream);pending &= ~period_bitmask;ret = IRQ_HANDLED;- }
If we miss an interrupt then...
+static snd_pcm_uframes_t lpass_pcm_mi2s_pointer(
struct snd_pcm_substream *substream)+{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
- struct lpass_mi2s_data *drvdata =
snd_soc_platform_get_drvdata(soc_runtime->platform);- snd_pcm_uframes_t offset;
- offset = drvdata->period_index * runtime->period_size;
- return offset >= (runtime->buffer_size) ? 0 : offset;
+}
...this will be broken. Why is the pointer operation not reading from the hardware?
+static struct snd_pcm_ops lpass_pcm_mi2s_soc_ops = {
- .open = lpass_pcm_mi2s_open,
- .hw_params = lpass_pcm_mi2s_hw_params,
- .hw_free = lpass_pcm_mi2s_hw_free,
- .prepare = lpass_pcm_mi2s_prepare,
- .mmap = lpass_pcm_mi2s_mmap,
- .pointer = lpass_pcm_mi2s_pointer,
- .ioctl = snd_pcm_lib_ioctl,
+};
Again the start and stopn are misplaced and should be in the trigger like with other drivers.
+static void lpass_pcm_mi2s_soc_free(struct snd_pcm *pcm) +{
- struct snd_pcm_substream *substream =
pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream;- struct snd_soc_pcm_runtime *soc_runtime = substream->private_data;
- struct lpass_mi2s_data *drvdata =
snd_soc_platform_get_drvdata(soc_runtime->platform);- lpass_pcm_mi2s_free_buffer(pcm, SNDRV_PCM_STREAM_PLAYBACK);
- disable_irq(drvdata->irqnum);
- free_irq(drvdata->irqnum, NULL);
This is weird, why the manual disable_irq()?