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

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

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



On Fri, 15 Oct 2021, 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?

Hmm.  I see that the definition of 3 for PREAMBLE_SHORT is in some
different drivers.  They seem to have a concept of preamble mode.

The following code is in your driver
(drivers/staging/vt6655/baseband.c, function bb_get_frame_time):

                if (by_preamble_type == 1) /* Short */
                        preamble = 96;
                else
	                preamble = 192;

It is indeed the case in your driver that

drivers/staging/vt6655/baseband.h:#define PREAMBLE_SHORT  1

Therefore, it looks like it is not a boolean, the by prefix should be
dropped, the == 1 should be turned into == PREAMBLE_SHORT, and the comment
should be removed.

julia