[Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Shreeya Patel]

Remove if and else conditions since both are leading to the
initialization of "valueDMATimeout" and "valueDMAPageCount" with
the same value.

Found using coccinelle script.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/rtl8723bs/hal/sdio_halinit.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
index e813382e78a6..643592b0bd38 100644
--- a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
+++ b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
@@ -551,18 +551,11 @@ static void HalRxAggr8723BSdio(struct adapter *padapter)
 
 	pregistrypriv = &padapter->registrypriv;
 
-	if (pregistrypriv->wifi_spec) {
-		/*  2010.04.27 hpfan */
-		/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested by designer */
-		/*  Timeout value is calculated by 34 / (2^n) */
-		valueDMATimeout = 0x06;
-		valueDMAPageCount = 0x06;
-	} else {
-		/*  20130530, Isaac@SD1 suggest 3 kinds of parameter */
-		/*  TX/RX Balance */
-		valueDMATimeout = 0x06;
-		valueDMAPageCount = 0x06;
-	}
+	/*  2010.04.27 hpfan */
+	/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested by designer */
+	/*  Timeout value is calculated by 34 / (2^n) */
+	valueDMATimeout = 0x06;
+	valueDMAPageCount = 0x06;
 
 	rtw_write8(padapter, REG_RXDMA_AGG_PG_TH + 1, valueDMATimeout);
 	rtw_write8(padapter, REG_RXDMA_AGG_PG_TH, valueDMAPageCount);
-- 
2.17.1

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Stefano Brivio]

On Wed, 11 Mar 2020 19:08:11 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:

> Remove if and else conditions since both are leading to the
> initialization of "valueDMATimeout" and "valueDMAPageCount" with
> the same value.
> 
> Found using coccinelle script.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>

-- 
Stefano

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Joe Perches]

On Wed, 2020-03-11 at 19:08 +0530, Shreeya Patel wrote:
> Remove if and else conditions since both are leading to the
> initialization of "valueDMATimeout" and "valueDMAPageCount" with
> the same value.

You might consider removing the
	/* Timeout value is calculated by 34 / (2^n) */
comment entirely as it doesn't make much sense.

For what N is "(34 / (2 ^ N))" = 6 ?

> diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
[]
> @@ -551,18 +551,11 @@ static void HalRxAggr8723BSdio(struct adapter *padapter)
>  
>  	pregistrypriv = &padapter->registrypriv;
>  
> -	if (pregistrypriv->wifi_spec) {
> -		/*  2010.04.27 hpfan */
> -		/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested by designer */
> -		/*  Timeout value is calculated by 34 / (2^n) */
> -		valueDMATimeout = 0x06;
> -		valueDMAPageCount = 0x06;
> -	} else {
> -		/*  20130530, Isaac@SD1 suggest 3 kinds of parameter */
> -		/*  TX/RX Balance */
> -		valueDMATimeout = 0x06;
> -		valueDMAPageCount = 0x06;
> -	}
> +	/*  2010.04.27 hpfan */
> +	/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested by designer */
> +	/*  Timeout value is calculated by 34 / (2^n) */
> +	valueDMATimeout = 0x06;
> +	valueDMAPageCount = 0x06;

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Shreeya Patel]

Hey Joe,

On March 11, 2020 10:56:29 PM GMT+05:30, Joe Perches <joe@perches.com> wrote:
>On Wed, 2020-03-11 at 19:08 +0530, Shreeya Patel wrote:
>> Remove if and else conditions since both are leading to the
>> initialization of "valueDMATimeout" and "valueDMAPageCount" with
>> the same value.
>
>You might consider removing the
>	/* Timeout value is calculated by 34 / (2^n) */
>comment entirely as it doesn't make much sense.
>

You want me to remove the other comments as well?
Since Julia suggested in another email that the comments are not useful if we are removing the condition since they were applied to only one branch ( i.e. "if" branch )


Thanks

>For what N is "(34 / (2 ^ N))" = 6 ?
>
>> diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
>b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
>[]
>> @@ -551,18 +551,11 @@ static void HalRxAggr8723BSdio(struct adapter
>*padapter)
>>  
>>  	pregistrypriv = &padapter->registrypriv;
>>  
>> -	if (pregistrypriv->wifi_spec) {
>> -		/*  2010.04.27 hpfan */
>> -		/*  Adjust RxAggrTimeout to close to zero disable RxAggr,
>suggested by designer */
>> -		/*  Timeout value is calculated by 34 / (2^n) */
>> -		valueDMATimeout = 0x06;
>> -		valueDMAPageCount = 0x06;
>> -	} else {
>> -		/*  20130530, Isaac@SD1 suggest 3 kinds of parameter */
>> -		/*  TX/RX Balance */
>> -		valueDMATimeout = 0x06;
>> -		valueDMAPageCount = 0x06;
>> -	}
>> +	/*  2010.04.27 hpfan */
>> +	/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested
>by designer */
>> +	/*  Timeout value is calculated by 34 / (2^n) */
>> +	valueDMATimeout = 0x06;
>> +	valueDMAPageCount = 0x06;

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Joe Perches]

On Thu, 2020-03-12 at 03:58 +0530, Shreeya Patel wrote:
> Hey Joe,
> 
> On March 11, 2020 10:56:29 PM GMT+05:30, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2020-03-11 at 19:08 +0530, Shreeya Patel wrote:
> > > Remove if and else conditions since both are leading to the
> > > initialization of "valueDMATimeout" and "valueDMAPageCount" with
> > > the same value.
> > 
> > You might consider removing the
> > 	/* Timeout value is calculated by 34 / (2^n) */
> > comment entirely as it doesn't make much sense.
> > 
> 
> You want me to remove the other comments as well?
> Since Julia suggested in another email that the comments are not useful if we are removing the condition since they were applied to only one branch ( i.e. "if" branch )

It's not an either/or/both situation.

Code like:

	if (test) {
		[block 1]
	} else {
		[block 2]
	}

where the contents of block 1 and block 2 are identical
exist for many reasons.  All of them need situational
analysis to determine what the right thing to do is.

Sometimes it's a defect from a copy/paste where some other
code was intended on one of the blocks and so that one path
has a defect somewhere and actually needs to be changed.

Sometimes the blocks were originally different, but some
code was removed from one of the blocks that lead to the
blocks being identical.  Those can be consolidated.

Sometimes test can be removed, sometimes not.

For hardware drivers like this, it's sometimes useful to
look at the latest code from the vendor and sometimes it's
better to ask the vendor what the right thing do is.

It does appear in this case that the branches should be
consolidated and comments in both branches removed.

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Dan Carpenter]

On Wed, Mar 11, 2020 at 07:08:11PM +0530, Shreeya Patel wrote:
> diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> index e813382e78a6..643592b0bd38 100644
> --- a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> +++ b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> @@ -551,18 +551,11 @@ static void HalRxAggr8723BSdio(struct adapter *padapter)
>  
>  	pregistrypriv = &padapter->registrypriv;
>  
> -	if (pregistrypriv->wifi_spec) {
> -		/*  2010.04.27 hpfan */
> -		/*  Adjust RxAggrTimeout to close to zero disable RxAggr, suggested by designer */
> -		/*  Timeout value is calculated by 34 / (2^n) */
> -		valueDMATimeout = 0x06;
> -		valueDMAPageCount = 0x06;
> -	} else {
> -		/*  20130530, Isaac@SD1 suggest 3 kinds of parameter */
> -		/*  TX/RX Balance */
> -		valueDMATimeout = 0x06;
> -		valueDMAPageCount = 0x06;
> -	}
> +	/*  2010.04.27 hpfan */

Delete these sorts of comments where it's just a name of someone and
a time stamp when they wrote it.  We don't know how to contact "hpfan"
so it's useless.

regards,
dan carpenter

Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: sdio_halinit: Remove unnecessary conditions

[Shreeya Patel]

On Fri, 2020-03-13 at 10:20 +0300, Dan Carpenter wrote:
> On Wed, Mar 11, 2020 at 07:08:11PM +0530, Shreeya Patel wrote:
> > diff --git a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > index e813382e78a6..643592b0bd38 100644
> > --- a/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > +++ b/drivers/staging/rtl8723bs/hal/sdio_halinit.c
> > @@ -551,18 +551,11 @@ static void HalRxAggr8723BSdio(struct adapter
> > *padapter)
> >  
> >  	pregistrypriv = &padapter->registrypriv;
> >  
> > -	if (pregistrypriv->wifi_spec) {
> > -		/*  2010.04.27 hpfan */
> > -		/*  Adjust RxAggrTimeout to close to zero disable
> > RxAggr, suggested by designer */
> > -		/*  Timeout value is calculated by 34 / (2^n) */
> > -		valueDMATimeout = 0x06;
> > -		valueDMAPageCount = 0x06;
> > -	} else {
> > -		/*  20130530, Isaac@SD1 suggest 3 kinds of parameter */
> > -		/*  TX/RX Balance */
> > -		valueDMATimeout = 0x06;
> > -		valueDMAPageCount = 0x06;
> > -	}
> > +	/*  2010.04.27 hpfan */
> 
> Delete these sorts of comments where it's just a name of someone and
> a time stamp when they wrote it.  We don't know how to contact
> "hpfan"
> so it's useless.
> 

Thanks Joe and Dan for your explanation. I will remove the comments and
send the patch again.

> regards,
> dan carpenter
> 
>