Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Rename byPreambleType field

[Mike Rapoport <mike.rapoport@gmail.com>]

On Fri, Oct 15, 2021 at 09:32:17AM +0100, Karolina Drobnik wrote:
> On Thu, 2021-10-14 at 19:53 +0200, Greg KH wrote:
> > Each patch should only do one logical thing.  And every "thing" you
> > do, has to be described in the changelog text.
> 
> Ah, I see. If this is the case, I'll leave tidying the comments up
> until the end of the cleanup work and make a patch of it.
> 
> 
> On Thu, 2021-10-14 at 20:40 +0200, Julia Lawall wrote:
> > > I saw that functions and parameters use it[1] so I deduced that
> > > "by" might stand for byte.
> >
> > OK, I see your point.  I originally thought that doing something by
> > the preamble type might be true or false.
> 
> Agree, in such case that would be a well-named variable.
> 
> > But it looks like the developers
> > have considered the use of booleans and that is not what they are�
> > using here, so it seems that my original idea was off the mark.
> 
> I read through the code and it looks like indeed the developers
> intended to use it as a boolean but this "by" didn't mean to mark it as
> a predicate (if that makes sense).
> 
> > There is also this code:
> >
> > vt6655/rxtx.c:          priv->byPreambleType = PREAMBLE_SHORT;
> > vt6655/rxtx.c:          priv->byPreambleType = PREAMBLE_LONG;
> >
> > PREAMBLE_SHORT seems to be 3, which is not a boolean.
> 
> Not sure how I missed that, thanks for digging it out.�
> Hmm, but in baseband.h they are defined as 1 and 0:
> 
> ```
> #define PREAMBLE_LONG   0
> #define PREAMBLE_SHORT  1
> ```
> 
> Or am I looking at the wrong definition?
> 
> > There is also
> > vt6655/rxtx.c:  return cpu_to_le16(wTimeStampOff[priv->byPreambleType
> > % 2]
> > which also makes no sense for a boolean.
> 
> `wTimeStampOff`, defined in rxtx.c, line 57, stores two arrays of
> shorts, first for the long preamble and second for the short one. That
> modulo confuses me because if we were to take these values as they are,
> we'd get the right array anyway. Or maybe there's something more to
> it/I am misreading it.
> 
> It looks like there are two issues here:
> 1) Probably we're dealing with a predicate so "byPreableType" can keep
> its "by" prefix
> 2) This variable is not defined as a boolean but it looks like it
> should
> 
> For this patch, I think I can add `by` back, so `byPreambleType`
> becomes `by_preamble_type` and exclude the space change pointed out by
> Greg. After doing this, I can take a look and try to define this member
> as a boolean. To be honest, I'm worried of doing so as there's no way
> for me to test it.
> 
> What do you think about this?

AFAIU, this variable defines what type of a preamble will a packet have and
its value determined by a bit in WiFi packet header. In a sense it's a
boolean, but it's perfectly fine to use u8 and 0 and 1 for it.

I'd say keep it u8 and rename byPreambleType to preamble_type.

-- 
Sincerely yours,
Mike.