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 <[email protected]>
---
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
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 <[email protected]>
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
Hi Lakshmi,
On Wed, 11 Mar 2020 19:42:06 -0700
Lakshmi Ramasubramanian <[email protected]> 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 <[email protected]>
>
> 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
On Thu, 12 Mar 2020, Stefano Brivio wrote:
> Hi Lakshmi,
>
> On Wed, 11 Mar 2020 19:42:06 -0700
> Lakshmi Ramasubramanian <[email protected]> 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 <[email protected]>
> >
> > 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20200312113416.23d3db5c%40elisabeth.
>
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
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 <[email protected]>
> ---
> 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
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