Re: [PATCH] staging: rtl8723bs: remove unnecessary type encoding in variable names

[Julia Lawall <julia.lawall@inria.fr>]

[-- Attachment #1: Type: text/plain, Size: 3432 bytes --]



On Sat, 29 Mar 2025, Samuel Abraham wrote:

> On Sat, Mar 29, 2025 at 3:20 PM Julia Lawall <julia.lawall@inria.fr> wrote:
> >
> >
> >
> > On Sat, 29 Mar 2025, Samuel Abraham wrote:
> >
> > > On Sat, Mar 29, 2025 at 12:13 PM Julia Lawall <julia.lawall@inria.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
> >
> Yes.
> Should I make any modifications to the patch?

No.

julia