[PATCH] staging: vt6655: Rename byPreambleType field

[PATCH] staging: vt6655: Rename byPreambleType field

[Karolina Drobnik]

Drop Hungarian notation prefix in `byPreambleType` member of
struct vnt_private. Change it to use snake case.

Fix issue detected by checkpatch.pl:
  CHECK: Avoid CamelCase: <byPreambleType>

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/baseband.c    | 10 +++---
 drivers/staging/vt6655/baseband.h    |  2 +-
 drivers/staging/vt6655/device.h      |  2 +-
 drivers/staging/vt6655/device_main.c |  8 ++---
 drivers/staging/vt6655/rxtx.c        | 50 ++++++++++++++--------------
 5 files changed, 36 insertions(+), 36 deletions(-)

diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
index 0bae35af6e0f..69739de927be 100644
--- a/drivers/staging/vt6655/baseband.c
+++ b/drivers/staging/vt6655/baseband.c
@@ -1691,8 +1691,8 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
  *
  * Parameters:
  *  In:
- *      by_preamble_type  - Preamble Type
- *      by_pkt_type        - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
+ *      preamble_type     - Preamble Type
+ *      by_pkt_type       - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
  *      cb_frame_length   - Baseband Type
  *      tx_rate           - Tx Rate
  *  Out:
@@ -1700,7 +1700,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
  * Return Value: FrameTime
  *
  */
-unsigned int bb_get_frame_time(unsigned char by_preamble_type,
+unsigned int bb_get_frame_time(unsigned char preamble_type,
 			       unsigned char by_pkt_type,
 			       unsigned int cb_frame_length,
 			       unsigned short tx_rate)
@@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char by_preamble_type,
 	rate = (unsigned int)awc_frame_time[rate_idx];
 
 	if (rate_idx <= 3) {		    /* CCK mode */
-		if (by_preamble_type == 1) /* Short */
+		if (preamble_type == 1)     /* Short */
 			preamble = 96;
 		else
 			preamble = 192;
@@ -1764,7 +1764,7 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
 	u32 count = 0;
 	u32 tmp;
 	int ext_bit;
-	u8 preamble_type = priv->byPreambleType;
+	u8 preamble_type = priv->preamble_type;
 
 	bit_count = frame_length * 8;
 	ext_bit = false;
diff --git a/drivers/staging/vt6655/baseband.h b/drivers/staging/vt6655/baseband.h
index 0a30afaa7cc3..15b2802ed727 100644
--- a/drivers/staging/vt6655/baseband.h
+++ b/drivers/staging/vt6655/baseband.h
@@ -44,7 +44,7 @@
 #define TOP_RATE_2M         0x00200000
 #define TOP_RATE_1M         0x00100000
 
-unsigned int bb_get_frame_time(unsigned char by_preamble_type,
+unsigned int bb_get_frame_time(unsigned char preamble_type,
 			       unsigned char by_pkt_type,
 			       unsigned int cb_frame_length,
 			       unsigned short w_rate);
diff --git a/drivers/staging/vt6655/device.h b/drivers/staging/vt6655/device.h
index 92d6f82f939f..4706bde1ec1d 100644
--- a/drivers/staging/vt6655/device.h
+++ b/drivers/staging/vt6655/device.h
@@ -203,7 +203,7 @@ struct vnt_private {
 	unsigned char byMinChannel;
 	unsigned char byMaxChannel;
 
-	unsigned char byPreambleType;
+	unsigned char preamble_type;
 	unsigned char byShortPreamble;
 
 	unsigned short wCurrentRate;
diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
index 24f0e346da12..212d2a287b2c 100644
--- a/drivers/staging/vt6655/device_main.c
+++ b/drivers/staging/vt6655/device_main.c
@@ -177,11 +177,11 @@ device_set_options(struct vnt_private *priv)
 	priv->byPacketType = priv->byBBType;
 	priv->byAutoFBCtrl = AUTO_FB_0;
 	priv->bUpdateBBVGA = true;
-	priv->byPreambleType = 0;
+	priv->preamble_type = 0;
 
 	pr_debug(" byShortRetryLimit= %d\n", (int)priv->byShortRetryLimit);
 	pr_debug(" byLongRetryLimit= %d\n", (int)priv->byLongRetryLimit);
-	pr_debug(" byPreambleType= %d\n", (int)priv->byPreambleType);
+	pr_debug(" preamble_type= %d\n", (int)priv->preamble_type);
 	pr_debug(" byShortPreamble= %d\n", (int)priv->byShortPreamble);
 	pr_debug(" byBBType= %d\n", (int)priv->byBBType);
 }
@@ -1424,10 +1424,10 @@ static void vnt_bss_info_changed(struct ieee80211_hw *hw,
 	if (changed & BSS_CHANGED_ERP_PREAMBLE) {
 		if (conf->use_short_preamble) {
 			MACvEnableBarkerPreambleMd(priv->port_offset);
-			priv->byPreambleType = true;
+			priv->preamble_type = true;
 		} else {
 			MACvDisableBarkerPreambleMd(priv->port_offset);
-			priv->byPreambleType = false;
+			priv->preamble_type = false;
 		}
 	}
 
diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
index ff0b5391ea0b..0de801b666da 100644
--- a/drivers/staging/vt6655/rxtx.c
+++ b/drivers/staging/vt6655/rxtx.c
@@ -142,7 +142,7 @@ s_uFillDataHead(
 
 static __le16 vnt_time_stamp_off(struct vnt_private *priv, u16 rate)
 {
-	return cpu_to_le16(wTimeStampOff[priv->byPreambleType % 2]
+	return cpu_to_le16(wTimeStampOff[priv->preamble_type % 2]
 							[rate % MAX_RATE]);
 }
 
@@ -163,7 +163,7 @@ s_uGetTxRsvTime(
 {
 	unsigned int uDataTime, uAckTime;
 
-	uDataTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, cbFrameLength, wRate);
+	uDataTime = bb_get_frame_time(pDevice->preamble_type, byPktType, cbFrameLength, wRate);
 
 	if (!bNeedAck)
 		return uDataTime;
@@ -172,7 +172,7 @@ s_uGetTxRsvTime(
 	 * CCK mode  - 11b
 	 * OFDM mode - 11g 2.4G & 11a 5G
 	 */
-	uAckTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14,
+	uAckTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14,
 				     byPktType == PK_TYPE_11B ?
 				     pDevice->byTopCCKBasicRate :
 				     pDevice->byTopOFDMBasicRate);
@@ -200,22 +200,22 @@ static __le16 get_rtscts_time(struct vnt_private *priv,
 	unsigned int ack_time = 0;
 	unsigned int data_time = 0;
 
-	data_time = bb_get_frame_time(priv->byPreambleType, pkt_type, frame_length, current_rate);
+	data_time = bb_get_frame_time(priv->preamble_type, pkt_type, frame_length, current_rate);
 	if (rts_rsvtype == 0) { /* RTSTxRrvTime_bb */
-		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopCCKBasicRate);
-		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
+		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
+		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
 		cts_time = ack_time;
 	} else if (rts_rsvtype == 1) { /* RTSTxRrvTime_ba, only in 2.4GHZ */
-		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopCCKBasicRate);
-		cts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
-		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
+		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
+		cts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
+		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
 	} else if (rts_rsvtype == 2) { /* RTSTxRrvTime_aa */
-		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopOFDMBasicRate);
-		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
+		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopOFDMBasicRate);
+		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
 		cts_time = ack_time;
 	} else if (rts_rsvtype == 3) { /* CTSTxRrvTime_ba, only in 2.4GHZ */
-		cts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
-		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
+		cts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
+		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
 		rrv_time = cts_time + ack_time + data_time + 2 * priv->uSIFS;
 		return cpu_to_le16((u16)rrv_time);
 	}
@@ -255,7 +255,7 @@ s_uGetDataDuration(
 	switch (byDurType) {
 	case DATADUR_B:    /* DATADUR_B */
 		if (bNeedAck) {
-			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
+			uAckTime = bb_get_frame_time(pDevice->preamble_type,
 						     byPktType, 14,
 						     pDevice->byTopCCKBasicRate);
 		}
@@ -273,7 +273,7 @@ s_uGetDataDuration(
 
 	case DATADUR_A:    /* DATADUR_A */
 		if (bNeedAck) {
-			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
+			uAckTime = bb_get_frame_time(pDevice->preamble_type,
 						     byPktType, 14,
 						     pDevice->byTopOFDMBasicRate);
 		}
@@ -292,7 +292,7 @@ s_uGetDataDuration(
 	case DATADUR_A_F0:    /* DATADUR_A_F0 */
 	case DATADUR_A_F1:    /* DATADUR_A_F1 */
 		if (bNeedAck) {
-			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
+			uAckTime = bb_get_frame_time(pDevice->preamble_type,
 						     byPktType, 14,
 						     pDevice->byTopOFDMBasicRate);
 		}
@@ -344,17 +344,17 @@ s_uGetRTSCTSDuration(
 
 	switch (byDurType) {
 	case RTSDUR_BB:    /* RTSDuration_bb */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
 		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
 		break;
 
 	case RTSDUR_BA:    /* RTSDuration_ba */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
 		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
 		break;
 
 	case RTSDUR_AA:    /* RTSDuration_aa */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
 		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
 		break;
 
@@ -363,7 +363,7 @@ s_uGetRTSCTSDuration(
 		break;
 
 	case RTSDUR_BA_F0: /* RTSDuration_ba_f0 */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
 		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
 			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate - RATE_18M], bNeedAck);
 		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
@@ -372,7 +372,7 @@ s_uGetRTSCTSDuration(
 		break;
 
 	case RTSDUR_AA_F0: /* RTSDuration_aa_f0 */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
 		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
 			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate - RATE_18M], bNeedAck);
 		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
@@ -381,7 +381,7 @@ s_uGetRTSCTSDuration(
 		break;
 
 	case RTSDUR_BA_F1: /* RTSDuration_ba_f1 */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
 		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
 			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate - RATE_18M], bNeedAck);
 		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
@@ -390,7 +390,7 @@ s_uGetRTSCTSDuration(
 		break;
 
 	case RTSDUR_AA_F1: /* RTSDuration_aa_f1 */
-		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
+		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
 		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
 			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate - RATE_18M], bNeedAck);
 		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
@@ -1289,9 +1289,9 @@ int vnt_generate_fifo_header(struct vnt_private *priv, u32 dma_idx,
 		tx_buffer_head->fifo_ctl |= cpu_to_le16(FIFOCTL_LRETRY);
 
 	if (tx_rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
-		priv->byPreambleType = PREAMBLE_SHORT;
+		priv->preamble_type = PREAMBLE_SHORT;
 	else
-		priv->byPreambleType = PREAMBLE_LONG;
+		priv->preamble_type = PREAMBLE_LONG;
 
 	if (tx_rate->flags & IEEE80211_TX_RC_USE_RTS_CTS)
 		tx_buffer_head->fifo_ctl |= cpu_to_le16(FIFOCTL_RTS);
-- 
2.30.2

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

[Julia Lawall]

On Thu, 14 Oct 2021, Karolina Drobnik wrote:

> Drop Hungarian notation prefix in `byPreambleType` member of
> struct vnt_private. Change it to use snake case.

Are you sure that it is an instance of Hungarian notation?  Could it be
that "by" is meant to be the word "by"?  I have the imrepssion from a
quick look through the code that it should be a boolean.  Maybe it would
be helpful to give it boolean value, eg true instead of 1.

julia

>
> Fix issue detected by checkpatch.pl:
>   CHECK: Avoid CamelCase: <byPreambleType>
>
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/baseband.c    | 10 +++---
>  drivers/staging/vt6655/baseband.h    |  2 +-
>  drivers/staging/vt6655/device.h      |  2 +-
>  drivers/staging/vt6655/device_main.c |  8 ++---
>  drivers/staging/vt6655/rxtx.c        | 50 ++++++++++++++--------------
>  5 files changed, 36 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
> index 0bae35af6e0f..69739de927be 100644
> --- a/drivers/staging/vt6655/baseband.c
> +++ b/drivers/staging/vt6655/baseband.c
> @@ -1691,8 +1691,8 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
>   *
>   * Parameters:
>   *  In:
> - *      by_preamble_type  - Preamble Type
> - *      by_pkt_type        - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
> + *      preamble_type     - Preamble Type
> + *      by_pkt_type       - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
>   *      cb_frame_length   - Baseband Type
>   *      tx_rate           - Tx Rate
>   *  Out:
> @@ -1700,7 +1700,7 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
>   * Return Value: FrameTime
>   *
>   */
> -unsigned int bb_get_frame_time(unsigned char by_preamble_type,
> +unsigned int bb_get_frame_time(unsigned char preamble_type,
>  			       unsigned char by_pkt_type,
>  			       unsigned int cb_frame_length,
>  			       unsigned short tx_rate)
> @@ -1717,7 +1717,7 @@ unsigned int bb_get_frame_time(unsigned char by_preamble_type,
>  	rate = (unsigned int)awc_frame_time[rate_idx];
>
>  	if (rate_idx <= 3) {		    /* CCK mode */
> -		if (by_preamble_type == 1) /* Short */
> +		if (preamble_type == 1)     /* Short */
>  			preamble = 96;
>  		else
>  			preamble = 192;
> @@ -1764,7 +1764,7 @@ void vnt_get_phy_field(struct vnt_private *priv, u32 frame_length,
>  	u32 count = 0;
>  	u32 tmp;
>  	int ext_bit;
> -	u8 preamble_type = priv->byPreambleType;
> +	u8 preamble_type = priv->preamble_type;
>
>  	bit_count = frame_length * 8;
>  	ext_bit = false;
> diff --git a/drivers/staging/vt6655/baseband.h b/drivers/staging/vt6655/baseband.h
> index 0a30afaa7cc3..15b2802ed727 100644
> --- a/drivers/staging/vt6655/baseband.h
> +++ b/drivers/staging/vt6655/baseband.h
> @@ -44,7 +44,7 @@
>  #define TOP_RATE_2M         0x00200000
>  #define TOP_RATE_1M         0x00100000
>
> -unsigned int bb_get_frame_time(unsigned char by_preamble_type,
> +unsigned int bb_get_frame_time(unsigned char preamble_type,
>  			       unsigned char by_pkt_type,
>  			       unsigned int cb_frame_length,
>  			       unsigned short w_rate);
> diff --git a/drivers/staging/vt6655/device.h b/drivers/staging/vt6655/device.h
> index 92d6f82f939f..4706bde1ec1d 100644
> --- a/drivers/staging/vt6655/device.h
> +++ b/drivers/staging/vt6655/device.h
> @@ -203,7 +203,7 @@ struct vnt_private {
>  	unsigned char byMinChannel;
>  	unsigned char byMaxChannel;
>
> -	unsigned char byPreambleType;
> +	unsigned char preamble_type;
>  	unsigned char byShortPreamble;
>
>  	unsigned short wCurrentRate;
> diff --git a/drivers/staging/vt6655/device_main.c b/drivers/staging/vt6655/device_main.c
> index 24f0e346da12..212d2a287b2c 100644
> --- a/drivers/staging/vt6655/device_main.c
> +++ b/drivers/staging/vt6655/device_main.c
> @@ -177,11 +177,11 @@ device_set_options(struct vnt_private *priv)
>  	priv->byPacketType = priv->byBBType;
>  	priv->byAutoFBCtrl = AUTO_FB_0;
>  	priv->bUpdateBBVGA = true;
> -	priv->byPreambleType = 0;
> +	priv->preamble_type = 0;
>
>  	pr_debug(" byShortRetryLimit= %d\n", (int)priv->byShortRetryLimit);
>  	pr_debug(" byLongRetryLimit= %d\n", (int)priv->byLongRetryLimit);
> -	pr_debug(" byPreambleType= %d\n", (int)priv->byPreambleType);
> +	pr_debug(" preamble_type= %d\n", (int)priv->preamble_type);
>  	pr_debug(" byShortPreamble= %d\n", (int)priv->byShortPreamble);
>  	pr_debug(" byBBType= %d\n", (int)priv->byBBType);
>  }
> @@ -1424,10 +1424,10 @@ static void vnt_bss_info_changed(struct ieee80211_hw *hw,
>  	if (changed & BSS_CHANGED_ERP_PREAMBLE) {
>  		if (conf->use_short_preamble) {
>  			MACvEnableBarkerPreambleMd(priv->port_offset);
> -			priv->byPreambleType = true;
> +			priv->preamble_type = true;
>  		} else {
>  			MACvDisableBarkerPreambleMd(priv->port_offset);
> -			priv->byPreambleType = false;
> +			priv->preamble_type = false;
>  		}
>  	}
>
> diff --git a/drivers/staging/vt6655/rxtx.c b/drivers/staging/vt6655/rxtx.c
> index ff0b5391ea0b..0de801b666da 100644
> --- a/drivers/staging/vt6655/rxtx.c
> +++ b/drivers/staging/vt6655/rxtx.c
> @@ -142,7 +142,7 @@ s_uFillDataHead(
>
>  static __le16 vnt_time_stamp_off(struct vnt_private *priv, u16 rate)
>  {
> -	return cpu_to_le16(wTimeStampOff[priv->byPreambleType % 2]
> +	return cpu_to_le16(wTimeStampOff[priv->preamble_type % 2]
>  							[rate % MAX_RATE]);
>  }
>
> @@ -163,7 +163,7 @@ s_uGetTxRsvTime(
>  {
>  	unsigned int uDataTime, uAckTime;
>
> -	uDataTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, cbFrameLength, wRate);
> +	uDataTime = bb_get_frame_time(pDevice->preamble_type, byPktType, cbFrameLength, wRate);
>
>  	if (!bNeedAck)
>  		return uDataTime;
> @@ -172,7 +172,7 @@ s_uGetTxRsvTime(
>  	 * CCK mode  - 11b
>  	 * OFDM mode - 11g 2.4G & 11a 5G
>  	 */
> -	uAckTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14,
> +	uAckTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14,
>  				     byPktType == PK_TYPE_11B ?
>  				     pDevice->byTopCCKBasicRate :
>  				     pDevice->byTopOFDMBasicRate);
> @@ -200,22 +200,22 @@ static __le16 get_rtscts_time(struct vnt_private *priv,
>  	unsigned int ack_time = 0;
>  	unsigned int data_time = 0;
>
> -	data_time = bb_get_frame_time(priv->byPreambleType, pkt_type, frame_length, current_rate);
> +	data_time = bb_get_frame_time(priv->preamble_type, pkt_type, frame_length, current_rate);
>  	if (rts_rsvtype == 0) { /* RTSTxRrvTime_bb */
> -		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopCCKBasicRate);
> -		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
> +		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
> +		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
>  		cts_time = ack_time;
>  	} else if (rts_rsvtype == 1) { /* RTSTxRrvTime_ba, only in 2.4GHZ */
> -		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopCCKBasicRate);
> -		cts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
> -		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
> +		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopCCKBasicRate);
> +		cts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
> +		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
>  	} else if (rts_rsvtype == 2) { /* RTSTxRrvTime_aa */
> -		rts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 20, priv->byTopOFDMBasicRate);
> -		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
> +		rts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 20, priv->byTopOFDMBasicRate);
> +		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
>  		cts_time = ack_time;
>  	} else if (rts_rsvtype == 3) { /* CTSTxRrvTime_ba, only in 2.4GHZ */
> -		cts_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopCCKBasicRate);
> -		ack_time = bb_get_frame_time(priv->byPreambleType, pkt_type, 14, priv->byTopOFDMBasicRate);
> +		cts_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopCCKBasicRate);
> +		ack_time = bb_get_frame_time(priv->preamble_type, pkt_type, 14, priv->byTopOFDMBasicRate);
>  		rrv_time = cts_time + ack_time + data_time + 2 * priv->uSIFS;
>  		return cpu_to_le16((u16)rrv_time);
>  	}
> @@ -255,7 +255,7 @@ s_uGetDataDuration(
>  	switch (byDurType) {
>  	case DATADUR_B:    /* DATADUR_B */
>  		if (bNeedAck) {
> -			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
> +			uAckTime = bb_get_frame_time(pDevice->preamble_type,
>  						     byPktType, 14,
>  						     pDevice->byTopCCKBasicRate);
>  		}
> @@ -273,7 +273,7 @@ s_uGetDataDuration(
>
>  	case DATADUR_A:    /* DATADUR_A */
>  		if (bNeedAck) {
> -			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
> +			uAckTime = bb_get_frame_time(pDevice->preamble_type,
>  						     byPktType, 14,
>  						     pDevice->byTopOFDMBasicRate);
>  		}
> @@ -292,7 +292,7 @@ s_uGetDataDuration(
>  	case DATADUR_A_F0:    /* DATADUR_A_F0 */
>  	case DATADUR_A_F1:    /* DATADUR_A_F1 */
>  		if (bNeedAck) {
> -			uAckTime = bb_get_frame_time(pDevice->byPreambleType,
> +			uAckTime = bb_get_frame_time(pDevice->preamble_type,
>  						     byPktType, 14,
>  						     pDevice->byTopOFDMBasicRate);
>  		}
> @@ -344,17 +344,17 @@ s_uGetRTSCTSDuration(
>
>  	switch (byDurType) {
>  	case RTSDUR_BB:    /* RTSDuration_bb */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
>  		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
>  		break;
>
>  	case RTSDUR_BA:    /* RTSDuration_ba */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
>  		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
>  		break;
>
>  	case RTSDUR_AA:    /* RTSDuration_aa */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
>  		uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wRate, bNeedAck);
>  		break;
>
> @@ -363,7 +363,7 @@ s_uGetRTSCTSDuration(
>  		break;
>
>  	case RTSDUR_BA_F0: /* RTSDuration_ba_f0 */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
>  		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
>  			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate - RATE_18M], bNeedAck);
>  		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
> @@ -372,7 +372,7 @@ s_uGetRTSCTSDuration(
>  		break;
>
>  	case RTSDUR_AA_F0: /* RTSDuration_aa_f0 */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
>  		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
>  			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE0][wRate - RATE_18M], bNeedAck);
>  		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
> @@ -381,7 +381,7 @@ s_uGetRTSCTSDuration(
>  		break;
>
>  	case RTSDUR_BA_F1: /* RTSDuration_ba_f1 */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopCCKBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopCCKBasicRate);
>  		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
>  			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate - RATE_18M], bNeedAck);
>  		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
> @@ -390,7 +390,7 @@ s_uGetRTSCTSDuration(
>  		break;
>
>  	case RTSDUR_AA_F1: /* RTSDuration_aa_f1 */
> -		uCTSTime = bb_get_frame_time(pDevice->byPreambleType, byPktType, 14, pDevice->byTopOFDMBasicRate);
> +		uCTSTime = bb_get_frame_time(pDevice->preamble_type, byPktType, 14, pDevice->byTopOFDMBasicRate);
>  		if ((byFBOption == AUTO_FB_0) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
>  			uDurTime = uCTSTime + 2 * pDevice->uSIFS + s_uGetTxRsvTime(pDevice, byPktType, cbFrameLength, wFB_Opt0[FB_RATE1][wRate - RATE_18M], bNeedAck);
>  		else if ((byFBOption == AUTO_FB_1) && (wRate >= RATE_18M) && (wRate <= RATE_54M))
> @@ -1289,9 +1289,9 @@ int vnt_generate_fifo_header(struct vnt_private *priv, u32 dma_idx,
>  		tx_buffer_head->fifo_ctl |= cpu_to_le16(FIFOCTL_LRETRY);
>
>  	if (tx_rate->flags & IEEE80211_TX_RC_USE_SHORT_PREAMBLE)
> -		priv->byPreambleType = PREAMBLE_SHORT;
> +		priv->preamble_type = PREAMBLE_SHORT;
>  	else
> -		priv->byPreambleType = PREAMBLE_LONG;
> +		priv->preamble_type = PREAMBLE_LONG;
>
>  	if (tx_rate->flags & IEEE80211_TX_RC_USE_RTS_CTS)
>  		tx_buffer_head->fifo_ctl |= cpu_to_le16(FIFOCTL_RTS);
> --
> 2.30.2
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/20211014151543.46764-1-karolinadrobnik%40gmail.com.
>

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

[Greg KH]

On Thu, Oct 14, 2021 at 04:15:43PM +0100, Karolina Drobnik wrote:
> Drop Hungarian notation prefix in `byPreambleType` member of
> struct vnt_private. Change it to use snake case.
> 
> Fix issue detected by checkpatch.pl:
>   CHECK: Avoid CamelCase: <byPreambleType>
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/baseband.c    | 10 +++---
>  drivers/staging/vt6655/baseband.h    |  2 +-
>  drivers/staging/vt6655/device.h      |  2 +-
>  drivers/staging/vt6655/device_main.c |  8 ++---
>  drivers/staging/vt6655/rxtx.c        | 50 ++++++++++++++--------------
>  5 files changed, 36 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/baseband.c b/drivers/staging/vt6655/baseband.c
> index 0bae35af6e0f..69739de927be 100644
> --- a/drivers/staging/vt6655/baseband.c
> +++ b/drivers/staging/vt6655/baseband.c
> @@ -1691,8 +1691,8 @@ static const unsigned short awc_frame_time[MAX_RATE] = {
>   *
>   * Parameters:
>   *  In:
> - *      by_preamble_type  - Preamble Type
> - *      by_pkt_type        - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA
> + *      preamble_type     - Preamble Type
> + *      by_pkt_type       - PK_TYPE_11A, PK_TYPE_11B, PK_TYPE_11GB, PK_TYPE_11GA

This is not described in your changelog as something you did.  Are you
sure you are not mixing up different changes in one patch?

thanks,

greg k-h

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

[Karolina Drobnik]

Hi,

On Thu, 2021-10-14 at 17:26 +0200, Julia Lawall wrote:
> On Thu, 14 Oct 2021, Karolina Drobnik wrote:
>
> > Drop Hungarian notation prefix in `byPreambleType` member of
> > struct vnt_private. Change it to use snake case.
>
> Are you sure that it is an instance of Hungarian notation?  Could it
> be that "by" is meant to be the word "by"?

I saw that functions and parameters use it[1] so I deduced that "by"
might stand for byte. If this is not a case, I can revert that change
and leave `by_preamble_type` name. What do you think?

> I have the imrepssion from a
> quick look through the code that it should be a boolean. 

Hmm, it really looks like it. In `vnt_bss_info_changed` definition we
use true/false values but in `bb_get_frame_time` we have such a line:

```
if (byPreambleType == 1)     /* Short */
```

I'm not quite sure to what this "short" might refer to.

> Maybe it  would be helpful to give it boolean value, 
> eg true instead of 1.

This is a good idea but I'm not sure if it's only used as a boolean. 
I could add this change but I'd like to test it more than just by
loading/unloading the module. Is there any way of testing drivers
without an actual hardware?


Thanks,
Karolina

----------------------------------------
[1] - For example in rxtx.c, lines 154-161:
```
static
unsigned int
s_uGetTxRsvTime(                     /* s - static, u - unsigned */
        struct vnt_private *pDevice, /* p - pointer */
        unsigned char byPktType,     /* by - byte */
        unsigned int cbFrameLength,  /* not sure */
        unsigned short wRate,        /* w - word */
        bool bNeedAck                /* b - boolean */
)
```

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

[Karolina Drobnik]

Hi,

On Thu, 2021-10-14 at 17:36 +0200, Greg KH wrote:
> On Thu, Oct 14, 2021 at 04:15:43PM +0100, Karolina Drobnik wrote:
> > Drop Hungarian notation prefix in `byPreambleType` member of
> > struct vnt_private. Change it to use snake case.
> >
> > Fix issue detected by checkpatch.pl:
> >   CHECK: Avoid CamelCase: <byPreambleType>
> >
> > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > ---
> >  drivers/staging/vt6655/baseband.c    | 10 +++---
> >  drivers/staging/vt6655/baseband.h    |  2 +-
> >  drivers/staging/vt6655/device.h      |  2 +-
> >  drivers/staging/vt6655/device_main.c |  8 ++---
> >  drivers/staging/vt6655/rxtx.c        | 50
> > ++++++++++++++--------------
> >  5 files changed, 36 insertions(+), 36 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/baseband.c
> > b/drivers/staging/vt6655/baseband.c
> > index 0bae35af6e0f..69739de927be 100644
> > --- a/drivers/staging/vt6655/baseband.c
> > +++ b/drivers/staging/vt6655/baseband.c
> > @@ -1691,8 +1691,8 @@ static const unsigned short
> > awc_frame_time[MAX_RATE] = {
> >   *
> >   * Parameters:
> >   *  In:
> > - *      by_preamble_type  - Preamble Type
> > - *      by_pkt_type        - PK_TYPE_11A, PK_TYPE_11B,
> > PK_TYPE_11GB, PK_TYPE_11GA
> > + *      preamble_type     - Preamble Type
> > + *      by_pkt_type       - PK_TYPE_11A, PK_TYPE_11B,
> > PK_TYPE_11GB, PK_TYPE_11GA
>
> This is not described in your changelog as something you did.  Are
> you sure you are not mixing up different changes in one patch?

I added whitespace in `by_pkt_type` line to align hyphens after the
name change. Should I mention it in the description or just revert it?


Thanks,
Karolina

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

[Greg KH]

On Thu, Oct 14, 2021 at 06:47:29PM +0100, Karolina Drobnik wrote:
> Hi,
> 
> On Thu, 2021-10-14 at 17:36 +0200, Greg KH wrote:
> > On Thu, Oct 14, 2021 at 04:15:43PM +0100, Karolina Drobnik wrote:
> > > Drop Hungarian notation prefix in `byPreambleType` member of
> > > struct vnt_private. Change it to use snake case.
> > >
> > > Fix issue detected by checkpatch.pl:
> > >   CHECK: Avoid CamelCase: <byPreambleType>
> > >
> > > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > > ---
> > >  drivers/staging/vt6655/baseband.c    | 10 +++---
> > >  drivers/staging/vt6655/baseband.h    |  2 +-
> > >  drivers/staging/vt6655/device.h      |  2 +-
> > >  drivers/staging/vt6655/device_main.c |  8 ++---
> > >  drivers/staging/vt6655/rxtx.c        | 50
> > > ++++++++++++++--------------
> > >  5 files changed, 36 insertions(+), 36 deletions(-)
> > >
> > > diff --git a/drivers/staging/vt6655/baseband.c
> > > b/drivers/staging/vt6655/baseband.c
> > > index 0bae35af6e0f..69739de927be 100644
> > > --- a/drivers/staging/vt6655/baseband.c
> > > +++ b/drivers/staging/vt6655/baseband.c
> > > @@ -1691,8 +1691,8 @@ static const unsigned short
> > > awc_frame_time[MAX_RATE] = {
> > >   *
> > >   * Parameters:
> > >   *  In:
> > > - *      by_preamble_type  - Preamble Type
> > > - *      by_pkt_type        - PK_TYPE_11A, PK_TYPE_11B,
> > > PK_TYPE_11GB, PK_TYPE_11GA
> > > + *      preamble_type     - Preamble Type
> > > + *      by_pkt_type       - PK_TYPE_11A, PK_TYPE_11B,
> > > PK_TYPE_11GB, PK_TYPE_11GA
> >
> > This is not described in your changelog as something you did.  Are
> > you sure you are not mixing up different changes in one patch?
> 
> I added whitespace in `by_pkt_type` line to align hyphens after the
> name change. Should I mention it in the description or just revert it?

Each patch should only do one logical thing.  And every "thing" you do,
has to be described in the changelog text.

thanks,

greg k-h

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

[Julia Lawall]

On Thu, 14 Oct 2021, Karolina Drobnik wrote:

> Hi,
>
> On Thu, 2021-10-14 at 17:26 +0200, Julia Lawall wrote:
> > On Thu, 14 Oct 2021, Karolina Drobnik wrote:
> >
> > > Drop Hungarian notation prefix in `byPreambleType` member of
> > > struct vnt_private. Change it to use snake case.
> >
> > Are you sure that it is an instance of Hungarian notation?  Could it
> > be that "by" is meant to be the word "by"?
>
> 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.  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.

> If this is not a case, I can revert that change
> and leave `by_preamble_type` name. What do you think?
>
> > I have the imrepssion from a
> > quick look through the code that it should be a boolean.
>
> Hmm, it really looks like it. In `vnt_bss_info_changed` definition we
> use true/false values but in `bb_get_frame_time` we have such a line:
>
> ```
> if (byPreambleType == 1)     /* Short */
> ```
>
> I'm not quite sure to what this "short" might refer to.
>
> > Maybe it  would be helpful to give it boolean value,
> > eg true instead of 1.
>
> This is a good idea but I'm not sure if it's only used as a boolean.
> I could add this change but I'd like to test it more than just by
> loading/unloading the module. Is there any way of testing drivers
> without an actual hardware?

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.

There is also
vt6655/rxtx.c:  return cpu_to_le16(wTimeStampOff[priv->byPreambleType % 2]
which also makes no sense for a boolean.

You would have to trace through the code and see what values are stored in
this field and how they are used.

julia

>
> Thanks,
> Karolina
>
> ----------------------------------------
> [1] - For example in rxtx.c, lines 154-161:
> ```
> static
> unsigned int
> s_uGetTxRsvTime(                     /* s - static, u - unsigned */
>         struct vnt_private *pDevice, /* p - pointer */
>         unsigned char byPktType,     /* by - byte */
>         unsigned int cbFrameLength,  /* not sure */
>         unsigned short wRate,        /* w - word */
>         bool bNeedAck                /* b - boolean */
> )
> ```
>
>

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

[Karolina Drobnik]

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?


Thanks,
Karolina

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

[Julia Lawall]

[-- 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

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

[Mike Rapoport]

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.

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

[Karolina Drobnik]

On Fri, 2021-10-15 at 10:48 +0200, Julia Lawall wrote:
> 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.

I see, it makes sense to keep it consistent type-wise and leave it as
u8 then.

> 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.

Sounds like a plan, I can address that magical number in the next
patch, thank you.


On Fri, 2021-10-15 at 11:52 +0300, Mike Rapoport wrote:
> 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.

Thanks for explanation, I wasn't sure what the convention is.

Who would've thought that so many interesting things can come out from
just one variable rename :)

Just to double-check - should I cc the mailing lists returned by
get_maintainers in my V2 patch?


Thanks,
Karolina

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

[Julia Lawall]

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



On Fri, 15 Oct 2021, Karolina Drobnik wrote:

> On Fri, 2021-10-15 at 10:48 +0200, Julia Lawall wrote:
> > 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.
>
> I see, it makes sense to keep it consistent type-wise and leave it as
> u8 then.
>
> > 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.
>
> Sounds like a plan, I can address that magical number in the next
> patch, thank you.
>
>
> On Fri, 2021-10-15 at 11:52 +0300, Mike Rapoport wrote:
> > 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.
>
> Thanks for explanation, I wasn't sure what the convention is.
>
> Who would've thought that so many interesting things can come out from
> just one variable rename :)

It's a pretty common phenomenon :)

> Just to double-check - should I cc the mailing lists returned by
> get_maintainers in my V2 patch?

Yes.

julia