Re: [PATCH] staging: rtl8723bs: Remove redundant else branches.

[Alison Schofield <alison.schofield@intel.com>]

On Tue, Mar 29, 2022 at 01:59:52PM -0700, Sevinj Aghayeva wrote:
> Hi Alison,
> 
> Thanks for the feedback! Please see inline.
> 
> On Tue, Mar 29, 2022 at 12:58 PM Alison Schofield <
> alison.schofield@intel.com> wrote:
> 
> > Hi Sevinj,
> >
> > You can do this to make sure your commit msg 'looks' similar to
> > others in the file:
> >
> > $git log --pretty=oneline --abbrev-commit
> >
> > d6ad258ae15d staging: rtl8723bs: Remove redundant else branches.
> > 24e65aac9457 staging: rtl8723bs: remove rf type branching (fourth patch)
> > 167fc30e8e51 staging: rtl8723bs: remove unused macros
> > d7361874468f staging: rtl8723bs: fix camel case in struct wlan_bcn_info
> > 6994aa430368 staging: rtl8723bs: fix camel case in struct ndis_802_11_ssid
> >
> > That tells me the 'Remove' should not start with an uppercase and
> > there should be no period at the end of line. Note that I look at this
> > per patch, because different drivers and subsystems have different
> > conventions. The point is to try to discern the prevailing style,
> > and follow it.
> >
> 
> Got it. The few examples that I saw in other places were using
> grammatically formatted sentences, but I'll take the subsystem conventions
> into account.
> 
> 
> > Please check the get_maintainers instruction in the first patch tutorial.
> > I only got this:
> >
> > $ git show HEAD | perl scripts/get_maintainer.pl --separator ,
> > --nokeywords --nogit --nogit-fallback --norolestats
> > Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
> > linux-staging@lists.linux.dev,linux-kernel@vger.kernel.org
> 
> 
> My bad! I ran get_maintainer.pl without those options and got those a lot
> more email addresses.
> 
> 
> >
> >
> >
> > On Tue, Mar 29, 2022 at 07:09:04AM -0700, Sevinj Aghayeva wrote:
> >
> > Please state the 'why' of this patch. ie something like:
> > Adhere to Linux kernel coding style.
> >
> > > This patch fixes the following checkpatch.pl warning:
> > The phrases 'This patch' is frowned upon as unecessarily wordy.
> > Perhaps simply:
> > Reported by checkpatch:
> >
> 
> Okay. I think we should update the https://kernelnewbies.org/PatchPhilosophy
> because I took "This patch..." from there.

Yeah, there's a lot of 'This patch...' and 'Fixes...' and some other
stuff that has fallen out of favor. 
I just made a small edit getting rid of a 'Fixes' subject line.

How about if you update this example:
Subject: [OPW kernel] [PATCH] Staging: rtl8192e: Fix printk() warning style
with your patch right here..once it is done!

Thanks!
Alison

> 
> 
> >
> > You have another patch in flight with similar things to update.
> > Please post a v2 of that one that addresses the feedback given
> > here.
> >
> 
> That one has already landed. Is it okay if I send v2 just for this patch?

Did you see it applied already?

>
> Thanks!
> 
>



> >
> > I thought both patches had good scope in that they addressed multiple
> > instances of the same issue in a single file.
> >
> > Thanks!
> > Alison
> >
> > >
> > > WARNING: else is not generally useful after a break or return
> > >
> > > Signed-off-by: Sevinj Aghayeva <sevinj.aghayeva@gmail.com>
> > > ---
> > >  .../staging/rtl8723bs/core/rtw_ieee80211.c    | 32 ++++++++-----------
> > >  1 file changed, 13 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > index b449be537376..27de086903e2 100644
> > > --- a/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > +++ b/drivers/staging/rtl8723bs/core/rtw_ieee80211.c
> > > @@ -94,16 +94,14 @@ bool rtw_is_cckratesonly_included(u8 *rate)
> > >
> > >  int rtw_check_network_type(unsigned char *rate, int ratelen, int
> > channel)
> > >  {
> > > -     if (channel > 14) {
> > > +     if (channel > 14)
> > >               return WIRELESS_INVALID;
> > > -     } else { /*  could be pure B, pure G, or B/G */
> > > -             if (rtw_is_cckratesonly_included(rate))
> > > -                     return WIRELESS_11B;
> > > -             else if (rtw_is_cckrates_included(rate))
> > > -                     return  WIRELESS_11BG;
> > > -             else
> > > -                     return WIRELESS_11G;
> > > -     }
> > > +     /* could be pure B, pure G, or B/G */
> > > +     if (rtw_is_cckratesonly_included(rate))
> > > +             return WIRELESS_11B;
> > > +     if (rtw_is_cckrates_included(rate))
> > > +             return WIRELESS_11BG;
> > > +     return WIRELESS_11G;
> > >  }
> > >
> > >  u8 *rtw_set_fixed_ie(unsigned char *pbuf, unsigned int len, unsigned
> > char *source,
> > > @@ -151,11 +149,10 @@ u8 *rtw_get_ie(u8 *pbuf, signed int index, signed
> > int *len, signed int limit)
> > >               if (*p == index) {
> > >                       *len = *(p + 1);
> > >                       return p;
> > > -             } else {
> > > -                     tmp = *(p + 1);
> > > -                     p += (tmp + 2);
> > > -                     i += (tmp + 2);
> > >               }
> > > +             tmp = *(p + 1);
> > > +             p += (tmp + 2);
> > > +             i += (tmp + 2);
> > >               if (i >= limit)
> > >                       break;
> > >       }
> > > @@ -199,9 +196,8 @@ u8 *rtw_get_ie_ex(u8 *in_ie, uint in_len, u8 eid, u8
> > *oui, u8 oui_len, u8 *ie, u
> > >                               *ielen = in_ie[cnt+1]+2;
> > >
> > >                       break;
> > > -             } else {
> > > -                     cnt += in_ie[cnt+1]+2; /* goto next */
> > >               }
> > > +             cnt += in_ie[cnt+1]+2; /* goto next */
> > >       }
> > >
> > >       return target_ie;
> > > @@ -697,9 +693,8 @@ u8 *rtw_get_wps_ie(u8 *in_ie, uint in_len, u8
> > *wps_ie, uint *wps_ielen)
> > >                       cnt += in_ie[cnt+1]+2;
> > >
> > >                       break;
> > > -             } else {
> > > -                     cnt += in_ie[cnt+1]+2; /* goto next */
> > >               }
> > > +             cnt += in_ie[cnt+1]+2; /* goto next */
> > >       }
> > >
> > >       return wpsie_ptr;
> > > @@ -748,9 +743,8 @@ u8 *rtw_get_wps_attr(u8 *wps_ie, uint wps_ielen, u16
> > target_attr_id, u8 *buf_att
> > >                               *len_attr = attr_len;
> > >
> > >                       break;
> > > -             } else {
> > > -                     attr_ptr += attr_len; /* goto next */
> > >               }
> > > +             attr_ptr += attr_len; /* goto next */
> > >       }
> > >
> > >       return target_attr_ptr;
> > > --
> > > 2.25.1
> > >
> > >
> >
> 
> 
> -- 
> 
> Sevinj.Aghayeva