Re: [Patch V3 07/10] ASoC: ipq806x: Add I2S PCM platform driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3561 bytes --]

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()?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]