[PATCH 0/7] staging: vt6655: Fix line wrapping in `RFvWriteWakeProgSyn`

[PATCH 0/7] 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 `data`)
  * 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 and updates the function description.

Karolina Drobnik (7):
  staging: vt6655: Introduce `idx` temporary variable
  staging: vt6655: Use incrementation in `idx`
  staging: vt6655: Remove unused `i` increments
  staging: vt6655: Introduce `data` temporary variable
  staging: vt6655: Rewrite conditional in AL7320 initialization
  staging: vt6655: Rename `RFvWriteWakeProgSyn` function
  staging: vt6655: Update comment for `rf_write_wake_prog_syn`

 drivers/staging/vt6655/channel.c |  2 +-
 drivers/staging/vt6655/rf.c      | 41 +++++++++++++++-----------------
 drivers/staging/vt6655/rf.h      |  2 +-
 3 files changed, 21 insertions(+), 24 deletions(-)

-- 
2.30.2

[PATCH 1/7] 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 2/7] 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 3/7] 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 4/7] staging: vt6655: Introduce `data` temporary variable

[Karolina Drobnik]

Add a variable to store initialization tables. Use this variable
in AL2230 initialization.

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

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index ea74701917e5..afd202ea3356 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 *data;
 
 	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
 	switch (rf_type) {
@@ -699,8 +700,9 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char rf_type,
 		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
 			return false;
 
+		data = al2230_init_table;
 		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
-			MACvSetMISCFifo(priv, idx++, al2230_init_table[i]);
+			MACvSetMISCFifo(priv, idx++, *(data++));
 
 		MACvSetMISCFifo(priv, idx++, al2230_channel_table0[channel - 1]);
 		MACvSetMISCFifo(priv, idx++, al2230_channel_table1[channel - 1]);
-- 
2.30.2

[PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

[Karolina Drobnik]

Use conditional operator to determine which table for AL7320
initialization should be used. Use `data` temporary value to
store this value.

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

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index afd202ea3356..af4eb7eb8e7d 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -716,13 +716,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]);
-		}
+		data = (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++, *(data++));
 
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table0[channel - 1]);
 		MACvSetMISCFifo(priv, idx++, al7230_channel_table1[channel - 1]);
-- 
2.30.2

[PATCH 6/7] 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 af4eb7eb8e7d..7caac1b44a68 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 7/7] 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 7caac1b44a68..fb31ec70c019 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

Re: [Outreachy kernel] [PATCH 4/7] staging: vt6655: Introduce `data` temporary variable

[Fabio M. De Francesco]

On Thursday, October 28, 2021 12:35:34 PM CEST Karolina Drobnik wrote:
> Add a variable to store initialization tables. Use this variable
> in AL2230 initialization.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index ea74701917e5..afd202ea3356 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 *data;
>  
>  	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
>  	switch (rf_type) {
> @@ -699,8 +700,9 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, 
unsigned char rf_type,
>  		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
>  			return false;
>  
> +		data = al2230_init_table;
>  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> -			MACvSetMISCFifo(priv, idx++, 
al2230_init_table[i]);
> +			MACvSetMISCFifo(priv, idx++, *(data++));

Hi Karolina,

I think you are using redundant parentheses in "* (data ++)" but understand 
that those increments and dereferences are equivalent to "* data ++" 
(according to the C precedence rules).

Some time ago I suggested that you use those redundant parentheses because
Greg Kroah-Hartman had previously explained that he prefers not to see 
"*foo++" because maintainers and reviewers are not required to remember the C 
precedence rules.

I hope my suggestion isn't based on a misunderstanding of what Greg wants 
here and that your patch can be accepted as is.

While we are at it, please notice that Maintainers of different subsystems 
may see this topic from a different point of view and that they might very 
well ask you for removing those redundant parentheses.

I'd suggest to use grep and find out what is preferred in other subsystems, 
when you want to contribute to other parts of Linux.

Thanks,

Fabio

>  		MACvSetMISCFifo(priv, idx++, 
al2230_channel_table0[channel - 1]);
>  		MACvSetMISCFifo(priv, idx++, 
al2230_channel_table1[channel - 1]);
> -- 
> 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/dc72a4c3539aed70569f66396ed3b51818bc2aea.
1635415820.git.karolinadrobnik%40gmail.com.
> 

Re: [Outreachy kernel] [PATCH 4/7] staging: vt6655: Introduce `data` temporary variable

[Julia Lawall]

On Thu, 28 Oct 2021, Fabio M. De Francesco wrote:

> On Thursday, October 28, 2021 12:35:34 PM CEST Karolina Drobnik wrote:
> > Add a variable to store initialization tables. Use this variable
> > in AL2230 initialization.
> >
> > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > ---
> >  drivers/staging/vt6655/rf.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> > index ea74701917e5..afd202ea3356 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 *data;
> >
> >  	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
> >  	switch (rf_type) {
> > @@ -699,8 +700,9 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv,
> unsigned char rf_type,
> >  		if (init_count > (MISCFIFO_SYNDATASIZE - sleep_count))
> >  			return false;
> >
> > +		data = al2230_init_table;
> >  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> > -			MACvSetMISCFifo(priv, idx++,
> al2230_init_table[i]);
> > +			MACvSetMISCFifo(priv, idx++, *(data++));
>
> Hi Karolina,
>
> I think you are using redundant parentheses in "* (data ++)" but understand
> that those increments and dereferences are equivalent to "* data ++"
> (according to the C precedence rules).
>
> Some time ago I suggested that you use those redundant parentheses because
> Greg Kroah-Hartman had previously explained that he prefers not to see
> "*foo++" because maintainers and reviewers are not required to remember the C
> precedence rules.
>
> I hope my suggestion isn't based on a misunderstanding of what Greg wants
> here and that your patch can be accepted as is.
>
> While we are at it, please notice that Maintainers of different subsystems
> may see this topic from a different point of view and that they might very
> well ask you for removing those redundant parentheses.
>
> I'd suggest to use grep and find out what is preferred in other subsystems,
> when you want to contribute to other parts of Linux.

Would it be better as data[i] ?

Could there be a better name than "data"?  Perhaps "table"?

julia


>
> Thanks,
>
> Fabio
>
> >  		MACvSetMISCFifo(priv, idx++,
> al2230_channel_table0[channel - 1]);
> >  		MACvSetMISCFifo(priv, idx++,
> al2230_channel_table1[channel - 1]);
> > --
> > 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/dc72a4c3539aed70569f66396ed3b51818bc2aea.
> 1635415820.git.karolinadrobnik%40gmail.com.
> >
>
>
>
>
> --
> 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/2039159.k92FijXA2m%40localhost.localdomain.
>

Re: [Outreachy kernel] [PATCH 4/7] staging: vt6655: Introduce `data` temporary variable

[Fabio M. De Francesco]

On Thursday, October 28, 2021 1:32:58 PM CEST Julia Lawall wrote:
> 
> On Thu, 28 Oct 2021, Fabio M. De Francesco wrote:
> 
> > On Thursday, October 28, 2021 12:35:34 PM CEST Karolina Drobnik wrote:
> > > Add a variable to store initialization tables. Use this variable
> > > in AL2230 initialization.
> > >
> > > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > > ---
> > >  drivers/staging/vt6655/rf.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> > > index ea74701917e5..afd202ea3356 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 *data;
> > >
> > >  	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
> > >  	switch (rf_type) {
> > > @@ -699,8 +700,9 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv,
> > unsigned char rf_type,
> > >  		if (init_count > (MISCFIFO_SYNDATASIZE - 
sleep_count))
> > >  			return false;
> > >
> > > +		data = al2230_init_table;
> > >  		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
> > > -			MACvSetMISCFifo(priv, idx++,
> > al2230_init_table[i]);
> > > +			MACvSetMISCFifo(priv, idx++, *(data++));
> >
> > Hi Karolina,
> >
> > I think you are using redundant parentheses in "* (data ++)" but 
understand
> > that those increments and dereferences are equivalent to "* data ++"
> > (according to the C precedence rules).
> >
> > Some time ago I suggested that you use those redundant parentheses 
because
> > Greg Kroah-Hartman had previously explained that he prefers not to see
> > "*foo++" because maintainers and reviewers are not required to remember 
the C
> > precedence rules.
> >
> > I hope my suggestion isn't based on a misunderstanding of what Greg wants
> > here and that your patch can be accepted as is.
> >
> > While we are at it, please notice that Maintainers of different 
subsystems
> > may see this topic from a different point of view and that they might 
very
> > well ask you for removing those redundant parentheses.
> >
> > I'd suggest to use grep and find out what is preferred in other 
subsystems,
> > when you want to contribute to other parts of Linux.
> 
> Would it be better as data[i] ?

In this case, yes for sure. It would be better because we are inside a 'for' 
loop where the code already as an "i" index.

I didn't notice that we are inside the loop and just looked at that increment 
and dereference for checking the proper syntax.

> 
> Could there be a better name than "data"?  Perhaps "table"?

Again, yes if we look at what "data" has been assigned with. 

Thanks,

Fabio

> 
> julia
> 
> 
> >
> > Thanks,
> >
> > Fabio
> >
> > >  		MACvSetMISCFifo(priv, idx++,
> > al2230_channel_table0[channel - 1]);
> > >  		MACvSetMISCFifo(priv, idx++,
> > al2230_channel_table1[channel - 1]);
> > > --
> > > 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/dc72a4c3539aed70569f66396ed3b51818bc2aea.
> > 1635415820.git.karolinadrobnik%40gmail.com.
> > >
> >
> >
> >
> >
> > --
> > 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/2039159.k92FijXA2m%40localhost.localdomain.
> >
> 

Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

[Fabio M. De Francesco]

On Thursday, October 28, 2021 12:35:35 PM CEST Karolina Drobnik wrote:
> Use conditional operator to determine which table for AL7320
> initialization should be used. Use `data` temporary value to
> store this value.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index afd202ea3356..af4eb7eb8e7d 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -716,13 +716,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]);
> -		}
> +		data = (channel <= CB_MAX_CHANNEL_24G) ?
> +			al7230_init_table : 
al7230_init_table_a_mode;

As far as I know by reading some Greg K-H's replies to other developers, this 
"<test> ? <true> : <false>" style is not well accepted here.

I'd prefer to see an explicit "if-else" statement because with that style you 
sacrifice readability and gain nothing here.

> +		for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
> +			MACvSetMISCFifo(priv, idx++, *(data++));

Again, as Julia pointed out, "*data++" and "*(data++)" are syntactically 
correct instructions but are not required here. I'm also pretty sure that, by 
not reusing that index, you're adding additional unnecessary instructions to 
the resulting assembly code (unless the compiler is able to optimize it).

"*foo++" and the like are powerful and compact instructions, but you should 
use them in other, more suitable contexts.

Thanks,

Fabio


>  
>  		MACvSetMISCFifo(priv, idx++, 
al7230_channel_table0[channel - 1]);
>  		MACvSetMISCFifo(priv, idx++, 
al7230_channel_table1[channel - 1]);
> -- 
> 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/
948406a3e7d23f1cdf866aa4448d9428bdd32512.1635415820.git.karolinadrobnik%40gmail.com.
> 

Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

[Julia Lawall]

On Thu, 28 Oct 2021, Fabio M. De Francesco wrote:

> On Thursday, October 28, 2021 12:35:35 PM CEST Karolina Drobnik wrote:
> > Use conditional operator to determine which table for AL7320
> > initialization should be used. Use `data` temporary value to
> > store this value.
> >
> > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > ---
> >  drivers/staging/vt6655/rf.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> > index afd202ea3356..af4eb7eb8e7d 100644
> > --- a/drivers/staging/vt6655/rf.c
> > +++ b/drivers/staging/vt6655/rf.c
> > @@ -716,13 +716,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]);
> > -		}
> > +		data = (channel <= CB_MAX_CHANNEL_24G) ?
> > +			al7230_init_table :
> al7230_init_table_a_mode;
>
> As far as I know by reading some Greg K-H's replies to other developers, this
> "<test> ? <true> : <false>" style is not well accepted here.
>
> I'd prefer to see an explicit "if-else" statement because with that style you
> sacrifice readability and gain nothing here.

Actually, I liked the ?: usage in this case.  We are making an alias, and
it is one thing or another.  One can easily see that the same variable is
assigned in both cases.

Othes could have another opinion, though.

julia


>
> > +		for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
> > +			MACvSetMISCFifo(priv, idx++, *(data++));
>
> Again, as Julia pointed out, "*data++" and "*(data++)" are syntactically
> correct instructions but are not required here. I'm also pretty sure that, by
> not reusing that index, you're adding additional unnecessary instructions to
> the resulting assembly code (unless the compiler is able to optimize it).
>
> "*foo++" and the like are powerful and compact instructions, but you should
> use them in other, more suitable contexts.
>
> Thanks,
>
> Fabio
>
>
> >
> >  		MACvSetMISCFifo(priv, idx++,
> al7230_channel_table0[channel - 1]);
> >  		MACvSetMISCFifo(priv, idx++,
> al7230_channel_table1[channel - 1]);
> > --
> > 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/
> 948406a3e7d23f1cdf866aa4448d9428bdd32512.1635415820.git.karolinadrobnik%40gmail.com.
> >
>
>
>
>
> --
> 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/1683328.aCfAWUeHFl%40localhost.localdomain.
>

Re: [Outreachy kernel] [PATCH 4/7] staging: vt6655: Introduce `data` temporary variable

[Karolina Drobnik]

Hi Fabio and Julia,

Thank you very much for looking at my changes.

On Thu, 2021-10-28 at 13:21 +0200, Fabio M. De Francesco wrote:
> Hi Karolina,
> 
> I think you are using redundant parentheses in "* (data ++)" but
> understand that those increments and dereferences are equivalent to 
> "* data ++" (according to the C precedence rules).

Yes, I added them on purpose to improve readability (+ that's also my
preference anyway)

> While we are at it, please notice that Maintainers of different
> subsystems may see this topic from a different point of view and that
> they might very well ask you for removing those redundant
> parentheses.

I understand, thanks for letting me know.


On Thu, 2021-10-28 at 13:32 +0200, Julia Lawall wrote:
> Would it be better as data[i] ?
> 
> Could there be a better name than "data"?  Perhaps "table"?

Hmm, now when I'm thinking about, it indeed looks like a better option.
I would even say that `data` (or `table`/`init_table`) can be only used
in the AL7320 case and we can go with `al2230_init_table` for AL2230.
The line would be 61 characters long, way below the limit.

What do you think?


Many thanks,
Karolina

Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

[Karolina Drobnik]

On Thu, 2021-10-28 at 14:36 +0200, Fabio M. De Francesco wrote:
> As far as I know by reading some Greg K-H's replies to other
> developers, this 
> "<test> ? <true> : <false>" style is not well accepted here.

I thought that the expression is simple enough that it can be written
this way. Julia nicely summarised why I think it's a good usage of the
conditional operator. Still, there's no problem in changing it to "if-
else" statement if that's the preferred option.


Thanks,
Karolina
> 

Re: [Outreachy kernel] [PATCH 4/7] staging: vt6655: Introduce `data` temporary variable

[Julia Lawall]

> On Thu, 2021-10-28 at 13:32 +0200, Julia Lawall wrote:
> > Would it be better as data[i] ?
> >
> > Could there be a better name than "data"?  Perhaps "table"?
>
> Hmm, now when I'm thinking about, it indeed looks like a better option.
> I would even say that `data` (or `table`/`init_table`) can be only used
> in the AL7320 case and we can go with `al2230_init_table` for AL2230.
> The line would be 61 characters long, way below the limit.
>
> What do you think?

That would amount to dropping patch 4?  That seems fine.  It is better to
avoid creating aliases.

julia

Re: [Outreachy kernel] [PATCH 5/7] staging: vt6655: Rewrite conditional in AL7320 initialization

[Fabio M. De Francesco]

On Thursday, October 28, 2021 4:35:30 PM CEST Karolina Drobnik wrote:
> On Thu, 2021-10-28 at 14:36 +0200, Fabio M. De Francesco wrote:
> > As far as I know by reading some Greg K-H's replies to other
> > developers, this 
> > "<test> ? <true> : <false>" style is not well accepted here.
> 
> I thought that the expression is simple enough that it can be written
> this way. Julia nicely summarised why I think it's a good usage of the
> conditional operator. Still, there's no problem in changing it to "if-
> else" statement if that's the preferred option.

If I were you, I'd leave the patch as-is and wait for Greg review.

I was only reporting some words that I recall I read in some emails of Greg. 
But it is highly probable that those contexts were a bit different or that 
the statements were much more complex.

As far as what my personal preference is, I think that you shouldn't care 
because I'm not one of the maintainers. Above all, even if I were one of the 
maintainers I'd never prevent developers to use their own style with this 
kind of statements.

To summarize, you'd better leave the patch as-is.

Thanks,

Fabio

> 
> Thanks,
> Karolina
> > 
> 
> 

Re: [Outreachy kernel] [PATCH 1/7] staging: vt6655: Introduce `idx` temporary variable

[Praveen Kumar]

On 28-10-2021 16:05, 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.
> 
> 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;

Why not to make *i* as unsigned short and get rid of all the type conversions below ?

> +	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:
> 

Regards,

~Praveen.

Re: [Outreachy kernel] [PATCH 2/7] staging: vt6655: Use incrementation in `idx`

[Praveen Kumar]

On 28-10-2021 16:05, 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]);
>  
> -		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]);

If I'm not wrong, there is a problem here, we are using the modified idx value here, instead of original which is *MISCFIFO_SYNDATA_IDX*.
I don't see idx value being reset either. Am I missing something ?

Further, this bring a question, how are you validating or planning to validate these changes ?

>  		} 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:
> 

Regards,

~Praveen.

Re: [Outreachy kernel] [PATCH 2/7] staging: vt6655: Use incrementation in `idx`

[Praveen Kumar]

On 29-10-2021 20:26, Praveen Kumar wrote:
> On 28-10-2021 16:05, 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]);
>>  
>> -		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]);
> 
> If I'm not wrong, there is a problem here, we are using the modified idx value here, instead of original which is *MISCFIFO_SYNDATA_IDX*.
> I don't see idx value being reset either. Am I missing something ?
> 

Looks like I missed, the case statement, thus, idx will be intact. Please ignore my previous comment.

However, as mentioned in previous patch, we can also make *i* as unsigned short and that should IMO remove the requirement of cast.
But again, this works as well. I'm fine with either. Thanks and sorry for the confusion.

> Further, this bring a question, how are you validating or planning to validate these changes ?
> 
>>  		} 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:
>>

Regards,

~Praveen.

Re: [Outreachy kernel] [PATCH 2/7] staging: vt6655: Use incrementation in `idx`

[Karolina Drobnik]

Hi Praveen,

On Fri, 2021-10-29 at 21:11 +0530, Praveen Kumar wrote:
> However, as mentioned in previous patch, we can also make *i* as
> unsigned short and that should IMO remove the requirement of cast.

I agree, it should remove the cast but I think I'll leave `idx` as it
is.

> But again, this works as well. I'm fine with either. Thanks and sorry
> for the confusion.

No worries, thanks for looking at my changes. It's good to double check
everything :)


Thanks,
Karolina

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

[Dan Carpenter]

On Thu, Oct 28, 2021 at 11:35:31AM +0100, 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.
> 

No, this just makes the code harder to read.  Don't introduce a local
variable which only holds a global constant.  Variables should be
variable and constants should be constants.  No one wants to have to
look up things for no reason.

regards,
dan carpenter