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

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@xxxxxxxxxxxxxxxx.
> > 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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/1683328.aCfAWUeHFl%40localhost.localdomain.
>