On 02/05/2012 02:12 AM, Jan Ceuleers wrote:
On 01/30/2012 07:50 AM, Devendra.Naga wrote:
From: "Devendra.Naga"<devendra.aaru@xxxxxxxxx>
The regaddr is compared against different rates in each case and the
powerlevels are set
accordingly. as this can match for the first if and we still check for other
powerlevels too,
use else if ratherthan using if statements
Signed-off-by: Devendra.Naga<devendra.aaru@xxxxxxxxx>
If this is correct, and I'm not saying it is or isn't, would it not be clearer
to turn this whole section into a switch statement? The cases that requiring
fiddling with the bitmask would need to be dealt with in the default: case.
Yes, it is correct. It might be clearer if it were written as a switch
statement, but that would be a lot more intrusive. Besides, have you ever looked
at the way gcc compiles a switch? I have done some reverse engineering, and at
least some implementations generate code that looks like if .. else if .. ..
else. I have not looked at the generated code, but I'm quite certain that the
optimizer in gcc is good enough to know that once it gets a match between
regaddr and one of the RTXAGC constants, there is no need to test the rest, and
that the change to "else if" is mostly cosmetic, and will not greatly affect the
execution.
Why was it written as a series of if statements to begin with: can (or could)
any of these if statements change the value of regaddr such that later ifs
evaluate differently? From reading only the patch the answer seems to be "no",
so that looks good.
I cannot answer the question of why it was written this way - you would have to
ask the original authors. As to the possibility of the code changing regaddr,
you either need to look more carefully at the code, or you need to restudy how C
works.
Or what about turning this into a data-driven section? It looks like values of
regaddr are being translated into offsets into
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt]. Can this be done with a lookup table?
I would state it a little differently - the values of regaddr are used to
determine where in the double-indexed array rtlphy->MCS_TXPWR[][] that the
quantity "data" should be placed. There are always multiple ways to do anything.
For the kernel, a major rule is to make minimal changes that are easy to verify.
The patch in question meets that condition. If this code were in the hot path,
it might be worth the effort required to do a complete rewrite with the
associated testing, but in my mind, that effort would be wasted. Something that
would be easy to do, easy to verify, and would reduce the size of the generated
code would be
int index;
if (regaddr == RTXAGC_A_RATE18_06) {
index = 0;
else if (if (regaddr == RTXAGC_A_RATE54_24) {
index = 1;
.....
}
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][index] = data;
RT_TRACE(rtlpriv, COMP_INIT, DBG_TRACE,
"MCSTxPowerLevelOriginalOffset[%d][%d] = 0x%x\n",
rtlphy->pwrgroup_cnt, index,
rtlphy->MCS_TXPWR[rtlphy->pwrgroup_cnt][index]);
...
Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
