S2io: cosmetic changes
Subsystems (2)
PATCH 1/8 S2io: cosmetic changes
Hi All,
This is the first patch in the 8 part series. I'll be sending the remaining immediately following this patch.
This first patch contains cosmetic changes such as indentation, change in comment styles, variable name changes etc.
Randy, we'll implement your comment about C99 syntax along with all the other comments received, in the next submission.
Quoted 3 lines · Ravinandan Arakali
Hi All,
This is the first patch in the 8 part series. I'll be sending the remaining immediately following this patch.
OK this is MUCH improved, thanks much.
There is one final problem with your patches: your email program (Outlook, it appears?) is mangling patches, preventing them from being applied by a script that looks directly at the email. Example from the first few lines of your first patch:
Quoted 14 lines
drivers/net/s2io.c +0
-0@@ -69,7 +69,7 @@=20
/* S2io Driver name & version. */
static char s2io_driver_name[] =3D "s2io";drivers/net/s2io.c +1
-1-static char s2io_driver_version[] =3D "Version 1.0"; +static char s2io_driver_version[] =3D "Version 1.7.5.1";=20
#defineLINK_IS_UP(val64)(!(val64 & (ADAPTER_STATUS_RMAC_REMOTE_FAULT=
| \drivers/net/s2io.c +0
-0ADAPTER_STATUS_RMAC_LOCAL_FAULT)))@@ -99,45 +99,45 @@ };
Quoted 2 lines
=20
static char ethtool_stats_keys[][ETH_GSTRING_LEN] =3D {
Now we move on the content portion of the review :)
Quoted 6 lines
This first patch contains cosmetic changes such as indentation, change in comment styles, variable name changes etc.
Randy, we'll implement your comment about C99 syntax along with all the other comments received, in the next submission.
Signed-off-by Raghavendra Koushik <[email protected]>
Patch 1 comments:
1) undescribed non-cosmetic change:
Quoted 7 lines
drivers/net/s2io.c +1
-1@@ -1240,82 +1290,72 @@* force link down. Since link is already up, we will get * link state change interrupt after this reset */ -writeq (0x8007051500000000ULL , &bar0->dtx_control); +writeq (0x80010515001E0000ULL , &bar0->dtx_control); val64 =readq (&bar0->dtx_control);
drivers/net/s2io.c line 1303
Quoted 10 lines
drivers/net/s2io.c +4
-1-writeq (0x80070515000000E0ULL , &bar0->dtx_control); +udelay (50 ); +writeq (0x80010515001E00E0ULL , &bar0->dtx_control); val64 =readq (&bar0->dtx_control); +udelay (50 );writeq (0x80070515001F00E4ULL , &bar0->dtx_control); val64 =readq (&bar0->dtx_control); +udelay (50 );return SUCCESS ;
Resolution: describe change in patch description, or move change to another patch
2) undescribed non-cosmetic change:
drivers/net/s2io.c line 1307
Quoted 5 lines
drivers/net/s2io.c +1
-1#ifdef CONFIG_S2IO_NAPI dev->poll = s2io_poll; - dev->weight =128 ;/* For now. */ + dev->weight =90 ;/* For now. */ #endif
Resolution: describe change in patch description, or move change to another patch
3) non-cosmetic change, that should not be present in upstream sources:
drivers/net/s2io.c line 1310
Quoted 4 lines
drivers/net/s2io.c +2
-1+#ifdef SET_ETHTOOL_OPS SET_ETHTOOL_OPS (dev, &netdev_ethtool_ops); - +#endif
Compatibility defines should be kept in an external package, to keep the kernel source relatively clean. There exist compatibility packages such as "kcompat" (http://sf.net/projects/gkernel/) that allow you to develop a driver using the current kernel API, and then add the needed compat definitions in a separate module.
Resolution: remove #ifdef
4) non-cosmetic change, that should not be present in upstream sources:
drivers/net/s2io.c line 1313
Quoted 3 lines
drivers/net/s2io.c +3
-0+#ifndef SET_ETHTOOL_OPS +#define SUPPORTED_10000baseT_Full (1 << 12) +#endif
Resolution: remove all three lines I quoted
5) don't add this #ifdef either:
drivers/net/s2io.c line 1316
Quoted 3 lines
drivers/net/s2io.c +2
-0+#ifdef SET_ETHTOOL_OPS static struct ethtool_ops netdev_ethtool_ops; +#endif
More comments follow in upcoming emails.