[PATCH] staging: vt6655: Fix line wrapping in rf.c file

[PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Karolina Drobnik]

Fix line length warnings raised by checkpatch.pl in
rf.c file for `RFvWriteWakeProgSyn`,`RFbRawSetPower`
and `RFbAL7230SelectChannelPostProcess`functions.

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

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index a6f17162d017..206d34b555bc 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -699,11 +699,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 			return false;
 
 		for (ii = 0; ii < CB_AL2230_INIT_SEQ; ii++)
-			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230InitTable[ii]);
+			MACvSetMISCFifo(priv,
+					(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+					dwAL2230InitTable[ii]);
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable0[uChannel - 1]);
+		MACvSetMISCFifo(priv,
+				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+				dwAL2230ChannelTable0[uChannel - 1]);
 		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable1[uChannel - 1]);
+		MACvSetMISCFifo(priv,
+				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+				dwAL2230ChannelTable1[uChannel - 1]);
 		break;
 
 		/* Need to check, PLLON need to be low for channel setting */
@@ -716,17 +722,28 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 
 		if (uChannel <= CB_MAX_CHANNEL_24G) {
 			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTable[ii]);
+				MACvSetMISCFifo(priv,
+						(unsigned short)(MISCFIFO_SYNDATA_IDX
+						+ ii), dwAL7230InitTable[ii]);
 		} else {
 			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTableAMode[ii]);
+				MACvSetMISCFifo(priv,
+						(unsigned short)(MISCFIFO_SYNDATA_IDX
+						+ ii),
+					dwAL7230InitTableAMode[ii]);
 		}
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable0[uChannel - 1]);
+		MACvSetMISCFifo(priv,
+				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+				dwAL7230ChannelTable0[uChannel - 1]);
 		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable1[uChannel - 1]);
+		MACvSetMISCFifo(priv,
+				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+				dwAL7230ChannelTable1[uChannel - 1]);
 		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable2[uChannel - 1]);
+		MACvSetMISCFifo(priv,
+				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
+				dwAL7230ChannelTable2[uChannel - 1]);
 		break;
 
 	case RF_NOTHING:
@@ -736,7 +753,8 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 		return false;
 	}
 
-	MACvSetMISCFifo(priv, MISCFIFO_SYNINFO_IDX, (unsigned long)MAKEWORD(bySleepCount, byInitCount));
+	MACvSetMISCFifo(priv, MISCFIFO_SYNINFO_IDX,
+			(unsigned long)MAKEWORD(bySleepCount, byInitCount));
 
 	return true;
 }
@@ -836,20 +854,32 @@ bool RFbRawSetPower(struct vnt_private *priv, unsigned char byPwr,
 	case RF_AIROHA:
 		ret &= IFRFbWriteEmbedded(priv, dwAL2230PowerTable[byPwr]);
 		if (rate <= RATE_11M)
-			ret &= IFRFbWriteEmbedded(priv, 0x0001B400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x0001B400
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
 		else
-			ret &= IFRFbWriteEmbedded(priv, 0x0005A400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x0005A400
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
 
 		break;
 
 	case RF_AL2230S:
 		ret &= IFRFbWriteEmbedded(priv, dwAL2230PowerTable[byPwr]);
 		if (rate <= RATE_11M) {
-			ret &= IFRFbWriteEmbedded(priv, 0x040C1400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
-			ret &= IFRFbWriteEmbedded(priv, 0x00299B00 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x040C1400
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x00299B00
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
 		} else {
-			ret &= IFRFbWriteEmbedded(priv, 0x0005A400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
-			ret &= IFRFbWriteEmbedded(priv, 0x00099B00 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x0005A400
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
+			ret &= IFRFbWriteEmbedded(priv, 0x00099B00
+					+ (BY_AL2230_REG_LEN << 3)
+					+ IFREGCTL_REGW);
 		}
 
 		break;
@@ -921,7 +951,8 @@ bool RFbAL7230SelectChannelPostProcess(struct vnt_private *priv,
 	 * register
 	 * Channel Index 1~14
 	 */
-	if ((byOldChannel <= CB_MAX_CHANNEL_24G) && (byNewChannel > CB_MAX_CHANNEL_24G)) {
+	if ((byOldChannel <= CB_MAX_CHANNEL_24G) &&
+	    (byNewChannel > CB_MAX_CHANNEL_24G)) {
 		/* Change from 2.4G to 5G [Reg] */
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[2]);
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[3]);
@@ -930,7 +961,8 @@ bool RFbAL7230SelectChannelPostProcess(struct vnt_private *priv,
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[10]);
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[12]);
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[15]);
-	} else if ((byOldChannel > CB_MAX_CHANNEL_24G) && (byNewChannel <= CB_MAX_CHANNEL_24G)) {
+	} else if ((byOldChannel > CB_MAX_CHANNEL_24G) &&
+		   (byNewChannel <= CB_MAX_CHANNEL_24G)) {
 		/* Change from 5G to 2.4G [Reg] */
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[2]);
 		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[3]);
-- 
2.30.2

Re: [Outreachy kernel] [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Julia Lawall]

On Mon, 18 Oct 2021, Karolina Drobnik wrote:

> Fix line length warnings raised by checkpatch.pl in
> rf.c file for `RFvWriteWakeProgSyn`,`RFbRawSetPower`
> and `RFbAL7230SelectChannelPostProcess`functions.
>
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 66 +++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index a6f17162d017..206d34b555bc 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -699,11 +699,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
>  			return false;
>
>  		for (ii = 0; ii < CB_AL2230_INIT_SEQ; ii++)
> -			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230InitTable[ii]);
> +			MACvSetMISCFifo(priv,
> +					(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +					dwAL2230InitTable[ii]);
>
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable0[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL2230ChannelTable0[uChannel - 1]);
>  		ii++;
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable1[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL2230ChannelTable1[uChannel - 1]);
>  		break;
>
>  		/* Need to check, PLLON need to be low for channel setting */
> @@ -716,17 +722,28 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
>
>  		if (uChannel <= CB_MAX_CHANNEL_24G) {
>  			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
> -				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTable[ii]);
> +				MACvSetMISCFifo(priv,
> +						(unsigned short)(MISCFIFO_SYNDATA_IDX
> +						+ ii), dwAL7230InitTable[ii]);

The placement of + ii) is not a good solution.  Can it be moved up to the
line before?  Likewise in the next case.

>  		} else {
>  			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
> -				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTableAMode[ii]);
> +				MACvSetMISCFifo(priv,
> +						(unsigned short)(MISCFIFO_SYNDATA_IDX
> +						+ ii),
> +					dwAL7230InitTableAMode[ii]);
>  		}
>
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable0[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL7230ChannelTable0[uChannel - 1]);
>  		ii++;
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable1[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL7230ChannelTable1[uChannel - 1]);
>  		ii++;
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable2[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL7230ChannelTable2[uChannel - 1]);
>  		break;
>
>  	case RF_NOTHING:
> @@ -736,7 +753,8 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
>  		return false;
>  	}
>
> -	MACvSetMISCFifo(priv, MISCFIFO_SYNINFO_IDX, (unsigned long)MAKEWORD(bySleepCount, byInitCount));
> +	MACvSetMISCFifo(priv, MISCFIFO_SYNINFO_IDX,
> +			(unsigned long)MAKEWORD(bySleepCount, byInitCount));
>
>  	return true;
>  }
> @@ -836,20 +854,32 @@ bool RFbRawSetPower(struct vnt_private *priv, unsigned char byPwr,
>  	case RF_AIROHA:
>  		ret &= IFRFbWriteEmbedded(priv, dwAL2230PowerTable[byPwr]);
>  		if (rate <= RATE_11M)
> -			ret &= IFRFbWriteEmbedded(priv, 0x0001B400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x0001B400
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);

Also not great.  Maybe there could be a newline before the 0x0001B400 too.

julia

>  		else
> -			ret &= IFRFbWriteEmbedded(priv, 0x0005A400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x0005A400
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);
>
>  		break;
>
>  	case RF_AL2230S:
>  		ret &= IFRFbWriteEmbedded(priv, dwAL2230PowerTable[byPwr]);
>  		if (rate <= RATE_11M) {
> -			ret &= IFRFbWriteEmbedded(priv, 0x040C1400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> -			ret &= IFRFbWriteEmbedded(priv, 0x00299B00 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x040C1400
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x00299B00
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);
>  		} else {
> -			ret &= IFRFbWriteEmbedded(priv, 0x0005A400 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> -			ret &= IFRFbWriteEmbedded(priv, 0x00099B00 + (BY_AL2230_REG_LEN << 3) + IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x0005A400
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);
> +			ret &= IFRFbWriteEmbedded(priv, 0x00099B00
> +					+ (BY_AL2230_REG_LEN << 3)
> +					+ IFREGCTL_REGW);
>  		}
>
>  		break;
> @@ -921,7 +951,8 @@ bool RFbAL7230SelectChannelPostProcess(struct vnt_private *priv,
>  	 * register
>  	 * Channel Index 1~14
>  	 */
> -	if ((byOldChannel <= CB_MAX_CHANNEL_24G) && (byNewChannel > CB_MAX_CHANNEL_24G)) {
> +	if ((byOldChannel <= CB_MAX_CHANNEL_24G) &&
> +	    (byNewChannel > CB_MAX_CHANNEL_24G)) {
>  		/* Change from 2.4G to 5G [Reg] */
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[2]);
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[3]);
> @@ -930,7 +961,8 @@ bool RFbAL7230SelectChannelPostProcess(struct vnt_private *priv,
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[10]);
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[12]);
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTableAMode[15]);
> -	} else if ((byOldChannel > CB_MAX_CHANNEL_24G) && (byNewChannel <= CB_MAX_CHANNEL_24G)) {
> +	} else if ((byOldChannel > CB_MAX_CHANNEL_24G) &&
> +		   (byNewChannel <= CB_MAX_CHANNEL_24G)) {
>  		/* Change from 5G to 2.4G [Reg] */
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[2]);
>  		ret &= IFRFbWriteEmbedded(priv, dwAL7230InitTable[3]);
> --
> 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/20211018150526.9718-1-karolinadrobnik%40gmail.com.
>

Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Greg KH]

On Mon, Oct 18, 2021 at 04:05:26PM +0100, Karolina Drobnik wrote:
> Fix line length warnings raised by checkpatch.pl in
> rf.c file for `RFvWriteWakeProgSyn`,`RFbRawSetPower`
> and `RFbAL7230SelectChannelPostProcess`functions.
> 
> Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> ---
>  drivers/staging/vt6655/rf.c | 66 +++++++++++++++++++++++++++----------
>  1 file changed, 49 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> index a6f17162d017..206d34b555bc 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -699,11 +699,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
>  			return false;
>  
>  		for (ii = 0; ii < CB_AL2230_INIT_SEQ; ii++)
> -			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230InitTable[ii]);
> +			MACvSetMISCFifo(priv,
> +					(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +					dwAL2230InitTable[ii]);
>  
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable0[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL2230ChannelTable0[uChannel - 1]);
>  		ii++;
> -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable1[uChannel - 1]);
> +		MACvSetMISCFifo(priv,
> +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> +				dwAL2230ChannelTable1[uChannel - 1]);
>  		break;
>  
>  		/* Need to check, PLLON need to be low for channel setting */
> @@ -716,17 +722,28 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
>  
>  		if (uChannel <= CB_MAX_CHANNEL_24G) {
>  			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
> -				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTable[ii]);
> +				MACvSetMISCFifo(priv,
> +						(unsigned short)(MISCFIFO_SYNDATA_IDX
> +						+ ii), dwAL7230InitTable[ii]);

You shouldn't put the "+" on the start of a new line.

Also, these are all just fine as-is for now.  A better way to make these
lines smaller is to use better variable and function names that are
shorter and make sense :)

thanks,

greg k-h

Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Joe Perches]

On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> On Mon, Oct 18, 2021 at 04:05:26PM +0100, Karolina Drobnik wrote:
> > Fix line length warnings raised by checkpatch.pl in
> > rf.c file for `RFvWriteWakeProgSyn`,`RFbRawSetPower`
> > and `RFbAL7230SelectChannelPostProcess`functions.
> > 
> > Signed-off-by: Karolina Drobnik <karolinadrobnik@gmail.com>
> > ---
> >  drivers/staging/vt6655/rf.c | 66 +++++++++++++++++++++++++++----------
> >  1 file changed, 49 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
> > index a6f17162d017..206d34b555bc 100644
> > --- a/drivers/staging/vt6655/rf.c
> > +++ b/drivers/staging/vt6655/rf.c
> > @@ -699,11 +699,17 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
> >  			return false;
> >  
> >  		for (ii = 0; ii < CB_AL2230_INIT_SEQ; ii++)
> > -			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230InitTable[ii]);
> > +			MACvSetMISCFifo(priv,
> > +					(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> > +					dwAL2230InitTable[ii]);
> >  
> > -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable0[uChannel - 1]);
> > +		MACvSetMISCFifo(priv,
> > +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> > +				dwAL2230ChannelTable0[uChannel - 1]);
> >  		ii++;
> > -		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable1[uChannel - 1]);
> > +		MACvSetMISCFifo(priv,
> > +				(unsigned short)(MISCFIFO_SYNDATA_IDX + ii),
> > +				dwAL2230ChannelTable1[uChannel - 1]);
> >  		break;
> >  
> >  		/* Need to check, PLLON need to be low for channel setting */
> > @@ -716,17 +722,28 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
> >  
> >  		if (uChannel <= CB_MAX_CHANNEL_24G) {
> >  			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
> > -				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTable[ii]);
> > +				MACvSetMISCFifo(priv,
> > +						(unsigned short)(MISCFIFO_SYNDATA_IDX
> > +						+ ii), dwAL7230InitTable[ii]);
> 
> You shouldn't put the "+" on the start of a new line.
> 
> Also, these are all just fine as-is for now.  A better way to make these
> lines smaller is to use better variable and function names that are
> shorter and make sense :)

And maybe use temporaries for the multiply used
MISCFIFO_SYNDATA_IDX + ii and a few other changes.

Maybe some refactoring like:
---
 drivers/staging/vt6655/rf.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/vt6655/rf.c b/drivers/staging/vt6655/rf.c
index 0dae593c6944f..7beb0cd5a62df 100644
--- a/drivers/staging/vt6655/rf.c
+++ b/drivers/staging/vt6655/rf.c
@@ -680,16 +680,19 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 			 u16 uChannel)
 {
 	void __iomem *iobase = priv->PortOffset;
-	int   ii;
+	int i;
+	unsigned short idx = MISCFIFO_SYNDATA_IDX;
 	unsigned char byInitCount = 0;
 	unsigned char bySleepCount = 0;
+	const unsigned long *data;
 
+	uChannel--;
 	VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
+
 	switch (byRFType) {
 	case RF_AIROHA:
 	case RF_AL2230S:
-
-		if (uChannel > CB_MAX_CHANNEL_24G)
+		if (uChannel >= CB_MAX_CHANNEL_24G)
 			return false;
 
 		 /* Init Reg + Channel Reg (2) */
@@ -698,12 +701,12 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 		if (byInitCount > (MISCFIFO_SYNDATASIZE - bySleepCount))
 			return false;
 
-		for (ii = 0; ii < CB_AL2230_INIT_SEQ; ii++)
-			MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230InitTable[ii]);
+		data = dwAL2230InitTable;
+		for (i = 0; i < CB_AL2230_INIT_SEQ; i++)
+			MACvSetMISCFifo(priv, idx++, *data++);
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable0[uChannel - 1]);
-		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL2230ChannelTable1[uChannel - 1]);
+		MACvSetMISCFifo(priv, idx++, dwAL2230ChannelTable0[uChannel]);
+		MACvSetMISCFifo(priv, idx++, dwAL2230ChannelTable1[uChannel]);
 		break;
 
 		/* Need to check, PLLON need to be low for channel setting */
@@ -714,19 +717,14 @@ bool RFvWriteWakeProgSyn(struct vnt_private *priv, unsigned char byRFType,
 		if (byInitCount > (MISCFIFO_SYNDATASIZE - bySleepCount))
 			return false;
 
-		if (uChannel <= CB_MAX_CHANNEL_24G) {
-			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTable[ii]);
-		} else {
-			for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii++)
-				MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230InitTableAMode[ii]);
-		}
+		data = (uChannel < CB_MAX_CHANNEL_24G) ?
+			dwAL7230InitTable : dwAL7230InitTableAMode;
+		for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
+			MACvSetMISCFifo(priv, idx++, *data++);
 
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable0[uChannel - 1]);
-		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable1[uChannel - 1]);
-		ii++;
-		MACvSetMISCFifo(priv, (unsigned short)(MISCFIFO_SYNDATA_IDX + ii), dwAL7230ChannelTable2[uChannel - 1]);
+		MACvSetMISCFifo(priv, idx++, dwAL7230ChannelTable0[uChannel]);
+		MACvSetMISCFifo(priv, idx++, dwAL7230ChannelTable1[uChannel]);
+		MACvSetMISCFifo(priv, idx++, dwAL7230ChannelTable2[uChannel]);
 		break;
 
 	case RF_NOTHING:

Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Karolina Drobnik]

Hi,

Thank you very much for your comments.

On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> Also, these are all just fine as-is for now.  A better way to make
> these lines smaller is to use better variable and function names 
> that are shorter and make sense :)

I have v2 ready but I'm not sure, given the Joe's patch, if my solution
is a satisfactory one. I didn't jump on such refactoring as I'm still
learning about the codebase/process and didn't want to muddle the
waters (...more than I do already).

Greg, what would you prefer? Should I back up with my patch, pick
something else and let Joe's patch be merged?


Also, I have a question about the patch if that's ok :)

On Mon, 2021-10-18 at 22:56 -0700, Joe Perches wrote:
> Maybe some refactoring like:
> ---
>  drivers/staging/vt6655/rf.c | 38
> ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/vt6655/rf.c
> b/drivers/staging/vt6655/rf.c
> index 0dae593c6944f..7beb0cd5a62df 100644
> --- a/drivers/staging/vt6655/rf.c
> +++ b/drivers/staging/vt6655/rf.c
> @@ -680,16 +680,19 @@ bool RFvWriteWakeProgSyn(struct vnt_private
> *priv, unsigned char byRFType,
>                          u16 uChannel)
>  {
>         void __iomem *iobase = priv->PortOffset;
> -       int   ii;
> +       int i;
> +       unsigned short idx = MISCFIFO_SYNDATA_IDX;
>         unsigned char byInitCount = 0;
>         unsigned char bySleepCount = 0;
> +       const unsigned long *data;
>  
> +       uChannel--;
>         VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);

I see that you introduced `uChannel--` to further tidy up the lines
with `[uChannel - 1]`. In general, is there anything wrong with
indexing like `i - 1`? What's the preference here? DRY things up as
much as possible?

I'm asking because when I was reading this line, at first, it wasn't
clear to me why we could decrement it (example though: "Was this
modified earlier? Do we need to "correct" it?").


Thanks,
Karolina

Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Greg KH]

On Tue, Oct 19, 2021 at 11:59:56AM +0100, Karolina Drobnik wrote:
> Hi,
> 
> Thank you very much for your comments.
> 
> On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> > Also, these are all just fine as-is for now.  A better way to make
> > these lines smaller is to use better variable and function names 
> > that are shorter and make sense :)
> 
> I have v2 ready but I'm not sure, given the Joe's patch, if my solution
> is a satisfactory one. I didn't jump on such refactoring as I'm still
> learning about the codebase/process and didn't want to muddle the
> waters (...more than I do already).
> 
> Greg, what would you prefer? Should I back up with my patch, pick
> something else and let Joe's patch be merged?

Joe hasn't submitted it in a format that I can take it in, so that is up
to you, I can't tell you want to work on :)

good luck!

greg k-h

Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Joe Perches]

On Tue, 2021-10-19 at 11:59 +0100, Karolina Drobnik wrote:
> Hi,
> 
> Thank you very much for your comments.
> 
> On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> > Also, these are all just fine as-is for now.  A better way to make
> > these lines smaller is to use better variable and function names 
> > that are shorter and make sense :)
> 
> I have v2 ready but I'm not sure, given the Joe's patch, if my solution
> is a satisfactory one. I didn't jump on such refactoring as I'm still
> learning about the codebase/process and didn't want to muddle the
> waters (...more than I do already).
> 
> Greg, what would you prefer? Should I back up with my patch, pick
> something else and let Joe's patch be merged?

What I suggested is not a patch it's just an example.

There's quite a lot of code in that driver that _could_
be updated/refined/refactored (none of which _I_ will
submit), but it's up to you do whatever _you_ want.

You could:

o remove the Hungarian notation
o convert the mixed case variables to snake case
o remove unnecessary function definitions and make them static
o refactor various functions

Generally, I prefer refactoring code to make it simpler or
more like the generally preferred kernel styles.

Another option would be to submit a completely new driver for
this device based on this existing driver as what's there isn't
particularly great IMO, but read the vt6655 TODO file and see if
there's something you actually want to do there.

> Also, I have a question about the patch if that's ok :)

It's OK to ask.

> On Mon, 2021-10-18 at 22:56 -0700, Joe Perches wrote:
> > Maybe some refactoring like:
> > ---
[]
> > diff --git a/drivers/staging/vt6655/rf.c
[]
> > +       uChannel--;
> 
> I see that you introduced `uChannel--` to further tidy up the lines
> with `[uChannel - 1]`. In general, is there anything wrong with
> indexing like `i - 1`?

Depends on how often it's used and if it's ever missed accidentally.

> What's the preference here? DRY things up as much as possible?

Up to you

> I'm asking because when I was reading this line, at first, it wasn't
> clear to me why we could decrement it (example though: "Was this
> modified earlier? Do we need to "correct" it?").

Generally, just try to make code clear for a reader.

When you do that, the compiler will also do a better job
at what it does.

If you look at the callers of the function, see if it's better
to decrement the argument instead.

Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Fabio M. De Francesco]

On Tuesday, October 19, 2021 2:26:05 PM CEST Joe Perches wrote:
> On Tue, 2021-10-19 at 11:59 +0100, Karolina Drobnik wrote:
> > Hi,
> > 
> > Thank you very much for your comments.
> > 
> > On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> > > Also, these are all just fine as-is for now.  A better way to make
> > > these lines smaller is to use better variable and function names 
> > > that are shorter and make sense :)
> > 
> > I have v2 ready but I'm not sure, given the Joe's patch, if my solution
> > is a satisfactory one. I didn't jump on such refactoring as I'm still
> > learning about the codebase/process and didn't want to muddle the
> > waters (...more than I do already).
> > 
> > Greg, what would you prefer? Should I back up with my patch, pick
> > something else and let Joe's patch be merged?
> 
> What I suggested is not a patch it's just an example.

Sorry, Joe. I sent a message trying to explain what you were showing to 
Karolina with your previous email. Soon after sending my reply, I noticed 
that you had already elaborated a bit more.

I hope that you don't mind. If I wrote something that contradicts your 
thoughts and intentions please accept my apologies.

Regards,

Fabio

Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Fabio M. De Francesco]

On Tuesday, October 19, 2021 12:59:56 PM CEST Karolina Drobnik wrote:
> Hi,
> 
> Thank you very much for your comments.
> 
> On Mon, 2021-10-18 at 17:12 +0200, Greg KH wrote:
> > Also, these are all just fine as-is for now.  A better way to make
> > these lines smaller is to use better variable and function names 
> > that are shorter and make sense :)
> 
> I have v2 ready but I'm not sure, given the Joe's patch, if my solution
> is a satisfactory one. I didn't jump on such refactoring as I'm still
> learning about the codebase/process and didn't want to muddle the
> waters (...more than I do already).
> 
> Greg, what would you prefer? Should I back up with my patch, pick
> something else and let Joe's patch be merged?
> 
> 
> Also, I have a question about the patch if that's ok :)
> 
> On Mon, 2021-10-18 at 22:56 -0700, Joe Perches wrote:
> > Maybe some refactoring like:
> > ---
> >  drivers/staging/vt6655/rf.c | 38
> > ++++++++++++++++++--------------------
> >  1 file changed, 18 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/staging/vt6655/rf.c
> > b/drivers/staging/vt6655/rf.c
> > index 0dae593c6944f..7beb0cd5a62df 100644
> > --- a/drivers/staging/vt6655/rf.c
> > +++ b/drivers/staging/vt6655/rf.c
> > @@ -680,16 +680,19 @@ bool RFvWriteWakeProgSyn(struct vnt_private
> > *priv, unsigned char byRFType,
> >                          u16 uChannel)
> >  {
> >         void __iomem *iobase = priv->PortOffset;
> > -       int   ii;
> > +       int i;
> > +       unsigned short idx = MISCFIFO_SYNDATA_IDX;
> >         unsigned char byInitCount = 0;
> >         unsigned char bySleepCount = 0;
> > +       const unsigned long *data;
> >  
> > +       uChannel--;
> >         VNSvOutPortW(iobase + MAC_REG_MISCFFNDEX, 0);
> 
> I see that you introduced `uChannel--` to further tidy up the lines
> with `[uChannel - 1]`. In general, is there anything wrong with
> indexing like `i - 1`? What's the preference here? DRY things up as
> much as possible?

Hi Karolina,

No, there is no problem in using a[i - 1]. Personally I prefer the former 
when 1 <= index <= ARRAY_SIZE(a). If you code "index = index -1;" or 
"index--;" (that is the same) and then you use 'index' many lines below that 
decrement in "a[index]" it may be not immediately clear that you are not 
indexing past the end of the array.

But this is not the point. You may still use Joe's style or leave it as 
"index -1". The point is that Joe is just showing you some different way that 
you can use to accomplish the task of "Fix line wrapping in rf.c file".

He put many different changes in one only single patch. Maybe that those kind 
of patch are permitted to developers who have gained so much trust that Greg 
doesn't need anymore to check "one {,logical} change at a time" (but still 
I'm not sure about it). 

I guess that if Linus T. or Greg K-H. want to put many different things in 
one big "Clean up rf.c" patch they can. This (yours) is not the case. If you 
decide to use one or more of the example Joe showed you you must be careful 
to split changes in a series of patch, according to the instructions you read 
in the Outreachy pages at kernelnewbies.org.

Joe is showing that you can shorten lines with several techniques...

1) renaming variables:
	("ii" => "i") and getting rid of hungarian notation ("bySomething" 
	=> "something");

2) contracting instructions: 
	"uChannel--" or "*data++" - for the latter take care of preceding
	rules or, better, use redundant parenthesis like in "*(data++)" to 
	facilitate readability and comprehensibility);

3) using temporary variables:
	"unsigned short idx = MISCFIFO_SYNDATA_IDX;" or "const unsigned
	long *data = dwAL2230InitTable;");

4) refactoring lines of code (e.g.,
	-               if (uChannel <= CB_MAX_CHANNEL_24G) {
	-                       for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii+
+)
	-                               MACvSetMISCFifo(priv, (unsigned
		short)(MISCFIFO_SYNDATA_IDX + ii),
		dwAL7230InitTable[ii]);
	-               } else {
	-                       for (ii = 0; ii < CB_AL7230_INIT_SEQ; ii+
+)
	-                               MACvSetMISCFifo(priv, (unsigned
		short)(MISCFIFO_SYNDATA_IDX + ii),
		dwAL7230InitTableAMode[ii]);
	-               }
	+               data = (uChannel < CB_MAX_CHANNEL_24G) ?
	+                       dwAL7230InitTable :
		dwAL7230InitTableAMode;
	+               for (i = 0; i < CB_AL7230_INIT_SEQ; i++)
	+                       MACvSetMISCFifo(priv, idx++, *data++);

5) something else that I'm missing and that you may easily notice :)

I prefer to state it again: if you choose to do such kind of works, be 
careful to split self-contained patches in a series and explain each change 
you make and why you make it. Each patch must do only one logical change.
Each patch of a series must be self-contained also in the sense that it must 
build without introducing errors or warnings at any point: for instance, five 
patches => five clean builds.

Thanks,

Fabio M. De Francesco

> 
> I'm asking because when I was reading this line, at first, it wasn't
> clear to me why we could decrement it (example though: "Was this
> modified earlier? Do we need to "correct" it?").
> 
> 
> Thanks,
> Karolina
> 
> 
> -- 
> 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/810a4e29b0c54520a30cae4d37fde0a59ea3d83b.camel%40gmail.com.
> 

Re: [Outreachy kernel] Re: [PATCH] staging: vt6655: Fix line wrapping in rf.c file

[Karolina Drobnik]

On Tue, 2021-10-19 at 05:26 -0700, Joe Perches wrote:
> What I suggested is not a patch it's just an example.

Sure, I understand. In this case, I'll take some inspiration from
your example and break down the changes into smaller chunks, thank you.

> There's quite a lot of code in that driver that _could_
> be updated/refined/refactored (none of which _I_ will submit),
> but it's up to you do whatever _you_ want.

Indeed there is, I'm trying to tackle one thing at a time. I thought
I could fix a couple of line length warnings in an easy way but I was
wrong.

Ok, I'll come back to CamelCase squashing and removing the Hungarian
notation before working on this refactor. I think this is a good
candidate for a patchset.

On Tue, 2021-10-19 at 15:07 +0200, Fabio M. De Francesco wrote:
> Hi Karolina,

Hi Fabio,
Thank you for describing everything in such detail, really appreciate
it.

> No, there is no problem in using a[i - 1]. Personally I prefer the
> former when 1 <= index <= ARRAY_SIZE(a). 

I see, thanks for explaining this.

> If you code "index = index -1;" or 
> "index--;" (that is the same) and then you use 'index' many lines
> below that decrement in "a[index]" it may be not immediately clear
> that you are not indexing past the end of the array.

That's what I thought as well.

> I prefer to state it again: if you choose to do such kind of works,
> be careful to split self-contained patches in a series and explain
> each change you make and why you make it.
> Each patch must do only one logical change.

Will definitely do so, thank you.

> Each patch of a series must be self-contained also in the sense that
> it must build without introducing errors or warnings at any point:
> for instance, five patches => five clean builds.

Makes sense, will keep that in mind. Also, I think it would be good to
mention it on the FPT page. I can suggest adding such comment in later
on.


Thanks,
Karolina