staging: octeon: fixed few coding style warnings

Fixed allignment issues and line over 80 characters

Signed-off-by Sakshi Bansal <[email protected]>

I am testing it on Fedora 21.

The change under concern was mentioned as
a "Check ".

On Oct 14, 2015 9:17 PM, "David Daney" <[email protected] <mailto:[email protected]>> wrote:

    On 10/14/2015 07:06 AM, Sakshi Bansal wrote:

        Fixed allignment issues and line over 80 characters


    Use spell checking on 'allignment'

    But that is not the main problem with the patch...


    You are changing things other than white space and comment
    formatting, can you tell us on which platforms the patch was tested
    to verify that you didn't break anything?


        Signed-off-by: Sakshi Bansal <[email protected]
        <mailto:[email protected]>>



    NAK.

    [...]

        diff --git a/drivers/staging/octeon/ethernet-mdio.c
        b/drivers/staging/octeon/ethernet-mdio.c
        index fd9b3d8..590a6cb 100644
        --- a/drivers/staging/octeon/ethernet-mdio.c
        +++ b/drivers/staging/octeon/ethernet-mdio.c
        @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct
        net_device *dev)
                 priv->phydev = of_phy_connect(dev, phy_node,
        cvm_oct_adjust_link, 0,
                                               PHY_INTERFACE_MODE_GMII);

        -       if (priv->phydev == NULL)
        +       if (!priv->phydev)



    Not a coding style change.  There is no WARNING generated for this case.



                         return -ENODEV;

                 priv->last_link = 0;
        diff --git a/drivers/staging/octeon/ethernet-mem.c
        b/drivers/staging/octeon/ethernet-mem.c
        index 5a5cdb3..d6172e4 100644
        --- a/drivers/staging/octeon/ethernet-mem.c
        +++ b/drivers/staging/octeon/ethernet-mem.c
        @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int pool,
        int size, int elements)
                 while (freed) {
                         struct sk_buff *skb = dev_alloc_skb(size + 256);

        -               if (unlikely(skb == NULL))
        +               if (unlikely(!skb))


    Same

                                 break;
                         skb_reserve(skb, 256 - (((unsigned
        long)skb->data) & 0x7f));
                         *(struct sk_buff **)(skb->data - sizeof(void
        *)) = skb;
        @@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int pool,
        int size, int elements)
                          * just before the block.
                          */
                         memory = kmalloc(size + 256, GFP_ATOMIC);
        -               if (unlikely(memory == NULL)) {
        +               if (unlikely(!memory)) {


    Same

                                 pr_warn("Unable to allocate %u bytes
        for FPA pool %d\n",
                                         elements * size, pool);
                                 break;
        diff --git a/drivers/staging/octeon/ethernet-rgmii.c
        b/drivers/staging/octeon/ethernet-rgmii.c
        index 51dcb61..3d7513c 100644
        --- a/drivers/staging/octeon/ethernet-rgmii.c
        +++ b/drivers/staging/octeon/ethernet-rgmii.c
        @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct
        net_device *dev)
                 struct octeon_ethernet *priv = netdev_priv(dev);
                 unsigned long flags = 0;
                 cvmx_helper_link_info_t link_info;
        -       int use_global_register_lock = (priv->phydev == NULL);
        +       int use_global_register_lock = (!priv->phydev);


    Same.

    I could go on, but I think you see the pattern here.

    Your changelog says you are fixing warnings, but none of these are
    warning fixes.

    In fact it is perfectly acceptable to compare a pointer to NULL.  It
    is a common idiom in the kernel.  The original author of the code
    thought it was more clear this way, and you are causing code churn
    for no reason.

    Try to run this command on the kernel sources:
    $ git grep -e '== NULL' | wc -l
    21488

    I would suggest that you convince people that the other 21,000 cases
    of comparison to NULL need changing before you do it to this driver.

    David Daney

Well I did not test the patch

since the changes were mostly coding style
related or would not affect the code.

As for the null check, if the original check is valid I can remove them from the patch.

On Oct 14, 2015 9:27 PM, "David Daney" <[email protected] <mailto:[email protected]>> wrote:

On 10/14/2015 08:51 AM, Sakshi Bansal wrote:

I am testing it on Fedora 21.

Does your Fedora 21 platform use this driver in any way? Does it even build the driver (produce any .o files for any of the files you changed)?

No, I didn't think so.

You have to have a way to test the patch (or at least compile the files), or have other people test it for you if you make changes.

The change under concern was mentioned as
a "Check ".

Yes, notice that 'CHECK:' is emitted in the color green. It is not a warning. Your patch subject line said you were fixing warnings. This is not true.

On Oct 14, 2015 9:17 PM, "David Daney" <[email protected] <mailto:[email protected]> <mailto:[email protected] <mailto:[email protected]>>> wrote:

On 10/14/2015 07:06 AM, Sakshi Bansal wrote:

Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...

You are changing things other than white space and comment formatting, can you tell us on which platforms the patch
was tested
to verify that you didn't break anything?

Signed-off-by Sakshi Bansal <[email protected]

<mailto:[email protected]>
<mailto:[email protected]
<mailto:[email protected]>>>

NAK.

[...]

diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c index fd9b3d8..590a6cb 100644
--- a/drivers/staging/octeon/ethernet-mdio.c +++ b/drivers/staging/octeon/ethernet-mdio.c @@ -180,7 +180,7 @@ int cvm_oct_phy_setup_device(struct net_device *dev)
priv->phydev = of_phy_connect(dev, phy_node,
cvm_oct_adjust_link, 0,

PHY_INTERFACE_MODE_GMII);

- if (priv->phydev == NULL)
+ if (!priv->phydev)

Not a coding style change. There is no WARNING generated
for this case.

return -ENODEV;

priv->last_link = 0;
diff --git a/drivers/staging/octeon/ethernet-mem.c b/drivers/staging/octeon/ethernet-mem.c index 5a5cdb3..d6172e4 100644
--- a/drivers/staging/octeon/ethernet-mem.c +++ b/drivers/staging/octeon/ethernet-mem.c @@ -34,7 +34,7 @@ static int cvm_oct_fill_hw_skbuff(int
pool,
int size, int elements)
while (freed) {
struct sk_buff *skb =
dev_alloc_skb(size + 256);

- if (unlikely(skb == NULL))
+ if (unlikely(!skb))

Same

break;
skb_reserve(skb, 256 - (((unsigned
long)skb->data) & 0x7f));
*(struct sk_buff **)(skb->data -
sizeof(void
*)) = skb;
@@ -98,7 +98,7 @@ static int cvm_oct_fill_hw_memory(int
pool,
int size, int elements)
* just before the block.
*/
memory = kmalloc(size + 256, GFP_ATOMIC);
- if (unlikely(memory == NULL)) {
+ if (unlikely(!memory)) {

Same

pr_warn("Unable to allocate %u
bytes
for FPA pool %d\n",
elements * size, pool);
break;
diff --git a/drivers/staging/octeon/ethernet-rgmii.c b/drivers/staging/octeon/ethernet-rgmii.c index 51dcb61..3d7513c 100644
--- a/drivers/staging/octeon/ethernet-rgmii.c +++ b/drivers/staging/octeon/ethernet-rgmii.c @@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
struct octeon_ethernet *priv = netdev_priv(dev); unsigned long flags = 0; cvmx_helper_link_info_t link_info;
- int use_global_register_lock = (priv->phydev ==
NULL);
+ int use_global_register_lock = (!priv->phydev);

Same.

I could go on, but I think you see the pattern here.

Your changelog says you are fixing warnings, but none of
these are
warning fixes.

In fact it is perfectly acceptable to compare a pointer to
NULL. It
is a common idiom in the kernel. The original author of
the code
thought it was more clear this way, and you are causing
code churn
for no reason.

Try to run this command on the kernel sources:
$ git grep -e '== NULL' | wc -l
21488

I would suggest that you convince people that the other
21,000 cases
of comparison to NULL need changing before you do it to
this driver.

David Daney

and make sure you don't break the driver.

Quoted 1 line · David Daney

and make sure you don't break the driver.

I'm not seeing how this patch breaks anything? What am I missing?

Quoted 1 line · Sakshi Bansal

Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...

You are changing things other than white space and comment formatting, can you tell us on which platforms the patch was tested to verify that you didn't break anything?

Quoted 1 line

Signed-off-by Sakshi Bansal <[email protected]>

NAK.

drivers/staging/octeon/ethernet-rgmii.c static void cvm_oct_rgmii_poll(struct net_device *dev) · line 71

Quoted 6 lines

drivers/staging/octeon/ethernet-rgmii.c +1 -1

@@ -68,7 +68,7 @@ static void cvm_oct_rgmii_poll(struct net_device *dev)
      struct octeon_ethernet *priv = netdev_priv(dev);
      unsigned long flags = 0;
      cvmx_helper_link_info_t link_info;
-     int use_global_register_lock = (priv->phydev == NULL);
+     int use_global_register_lock = (!priv->phydev);

Same.

I could go on, but I think you see the pattern here.

Your changelog says you are fixing warnings, but none of these are warning fixes.

In fact it is perfectly acceptable to compare a pointer to NULL. It is a common idiom in the kernel. The original author of the code thought it was more clear this way, and you are causing code churn for no reason.

Quoted 11 lines · On Wednesday 14 October 2015 08:47:44 David Daney wrote:

Quoted 1 line · Sakshi Bansal

Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...

You are changing things other than white space and comment formatting, can you tell us on which platforms the patch was tested to verify that you didn't break anything?

In general a good advice, but for trivial whitespace or comment changes, this is normally not necessary. Compile-testing a patch as you say is normally expected, if only to avoid embarrassing complaints if it does break later.

For drivers that are not enabled in the x86 allmodconfig, it sure helps to say something like "Compile-tested using MIPS cross toolchain from https://www.kernel.org/pub/tools/crosstool/". Even better would be to send a fix to decouple the driver from asm/octeon/*.h to make it build on all architectures, but that is much more work than I'd expect for a trivial patch.

Quoted 18 lines · Arnd Bergmann

Quoted 11 lines · On Wednesday 14 October 2015 08:47:44 David Daney wrote:

Quoted 1 line · Sakshi Bansal

Fixed allignment issues and line over 80 characters

Use spell checking on 'allignment'

But that is not the main problem with the patch...

You are changing things other than white space and comment formatting, can you tell us on which platforms the patch was tested to verify that you didn't break anything?

In general a good advice, but for trivial whitespace or comment changes, this is normally not necessary. Compile-testing a patch as you say is normally expected, if only to avoid embarrassing complaints if it does break later.

Quoted 6 lines

For drivers that are not enabled in the x86 allmodconfig, it sure helps to say something like "Compile-tested using MIPS cross toolchain from https://www.kernel.org/pub/tools/crosstool/". Even better would be to send a fix to decouple the driver from asm/octeon/*.h to make it build on all architectures, but that is much more work than I'd expect for a trivial patch.

There is no point in such a patch. We know, a priori, that there will *never* be a non-MIPS system with this hardware.

Fixed allignment issues and line over 80 characters