[PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[Phillip Potter]

Remove set but unused variable init_rate from rtl8188e_Add_RateATid
function in hal/rtl8188e_cmd.c, as it fixes a kernel test robot warning.
Removing the call to get_highest_rate_idx has no side effects here so is
safe.

Also remove the DBG_88E macro call in this function, as it is not
particularly clear in my opinion. Additionally, rename variable
shortGIrate to short_gi_rate to conform to kernel camel case rules,
and improve general spacing around operators, some of which triggers
checkpatch 'CHECK' messages. These are not related to the test robot
warning.

Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
---
 drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
index 6cbda9ab6e3f..8d03b24dc5af 100644
--- a/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
+++ b/drivers/staging/r8188eu/hal/rtl8188e_cmd.c
@@ -155,33 +155,25 @@ void rtl8188e_Add_RateATid(struct adapter *pAdapter, u32 bitmap, u8 arg, u8 rssi
 {
 	struct hal_data_8188e *haldata = GET_HAL_DATA(pAdapter);
 
-	u8 macid, init_rate, raid, shortGIrate = false;
+	u8 macid, raid, short_gi_rate = false;
 
-	macid = arg&0x1f;
+	macid = arg & 0x1f;
 
-	raid = (bitmap>>28) & 0x0f;
+	raid = (bitmap >> 28) & 0x0f;
 	bitmap &= 0x0fffffff;
 
 	if (rssi_level != DM_RATR_STA_INIT)
 		bitmap = ODM_Get_Rate_Bitmap(&haldata->odmpriv, macid, bitmap, rssi_level);
 
-	bitmap |= ((raid<<28)&0xf0000000);
+	bitmap |= ((raid << 28) & 0xf0000000);
 
-	init_rate = get_highest_rate_idx(bitmap&0x0fffffff)&0x3f;
+	short_gi_rate = (arg & BIT(5)) ? true : false;
 
-	shortGIrate = (arg&BIT(5)) ? true : false;
-
-	if (shortGIrate)
-		init_rate |= BIT(6);
-
-	raid = (bitmap>>28) & 0x0f;
+	raid = (bitmap >> 28) & 0x0f;
 
 	bitmap &= 0x0fffffff;
 
-	DBG_88E("%s=> mac_id:%d, raid:%d, ra_bitmap=0x%x, shortGIrate=0x%02x\n",
-		__func__, macid, raid, bitmap, shortGIrate);
-
-	ODM_RA_UpdateRateInfo_8188E(&haldata->odmpriv, macid, raid, bitmap, shortGIrate);
+	ODM_RA_UpdateRateInfo_8188E(&haldata->odmpriv, macid, raid, bitmap, short_gi_rate);
 }
 
 void rtl8188e_set_FwPwrMode_cmd(struct adapter *adapt, u8 Mode)
-- 
2.31.1

Re: [PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[Fabio M. De Francesco]

On Monday, August 16, 2021 1:05:18 AM CEST Phillip Potter wrote:
> Remove set but unused variable init_rate from rtl8188e_Add_RateATid
> function in hal/rtl8188e_cmd.c, as it fixes a kernel test robot warning.
> Removing the call to get_highest_rate_idx has no side effects here so is
> safe.
> 
> Also remove the DBG_88E macro call in this function, as it is not
> particularly clear in my opinion. Additionally, rename variable
> shortGIrate to short_gi_rate to conform to kernel camel case rules,
> and improve general spacing around operators, some of which triggers
> checkpatch 'CHECK' messages. These are not related to the test robot
> warning.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
>  drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
Dear Philip,

I'm sorry but, although every change here is fine, I cannot ack your patch as-
is. It shouldn't address so many different issues all at once, according to 
the best practices in patching and the kernel development rules.

I understand that you think that, while you are at the removal of "init_rate", 
why shouldn't I address all other trivial issues at once? 

Even if the patch is short and it probably doesn't require particular hard 
effort to review it, that mix-up of different works shouldn't be done, mainly 
because this attitude could potentially lead you to add more and more 
different work in future patches. Where is the limit? Why not add some more 
different works next time you find some more problems into the same file/
directory?

If I were you I'd, at least, prepare a series of two or three patches:

1/3 - Remove init_rate as reported by KTR;
2/3 - Remove unneeded DBG_88E macro;
3/3 - Do some clean-up of rtl8188e_cmd.c;

Perhaps patches 2/3 and 3/3 could be merged into one, but I'm not really sure.

Thanks,

Fabio

Re: [PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[Fabio M. De Francesco]

On Monday, August 16, 2021 8:55:06 AM CEST Fabio M. De Francesco wrote:
> On Monday, August 16, 2021 1:05:18 AM CEST Phillip Potter wrote:
> > Remove set but unused variable init_rate from rtl8188e_Add_RateATid
> > function in hal/rtl8188e_cmd.c, as it fixes a kernel test robot warning.
> > Removing the call to get_highest_rate_idx has no side effects here so is
> > safe.
> > 
> > Also remove the DBG_88E macro call in this function, as it is not
> > particularly clear in my opinion. Additionally, rename variable
> > shortGIrate to short_gi_rate to conform to kernel camel case rules,
> > and improve general spacing around operators, some of which triggers
> > checkpatch 'CHECK' messages. These are not related to the test robot
> > warning.
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> > ---
> > 
> >  drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 22 +++++++---------------
> >  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> Dear Philip,
> 
> I'm sorry but, although every change here is fine, I cannot ack your patch 
as-
> is. It shouldn't address so many different issues all at once, according to
> the best practices in patching and the kernel development rules.
> 
> I understand that you think that, while you are at the removal of 
"init_rate",
> why shouldn't I address all other trivial issues at once?
> 
> Even if the patch is short and it probably doesn't require particular hard
> effort to review it, that mix-up of different works shouldn't be done, 
mainly
> because this attitude could potentially lead you to add more and more
> different work in future patches. Where is the limit? Why not add some more
> different works next time you find some more problems into the same file/
> directory?
> 
> If I were you I'd, at least, prepare a series of two or three patches:
> 
> 1/3 - Remove init_rate as reported by KTR;
> 2/3 - Remove unneeded DBG_88E macro;
> 3/3 - Do some clean-up of rtl8188e_cmd.c;
> 
> Perhaps patches 2/3 and 3/3 could be merged into one, but I'm not really 
sure.
> 
> Thanks,
> 
> Fabio

Furthermore, I forgot to say that the "Subject" should summarize with few 
words the whole work you do and in this case it is not what it does.

Fabio

Re: [PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[Greg KH]

On Mon, Aug 16, 2021 at 12:05:18AM +0100, Phillip Potter wrote:
> Remove set but unused variable init_rate from rtl8188e_Add_RateATid
> function in hal/rtl8188e_cmd.c, as it fixes a kernel test robot warning.
> Removing the call to get_highest_rate_idx has no side effects here so is
> safe.
> 
> Also remove the DBG_88E macro call in this function, as it is not
> particularly clear in my opinion. Additionally, rename variable
> shortGIrate to short_gi_rate to conform to kernel camel case rules,
> and improve general spacing around operators, some of which triggers
> checkpatch 'CHECK' messages. These are not related to the test robot
> warning.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> ---
>  drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)

As Fabio said, this should be split up into multiple patches.

If you ever have to say "also" in a changelog text, that's a good hint
that the patch should be broken up.

thanks,

greg k-h

Re: [PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c

[Phillip Potter]

On Mon, 16 Aug 2021 at 08:01, Fabio M. De Francesco
<fmdefrancesco@gmail.com> wrote:
>
> On Monday, August 16, 2021 8:55:06 AM CEST Fabio M. De Francesco wrote:
> > On Monday, August 16, 2021 1:05:18 AM CEST Phillip Potter wrote:
> > > Remove set but unused variable init_rate from rtl8188e_Add_RateATid
> > > function in hal/rtl8188e_cmd.c, as it fixes a kernel test robot warning.
> > > Removing the call to get_highest_rate_idx has no side effects here so is
> > > safe.
> > >
> > > Also remove the DBG_88E macro call in this function, as it is not
> > > particularly clear in my opinion. Additionally, rename variable
> > > shortGIrate to short_gi_rate to conform to kernel camel case rules,
> > > and improve general spacing around operators, some of which triggers
> > > checkpatch 'CHECK' messages. These are not related to the test robot
> > > warning.
> > >
> > > Reported-by: kernel test robot <lkp@intel.com>
> > > Signed-off-by: Phillip Potter <phil@philpotter.co.uk>
> > > ---
> > >
> > >  drivers/staging/r8188eu/hal/rtl8188e_cmd.c | 22 +++++++---------------
> > >  1 file changed, 7 insertions(+), 15 deletions(-)
> >
> > Dear Philip,
> >
> > I'm sorry but, although every change here is fine, I cannot ack your patch
> as-
> > is. It shouldn't address so many different issues all at once, according to
> > the best practices in patching and the kernel development rules.
> >
> > I understand that you think that, while you are at the removal of
> "init_rate",
> > why shouldn't I address all other trivial issues at once?
> >
> > Even if the patch is short and it probably doesn't require particular hard
> > effort to review it, that mix-up of different works shouldn't be done,
> mainly
> > because this attitude could potentially lead you to add more and more
> > different work in future patches. Where is the limit? Why not add some more
> > different works next time you find some more problems into the same file/
> > directory?
> >
> > If I were you I'd, at least, prepare a series of two or three patches:
> >
> > 1/3 - Remove init_rate as reported by KTR;
> > 2/3 - Remove unneeded DBG_88E macro;
> > 3/3 - Do some clean-up of rtl8188e_cmd.c;
> >
> > Perhaps patches 2/3 and 3/3 could be merged into one, but I'm not really
> sure.
> >
> > Thanks,
> >
> > Fabio
>
> Furthermore, I forgot to say that the "Subject" should summarize with few
> words the whole work you do and in this case it is not what it does.
>
> Fabio
>
>
>

Dear Fabio,

Thank you for your feedback, I shall prepare a v2 series.

Regards,
Phil