[PATCH] Staging: rtl8723bs/core fix brace coding style issue in rtw_ioctl_set.c.

[PATCH] Staging: rtl8723bs/core fix brace coding style issue in rtw_ioctl_set.c.

[Brother Matthew De Angelis]

Fix a brace warning found by the checkpatch.pl tool at line 178.

WARNING: braces {} are not necessary for any arm of this statement.

Signed-off-by: Brother Matthew De Angelis <matthew.v.deangelis@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_ioctl_set.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
index 3adeca6f20ec..a7aa689ef4d4 100644
--- a/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
+++ b/drivers/staging/rtl8723bs/core/rtw_ioctl_set.c
@@ -175,11 +175,10 @@ u8 rtw_set_802_11_bssid(struct adapter *padapter, u8 *bssid)
 
 
 	DBG_871X("Set BSSID under fw_state = 0x%08x\n", get_fwstate(pmlmepriv));
-	if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY) = true) {
+	if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY) = true)
 		goto handle_tkip_countermeasure;
-	} else if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING) = true) {
+	else if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING) = true)
 		goto release_mlme_lock;
-	}
 
 	if (check_fwstate(pmlmepriv, _FW_LINKED|WIFI_ADHOC_MASTER_STATE) = true) {
 		RT_TRACE(_module_rtl871x_ioctl_set_c_, _drv_info_, ("set_bssid: _FW_LINKED||WIFI_ADHOC_MASTER_STATE\n"));
-- 
2.25.1

My apologies, I meant to sent this. 
Would a patch like this be worth Greg's time?

Re: [PATCH] Staging: rtl8723bs/core fix brace coding style issue in rtw_ioctl_set.c.

[Dan Carpenter]

Don't put a period at the end of the subject.  "rtw_ioctl_set.c." If I
were reviewing this as a real staging patch to be applied, I probably
would not comment on it the first time you did it.  I try ignore trivial
stuff like that.  But since you are going to resend the patch then you
may as well clean it up.

On Tue, Dec 01, 2020 at 03:49:15PM -0600, Brother Matthew De Angelis wrote:
> Fix a brace warning found by the checkpatch.pl tool at line 178.
> 
> WARNING: braces {} are not necessary for any arm of this statement.
> 
> Signed-off-by: Brother Matthew De Angelis <matthew.v.deangelis@gmail.com>
> ---
  ^^^
These three lines are the cut off line.  If you put notes after the cut
off then the notes are not include in the final commit message.  That's
the normal place to put questions and comments about the patch.

> My apologies, I meant to sent this. 

Ah...

> Would a patch like this be worth Greg's time?

Again, this is a situation where Greg will probably not take more than
15 seconds to review or think about your patch.  It fixes a checkpatch
warning and doesn't introduce any new issues.  Apply.  I review staging
patches as well and I follow the same philosophy as Greg on this.  But
often other maintainers have higher standards.

And it's always good for you the developer to take more than the minimum
15 seconds to consider the patch.  There are several other "WARNING:
braces {} are not necessary" checkpatch complaints in this file.  You
may as well fix them all.

There are other things to clean as well.  But they should be done in
separate patches.  For example, what does check_fwstate() mean?  What
does it return?  Normally kernel functions return 0 on success and
negative error codes.  Boolean functions are supposed to named more
obviously like fwstate_set() where the name tells you right away that
it returns true when the state is set and false otherwise.  Why is there
an underscore at the start of the _FW_UNDER_SURVEY name?

There are other ways we could write the if statement like:

	if (check_fwstate(pmlmepriv, _FW_UNDER_SURVEY))
 		goto handle_tkip_countermeasure;

	if (check_fwstate(pmlmepriv, _FW_UNDER_LINKING))
 		goto release_mlme_lock;

All that stuff would have to go into different patches, but it's worth
thinking about.  But if you're going to resend this, then please fix all
the "braces are not necessary" warnings in the file.

regards,
dan carpenter

Re: [PATCH] Staging: rtl8723bs/core fix brace coding style issue in rtw_ioctl_set.c.

[Dan Carpenter]

On Wed, Dec 02, 2020 at 09:42:23AM +0300, Dan Carpenter wrote:
> Don't put a period at the end of the subject.  "rtw_ioctl_set.c." If I
> were reviewing this as a real staging patch to be applied, I probably
> would not comment on it the first time you did it.  I try ignore trivial
> stuff like that.  But since you are going to resend the patch then you
> may as well clean it up.
> 
> On Tue, Dec 01, 2020 at 03:49:15PM -0600, Brother Matthew De Angelis wrote:
> > Fix a brace warning found by the checkpatch.pl tool at line 178.
> > 
> > WARNING: braces {} are not necessary for any arm of this statement.
> > 
> > Signed-off-by: Brother Matthew De Angelis <matthew.v.deangelis@gmail.com>
> > ---
>   ^^^
> These three lines are the cut off line.  If you put notes after the cut
> off then the notes are not include in the final commit message.  That's
> the normal place to put questions and comments about the patch.
> 
> > My apologies, I meant to sent this. 
> 
> Ah...
> 
> > Would a patch like this be worth Greg's time?
> 
> Again, this is a situation where Greg will probably not take more than
> 15 seconds to review or think about your patch.  It fixes a checkpatch
> warning and doesn't introduce any new issues.  Apply.  I review staging
> patches as well and I follow the same philosophy as Greg on this.  But
> often other maintainers have higher standards.
> 
> And it's always good for you the developer to take more than the minimum
> 15 seconds to consider the patch.  There are several other "WARNING:
> braces {} are not necessary" checkpatch complaints in this file.  You
> may as well fix them all.
> 
> There are other things to clean as well.  But they should be done in
> separate patches.  For example, what does check_fwstate() mean?  What
> does it return?  Normally kernel functions return 0 on success and
> negative error codes.  Boolean functions are supposed to named more
> obviously like fwstate_set() where the name tells you right away that
> it returns true when the state is set and false otherwise.

Of course, _set() can be a verb or a question.  "set this" vs "is this
set".  So maybe that's not a good name either.  Naming is hard.  Is it
worth changing?  Pointless churn is also bad.  Anyway, all these things
are stuff to think about.

regards,
dan carpenter