Add a variable to store initialization tables. Use this variable
in AL2230 initialization.
Signed-off-by: Karolina Drobnik <[email protected]>
---
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++));
MACvSetMISCFifo(priv, idx++, al2230_channel_table0[channel - 1]);
MACvSetMISCFifo(priv, idx++, al2230_channel_table1[channel - 1]);
--
2.30.2
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 <[email protected]>
> ---
> 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/
outreachy-kernel/dc72a4c3539aed70569f66396ed3b51818bc2aea.
1635415820.git.karolinadrobnik%40gmail.com.
>
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 <[email protected]>
> > ---
> > 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] ?
Could there be a better name than "data"? Perhaps "table"?
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 [email protected].
> > 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 [email protected].
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/2039159.k92FijXA2m%40localhost.localdomain.
>
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 <[email protected]>
> > > ---
> > > 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 [email protected].
> > > 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 [email protected].
> > To view this discussion on the web visit https://groups.google.com/d/
msgid/outreachy-kernel/2039159.k92FijXA2m%40localhost.localdomain.
> >
>
Hi Fabio and Julia,
Thank you very much for looking at my changes.
On Thu, 2021-10-28 at 13:21 +0200, Fabio M. De Francesco wrote:
> 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).
Yes, I added them on purpose to improve readability (+ that's also my
preference anyway)
> 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 understand, thanks for letting me know.
On Thu, 2021-10-28 at 13:32 +0200, Julia Lawall wrote:
> Would it be better as data[i] ?
>
> Could there be a better name than "data"? Perhaps "table"?
Hmm, now when I'm thinking about, it indeed looks like a better option.
I would even say that `data` (or `table`/`init_table`) can be only used
in the AL7320 case and we can go with `al2230_init_table` for AL2230.
The line would be 61 characters long, way below the limit.
What do you think?
Many thanks,
Karolina
> On Thu, 2021-10-28 at 13:32 +0200, Julia Lawall wrote:
> > Would it be better as data[i] ?
> >
> > Could there be a better name than "data"? Perhaps "table"?
>
> Hmm, now when I'm thinking about, it indeed looks like a better option.
> I would even say that `data` (or `table`/`init_table`) can be only used
> in the AL7320 case and we can go with `al2230_init_table` for AL2230.
> The line would be 61 characters long, way below the limit.
>
> What do you think?
That would amount to dropping patch 4? That seems fine. It is better to
avoid creating aliases.
julia