Add support for the AudioInjector Octo sound card, simplify bcm2835_i2s stereo check.

[flatmax]

This patch set does the following :
Adds support for the AudioInjector Octo sound card.
Simplifies the stereo channel check in the bcm2835_i2s.c driver.

The old switch statement for testing stereo channels is replaced with an if condition on mono channels.

[flatmax]

Phil, I have switched the order in the README and Makefile - as requested

[pelwell]

Can you rebase and force push to tidy up the PR?

[HiassofT]

again, please drop the I2S changes.

[flatmax]

Matthias, the I2S changes are entirely compliant. It checks that we aren't specifying mono (if so exiting with error as per before) and if stereo, it is setting the I2S format.

If you want an I2S compliant driver, then this bcm2835_i2s.c is entirely compliant.

[HiassofT]

The I2S patch doesn't simplify anything, is not needed and removes important semantics. IMO it makes the code harder to read.

The way that code part is currently implemented it's obvious that the chip only supports 2 channels.

Your patch changes the semantics of the code to "every setup excecpt 1 channels" are supported. Plus, the code makes no sense, there's still the channels_min = channels_max = 2 in the dai_driver.

[flatmax]

My original point was exactly that, why even test for mono when channels_min = channels_max = 2 ? Also why write 7 lines of code to test what we can in 1 line ?

…

On 08/03/17 21:44, Matthias Reichl wrote: The I2S patch doesn't simplify anything, is not needed and removes important semantics. IMO it makes the code harder to read. The way that code part is currently implemented it's obvious that the chip only supports 2 channels. Your patch changes the semantics of the code to "every setup excecpt 1 channels" are supported. Plus, the code makes no sense, there's still the channels_min = channels_max = 2 in the dai_driver. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1884 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq6B8zDPcKmPyxTRBZIh0K-9yOF4xjDks5rjoZ_gaJpZM4MWmsI>.

[HiassofT]

Simple answer: because it's a lot more readable and understandable. You look at the code part you changed and immediately know that only 2 channels will work, not 3 or 13.

[pelwell]

@flatmax After some debate we're happy to accept your one-line "case 8" patch and take the (probably non-existent) hit of supporting it downstream.

Just replace the bcm2835-i2s patch here and force push - I've already checked the rest is good - and I'll merge it.

[HiassofT]

@pelwell if you are going to relax the channel constraint in bcm2835-i2s there are several things to keep in mind: You'll need to enforce channel symmetry (symmetric_channels=1) otherwise configurations like 2 recording channels and 8 playback would not error out - hw_params doesn't do any checks if a stream is already running, it's not prepared for asymmetric configurations. All current and future machine drivers with codecs supporting 8 or more channels (usually in DSP mode) need do install additional constraints to limit to 2 channels. Otherwise userspace could try to access them in configurations that won't work. I'd rather see the octo driver being implemented properly, following alsa standards, than introducing issues plus the need to add patches to other soundcard drivers.

[pelwell]

I'm braced for the flood of issues.

[flatmax]

I have made those changes. thanks Matt

…

On 09/03/17 02:49, Phil Elwell wrote: I'm braced for the flood of issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1884 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAq6B7mnpQBLCXH_eHvnPyiLSH7ekm2Pks5rjs4RgaJpZM4MWmsI>.