login
Search
From: Márton Németh <nm127@freemail.hu>
Clean up the following errors and warnings reported by checkpatch.pl:
- space prohibited between function name and open parenthesis '('
- space required before the open brace '{'
- code indent should use tabs where possible
- line over 80 characters
- spaces required around that '=' (ctx:VxW)
Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--- a/drivers/net/8139too.c 2008-06-14 00:20:47.000000000 +0200
+++ b/drivers/net/8139too.c 2008-06-14 14:08:18.000000000 +0200
@@ -11,7 +11,7 @@
-----<snip>-----
- Written 1997-2001 by Donald Becker.
+ Written 1997-2001 by Donald Becker.
This software may be used and distributed according to the
terms of the GNU General Public License (GPL), incorporated
herein by reference. Drivers based on or derived from this
@@ -116,8 +116,8 @@
/* Default Message level */
#define RTL8139_DEF_MSG_ENABLE (NETIF_MSG_DRV | \
- NETIF_MSG_PROBE | \
- NETIF_MSG_LINK)
+ NETIF_MSG_PROBE | \
+ NETIF_MSG_LINK)
/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
@@ -134,7 +134,8 @@
#if RTL8139_DEBUG
/* note: prints function name for you */
-# define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
+# define DPRINTK(fmt, args...) \
+ printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
#else
# define DPRINTK(fmt, args...)
#endif
@@ -143,10 +144,10 @@
# define assert(expr) do {} while (0)
#else
# define assert(expr) \
- if(unlikely(!(expr))) { \
- printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
- #expr,__FILE__,__FUNCTION__,__LINE__); \
- }
+ if (unlikely(!(expr))) { \
+ printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
+ #expr, __FILE__, __FUNCTION__, __LINE__); \
+ }
#endif
@@ -196,7 +197,8 @@ static int debug = -1;
Threshold is bytes ...We don't write comments like this. -- Stefan Richter -=====-==--- -==- -===- http://arcgraph.de/sr/ --
From: Márton Németh <nm127@freemail.hu>
Clean up the following errors and warnings reported by checkpatch.pl:
- space prohibited between function name and open parenthesis '('
- space required before the open brace '{'
- code indent should use tabs where possible
- line over 80 characters
- spaces required around that '=' (ctx:VxW)
Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--- a/drivers/net/8139too.c 2008-06-14 00:20:47.000000000 +0200
+++ b/drivers/net/8139too.c 2008-06-14 20:32:45.000000000 +0200
@@ -11,7 +11,7 @@
-----<snip>-----
- Written 1997-2001 by Donald Becker.
+ Written 1997-2001 by Donald Becker.
This software may be used and distributed according to the
terms of the GNU General Public License (GPL), incorporated
herein by reference. Drivers based on or derived from this
@@ -116,8 +116,8 @@
/* Default Message level */
#define RTL8139_DEF_MSG_ENABLE (NETIF_MSG_DRV | \
- NETIF_MSG_PROBE | \
- NETIF_MSG_LINK)
+ NETIF_MSG_PROBE | \
+ NETIF_MSG_LINK)
/* enable PIO instead of MMIO, if CONFIG_8139TOO_PIO is selected */
@@ -134,7 +134,8 @@
#if RTL8139_DEBUG
/* note: prints function name for you */
-# define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
+# define DPRINTK(fmt, args...) \
+ printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
#else
# define DPRINTK(fmt, args...)
#endif
@@ -143,10 +144,10 @@
# define assert(expr) do {} while (0)
#else
# define assert(expr) \
- if(unlikely(!(expr))) { \
- printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
- #expr,__FILE__,__FUNCTION__,__LINE__); \
- }
+ if (unlikely(!(expr))) { \
+ printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
+ #expr, __FILE__, __FUNCTION__, __LINE__); \
+ }
#endif
@@ -196,7 +197,9 @@ static int debug = -1;
Threshold is bytes ...Is 8139too in active development or are there people actively fixing current bugs in it? If not, a whitespace cleanup may be considered a waste of time. There are even a few valid arguments that such changes Did you check that your whitespace changes are indeed only whitespace changes, i.e. that resulting assembler is unchanged? If you checked it, Why change this? (Besides for the reason that checkpatch says that new patches /should/ This line is merely 6 characters too long and does not contain anything This is not Linux multi-line comment style. (It comes close to the present style of 8139too.c though and may therefore be acceptable.) This decreases readability. You could leave this merely 81 columns long line as is. Or remove the superfluous parentheses. This printk trick does not work BTW. But it should of course not be fixed in a patch which is meant to be a whitespace change only. [...] -- Stefan Richter -=====-==--- -==- -===- http://arcgraph.de/sr/ --
A more useful thing would be to merge 8139too and 8139cp drivers together so user can just put RTL8139 card in the box and it will work. The current state is completely broken - two drivers for two versions of the chip which have -- Ondrej Zary --
Ondrej Zary <linux@rainbow-software.org> : I am curious to know which distro is not able to handle these drivers for the user. The drivers have little in common and the 8139cp driver hints loudly when it does not recognize the device. -- Ueimor --
Yes, I checked this. When I compared the "objdump -d" output I found that
only the function calls are changed where the __LINE__ is a parameter.
Sorry about taking your time. My intention was to make the 8139too source
code better as I am using this driver actively to communicate with the
digital world. I have chosen the checkpatch.pl's output to compare the
whether the old and the new code are better or not. Maybe that was a mistake.
I would like to try it again, but in a different way. I collected here
the different problems reported by checkpatch.pl. I divided it to two
groups according to your comments which I think it worth to deal with:
+ ERROR: Macros with complex values should be enclosed in parenthesis
+ WARNING: __func__ should be used instead of gcc specific __FUNCTION__
+ WARNING: plain inline is preferred over __inline__
+ WARNING: Use #include <linux/io.h> instead of <asm/io.h>
+ WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
and the second group which it seems that may hurt somebody:
- ERROR: code indent should use tabs where possible
- ERROR: space required after that ',' (ctx:VxV)
- ERROR: space required before the open brace '{'
- ERROR: space required before the open parenthesis '('
- ERROR: spaces required around that '=' (ctx:VxW)
- ERROR: trailing statements should be on next line
- WARNING: braces {} are not necessary for single statement blocks
- WARNING: do not add new typedefs
- WARNING: line over 80 characters
- WARNING: space prohibited between function name and open parenthesis '('
This is a much smaller set of changes. What do you think?
Regards,
Márton Németh
--
From: Márton Németh <nm127@freemail.hu>
Clean up the following errors and warnings reported by checkpatch.pl:
+ ERROR: Macros with complex values should be enclosed in parenthesis
+ WARNING: __func__ should be used instead of gcc specific __FUNCTION__
+ WARNING: plain inline is preferred over __inline__
+ WARNING: Use #include <linux/io.h> instead of <asm/io.h>
+ WARNING: Use #include <linux/uaccess.h> instead of <asm/uaccess.h>
The changes were verified with by comparing the "objdump -d 8139too.ko"
output which is exactly the same for the old and new version in case of
config CONFIG_8139TOO=m, CONFIG_8139TOO_PIO=n, CONFIG_8139TOO_TUNE_TWISTER=n,
CONFIG_8139TOO_8129=n, CONFIG_8139_OLD_RX_RESET=n.
Software versions used: gcc 4.2.3, objdump 2.18.0.20080103, on elf32-i386.
Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--- a/drivers/net/8139too.c 2008-06-14 00:20:47.000000000 +0200
+++ b/drivers/net/8139too.c 2008-06-15 09:10:14.000000000 +0200
@@ -107,8 +107,8 @@
#include <linux/mii.h>
#include <linux/completion.h>
#include <linux/crc32.h>
-#include <asm/io.h>
-#include <asm/uaccess.h>
+#include <linux/io.h>
+#include <linux/uaccess.h>
#include <asm/irq.h>
#define RTL8139_DRIVER_NAME DRV_NAME " Fast Ethernet driver " DRV_VERSION
@@ -134,7 +134,7 @@
#if RTL8139_DEBUG
/* note: prints function name for you */
-# define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __FUNCTION__ , ## args)
+# define DPRINTK(fmt, args...) printk(KERN_DEBUG "%s: " fmt, __func__ , ## args)
#else
# define DPRINTK(fmt, args...)
#endif
@@ -145,7 +145,7 @@
# define assert(expr) \
if(unlikely(!(expr))) { \
printk(KERN_ERR "Assertion failed! %s,%s,%s,line=%d\n", \
- #expr,__FILE__,__FUNCTION__,__LINE__); \
+ #expr, __FILE__, __func__, __LINE__); \
}
#endif
@@ -219,7 +219,7 @@ enum {
#define RTL8139B_IO_SIZE 256
#define RTL8129_CAPS HAS_MII_XCVR
-#define ...applied, thanks for whittling down the changes to this set --
Well, running checkpatch on source files (rather than patches) and
fixing the files up according to checkpatch's output and own good
judgement has two uses:
- bring new unmerged code into shape before submission,
- bring older mainline code into the canonical form as a basis
for further work. You certainly saw how lots and lots of such
checkpatch-assisted cleanups went into arch/x86. That's because
they were found useful for the work on unifying the two x86
architecture subtrees.
So, a coding style rework has its use even on legacy code if people have
plans with the code. But keep in mind that (a) the whitespace rules
aren't hard and universally agreed upon rules, (b) coding style has a
number of other aspects of arguably greater importance than whitespace
style, like proper modularization, good choices of names, use of common
idioms and APIs instead of own inventions, and so on.
--
Stefan Richter
-=====-==--- -==- =----
http://arcgraph.de/sr/
--
On Mon, 16 Jun 2008 17:07:38 +0200 There is no point in doing this kind of checkpatch cleanup on its own. It is worth doing more serious style work and rewriting of older drivers, in the areas where other work needs to be done. Please work on drivers that are ugly old vendor code, and/or you can find someone with the hardware to test it. --
These patches, which simply play around with white space, are undesirable, in my opinion. It doesn't make the code better, just changes the aesthetics to one that's more to your liking, and surely it makes it less pleasing to others. I see no benefit in such changes, as opposed to a certain cost and risk, which all changes carry. Every change requires an expense of other people's brain cycles, if I can put it that way, and on this sort of change, I sincerely think it's pointless. Sorry to come down all negative. --
| Greg KH | Og dreams of kernels |
| Jens Axboe | [PATCH 31/33] Fusion: sg chaining support |
| Arnd Bergmann | Re: |