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

On Sat, Mar 29, 2025 at 3:20 PM Julia Lawall <[email protected]> wrote:
>
>
>
> On Sat, 29 Mar 2025, Samuel Abraham wrote:
>
> > On Sat, Mar 29, 2025 at 12:13 PM Julia Lawall <[email protected]> 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?

Adekunle