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
Date: Wed, 26 Jan 2022 00:54:27 +0000 [thread overview]
Message-ID: <YfCbw3ENI/1tEdwi@equinox> (raw)
In-Reply-To: <20220125090233.GR1978@kadam>
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
next prev parent reply other threads:[~2022-01-26 0:54 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-24 22:44 [PATCH 00/10] Cleanup and removal of DBG_88E macro Phillip Potter
2022-01-24 22:44 ` [PATCH 01/10] staging: r8188eu: remove previously added dev_dbg and netdev_dbg calls Phillip Potter
2022-01-24 22:44 ` [PATCH 02/10] staging: r8188eu: remove smaller sets of DBG_88E calls from core dir Phillip Potter
2022-01-25 9:02 ` Dan Carpenter
2022-01-26 0:54 ` Phillip Potter [this message]
2022-01-26 6:00 ` Dan Carpenter
2022-01-25 18:17 ` Pavel Skripkin
2022-01-26 1:00 ` Phillip Potter
2022-01-24 22:44 ` [PATCH 03/10] staging: r8188eu: remove DBG_88E calls from core/rtw_mlme_ext.c Phillip Potter
2022-01-25 7:22 ` kernel test robot
2022-01-25 18:31 ` Pavel Skripkin
2022-01-26 1:02 ` Phillip Potter
2022-01-24 22:44 ` [PATCH 04/10] staging: r8188eu: remove all DBG_88E calls from hal dir Phillip Potter
2022-01-25 8:27 ` kernel test robot
2022-01-24 22:44 ` [PATCH 05/10] staging: r8188eu: remove smaller sets of DBG_88E calls from os_dep dir Phillip Potter
2022-01-25 12:48 ` Pavel Skripkin
2022-01-25 12:59 ` Dan Carpenter
2022-01-25 13:04 ` Pavel Skripkin
2022-01-25 13:07 ` Dan Carpenter
2022-01-26 0:58 ` Phillip Potter
2022-01-24 22:44 ` [PATCH 06/10] staging: r8188eu: remove DBG_88E calls from os_dep/ioctl_linux.c Phillip Potter
2022-01-25 9:31 ` kernel test robot
2022-01-25 19:03 ` Pavel Skripkin
2022-01-26 1:13 ` Phillip Potter
2022-01-26 10:26 ` Pavel Skripkin
2022-01-26 10:48 ` Greg KH
2022-01-26 10:58 ` Pavel Skripkin
2022-01-26 11:14 ` Greg KH
2022-01-27 8:53 ` Phillip Potter
2022-01-24 22:44 ` [PATCH 07/10] staging: r8188eu: remove DBG_88E call from include/usb_ops.h Phillip Potter
2022-01-24 22:44 ` [PATCH 08/10] staging: r8188eu: remove all remaining aliased DBG_88E calls Phillip Potter
2022-01-24 22:44 ` [PATCH 09/10] staging: r8188eu: remove DBG_88E macro definition Phillip Potter
2022-01-24 22:44 ` [PATCH 10/10] staging: r8188eu: remove GlobalDebugLevel flag Phillip Potter
2022-01-25 15:33 ` [PATCH 00/10] Cleanup and removal of DBG_88E macro Greg KH
2022-01-26 0:45 ` Phillip Potter
2022-01-24 22:50 [RESENT PATCH 00/10] staging: r8188eu: " Phillip Potter
2022-01-24 22:50 ` [PATCH 02/10] staging: r8188eu: remove smaller sets of DBG_88E calls from core dir Phillip Potter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YfCbw3ENI/1tEdwi@equinox \
--to=phil@philpotter.co.uk \
--cc=Larry.Finger@lwfinger.net \
--cc=dan.carpenter@oracle.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=martin@kaiser.cx \
--cc=paskripkin@gmail.com \
--cc=straube.linux@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).