Date:	Tue, 28 Oct 2014 21:34:26 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Max Filippov <jcmvbkbc@...il.com>
Cc:	alsa-devel@...a-project.org,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-xtensa@...ux-xtensa.org" <linux-xtensa@...ux-xtensa.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Takashi Iwai <tiwai@...e.de>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Grant Likely <grant.likely@...aro.org>
Subject: Re: [PATCH] ASoC: add xtensa xtfpga I2S interface and platform

On Tue, Oct 28, 2014 at 09:11:34PM +0300, Max Filippov wrote:
> On Tue, Oct 28, 2014 at 8:38 PM, Mark Brown <broonie@...nel.org> wrote:

> > You *really* need to explain how it's supposed to work - right now it's
> > not at all obvious, like I say the fact that this is a rarely used idiom
> > is not helping.  For example when we tear down the stream we just assign
> > the pointer in _stop() but don't bother with a sync until the stream is
> > closed - why?

> Because we can't wait in stop and syncing is not time critical, we can
> do it any time before the stream becomes invalid.

To be clear: the important part is that someone reading the code can
understand what's going on.

> >> > it'd be better to just add a DMA controller on the FPGA, everyone will
> >> > be much happier).

> >> Hardware people think different and it's been like that for more than 5
> >> years.

> > They appear to be the only hardware people who think this way, while we

> The whole audio subsystem on xtfpga boards is there for, I think, a single
> purpose: for the demonstration of CPU audio processing capabilities.
> That's why it's that simple.

This means that the demos include cycles spent taking interrupts and
stuffing data into the FIFO (with associated cache impacts) instead of
signal processing which isn't usually helpful for benchmarks.

> >> hw_params callback can change MCLK rate, so it has to disable and
> >> enable the clock anyway, and since enable can fail it does not guarantee
> >> that the clock will be left in the same state. Or should I adjust MCLK rate
> >> w/o disabling the clock?

> > So yet again: why not just enable the clock only when the device is in
> > use?  If it's being configured it stands to reason that the device isn't
> > actively in use...

> Mark, I don't get it, sorry ): My clock synthesizer is I2C controlled, so
> I can't prepare/unprepare it in the trigger callback. When should I do it?

Runtime PM is the normal way of doing it.

> >> The level field in the control register is 4 bit wide, so the allowed range of
> >> level is 0..15. FIFO size is 8192 entries, level = 1 corresponds to
> >> FIFO size / 2, level = 14 -- to FIFO size of 0. I guess this function
> >> won't get period_size of 0?

> > So if the IP gets changed and the code gets blown up this could well
> > explode then...  which doesn't seem entirely unlikely considering this
> > is a FPGA platform so presumably this is easy to update.  To repeat this
> > is about clarity and the code looking like it's probably hiding bugs as
> > much as if the code actually works if you really sit down and study it.

> The calculation does not depend on the actual hardware, but on the
> constant definitions in the same file. They need to be updated if the
> hardware changes. I'll try to rewrite it in a cleaner way.

Right, my point is that if someone changes the hardware they'll just
update the constants and then things will break.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)

Powered by blists - more mailing lists