I2S: switch to bcm2835

[HiassofT]

Please don't merge, for now this is just to gather some feedback and discuss this topic.

@notro mentioned that noone had looked yet into what's needed to switch from bcm2708-i2s to bcm2835-i2s, so I had a shot at it.

I haven't cared about support for non-devicetree configs, this made things a lot easier. DMA setup via DT means we dont' have to care about setting the DMA slave id, GPIO setup can be dropped completely and I2S card drivers also find the the cpu_dai via the i2s-controller in DT.

When switching to DT-only configs it might be a good idea to remove the cpu_dai/platform names in the soundcard drivers and error-out if the DT nodes can't be found. I haven't tackled this yet.

With non-DT support stripped out the differences between the two I2S drivers aren't huge and replacing "2708" with "2835" made diffing quite easy :)

I've split the differences into a series of commits, separating the various features and mentioned the original commits in the commit messages.

Most of the changes are pretty straight forward, but 2 things aren't quite clear to me:

@pelwell you added a change to setup the clock only when the bcm is I2S master. This makes sense to me, but was there some issue behind that change? The original commit message is about DT...

@koalo how did you set the PCM period/buffer size parameters? With the recent switch to bcm2835-dma I noticed that DMA transfers larger than 32k (the new DMA code supports 64k-4) result in repeated clicking. I didn't have the time yet to have a closer look at it, but maybe there's something wrong with the PCM parameters.

So far I have only tested this code on my RPi2 with the Cirrus audio card. Recording and playback at various samplerates and 16/24/32 bit worked fine and MMAP support seems to be fine as well. I'll later also test on a RPi B and B+ with the Wolfson card, but feedback from other people would be great as I don't have any other I2S cards here.

[pelwell]

For the benefit of other reviewers I've created a Gist showing what you would have to do to the downstream driver to make it the same as @HiassofT's proposed new upstream driver. To do this I took the new driver and replaced all occurences of 2835 with 2708. The result is here: https://gist.github.com/pelwell/4df57791157259758cf6

As you can see, the differences are small, and amount to removing the non-DT support and switching to the devm_ API functions where possible.

[pelwell]

The clock setup change (HiassofT@ad7880d) came from one of our interns, Zoltan Szenczi, although I ended up folding it into my DT-support changes. I joined after he left, and there are no explanations for the change, but as you say, it makes sense.

[iqaudio]

@HiassofT let me have your postal address and I'll send you a Pi-DAC+, I'm snowed under at the moment and not sure when I'll get around to doing a test build :-( (gordon at iqaudio dot com)

@pelwell Phil, is there anything that would need to be done on the sound card driver / codecs here?

best regards,
Gordon

[pelwell]

@iqaudio Gordon, there is nothing that needs doing - I've just tested the patch and my Pi-DAC+ worked just fine. All of the sound drivers I've touched have an "i2s-controller" Device Tree property that identifies the I2S device node. As a result it doesn't matter that the name of the device changes from "bcm2708-i2s" to "bcm2835-i2s" provided that Device Tree is enabled. This is therefore another death knell for non-DT support, which is looking like it will end with the switch to the 4.2 kernel.

[HiassofT]

@iqaudio thanks a lot, just sent you an email

Sound card drivers don't need to be changed if devicetree is used - and it looks like support for non-devicetree configs will be dropped very soon. That's also the reason why I didn't care for non-DT support in this PR.

Usually sound card drivers link(ed) to the I2S driver via cpu_dai and platform name. For example:

static struct snd_soc_dai_link snd_rpi_iqaudio_dac_dai[] = {
...
        .cpu_dai_name   = "bcm2708-i2s.0",
...
        .platform_name  = "bcm2708-i2s.0",

But in the probe function these names are cleared and instead the _of fields of the struct are filled with the devicetree node if the I2S driver - if devicetree support is enabled:

                dai->cpu_dai_name = NULL;
                dai->cpu_of_node = i2s_node;
                dai->platform_name = NULL;
                dai->platform_of_node = i2s_node;

Support for non-devicetree configs would be a little bit tricky, either we had to change all cpu_dai/platform names in the soundcard drivers to "bcm2835-i2s.0" or - as a crude hack - change the name under which bcm2835-i2s registers to bcm2708-i2s.

edit: oops, pelwell was faster :)

[iqaudio]

Cool - thanks for the clarification.

Gordon

On 13 Oct 2015, at 14:59, Phil Elwell notifications@github.com wrote:

@iqaudio Gordon, there is nothing that needs doing - I've just tested the patch and my Pi-DAC+ worked just fine. All of the sound drivers I've touched have an "i2s-controller" Device Tree property that identifies the I2S device node. As a result it doesn't matter that the name of the device changes from "bcm2708-i2s" to "bcm2835-i2s" provided that Device Tree is enabled. This is therefore another death knell for non-DT support, which is looking like it will end with the switch to the 4.2 kernel.

—
Reply to this email directly or view it on GitHub.

[pelwell]

edit: oops, pelwell was faster :)

It doesn't happen often. If I'd taken a bit longer I'd have remembered that even with this patch non-DT builds would work because the board support code still creates a platform device for bcm2708-i2s. It's only when that driver (and/or the platform device data) is removed that there will be a problem.

[HiassofT]

On Tue, Oct 13, 2015 at 07:07:54AM -0700, Phil Elwell wrote:

It's only when that driver (and/or the platform device data) is removed that there will be a problem.

Yes, ATM this will still work fine. I did a very quick test with a
non-DT kernel and bcm2708-i2s was loaded.

When looking at the current drivers I noticed that the RaspiDAC 3
depends only on the bcm2708-i2s driver, not on 2708 || 2835 like
the other ones. I've added a commit to fix this, so all drivers
should now build fine even if bcm2708-i2s is disabled.

I also did a RPi 1 build and everything looked fine there as well.

[popcornmix]

@HiassofT can you rebase this?
@pelwell @notro are you happy with this?

[notro]

I have no objections except that I would really like to see Signed-off-by in the commit messages. Especially those that touch mainline drivers. This would also make it easy for someone else to take the patch upstream.

[pelwell]

Yes, absolutely.

[HiassofT]

OK, branch is rebased and I changed the commit messages to make checkpatch happy. I also slightly modified the clock setup commit, changed the if to a switch statement, this makes it more readable and eliminates the checkpatch line length warning. Hope this is OK so now, if not drop me a line.

BTW: thanks a lot to Gordon for sending my a Pi-DAC+, I tested it at various samplerates and bit-depths and it works fine with this patch.

[popcornmix]

Thanks.

[clivem]

Tested at 44k1, 88k2, 96k, 176k4, and 192k sample rates, at 16, 24, and 32 bit depths, on Pi2B with external ES9018 I2S DAC. Thumbs up!