[alsa-devel] [PATCH - [hdspm] 0/3] Support for RME RayDAT and AIO

[Takashi Iwai]

At Tue, 18 Jan 2011 18:06:00 +0100,
Adrian Knoth wrote:
> 
> On 01/18/11 15:36, Takashi Iwai wrote:
> 
> Hi!
> 
> > Basically one patch should contain the changes that are build-clean,
> > i.e. you can build cleanly after patching.  Thus changing a header
> > and *.c in separate patches doesn't work well.
> 
> Ok. Better luck next time. ;) Speaking of which: should I resend
> everything as a whole-in-one patch?

That's OK.  At best, split a patch into some logical pieces
(i.e. MIDI handling changes, addition of RayDAT/AIO support,
code-refactoring, etc.)  But if this is hard to achieve, just "the big
one" is enough.

> I could also provide a download link (given we're talking 170+kb here)

This should be avoided.  The link content isn't archived, so we'll
loose the code history.  And it makes difficult to review.

> > Also, please use the generic variable types as much as possible
> > for ioctl structs. Avoid use of enum or such, especially ones that
> > are typedef'ed. In general, avoid typedefs in the kernel code as much
> > as possible. Defining each enum type is no-go.
> 
> If've dropped all the new typedefs and only kept the existing ones now.
> However, it still reads "enum foo something" in the structs for some
> ioctls.
> 
> Is this acceptable or do I have to change this to int? If so, would I
> manually add a couple of #defines?

You can use int (short or whatever) and keep enum as its values.

> I somehow feel losing the semantic
> connection to the enum is bad, but this might OTOH be intended, e.g. for
> the sake of a stable interface.

Exactly.

enum itself is nice to use, but the problem is that its size isn't
defined strictly, rather depending on its contents.  So, for ioctl,
types with fixed size is better.

> > Second, don't define arrays in the header file. They must be in the
> > body, not included headers.
> 
> Cross-checked with hdspmixer and moved to .c, except for the channel
> mappings as they're included from userspace. (channel_map*)

OK.

> > Also, remember checkpatch.pl is your friend.
> 
> It already was for the very last days. But I'm more excited about my new
> friend Lindent. ;)

Heh, but sometimes the sense of beauty conflicts between Lindent and
human being ;)  I recommend you to re-read the code after running it.


thanks,

Takashi