Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension

From: Mark Brown <broonie@kernel.org>
To: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Rob Herring <robh@kernel.org>,
	alsa-devel@alsa-project.org, Liam Girdwood <lgirdwood@gmail.com>,
	linux-kernel@vger.kernel.org,
	Alexey Brodkin <Alexey.Brodkin@synopsys.com>,
	Takashi Iwai <tiwai@suse.com>,
	devicetree@vger.kernel.org,
	Carlos Palminha <CARLOS.PALMINHA@synopsys.com>,
	linux-snps-arc@lists.infradead.org
Subject: Re: [PATCH 3/4 v7] ASoC: dwc: Add PIO PCM extension
Date: Tue, 24 May 2016 18:51:44 +0100	[thread overview]
Message-ID: <20160524175144.GO8206@sirena.org.uk> (raw)
In-Reply-To: <57448A42.9050606@synopsys.com>

[-- Attachment #1.1: Type: text/plain, Size: 2027 bytes --]

On Tue, May 24, 2016 at 06:07:14PM +0100, Jose Abreu wrote:
> On 24-05-2016 17:41, Mark Brown wrote:

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> >>  	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
> >>  		i2s_write_reg(dev->i2s_base, ITER, 1);

> > That seems wrong, or at least something that should be separate?
> > Previously we needed interrupts for DMA operation but now we enable
> > interrupts only if we don't use DMA.  It feels like we want to make the
> > change for DMA separately if only to make it clear for bisection, are we
> > 100% sure that masking the interrupt won't also mask the DMA request
> > signals?

> Indeed I thought about this and the interrupts must also be enabled when in DMA
> mode. Although there is no interrupt handler in the original driver (without
> this patches) in some setups the interrupt line may be connected to the DMA
> controller. I will drop this change and always enable interrupts. Please note
> that I don't have a setup with DMA support so I can only test using the PIO mode.

Presumably you can talk to your hardware colleagues and get them to make
you a FPGA with a DMA IP available?

> > This also seems wrong.  We're forcing PIO if an interrupt is provided
> > rather than based on DMA being configured which means that if the
> > interrupt is wired up and happens to be described in DT we'll get worse

> How should I then determine which mode to use?
>     - Check if DMA parameters are declared in DT, or
>     - Check if snd_dmaengine_pcm_register() fails, or
>     - Assume PIO mode will be used when compiling with PIO PCM, or
>     - Something else ?

You could either unconditionally register the PIO driver and only
actually start using it if the driver is instantiated or you could check
to see if the registration function works (handling deferred probe - if
the DMA driver just didn't load yet you should wait for it).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]