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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/
outreachy-kernel/
948406a3e7d23f1cdf866aa4448d9428bdd32512.1635415820.git.karolinadrobnik%40gmail.com.
> 








[Index of Archives]     [Linux Driver Development]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux