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