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

From: "Fabio M. De Francesco" <fmdefrancesco@gmail.com>
To: gregkh@linuxfoundation.org, Phillip Potter <phil@philpotter.co.uk>
Cc: Larry.Finger@lwfinger.net, straube.linux@gmail.com,
	martin@kaiser.cx, linux-staging@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: r8188eu: remove unused variable and DBG_88E in hal/rtl8188e_cmd.c
Date: Mon, 16 Aug 2021 08:55:06 +0200	[thread overview]
Message-ID: <11475165.p2zfeMNkXv@localhost.localdomain> (raw)
In-Reply-To: <20210815230518.91656-1-phil@philpotter.co.uk>

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