Message-ID: <alpine.DEB.2.22.394.2503291515080.58211@hadrien>
Date: Sat, 29 Mar 2025 15:20:47 +0100 (CET)
From: Julia Lawall <julia.lawall@...ia.fr>
To: Samuel Abraham <abrahamadekunle50@...il.com>
cc: outreachy@...ts.linux.dev, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: rtl8723bs: remove unnecessary type encoding in
 variable names



On Sat, 29 Mar 2025, Samuel Abraham wrote:

> On Sat, Mar 29, 2025 at 12:13 PM Julia Lawall <julia.lawall@...ia.fr> wrote:
> >
> >
> >
> > On Sat, 29 Mar 2025, Abraham Samuel Adekunle wrote:
> >
> > > type encoding in variable names are not a standard in Linux kernel coding
> > > style.
> > >
> > > Remove redundant type prefixes (e.g, `b`, `p`) in variable names,
> > > as explicit type encoding is not necessary in Linux kernel code which
> > > uses type definitions rather than variable name prefixes
> >
> > You seem to have also gotten rid of capitalization.
>
> Hello Julia, thank you for your review
> Yes, I should have added that to my commit message. Thank you.
>
> > It's also not clear how you have chosen which variables to update.  Mostly
> > it seems to be pDM_Odm, but there is also pRFCalibrateInfo in some
> > comments.  But you haven't updated eg bMaskDWord.
>
> I chose to update the boolean and pointer variables which have been
> declared in the source files
> I was working on. pDM_Odm, declared in the source file, is a pointer
> of type struct dm_odm_t,
> which has been declared in a header file, so altering the pointer name
> would have no compiler errors since
> it is declared in the source file I modified.
> Some function prototypes have been declared in header files, so
> altering their names in their definition in the files
> I was editing would result in compiler errors too if the headers were
> not modified.
> I could have modified the variables in those header files too in
> drivers/staging/rtl8723bs/include
> but I was not sure how many files would be affected by the change and
> how long my patch would be,
> considering the three files I modified already made my patch over 3000
> lines long.
>
> RFCalibrateInfo(without the p) is a pointer that is a member of the
> struct dm_odm_t, which has been
> declared in the header file, so altering that in the source file would
> result in compiler errors too, since the header file
>  was not modified in drivers/staging/rtl8723bs/include/
> >
> > I don't know what the r represents in rOFDM0_XATxIQImbalance.
>
> The bMaskWord is a macro defined in the
> drivers/staging/rtl8723bs/include/Hal8192CPhyReg.h
> as `#define bMaskDWord 0xffffffff and also rOFDM0_XATxIQImbalance is a
> macro defined as
> `#define rOFDM0_XATxIQImbalance 0xc80` in the header file; these two
> values are not boolean values and are
> also declared in the header, so altering them in the source files will
> result in compiler errors
>
> However, other Boolean variables declared in the source files were modified.

OK.  It shows how confusing the code is.  Normally in the Linux kernel
things defined with #define are fully capitalized, unless they refer to
some name from a hardware spec.

I'm a little surprised that there isn't some generic macro defined as
0xffffffff in the Linux kernel, but indeed I don't see one, and I see lots
of masks being defined as that.

julia



>
> >
> > julia
> >
> > >
>
> > >
> > > -     struct odm_rf_cal_t *pRFCalibrateInfo = &pDM_Odm->RFCalibrateInfo;
> > > +     struct odm_rf_cal_t *rf_calibrate_info = &dm_odm->RFCalibrateInfo;
>
> Example of RFCalibrateInfo (with the p ) being declared as a pointer
> of type struct odm_rf_cal_t
> and also a member of the struct dm_odm_t
>
> So the declared variable(with the p) was modified, but the
> member(without the p) was not modified.
> With these, please what suggestions do you have?
> Thanks
>
> Adekunle
>

Powered by blists - more mailing lists