Message-ID: <CAMo8BfL+vW7BjC1bQPNjhuBMTcRTkf8_DDS3NO2HmBUnfHYAFQ@mail.gmail.com>
Date:	Tue, 28 Oct 2014 21:11:34 +0300
From:	Max Filippov <jcmvbkbc@...il.com>
To:	Mark Brown <broonie@...nel.org>
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 8:38 PM, Mark Brown <broonie@...nel.org> wrote:
> On Tue, Oct 28, 2014 at 08:00:45PM +0300, Max Filippov wrote:
>> On Tue, Oct 28, 2014 at 6:42 PM, Mark Brown <broonie@...nel.org> wrote:
>> > So atomics don't work?  Simple flags are one of the few cases where they
>> > might cover it...  again, the fact that this isn't similar to other code
>> > doing the same thing is worrying.
>
>> tx_substream use pattern is the same as of a typical RCU variable.
>> RCU wrappers combine ACCESS_ONCE and barriers that I'd otherwise
>> have to write myself.
>
> 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.

> We also appear to be doing several different dereferences
> of the pointer inside a single rcu_read_lock() for some reason which is
> worrying.

I've cleaned that up.

>> > 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.

> do have some FIQ based PIO stuff in mainline all the examples I can
> think of are there because people didn't work out how to drive the DMA
> controller yet (and even there we're using FIQs not bashing things in
> from a regular interrupt).

Currently we don't support fast interrupt handlers written in C on xtensa.

>> >> Because this callback is said to be potentially called multiple times per
>> >> initialization. Is it not?
>
>> > It can but but I'm not seeing any connection between that and the idea
>> > of not keeping the clock enabled when the device isn't in use?
>
>> 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?

>> > I'm not sure I find that terribly convincing, I'd like to be able to
>> > look at the code and tell that it's not going to blow up.  This again
>> > comes back to the general comment about clarity - the code *looks*
>> > suspicious (strange indentation of the for loop with a comment in the
>> > middle of the statement itself for example).
>
>> 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.

-- 
Thanks.
-- Max
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists