[Meego-kernel] [PATCH 3/3] intel_mid_i2s, alsa_ssp and cmt_speech drivers

[Kai.Vehmanen at nokia.com]

Hi,

On 25 Sep 2010, Arjan van de Ven wrote:
>general comment: who wrote this code? I see Nokia mentioned in some of
>the files as author, others lack any and all copyright holder information..

I believe this is based on the N900 cmt_speech driver that I'm
one of the authors of. The original driver is available in Maemo5 SDK,
and a newer for Meego is at:
http://gitorious.org/nokia-n900-kernel/nokia-n900-kernel
http://gitorious.org/nokia-n900-kernel/nokia-n900-kernel/commit/7d98d2214bab6fbd10779a36f4846c7d1166a8c7

While I agree this is not the best starting point, as this driver is not yet 
in upstream, but OTOH, as there aren't any other similar drivers in the kernel, 
there's not a lot of choise. :P At least I'm happy to see the user-space interface 
reused (makes a lot of sense). My excuse for why the N900 driver is not in 
upstream is that it depends on the SSI/HSI framework and it has not made it 
in (first attempt in 2008: http://lwn.net/Articles/301918/, new attempt this 
year -> http://lwn.net/Articles/386792/).
 
>... yet the patch starts with a From: you and a Signed-off-by: you....
>is that all kosher?

So yes, this needs cleaning up.

With that foreword, thanks for the review! Some of the comments do apply
to the N900 driver version as well (especially the GFP_ATOMIC allocs, oops,
will fix those).

>+#define CS_IO_MAGIC           'C'
>+
>+#define CS_IOW(num, dtype)    _IOW(CS_IO_MAGIC,  num, dtype)
>+#define CS_IOR(num, dtype)    _IOR(CS_IO_MAGIC,  num, dtype)
>+#define CS_IOWR(num, dtype)   _IOWR(CS_IO_MAGIC, num, dtype)
>+#define CS_IO(num)            _IO(CS_IO_MAGIC,   num)
>
>are these ioctl numbers globally reserved ?
[...]
>(and what's with the weird abstraction)

Taking the blame for these as well. As at least for N900, as we could
not get the driver upstream, the ioctl number was never reserved.
Now that it seems we have multiple drivers (well at least two, both still out 
of upstrem trees though), and implementing the same ioctl interface, 
it's probably about time to consider reserving the numbers.

>+/** Deprecated V0.1.x interface (CS_CONFIG ioctl) */
>+struct cs_config {
>
>eh... already deprecated interfaces in a new driver???

So this bit comes directly from N900 legacy (and can be dropped 
from the patch).

PS For those wondering, what the heck this code is about, these drivers are used
   to transfer speech frames to/from the cellular modem (as is used by 
   Meego handset-ux code). The interface is in theory close to ALSA, but in 
   practise has very specific and funky timing reqs plus requiments to transfer
   some side-info (so that at least in case of N900, ALSA could not be used), and 
   thus the new ioctl interface. This is fairly new stuff, so not too many modems 
   support this  type of use yet (-> explains lack of existing similar drivers).
 
Br,
-- 
Kai Vehmanen