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

Fabio M. De Francesco
2021-10-28T11:21:24Z

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