[PATCH v2 0/8] subject: staging: vt6655: Fix line wrapping in `RFvWriteWakeProgSyn`

[PATCH v2 0/8] subject: staging: vt6655: Fix line wrapping in `RFvWriteWakeProgSyn`

[Karolina Drobnik]

This patch set is a series of small refactorings of the function
`RFvWriteWakeProgSyn`, now renamed to `rf_write_wake_prog_syn`.
The work here allowed to shorten lines marked by checkpatch.pl as
being too long by:
  * introducing two new temporary variables (`idx` and `init_table`)
  * rewriting a conditional to switch between two modes of
    AL7320 initialization.

In addition to this, the patch set renames the function to align it with
the kernel coding style, updates the function description and removes two
unnecessary conditionals checking if the value of `init_count` is lower than
a predefined value.

Changes in v2:
  * drop "staging: vt6655: Introduce `data` temporary variable" patch, suggested
    by Julia Lawall <julia.lawall@inria.fr> 
  * amend "staging: vt6655: Rewrite conditional in AL7320 initialization" patch
    to use indexing and `init_table` temporary variable
  * add two new patches to remove unnecessary checks for init count, suggested
    by Mike Rapoport <mike.rapoport@gmail.com>:
      - "staging: vt6655: Delete bogus check for `init_count` in AL2230"
      - "staging: vt6655: Delete bogus check for `init_count` in AL7230"

Karolina Drobnik (8):
  staging: vt6655: Introduce `idx` temporary variable
  staging: vt6655: Use incrementation in `idx`
  staging: vt6655: Remove unused `i` increments
  staging: vt6655: Rewrite conditional in AL7320 initialization
  staging: vt6655: Rename `RFvWriteWakeProgSyn` function
  staging: vt6655: Update comment for `rf_write_wake_prog_syn`
  staging: vt6655: Delete bogus check for `init_count` in AL2230
  staging: vt6655: Delete bogus check for `init_count` in AL7230

 drivers/staging/vt6655/channel.c |  2 +-
 drivers/staging/vt6655/rf.c      | 44 +++++++++++++-------------------
 drivers/staging/vt6655/rf.h      |  2 +-
 3 files changed, 20 insertions(+), 28 deletions(-)

-- 
2.30.2

[PATCH v2 1/8] staging: vt6655: Introduce `idx` temporary variable

[Karolina Drobnik]

Add a local variable to store `MISCFIFO_SYNDATA_IDX` offset.
This change helps in shortening the lines in `rf.c` that
are deemed too long by checkpatch.pl.

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index bc4abe77db7b..f195dafb6e63 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -681,6 +681,7 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 {
 	void __iomem *iobase = priv->port_offset;
 	int i;
+	unsigned short idx = MISCFIFO_SYNDATA_IDX;
 	unsigned char init_count = 0;
 	unsigned char sleep_count = 0;
 
@@ -699,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 			return false;
 
 		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
-			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al2230_init_table[i]);
+			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al2230_channel_table0[channel - 1]);
+		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_channel_table0[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al2230_channel_table1[channel - 1]);
+		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_channel_table1[channel - 1]);
 		break;
 
 		/* Need to check, PLLON need to be low for channel setting */
@@ -716,17 +717,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 
 		if (channel <= CB_MAX_CHANNEL_24G) {
 			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al7230_init_table[i]);
+				MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_init_table[i]);
 		} else {
 			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al7230_init_table_a_mode[i]);
+				MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_init_table_a_mode[i]);
 		}
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al7230_channel_table0[channel - 1]);
+		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table0[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al7230_channel_table1[channel - 1]);
+		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table1[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + i), al7230_channel_table2[channel - 1]);
+		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table2[channel - 1]);
 		break;
 
 	case RF_NOTHING:
-- 
2.30.2

[PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

[Karolina Drobnik]

Increment `idx` in a loop instead of adding the loop counter
`i` to do so. Thanks to this change, the cast to unsigned short
can be removed.

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index f195dafb6e63..c07653566d17 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 			return false;
 
 		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
-			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
+			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
 
-		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_channel_table0[channel - 1]);
+		MACvSetMISCFifo(priv, idx++, al2230_channel_table0[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_channel_table1[channel - 1]);
+		MACvSetMISCFifo(priv, idx++, al2230_channel_table1[channel - 1]);
 		break;
 
 		/* Need to check, PLLON need to be low for channel setting */
@@ -717,17 +717,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 
 		if (channel <= CB_MAX_CHANNEL_24G) {
 			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_init_table[i]);
+				MACvSetMISCFifo(priv, idx++, al7230_init_table[i]);
 		} else {
 			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_init_table_a_mode[i]);
+				MACvSetMISCFifo(priv, idx++, al7230_init_table_a_mode[i]);
 		}
 
-		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table0[channel - 1]);
+		MACvSetMISCFifo(priv, idx++, al7230_channel_table0[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table1[channel - 1]);
+		MACvSetMISCFifo(priv, idx++, al7230_channel_table1[channel - 1]);
 		i++;
-		MACvSetMISCFifo(priv, (unsigned short)(idx + i), al7230_channel_table2[channel - 1]);
+		MACvSetMISCFifo(priv, idx++, al7230_channel_table2[channel - 1]);
 		break;
 
 	case RF_NOTHING:
-- 
2.30.2

[PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

[Karolina Drobnik]

Commit c569952d92ba ("staging: vt6655: Use incrementation in `idx`")
rendered the incrementation of `i` outside of the loop unnecessary
so it can be deleted.

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index c07653566d17..ea74701917e5 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -703,7 +703,6 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
 
 		MACvSetMISCFifo(priv, idx++, al2230_channel_table0[channel - 1]);
-		i++;
 		MACvSetMISCFifo(priv, idx++, al2230_channel_table1[channel - 1]);
 		break;
 
@@ -724,9 +723,7 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 		}
 
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table0[channel - 1]);
-		i++;
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table1[channel - 1]);
-		i++;
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table2[channel - 1]);
 		break;
 
-- 
2.30.2

[PATCH v2 4/8] staging: vt6655: Rewrite conditional in AL7320 initialization

[Karolina Drobnik]

Use conditional operator to determine which table for AL7320
initialization should be used. Introduce `init_table` variable
to store this value.

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index ea74701917e5..e7672da39a82 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -684,6 +684,7 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 	unsigned short idx = MISCFIFO_SYNDATA_IDX;
 	unsigned char init_count = 0;
 	unsigned char sleep_count = 0;
+	const unsigned long *init_table;
 
 	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
 	switch (rf_type) {
@@ -714,13 +715,10 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
 			return false;
 
-		if (channel <= CB_MAX_CHANNEL_24G) {
-			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, idx++, al7230_init_table[i]);
-		} else {
-			for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
-				MACvSetMISCFifo(priv, idx++, al7230_init_table_a_mode[i]);
-		}
+		init_table = (channel <= CB_MAX_CHANNEL_24G) ?
+			al7230_init_table : al7230_init_table_a_mode;
+		for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
+			MACvSetMISCFifo(priv, idx++, init_table[i]);
 
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table0[channel - 1]);
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table1[channel - 1]);
-- 
2.30.2

[PATCH v2 5/8] staging: vt6655: Rename `RFvWriteWakeProgSyn` function

[Karolina Drobnik]

To align with the kernel coding style, remove the type from
the function name and do not use CamelCase.

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

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/channel.c | 2 +-
 drivers/staging/vt6655/rf.c      | 4 ++--
 drivers/staging/vt6655/rf.h      | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/channel.c b/drivers/staging/vt6655/channel.c
index b550a1a0844e..e37c8e35a45b 100644
--- a/drivers/staging/vt6655/channel.c
+++ b/drivers/staging/vt6655/channel.c
@@ -189,7 +189,7 @@ bool set_channel(struct vnt_private *priv, struct ieee80211_channel *ch)
 
 	/* Init Synthesizer Table */
 	if (priv->bEnablePSMode)
-		RFvWriteWakeProgSyn(priv, priv->byRFType, ch->hw_value);
+		rf_write_wake_prog_syn(priv, priv->byRFType, ch->hw_value);
 
 	bb_software_reset(priv);
 
diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index e7672da39a82..34fa54f7a92d 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -676,8 +676,8 @@ bool RFbSelectChannel(struct vnt_private *priv, unsigned char byRFType,
  * Return Value: None.
  *
  */
-bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
-			 u16 channel)
+bool rf_write_wake_prog_syn(struct vnt_private *priv, unsigned char rf_type,
+			    u16 channel)
 {
 	void __iomem *iobase = priv->port_offset;
 	int i;
diff --git a/drivers/staging/vt6655/rf.h b/drivers/staging/vt6655/rf.h
index 0939937d47a8..9fef81846a9f 100644
--- a/drivers/staging/vt6655/rf.h
+++ b/drivers/staging/vt6655/rf.h
@@ -60,7 +60,7 @@
 bool IFRFbWriteEmbedded(struct vnt_private *priv, unsigned long dwData);
 bool RFbSelectChannel(struct vnt_private *priv, unsigned char byRFType, u16 byChannel);
 bool RFbInit(struct vnt_private *priv);
-bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type, u16 channel);
+bool rf_write_wake_prog_syn(struct vnt_private *priv, unsigned char rf_type, u16 channel);
 bool RFbSetPower(struct vnt_private *priv, unsigned int rate, u16 uCH);
 bool RFbRawSetPower(struct vnt_private *priv, unsigned char byPwr,
 		    unsigned int rate);
-- 
2.30.2

[PATCH v2 6/8] staging: vt6655: Update comment for `rf_write_wake_prog_syn`

[Karolina Drobnik]

Change the function description to include the actual parameters.
Update the comment on the return type.

Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index 34fa54f7a92d..1fe073c5fb6f 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -669,11 +669,11 @@ bool RFbSelectChannel(struct vnt_private *priv, unsigned char byRFType,
  *
  * Parameters:
  *  In:
- *      iobase      - I/O base address
- *      channel     - channel number
- *      bySleepCnt  - SleepProgSyn count
+ *      priv        - Device Structure
+ *      rf_type     - RF type
+ *      channel     - Channel number
  *
- * Return Value: None.
+ * Return Value: true if succeeded; false if failed.
  *
  */
 bool rf_write_wake_prog_syn(struct vnt_private *priv, unsigned char rf_type,
-- 
2.30.2

[PATCH v2 7/8] staging: vt6655: Delete bogus check for `init_count` in AL2230

[Karolina Drobnik]

Remove an unnecessary check in `rf_write_wake_prog_syn` in `RF_AL2230S`
switch case. This `if` conditional will never be true as `init_count` is
equal to 17 and can't be bigger than `MISCFIFO_SYNDATASIZE - 0`, which
is equal to 21.

Suggested-by: Mike Rapoport <mike.rapoport@gmail.com>
Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index 1fe073c5fb6f..a150f1df3824 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -697,8 +697,6 @@ bool rf_write_wake_prog_syn(struct vnt_private *priv, unsigned char rf_type,
 		 /* Init Reg + Channel Reg (2) */
 		init_count = CB_AL2230_INIT_SEQ + 2;
 		sleep_count = 0;
-		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
-			return false;
 
 		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
 			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
-- 
2.30.2

[PATCH v2 8/8] staging: vt6655: Delete bogus check for `init_count` in AL7230

[Karolina Drobnik]

Remove an unnecessary check in `rf_write_wake_prog_syn` in `RF_AIROHA7230`
switch case. This `if` conditional will never be true as `init_count` is
equal to 19 and can't be bigger than `MISCFIFO_SYNDATASIZE - 0`, which
is equal to 21.

Suggested-by: Mike Rapoport <mike.rapoport@gmail.com>
Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
---
 drivers/staging/vt6655/rf.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index a150f1df3824..653f72c7ce5b 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -710,8 +710,6 @@ bool rf_write_wake_prog_syn(struct vnt_private *priv, unsigned char rf_type,
 		 /* Init Reg + Channel Reg (3) */
 		init_count = CB_AL7230_INIT_SEQ + 3;
 		sleep_count = 0;
-		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
-			return false;
 
 		init_table = (channel <= CB_MAX_CHANNEL_24G) ?
 			al7230_init_table : al7230_init_table_a_mode;
-- 
2.30.2

Re: [PATCH v2 1/8] staging: vt6655: Introduce `idx` temporary variable

[Dan Carpenter]

On Mon, Nov 01, 2021 at 02:31:59PM +0000, Karolina Drobnik wrote:
> Add a local variable to store `MISCFIFO_SYNDATA_IDX` offset.
> This change helps in shortening the lines in `rf.c` that
> are deemed too long by checkpatch.pl.
> 

I started reviewing the v1 patch but I should have been reviewing this
patchset.  Please don't do this.  Leave the constant as a constant so
we don't have to look it up.

regards,
dan carpenter

Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

[Dan Carpenter]

On Mon, Nov 01, 2021 at 02:32:00PM +0000, Karolina Drobnik wrote:
> Increment `idx` in a loop instead of adding the loop counter
> `i` to do so. Thanks to this change, the cast to unsigned short
> can be removed.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index f195dafb6e63..c07653566d17 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
>  			return false;
>  
>  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> -			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
> +			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);

The cast was always pointless.  You can remove it without any other
changes.

regards,
dan carpenter

Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

[Dan Carpenter]

On Mon, Nov 01, 2021 at 02:32:00PM +0000, Karolina Drobnik wrote:
> Increment `idx` in a loop instead of adding the loop counter
> `i` to do so. Thanks to this change, the cast to unsigned short
> can be removed.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index f195dafb6e63..c07653566d17 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
>  			return false;
>  
>  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> -			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
> +			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);

Of course, idx is no longer a constant so declaring it as a variable
makes sense here.  But maybe just do it in the same patch because the
patch 1/1 doesn't make sense as a stand alone patch.

Also don't declare idx as an unsigned short.  It's better to declare it
as a int so it just works like a normal number and you don't have to
think about signedness bugs and wrapping and edge cases.

regards,
dan carpenter

Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

[Dan Carpenter]

On Mon, Nov 01, 2021 at 02:32:01PM +0000, Karolina Drobnik wrote:
> Commit c569952d92ba ("staging: vt6655: Use incrementation in `idx`")
> rendered the incrementation of `i` outside of the loop unnecessary
> so it can be deleted.
> 

That commit hash is something that only exists on your system.  Commit
hashes are stable once they hit Greg's tree (he only rebases in extremely
rarely cases).

This commit is cleaning something that was left in a different patch in
the same patch set.  Just merge it into the original patch.  Don't make
a mess and then fix it.

It's tricky to know how to break up patches.  My suggestion is:
patch 1: remove all the unnecesary (unsigned short) casts
patch 2: merge the rest of patches 1-3 together and send it at once

The rest of the patchset is fine.

regards,
dan carpenter

Re: [PATCH v2 1/8] staging: vt6655: Introduce `idx` temporary variable

[Joe Perches]

On Thu, 2021-11-04 at 15:09 +0300, Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 02:31:59PM +0000, Karolina Drobnik wrote:
> > Add a local variable to store `MISCFIFO_SYNDATA_IDX` offset.
> > This change helps in shortening the lines in `rf.c` that
> > are deemed too long by checkpatch.pl.
> > 
> 
> I started reviewing the v1 patch but I should have been reviewing this
> patchset.  Please don't do this.  Leave the constant as a constant so
> we don't have to look it up.

It's just an intermediate step to use idx++.
Personally, I'd combine patches to add and use idx++ directly.

Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

[Joe Perches]

On Thu, 2021-11-04 at 16:00 +0300, Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 02:32:00PM +0000, Karolina Drobnik wrote:
> > Increment `idx` in a loop instead of adding the loop counter
> > `i` to do so. Thanks to this change, the cast to unsigned short
> > can be removed.
> > 
> > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > ---
> >  drivers/staging/vt6655/rf.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
[]
> > @@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
> >  			return false;
> >  
> >  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> > -			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
> > +			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
> 
> Of course, idx is no longer a constant so declaring it as a variable
> makes sense here.  But maybe just do it in the same patch because the
> patch 1/1 doesn't make sense as a stand alone patch.
> 
> Also don't declare idx as an unsigned short.  It's better to declare it
> as a int so it just works like a normal number and you don't have to
> think about signedness bugs and wrapping and edge cases.

No, IMO it really should be a u16.
Look at the iowrite16() within the call to MACvSetMISCFifo.

Re: [PATCH v2 2/8] staging: vt6655: Use incrementation in `idx`

[Dan Carpenter]

On Thu, Nov 04, 2021 at 07:44:22AM -0700, Joe Perches wrote:
> On Thu, 2021-11-04 at 16:00 +0300, Dan Carpenter wrote:
> > On Mon, Nov 01, 2021 at 02:32:00PM +0000, Karolina Drobnik wrote:
> > > Increment `idx` in a loop instead of adding the loop counter
> > > `i` to do so. Thanks to this change, the cast to unsigned short
> > > can be removed.
> > > 
> > > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > > ---
> > >  drivers/staging/vt6655/rf.c | 16 ++++++++--------
> > >  1 file changed, 8 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> []
> > > @@ -700,11 +700,11 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
> > >  			return false;
> > >  
> > >  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> > > -			MACvSetMISCFifo(priv, (unsigned short)(idx + i), al2230_init_table[i]);
> > > +			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
> > 
> > Of course, idx is no longer a constant so declaring it as a variable
> > makes sense here.  But maybe just do it in the same patch because the
> > patch 1/1 doesn't make sense as a stand alone patch.
> > 
> > Also don't declare idx as an unsigned short.  It's better to declare it
> > as a int so it just works like a normal number and you don't have to
> > think about signedness bugs and wrapping and edge cases.
> 
> No, IMO it really should be a u16.
> Look at the iowrite16() within the call to MACvSetMISCFifo.

Obviously we all agree it has no effect on runtime at all.

I just think that people get into trouble getting too creative with
their types.  There was some discussion earlier about where the casting
was required (it's not required at all).  It seems like there was some
confusion about how types work.  Declaring it as "unsigned short" has
some documentation value, I guess, but you avoid signedness bugs when
you declare things as int.

I don't care too much, but I also try to tell people not to be fancy.

regards,
dan carpenter

Re: [PATCH v2 1/8] staging: vt6655: Introduce `idx` temporary variable

[Dan Carpenter]

On Thu, Nov 04, 2021 at 07:33:36AM -0700, Joe Perches wrote:
> On Thu, 2021-11-04 at 15:09 +0300, Dan Carpenter wrote:
> > On Mon, Nov 01, 2021 at 02:31:59PM +0000, Karolina Drobnik wrote:
> > > Add a local variable to store `MISCFIFO_SYNDATA_IDX` offset.
> > > This change helps in shortening the lines in `rf.c` that
> > > are deemed too long by checkpatch.pl.
> > > 
> > 
> > I started reviewing the v1 patch but I should have been reviewing this
> > patchset.  Please don't do this.  Leave the constant as a constant so
> > we don't have to look it up.
> 
> It's just an intermediate step to use idx++.
> Personally, I'd combine patches to add and use idx++ directly.

Yep.  I saw that when I reviewed the later patches.  This patch should
be combined with those as you say.

regards,
dan carpenter

Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

[Karolina Drobnik]

Hi Dan,

Thanks for taking a look at my patches.

On Thu, 2021-11-04 at 16:38 +0300, Dan Carpenter wrote:
> On Mon, Nov 01, 2021 at 02:32:01PM +0000, Karolina Drobnik wrote:
> > Commit c569952d92ba ("staging: vt6655: Use incrementation in
> > `idx`")
> > rendered the incrementation of `i` outside of the loop unnecessary
> > so it can be deleted.
> > 
> 
> That commit hash is something that only exists on your system. 
> Commit hashes are stable once they hit Greg's tree (he only rebases 
> in extremely rarely cases).

Ok, I can rewrite the message but I'm not sure how should I refer to
another patch from my patch set. I followed these guidelines[1] but if
there's a different way of describing it then please let me know.

> This commit is cleaning something that was left in a different patch
> in the same patch set.  Just merge it into the original patch.  Don't
> make a mess and then fix it.

I tried adding more than one logical change per patch some time ago and
Greg asked me to stop doing this.

> It's tricky to know how to break up patches.  My suggestion is:
> patch 1: remove all the unnecesary (unsigned short) casts
> patch 2: merge the rest of patches 1-3 together and send it at once

Sounds good. If Greg is happy with your approach, I can merge these
patches, no problem. 


Thanks,
Karolina

---------------------------------------------------------------
[1] -
https://elixir.bootlin.com/linux/latest/source/Documentation/process/submitting-patches.rst#L106

Re: [PATCH v2 3/8] staging: vt6655: Remove unused `i` increments

[Dan Carpenter]

On Mon, Nov 08, 2021 at 09:57:25AM +0000, Karolina Drobnik wrote:
> > This commit is cleaning something that was left in a different patch
> > in the same patch set.  Just merge it into the original patch.  Don't
> > make a mess and then fix it.
> 
> I tried adding more than one logical change per patch some time ago and
> Greg asked me to stop doing this.
> 
> > It's tricky to know how to break up patches.  My suggestion is:
> > patch 1: remove all the unnecesary (unsigned short) casts
> > patch 2: merge the rest of patches 1-3 together and send it at once
> 
> Sounds good. If Greg is happy with your approach, I can merge these
> patches, no problem.

The one thing per patch is tricky, but it means one *whole* thing per
patch, not half a thing per patch.  This patch does the anti-pattern
of half do something and then clean up the fall out later.  Sometimes
that is actually a good approach because it can make reviewing easier
if you're doing a ton of similar changes.

The one thing per patch is also about how you present it in the commit
message.  One way of thinking about this is that your first patch
introduces static warnings about "idx" set but not used warnings so you
*must* fix it in the same patch.  At that point it's not an optional
part of the fix.  If it were just a related cleanup then "Oh, well,
that could go either way." but now that you point out the static checker
warning then it's not optional or it sort of almost violates the rule on
bisectable code.

regards,
dan carpenter