Re: [PATCH 4/4] [Outreachy kernel] staging: qlge: qlge_ethtool.c: remove an unneeded variable

[Stefano Brivio <sbrivio@redhat.com>]

On Wed, 11 Mar 2020 17:50:35 +0530
Payal Kshirsagar <payalskshirsagar1234@gmail.com> wrote:

> On Wed, Mar 11, 2020, 5:38 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> 
> > On Wed, 11 Mar 2020 14:58:15 +0530
> > Payal Kshirsagar <payalskshirsagar1234@gmail.com> wrote:
> >  
> > > Remove unneeded temporary local variable, cleanup suggested by  
> > coccinelle.  
> > >
> > > Signed-off-by: Payal Kshirsagar <payalskshirsagar1234@gmail.com>
> > > ---
> > >  drivers/staging/qlge/qlge_ethtool.c | 5 +----
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/qlge/qlge_ethtool.c  
> > b/drivers/staging/qlge/qlge_ethtool.c  
> > > index 592ca7edfc44..e15f9723b5c0 100644
> > > --- a/drivers/staging/qlge/qlge_ethtool.c
> > > +++ b/drivers/staging/qlge/qlge_ethtool.c
> > > @@ -692,7 +692,6 @@ static int ql_set_pauseparam(struct net_device  
> > *netdev,  
> > >                            struct ethtool_pauseparam *pause)
> > >  {
> > >       struct ql_adapter *qdev = netdev_priv(netdev);
> > > -     int status = 0;
> > >
> > >       if ((pause->rx_pause) && (pause->tx_pause))
> > >               qdev->link_config |= CFG_PAUSE_STD;
> > > @@ -700,9 +699,7 @@ static int ql_set_pauseparam(struct net_device  
> > *netdev,  
> > >               qdev->link_config &= ~CFG_PAUSE_STD;
> > >       else
> > >               return -EINVAL;
> > > -
> > > -     status = ql_mb_set_port_cfg(qdev);
> > > -     return status;
> > > +     return ql_mb_set_port_cfg(qdev);  
> >
> > Here, you're also removing a newline that is typically inserted before
> > a second return (or the last one in a function) for readability, or
> > whenever there are two logically distinct branches, same in 3/4.
> >  
> 
> Thanks for the suggestion.
> I thought there is no need to keep the newline as is:D

Have a look at some "sane" examples (especially outside of
drivers/staging), maybe that helps getting a picture...

> But I did not understand what do you want me to do, patch is okay or need
> to be resend by adding a newline for readability:)

I would say you need to re-send the series, because 3/4 and 4/4
actually impair readability in their current form.

Note that I gave my Reviewed-by: on another patch of yours, not on
this series.

Another note: from the cover letter, it's apparent that "[Outreachy
kernel]" is part of commit titles. It shouldn't be, that's only a
prefix (git format-patch --subject-prefix) for email.

> 
> payal
> 
> And in 1/4, you might want to add one. In 2/4, there's no need.
> >
> > --
> > Stefano
> >  

One note about quoting: read above. Who wrote what?

I see you're using GMail, which is fine, by all means, but:

- you shouldn't send HTML emails to kernel lists. Especially with
  projects using patchwork
  (example: https://patchwork.ozlabs.org/project/netdev/list/) this
  has a big potential to mess things up

- you should fix your quoting: a sentence I wrote needs a "> >" prefix
  there, otherwise it seems to come from you, and I'll get confused.

That is, I suggest that you set up an email client as also covered by
the tutorial. The GMail web interface is probably not ideal. The
tutorial suggests 'mutt', but it's not mandatory. I'm using claws-mail
to send you this e-mail and for my work in general.

There are other graphical or text-based clients (Evolution, pine,
sylpheed, KMail, many others) that can also be used for this purpose. I
would suggest that you look at screenshots, maybe play with some of
them, find one you're comfortable with, and set it up.

-- 
Stefano