Message-ID: <4A2D51E6.6030607@s5r6.in-berlin.de>
Date:	Mon, 08 Jun 2009 20:01:10 +0200
From:	Stefan Richter <stefanr@...6.in-berlin.de>
To:	Pranith Kumar <pranith.hacks@...il.com>
CC:	greg@...ah.com, linux-kernel@...r.kernel.org, akpm@...l.org
Subject: Re: [TRIVIAL][PATCH 1/1] Fix warning in staging/otus/ioctl.c

Pranith Kumar wrote:
> Stefan Richter wrote:
...
>> This code uses defined types which are foreign to Linux.  We don't
>> define UCHAR in Linux.  /This/ needs to be fixed in the entire driver.
>> Until this is not done, there is no reason to add this pointer type cast
...
> Thanks for your comment. Going through the header, I found the following
> defines
> 
> typedef unsigned char		UINT8;
> typedef unsigned short		UINT16;
...
> Now if I delete all these defines and do a search and replace for those
> types, is it OK?
> 
> There might be an argument that the current state is much cleaner. 

Plain C's unsigned char and friends should be preferred.  Pointer types 
like PVOID should be replaced by straight-forward "void *" and so on, 
and BOOLEAN can be replaced by "bool" (with "true" and "false" as 
possible values).

In cases where the particular size of variables or struct members 
matters, there are u8, s16 etc. available.

Furthermore, when variables don't represent CPU endian (host endian) 
data but actually big endian or little endian, then we annotate the 
types with __be32, __le32 etc..  *However*, adding these endian 
annotations (*if* the driver in question has to deal with endianess 
other than CPU) should usually better be done in another separate patch 
or series of patches.  This is because the process of adding the endia 
annotations has a chance of unveiling sloppy or non-protable or even 
buggy code, and then some more intensive and non-trivial work on the 
code will be required.

Lastly, if there is a binary interface to userspace (e.g. character 
device file interface), then we use types like __u32 in header files 
which are going to be used by userspace programs.

Now, the type replacement will of course shuffle the text flow (line 
lengths...), so the question will arise whether to adjust whitespace 
(line wraps) in the same patch set.  However, the code which you looked 
at also has other trivial deviations from kernel style.  Notably, the 
use of CamelCase names rather than all-lowercase with underscores. 
Example:  Something like pClonedPkt should become p_cloned_pkt or better 
p_cloned_packet or cloned_packet (if the p prefix didn't mean anything 
else than that it was a pointer), or if it is clear from context, 
something brief like cp.

Such style adjustments also need to happen; if you are interested in 
doing that, then you can plan ahead and hold off with whitespace 
adjusting changes (indentation, line wraps...) until after you did those 
other changes regarding type names and variable/ function/ macro names.

In any case, before starting some bigger style adjustments, check the 
TODO file of the respective staging driver and notify Greg and 
devel@...uxdriverproject.org, so that clashes with ongoing work by 
others can be avoided.

Moreover, you should probably _not_ start such work on wireless drivers 
in staging like rt2860.  See the notes in the TODO files of these 
drivers; the real work is going on at other drivers for the same 
hardware in the normal Linux wireless development tree.
-- 
Stefan Richter
-=====-==--= -==- -=---
http://arcgraph.de/sr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists