Re: [PATCH] staging: vt6656: Use defines in preamble_type variables

From: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
To: Oscar Carter <oscar.carter@gmx.com>
Cc: Forest Bond <forest@alittletooquiet.net>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Malcolm Priestley <tvboxspy@gmail.com>,
	Amir Mahdi Ghorbanian <indigoomega021@gmail.com>,
	Colin Ian King <colin.king@canonical.com>,
	Gabriela Bittencourt <gabrielabittencourt00@gmail.com>,
	devel@driverdev.osuosl.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] staging: vt6656: Use defines in preamble_type variables
Date: Mon, 30 Mar 2020 16:54:19 +0100	[thread overview]
Message-ID: <20200330155419.GA133370@jiffies> (raw)
In-Reply-To: <20200330154902.GB125210@jiffies>

On 03/30/20 16:49:04, Quentin Deslandes wrote:
> On 03/28/20 15:09:55, Oscar Carter wrote:
> > Use the PREAMBLE_SHORT and PREAMBLE_LONG defines present in the file
> > "baseband.h" to assign values to preamble_type variables. Also, use the
> > same defines to make comparisons against this variables.
> > 
> > In this way, avoid the use of numerical literals or boolean values and
> > make the code more clear.
> > 
> > Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> > ---
> >  drivers/staging/vt6656/baseband.c | 8 ++++----
> >  drivers/staging/vt6656/main_usb.c | 6 +++---
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6656/baseband.c b/drivers/staging/vt6656/baseband.c
> > index a19a563d8bcc..9bbafa7fff61 100644
> > --- a/drivers/staging/vt6656/baseband.c
> > +++ b/drivers/staging/vt6656/baseband.c
> > @@ -142,7 +142,7 @@ unsigned int vnt_get_frame_time(u8 preamble_type, u8 pkt_type,
> >  	rate = (unsigned int)vnt_frame_time[tx_rate];
> > 
> >  	if (tx_rate <= 3) {
> > -		if (preamble_type == 1)
> > +		if (preamble_type == PREAMBLE_SHORT)
> >  			preamble = 96;
> >  		else
> >  			preamble = 192;
> > @@ -198,7 +198,7 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
> >  	case RATE_2M:
> >  		count = bit_count / 2;
> > 
> > -		if (preamble_type == 1)
> > +		if (preamble_type == PREAMBLE_SHORT)
> >  			phy->signal = 0x09;
> >  		else
> >  			phy->signal = 0x01;
> > @@ -207,7 +207,7 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
> >  	case RATE_5M:
> >  		count = DIV_ROUND_UP(bit_count * 10, 55);
> > 
> > -		if (preamble_type == 1)
> > +		if (preamble_type == PREAMBLE_SHORT)
> >  			phy->signal = 0x0a;
> >  		else
> >  			phy->signal = 0x02;
> > @@ -224,7 +224,7 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
> >  				ext_bit = true;
> >  		}
> > 
> > -		if (preamble_type == 1)
> > +		if (preamble_type == PREAMBLE_SHORT)
> >  			phy->signal = 0x0b;
> >  		else
> >  			phy->signal = 0x03;
> > diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> > index 8e7269c87ea9..dd89f98cc18c 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -99,7 +99,7 @@ static void vnt_set_options(struct vnt_private *priv)
> >  	priv->op_mode = NL80211_IFTYPE_UNSPECIFIED;
> >  	priv->bb_type = BBP_TYPE_DEF;
> >  	priv->packet_type = priv->bb_type;
> > -	priv->preamble_type = 0;
> > +	priv->preamble_type = PREAMBLE_LONG;
> >  	priv->exist_sw_net_addr = false;
> >  }
> > 
> > @@ -721,10 +721,10 @@ static void vnt_bss_info_changed(struct ieee80211_hw *hw,
> >  	if (changed & BSS_CHANGED_ERP_PREAMBLE) {
> >  		if (conf->use_short_preamble) {
> >  			vnt_mac_enable_barker_preamble_mode(priv);
> > -			priv->preamble_type = true;
> > +			priv->preamble_type = PREAMBLE_SHORT;
> >  		} else {
> >  			vnt_mac_disable_barker_preamble_mode(priv);
> > -			priv->preamble_type = false;
> > +			priv->preamble_type = PREAMBLE_LONG;
> >  		}
> >  	}
> > 
> > --
> > 2.20.1
> > 
> 
> Reviewed-by: Quentin Deslandes <quentin.deslandes@itdev.co.uk>
> 
> Thank,
> Quentin

Disregard this review, a v2 has been sent.

Thanks,
Quentin