Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 26 Jan 2022 00:54:27 +0000
From:   Phillip Potter <phil@philpotter.co.uk>
To:     Dan Carpenter <dan.carpenter@oracle.com>
Cc:     gregkh@linuxfoundation.org, Larry.Finger@lwfinger.net,
        straube.linux@gmail.com, martin@kaiser.cx,
        linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        paskripkin@gmail.com
Subject: Re: [PATCH 02/10] staging: r8188eu: remove smaller sets of DBG_88E
 calls from core dir
Message-ID: <YfCbw3ENI/1tEdwi@equinox>
References: <20220124224415.831-1-phil@philpotter.co.uk>
 <20220124224415.831-3-phil@philpotter.co.uk>
 <20220125090233.GR1978@kadam>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220125090233.GR1978@kadam>
Precedence: bulk

On Tue, Jan 25, 2022 at 12:02:33PM +0300, Dan Carpenter wrote:
> On Mon, Jan 24, 2022 at 10:44:07PM +0000, Phillip Potter wrote:
> >  void rtw_sctx_done_err(struct submit_ctx **sctx, int status)
> >  {
> >  	if (*sctx) {
> > -		if (rtw_sctx_chk_waring_status(status))
> > -			DBG_88E("%s status:%d\n", __func__, status);
> > +		rtw_sctx_chk_waring_status(status);
> 
> In a follow on patch, can you just remove the rtw_sctx_chk_waring_status()
> function?
> 
> Looking through these warnings, a mass delete is the right thing.  The
> debugging messages are garbage.  Use ftrace.  Some of the errors are
> debug level so they will never be seen.  If you enable printing error
> messages, it print a debug information but at error level so that's
> useless.  Forget about it.  Just delete it.
> 
> You're going to need to redo these again because kbuild found some
> build warnings...
> 
> Normally we would say if you're deleting a printk() and that makes the
> curly braces unnecessary, then you have to delete the curly braces in
> the same patch.  But in this case, you're changing a ton of code and
> deleting the curly braces makes it hard to review.  I would prefer it
> if you just deleted the printks and fixed up the curly braces in a
> follow up patch.  And please don't fix long line warnings either.  Do
> that in another even later patch if you want.
> 
> You do need to fix any build warnings about unused variables etc, but
> don't worry about checkpatch until the end.
> 
> Basically as much as possible I'd prefer to only see deleted lines in
> this patch.  No insertions.
> 
>  12 files changed, 66 insertions(+), 472 deletions(-)
> 
> There was one function where you removed the padatper argument because
> it wasn't necessary.  That's great but do it in a follow on patch.
> Keep a notepad app next to your vim and write down notes as you go:
> 
> 0) Re-run checkpatch and delete unnecessary curly braces
> 1) Delete rtw_sctx_chk_waring_status()
> 2) padatper not needed for frob_whatever()
> 3) Fix long lines
> 
> regards,
> dan carpenter

Hi Dan,

Thank you for the feedback - reason I removed the padapter arg is
because I was the one that added it. To be clear, you're happy with this
and the brace changes etc. as long as I take them out within a separate
patch at the end of the series?

I had quite a few unused variable errors along the way, but evidently
with the way my build system is setup it doesn't currently show set but
unused variables. I will fix this and deal with the kernel test robot
warnings. Many thanks.

Regards,
Phil