Re: [PATCH 2.6.9-rc2 1/8] S2io: cosmetic changes

[Jeff Garzik <jgarzik@xxxxxxxxx>]

Ravinandan Arakali wrote:
> 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:
> --- vanilla-linux/drivers/net/s2io.c    2004-10-06 11:31:09.552305224 -0700
> +++ linux-2.6.8.1/drivers/net/s2io.c    2004-10-06 13:03:08.087360376 -0700
> @@ -69,7 +69,7 @@
> =20
>  /* S2io Driver name & version. */
>  static char s2io_driver_name[] =3D "s2io";
> -static char s2io_driver_version[] =3D "Version 1.0";
> +static char s2io_driver_version[] =3D "Version 1.7.5.1";
> =20
>  #define LINK_IS_UP(val64) (!(val64 & (ADAPTER_STATUS_RMAC_REMOTE_FAULT =
> | \
>                                       ADAPTER_STATUS_RMAC_LOCAL_FAULT)))
> @@ -99,45 +99,45 @@
>  };
> =20
>  static char ethtool_stats_keys[][ETH_GSTRING_LEN] =3D {


Now we move on the content portion of the review :)

> 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 <raghavendra.koushik@xxxxxxxx>

Patch 1 comments:

1) undescribed non-cosmetic change:

> @@ -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);
> -       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:

>  #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:

> +#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:

> +#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:

> +#ifdef SET_ETHTOOL_OPS
>  static struct ethtool_ops netdev_ethtool_ops;
> +#endif




More comments follow in upcoming emails.