Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <cover.1666740522.git.tanjubrunostar0@gmail.com>
 <47da976cd02d262cebe520b21a0bf2451de6731b.1666740522.git.tanjubrunostar0@gmail.com>
 <Y1k7cDP4Dpdr5EOe@kroah.com>
In-Reply-To: <Y1k7cDP4Dpdr5EOe@kroah.com>
From:   Tanju Brunostar <tanjubrunostar0@gmail.com>
Date:   Thu, 27 Oct 2022 07:12:06 +0100
Message-ID: <CAHJEyKW8ofQ0fNQ0L4TzqJTEUMDfAPMufGATbX2Ep7JJKcWHQg@mail.gmail.com>
Subject: Re: [PATCH 05/17] staging: vt6655: changed variable name: pvRTS
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     linux-staging@lists.linux.dev, linux-kernel@vger.kernel.org,
        outreachy@lists.linux.dev
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Oct 26, 2022 at 2:50 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 25, 2022 at 11:37:01PM +0000, Tanjuate Brunostar wrote:
>
> Philipp has pointed out most of this already, but I'll just be specific
> and say what isn't ok in all of these patches:
>
> >       change variable names pvRTS to meet the
>
> "name" not "name"
>
> >         linux coding standard, as it says to avoid using camelCase naming
>
> "Linux" not "linux"
>
> >         style. Cought by checkpatch
>
> Why is this all indented?
>
> Please do not do that, look at existing accepted changes in the git log
> and match up what they look like.
>
> But worst of all, you didn't really fix the variable name at all.  You
> just appeased a tool that was trying to say "don't use camelCase, use
> sane names".
>
> > Signed-off-by: Tanjuate Brunostar <tanjubrunostar0@gmail.com>
> > ---
> >  drivers/staging/vt6655/rxtx.c | 56 +++++++++++++++++------------------
> >  1 file changed, 28 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> > index 2cac8f3882df..e97cba014adf 100644
> > --- a/drivers/staging/vt6655/rxtx.c
> > +++ b/drivers/staging/vt6655/rxtx.c
> > @@ -87,7 +87,7 @@ static const unsigned short w_fb_opt_1[2][5] = {
> >  /*---------------------  Static Functions  --------------------------*/
> >  static void s_v_fill_rts_head(struct vnt_private *p_device,
> >                             unsigned char by_pkt_type,
> > -                           void *pvRTS,
> > +                           void *pv_rts,
>
> "pvRTS" is using Hungarian Notation.  Look it up on Wikipedia for what
> it means, and why people used to use it.
>
> For us, we don't need that at all as the type of the variable is obvious
> in the code and the compiler checks it.
>
> So "pvRTS" is trying to say "this is a pointer to void called "RTS".
>
> We don't care about the "describe the variable type in the name" thing,
> so it should just be called "RTS", or better yet, "rts", right?
>
> But then, step back.  Why is this a void pointer at all?  This is really
> a structure of type struct vnt_rts_g_fb.  So why isn't that being passed
> here instead?
>
> So try to work on both, fixing up the names to be sane, and then,
> getting rid of the void * stuff, to better reflect how data is flowing
> around and what type that data is in.
>
> thanks,
>
> greg k-h

I see. thank you for the pointers

Tanju