All of lore.kernel.org
 help / color / mirror / Atom feed
* [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-11 13:58 ` Shreeya Patel
  0 siblings, 0 replies; 16+ messages in thread
From: Shreeya Patel @ 2020-03-11 13:58 UTC (permalink / raw)
  To: gregkh, devel, linux-kernel, outreachy-kernel, sbrivio,
	daniel.baluta, nramas, hverkuil, shreeya.patel23498,
	Larry.Finger

Remove unnecessary if and else conditions since both are leading to the
initialization of "phtpriv->ampdu_enable" with the same value.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index 71fcb466019a..48e9faf27321 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
 
 	/* maybe needs check if ap supports rx ampdu. */
 	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
-		if (pregistrypriv->wifi_spec == 1) {
-			/* remove this part because testbed AP should disable RX AMPDU */
-			/* phtpriv->ampdu_enable = false; */
-			phtpriv->ampdu_enable = true;
-		} else {
-			phtpriv->ampdu_enable = true;
-		}
+		/* remove this part because testbed AP should disable RX AMPDU */
+		/* phtpriv->ampdu_enable = false; */
+		phtpriv->ampdu_enable = true;
 	} else if (pregistrypriv->ampdu_enable == 2) {
 		/* remove this part because testbed AP should disable RX AMPDU */
 		/* phtpriv->ampdu_enable = true; */
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-11 13:58 ` Shreeya Patel
  0 siblings, 0 replies; 16+ messages in thread
From: Shreeya Patel @ 2020-03-11 13:58 UTC (permalink / raw)
  To: gregkh, devel, linux-kernel, outreachy-kernel, sbrivio,
	daniel.baluta, nramas, hverkuil, shreeya.patel23498,
	Larry.Finger

Remove unnecessary if and else conditions since both are leading to the
initialization of "phtpriv->ampdu_enable" with the same value.

Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
 drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
index 71fcb466019a..48e9faf27321 100644
--- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
+++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
@@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
 
 	/* maybe needs check if ap supports rx ampdu. */
 	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
-		if (pregistrypriv->wifi_spec == 1) {
-			/* remove this part because testbed AP should disable RX AMPDU */
-			/* phtpriv->ampdu_enable = false; */
-			phtpriv->ampdu_enable = true;
-		} else {
-			phtpriv->ampdu_enable = true;
-		}
+		/* remove this part because testbed AP should disable RX AMPDU */
+		/* phtpriv->ampdu_enable = false; */
+		phtpriv->ampdu_enable = true;
 	} else if (pregistrypriv->ampdu_enable == 2) {
 		/* remove this part because testbed AP should disable RX AMPDU */
 		/* phtpriv->ampdu_enable = true; */
-- 
2.17.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-11 13:58 ` Shreeya Patel
  (?)
@ 2020-03-11 20:17 ` Deepak Varma
  2020-03-11 20:26   ` Julia Lawall
  -1 siblings, 1 reply; 16+ messages in thread
From: Deepak Varma @ 2020-03-11 20:17 UTC (permalink / raw)
  To: outreachy-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1851 bytes --]



On Wednesday, 11 March 2020 19:29:13 UTC+5:30, Shreeya Patel wrote:
>
> Remove unnecessary if and else conditions since both are leading to the 
> initialization of "phtpriv->ampdu_enable" with the same value. 
>
> Signed-off-by: Shreeya Patel <shreeya....@gmail.com <javascript:>> 
> --- 
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++------- 
>  1 file changed, 3 insertions(+), 7 deletions(-) 
>
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> b/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> index 71fcb466019a..48e9faf27321 100644 
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c 
> @@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 
> *pie, uint ie_len, u8 channe 
>   
>          /* maybe needs check if ap supports rx ampdu. */ 
>          if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) 
> { 
> -                if (pregistrypriv->wifi_spec == 1) { 
> -                        /* remove this part because testbed AP should 
> disable RX AMPDU */ 
> -                        /* phtpriv->ampdu_enable = false; */ 
> -                        phtpriv->ampdu_enable = true; 
> -                } else { 
> -                        phtpriv->ampdu_enable = true; 
> -                } 
> +                /* remove this part because testbed AP should disable RX 
> AMPDU */ 
> +                /* phtpriv->ampdu_enable = false; */ 
> +                phtpriv->ampdu_enable = true; 
>          } else if (pregistrypriv->ampdu_enable == 2) { 
>                  /* remove this part because testbed AP should disable RX 
> AMPDU */ 
>                  /* phtpriv->ampdu_enable = true; */ 
> -- 
> 2.17.1 
>
>  Juia, can you comment if the 2 commented lines [with '+' sign now] may 
also be removed to further simplify the code?

Deepak.

[-- Attachment #1.2: Type: text/html, Size: 2649 bytes --]

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-11 20:17 ` Deepak Varma
@ 2020-03-11 20:26   ` Julia Lawall
  0 siblings, 0 replies; 16+ messages in thread
From: Julia Lawall @ 2020-03-11 20:26 UTC (permalink / raw)
  To: Deepak Varma; +Cc: outreachy-kernel

[-- Attachment #1: Type: text/plain, Size: 3084 bytes --]



On Wed, 11 Mar 2020, Deepak Varma wrote:

>
>
> On Wednesday, 11 March 2020 19:29:13 UTC+5:30, Shreeya Patel wrote:
>       Remove unnecessary if and else conditions since both are leading
>       to the
>       initialization of "phtpriv->ampdu_enable" with the same value.
>
>       Signed-off-by: Shreeya Patel <shreeya....@gmail.com>
>       ---
>        drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
>        1 file changed, 3 insertions(+), 7 deletions(-)
>
>       diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c
>       b/drivers/staging/rtl8723bs/core/rtw_mlme.c
>       index 71fcb466019a..48e9faf27321 100644
>       --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
>       +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
>       @@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter
>       *padapter, u8 *pie, uint ie_len, u8 channe
>        
>                /* maybe needs check if ap supports rx ampdu. */
>                if (!(phtpriv->ampdu_enable) &&
>       pregistrypriv->ampdu_enable == 1) {
>       -                if (pregistrypriv->wifi_spec == 1) {
>       -                        /* remove this part because testbed AP
>       should disable RX AMPDU */
>       -                        /* phtpriv->ampdu_enable = false; */
>       -                        phtpriv->ampdu_enable = true;
>       -                } else {
>       -                        phtpriv->ampdu_enable = true;
>       -                }
>       +                /* remove this part because testbed AP should
>       disable RX AMPDU */
>       +                /* phtpriv->ampdu_enable = false; */
>       +                phtpriv->ampdu_enable = true;
>                } else if (pregistrypriv->ampdu_enable == 2) {
>                        /* remove this part because testbed AP should
>       disable RX AMPDU */
>                        /* phtpriv->ampdu_enable = true; */
>       --
>       2.17.1
>
>  Juia, can you comment if the 2 commented lines [with '+' sign now] may also
> be removed to further simplify the code?

I feel uneasy about the whole change, without having more knowledge of the
device.  Someone already suggested that the code should be removed, but it
seems they didn't do it.  Maybe they didn't do it for a reason...

On the other hand, if the "if" is useless, the comments don't seem useful
either, since they applied to only one of the branches.

julia

>
> Deepak.
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/92b5d69a-d4e6-480d-82ef-
> 3a01b893fbd0%40googlegroups.com.
>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-11 13:58 ` Shreeya Patel
@ 2020-03-12  2:42   ` Lakshmi Ramasubramanian
  -1 siblings, 0 replies; 16+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-03-12  2:42 UTC (permalink / raw)
  To: Shreeya Patel, gregkh, devel, linux-kernel, outreachy-kernel,
	sbrivio, daniel.baluta, hverkuil, Larry.Finger

On 3/11/2020 6:58 AM, Shreeya Patel wrote:

> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Stating this based on the patch descriptions I have seen.
Others, please advise\correct me if I am wrong.

Patch description should state the problem first[1] and then describe 
how that is fixed in the given patch.

For example:

In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
same value in both if and else statements.

This patch removes this unnecessary if-else statement.


[1] Documentation\process\submitting-patches.rst
        2) Describe your changes

Thanks,
  -lakshmi


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-12  2:42   ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 16+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-03-12  2:42 UTC (permalink / raw)
  To: Shreeya Patel, gregkh, devel, linux-kernel, outreachy-kernel,
	sbrivio, daniel.baluta, hverkuil, Larry.Finger

On 3/11/2020 6:58 AM, Shreeya Patel wrote:

> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>

Stating this based on the patch descriptions I have seen.
Others, please advise\correct me if I am wrong.

Patch description should state the problem first[1] and then describe 
how that is fixed in the given patch.

For example:

In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
same value in both if and else statements.

This patch removes this unnecessary if-else statement.


[1] Documentation\process\submitting-patches.rst
        2) Describe your changes

Thanks,
  -lakshmi
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-12  2:42   ` Lakshmi Ramasubramanian
@ 2020-03-12 10:34     ` Stefano Brivio
  -1 siblings, 0 replies; 16+ messages in thread
From: Stefano Brivio @ 2020-03-12 10:34 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Shreeya Patel, gregkh, devel, linux-kernel, outreachy-kernel,
	daniel.baluta, hverkuil, Larry.Finger

Hi Lakshmi,

On Wed, 11 Mar 2020 19:42:06 -0700
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:

> On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> 
> > Remove unnecessary if and else conditions since both are leading to the
> > initialization of "phtpriv->ampdu_enable" with the same value.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>  
> 
> Stating this based on the patch descriptions I have seen.
> Others, please advise\correct me if I am wrong.
> 
> Patch description should state the problem first[1] and then describe 
> how that is fixed in the given patch.
> 
> For example:
> 
> In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
> same value in both if and else statements.
> 
> This patch removes this unnecessary if-else statement.

That's my general preference as well, but I can't find any point in the
"Describe your changes" section of submitting-patches.rst actually
defining the order. I wouldn't imply that from the sequence the steps
are presented in.

In case it's possible to say everything with a single statement as
Shreeya did here, though, I guess that becomes rather a linguistic
factor, and I personally prefer the concise version here.

-- 
Stefano



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-12 10:34     ` Stefano Brivio
  0 siblings, 0 replies; 16+ messages in thread
From: Stefano Brivio @ 2020-03-12 10:34 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: devel, daniel.baluta, gregkh, linux-kernel, hverkuil,
	outreachy-kernel, Shreeya Patel, Larry.Finger

Hi Lakshmi,

On Wed, 11 Mar 2020 19:42:06 -0700
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:

> On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> 
> > Remove unnecessary if and else conditions since both are leading to the
> > initialization of "phtpriv->ampdu_enable" with the same value.
> > 
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>  
> 
> Stating this based on the patch descriptions I have seen.
> Others, please advise\correct me if I am wrong.
> 
> Patch description should state the problem first[1] and then describe 
> how that is fixed in the given patch.
> 
> For example:
> 
> In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the 
> same value in both if and else statements.
> 
> This patch removes this unnecessary if-else statement.

That's my general preference as well, but I can't find any point in the
"Describe your changes" section of submitting-patches.rst actually
defining the order. I wouldn't imply that from the sequence the steps
are presented in.

In case it's possible to say everything with a single statement as
Shreeya did here, though, I guess that becomes rather a linguistic
factor, and I personally prefer the concise version here.

-- 
Stefano

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-12 10:34     ` Stefano Brivio
@ 2020-03-12 10:49       ` Julia Lawall
  -1 siblings, 0 replies; 16+ messages in thread
From: Julia Lawall @ 2020-03-12 10:49 UTC (permalink / raw)
  To: Stefano Brivio
  Cc: Lakshmi Ramasubramanian, Shreeya Patel, gregkh, devel,
	linux-kernel, outreachy-kernel, daniel.baluta, hverkuil,
	Larry.Finger



On Thu, 12 Mar 2020, Stefano Brivio wrote:

> Hi Lakshmi,
>
> On Wed, 11 Mar 2020 19:42:06 -0700
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:
>
> > On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> >
> > > Remove unnecessary if and else conditions since both are leading to the
> > > initialization of "phtpriv->ampdu_enable" with the same value.
> > >
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> >
> > Stating this based on the patch descriptions I have seen.
> > Others, please advise\correct me if I am wrong.
> >
> > Patch description should state the problem first[1] and then describe
> > how that is fixed in the given patch.
> >
> > For example:
> >
> > In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the
> > same value in both if and else statements.
> >
> > This patch removes this unnecessary if-else statement.
>
> That's my general preference as well, but I can't find any point in the
> "Describe your changes" section of submitting-patches.rst actually
> defining the order. I wouldn't imply that from the sequence the steps
> are presented in.
>
> In case it's possible to say everything with a single statement as
> Shreeya did here, though, I guess that becomes rather a linguistic
> factor, and I personally prefer the concise version here.

https://kernelnewbies.org/PatchPhilosophy suggests:

In patch descriptions and in the subject, it is common and preferable to
use present-tense, imperative language. Write as if you are telling git
what to do with your patch.

It provides the following as an example of a good description:

    somedriver: fix sleep while atomic in send_a_packet()

    The send_a_packet() function is called in atomic context but takes a mutex,
    causing a sleeping while atomic warning.  Change the skb_lock to be a spin
    lock instead of a mutex to fix.

So this illustrates the order that Lakshmi suggests, even though I don't
think that order is ever suggested explicitly.  On the other hand it
avoids "This patch...", which would add some clutter, in my opinion.

julia

>
> --
> Stefano
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200312113416.23d3db5c%40elisabeth.
>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-12 10:49       ` Julia Lawall
  0 siblings, 0 replies; 16+ messages in thread
From: Julia Lawall @ 2020-03-12 10:49 UTC (permalink / raw)
  To: Stefano Brivio
  Cc: devel, daniel.baluta, outreachy-kernel, gregkh, linux-kernel,
	hverkuil, Lakshmi Ramasubramanian, Shreeya Patel, Larry.Finger



On Thu, 12 Mar 2020, Stefano Brivio wrote:

> Hi Lakshmi,
>
> On Wed, 11 Mar 2020 19:42:06 -0700
> Lakshmi Ramasubramanian <nramas@linux.microsoft.com> wrote:
>
> > On 3/11/2020 6:58 AM, Shreeya Patel wrote:
> >
> > > Remove unnecessary if and else conditions since both are leading to the
> > > initialization of "phtpriv->ampdu_enable" with the same value.
> > >
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> >
> > Stating this based on the patch descriptions I have seen.
> > Others, please advise\correct me if I am wrong.
> >
> > Patch description should state the problem first[1] and then describe
> > how that is fixed in the given patch.
> >
> > For example:
> >
> > In the function rtw_update_ht_cap(), phtpriv->ampdu_enable is set to the
> > same value in both if and else statements.
> >
> > This patch removes this unnecessary if-else statement.
>
> That's my general preference as well, but I can't find any point in the
> "Describe your changes" section of submitting-patches.rst actually
> defining the order. I wouldn't imply that from the sequence the steps
> are presented in.
>
> In case it's possible to say everything with a single statement as
> Shreeya did here, though, I guess that becomes rather a linguistic
> factor, and I personally prefer the concise version here.

https://kernelnewbies.org/PatchPhilosophy suggests:

In patch descriptions and in the subject, it is common and preferable to
use present-tense, imperative language. Write as if you are telling git
what to do with your patch.

It provides the following as an example of a good description:

    somedriver: fix sleep while atomic in send_a_packet()

    The send_a_packet() function is called in atomic context but takes a mutex,
    causing a sleeping while atomic warning.  Change the skb_lock to be a spin
    lock instead of a mutex to fix.

So this illustrates the order that Lakshmi suggests, even though I don't
think that order is ever suggested explicitly.  On the other hand it
avoids "This patch...", which would add some clutter, in my opinion.

julia

>
> --
> Stefano
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200312113416.23d3db5c%40elisabeth.
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-12 10:49       ` Julia Lawall
@ 2020-03-12 16:31         ` Lakshmi Ramasubramanian
  -1 siblings, 0 replies; 16+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-03-12 16:31 UTC (permalink / raw)
  To: Julia Lawall, Stefano Brivio
  Cc: Shreeya Patel, gregkh, devel, linux-kernel, outreachy-kernel,
	daniel.baluta, hverkuil, Larry.Finger

On 3/12/2020 3:49 AM, Julia Lawall wrote:

Thanks for your input Julia and Stefano.

>> That's my general preference as well, but I can't find any point in the
>> "Describe your changes" section of submitting-patches.rst actually
>> defining the order. I wouldn't imply that from the sequence the steps
>> are presented in.
>>
>> In case it's possible to say everything with a single statement as
>> Shreeya did here, though, I guess that becomes rather a linguistic
>> factor, and I personally prefer the concise version here.
> 
> https://kernelnewbies.org/PatchPhilosophy suggests:
> 
> In patch descriptions and in the subject, it is common and preferable to
> use present-tense, imperative language. Write as if you are telling git
> what to do with your patch.

Use of imperative language is the approach I was thinking as well.

thanks,
  -lakshmi


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-12 16:31         ` Lakshmi Ramasubramanian
  0 siblings, 0 replies; 16+ messages in thread
From: Lakshmi Ramasubramanian @ 2020-03-12 16:31 UTC (permalink / raw)
  To: Julia Lawall, Stefano Brivio
  Cc: devel, daniel.baluta, gregkh, linux-kernel, hverkuil,
	outreachy-kernel, Shreeya Patel, Larry.Finger

On 3/12/2020 3:49 AM, Julia Lawall wrote:

Thanks for your input Julia and Stefano.

>> That's my general preference as well, but I can't find any point in the
>> "Describe your changes" section of submitting-patches.rst actually
>> defining the order. I wouldn't imply that from the sequence the steps
>> are presented in.
>>
>> In case it's possible to say everything with a single statement as
>> Shreeya did here, though, I guess that becomes rather a linguistic
>> factor, and I personally prefer the concise version here.
> 
> https://kernelnewbies.org/PatchPhilosophy suggests:
> 
> In patch descriptions and in the subject, it is common and preferable to
> use present-tense, imperative language. Write as if you are telling git
> what to do with your patch.

Use of imperative language is the approach I was thinking as well.

thanks,
  -lakshmi
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-11 13:58 ` Shreeya Patel
@ 2020-03-13  7:43   ` Dan Carpenter
  -1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-03-13  7:43 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: gregkh, devel, linux-kernel, outreachy-kernel, sbrivio,
	daniel.baluta, nramas, hverkuil, Larry.Finger

On Wed, Mar 11, 2020 at 07:28:59PM +0530, Shreeya Patel wrote:
> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 71fcb466019a..48e9faf27321 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
>  
>  	/* maybe needs check if ap supports rx ampdu. */
>  	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
> -		if (pregistrypriv->wifi_spec == 1) {
> -			/* remove this part because testbed AP should disable RX AMPDU */
> -			/* phtpriv->ampdu_enable = false; */
> -			phtpriv->ampdu_enable = true;
> -		} else {
> -			phtpriv->ampdu_enable = true;
> -		}
> +		/* remove this part because testbed AP should disable RX AMPDU */
> +		/* phtpriv->ampdu_enable = false; */

Please delete this dead code and the related comment.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-13  7:43   ` Dan Carpenter
  0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-03-13  7:43 UTC (permalink / raw)
  To: Shreeya Patel
  Cc: devel, nramas, daniel.baluta, outreachy-kernel, gregkh,
	linux-kernel, hverkuil, sbrivio, Larry.Finger

On Wed, Mar 11, 2020 at 07:28:59PM +0530, Shreeya Patel wrote:
> Remove unnecessary if and else conditions since both are leading to the
> initialization of "phtpriv->ampdu_enable" with the same value.
> 
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
>  drivers/staging/rtl8723bs/core/rtw_mlme.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/rtl8723bs/core/rtw_mlme.c b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> index 71fcb466019a..48e9faf27321 100644
> --- a/drivers/staging/rtl8723bs/core/rtw_mlme.c
> +++ b/drivers/staging/rtl8723bs/core/rtw_mlme.c
> @@ -2772,13 +2772,9 @@ void rtw_update_ht_cap(struct adapter *padapter, u8 *pie, uint ie_len, u8 channe
>  
>  	/* maybe needs check if ap supports rx ampdu. */
>  	if (!(phtpriv->ampdu_enable) && pregistrypriv->ampdu_enable == 1) {
> -		if (pregistrypriv->wifi_spec == 1) {
> -			/* remove this part because testbed AP should disable RX AMPDU */
> -			/* phtpriv->ampdu_enable = false; */
> -			phtpriv->ampdu_enable = true;
> -		} else {
> -			phtpriv->ampdu_enable = true;
> -		}
> +		/* remove this part because testbed AP should disable RX AMPDU */
> +		/* phtpriv->ampdu_enable = false; */

Please delete this dead code and the related comment.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
  2020-03-12  2:42   ` Lakshmi Ramasubramanian
@ 2020-03-13  7:48     ` Dan Carpenter
  -1 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-03-13  7:48 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: Shreeya Patel, gregkh, devel, linux-kernel, outreachy-kernel,
	sbrivio, daniel.baluta, hverkuil, Larry.Finger

The original patch description was basically fine.  Outreachy reviews
tend to be more pedantic about this sort of stuff than most maintainers.
There are a couple who have very strict rules, but try to avoid those
maintainers and your life will be happier.

regards,
dan carpenter



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions
@ 2020-03-13  7:48     ` Dan Carpenter
  0 siblings, 0 replies; 16+ messages in thread
From: Dan Carpenter @ 2020-03-13  7:48 UTC (permalink / raw)
  To: Lakshmi Ramasubramanian
  Cc: devel, daniel.baluta, outreachy-kernel, gregkh, linux-kernel,
	hverkuil, sbrivio, Shreeya Patel, Larry.Finger

The original patch description was basically fine.  Outreachy reviews
tend to be more pedantic about this sort of stuff than most maintainers.
There are a couple who have very strict rules, but try to avoid those
maintainers and your life will be happier.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-03-18 17:52 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-11 13:58 [Outreachy kernel] [PATCH] Staging: rtl8723bs: rtw_mlme: Remove unnecessary conditions Shreeya Patel
2020-03-11 13:58 ` Shreeya Patel
2020-03-11 20:17 ` Deepak Varma
2020-03-11 20:26   ` Julia Lawall
2020-03-12  2:42 ` Lakshmi Ramasubramanian
2020-03-12  2:42   ` Lakshmi Ramasubramanian
2020-03-12 10:34   ` Stefano Brivio
2020-03-12 10:34     ` Stefano Brivio
2020-03-12 10:49     ` Julia Lawall
2020-03-12 10:49       ` Julia Lawall
2020-03-12 16:31       ` Lakshmi Ramasubramanian
2020-03-12 16:31         ` Lakshmi Ramasubramanian
2020-03-13  7:48   ` Dan Carpenter
2020-03-13  7:48     ` Dan Carpenter
2020-03-13  7:43 ` Dan Carpenter
2020-03-13  7:43   ` Dan Carpenter

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.