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

["Fabio M. De Francesco" <fmdefrancesco@gmail.com>]

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