At Fri, 3 Jul 2009 11:36:03 +0530, Koul, Vinod wrote:

Following emails are a of series 13 patches for Intel SST drivers (including documentation). These drivers are for the audio engine/sound card in upcoming Intel based Mobile Internet Devices (MID) platform

There are 2 drivers involved. One is for the audio DSP engine that can decode and do audio processing and one is for the ALSA sound card driver with support for three different sound cards that are supported on the platform.

The dependant drivers for the platform are not yet available in the upstream. So please consider these patches only for RFC and provide your comments/suggestions.

The audio firmware binary associated with this platform that is downloaded by the driver at runtime will be shared in a public location and made available in firmware git when we release the drivers for up streaming.

The patches are compiled with 0 warnings and checkpatch-ed.

The first patch in the series [RFC 1/13] Intel SST driver's documentation gives the documentation about the drivers and the interface for the audio DSP driver. The rest of the patches that follow have a brief description about the contents.

First off, thanks for patches! They are lengthy, so I just took a quick glance over them without reading deeply inside. Some comments have been already sent back.

In general, please fix the following:

- Check whether each function is really global. Make functions static as much as possible.

This improves not only the compiler optimization but also the readability. (Without static, you'll have to remember that this function can be called from the outside.)

- Put blank lines appropriately. Sometimes hard to read too stuffed texts.

- Avoid unnecessary variable initializations at the beginning of functions. I know there are old school's text books of C recommending such a thing. But, it's bad. It just hides possible logical mistakes. Modern compilers would warn anyway uninitialized variables.

- Avoid the expression like if (0 == foo). This is annoying because the expression is illogical. We compare foo with 0, not vice versa.

I know this as yet another bad habit by some C books (to avoid a mistake like if (foo = 0)), but such a mistake would be, again, warned by a compiler. So, don't use tricks.

HTH,

Takashi

On Fri, Jul 03, 2009 at 11:36:03AM +0530, Koul, Vinod wrote:

There are 2 drivers involved. One is for the audio DSP engine that can decode and do audio processing and one is for the ALSA sound card driver with support for three different sound cards that are supported on the platform.

Looking at this I'm wondering how general purpose the DSP on these parts is. This sort of integrated DSP is fairly common in modern embedded CPUs but normally the DSP is exposed in a more general fashion, allowing the DSP to be used for non-audio purposes as well which means that the DSP management code tends to live outside the sound tree.

Given what you're saying about Intel-provided firmwares it is possible that while the hardware is capable of general purpose use you're not exposing sufficient information for people to actually write their own firmware in which case that's less of an issue and the situation is closer to that with things like WiFi cards but I thought it was better t ask.