Re: [PATCH 1/1] wireless: move ifs' in _rtl92c_store_pwrIndex_diffrate_offsetinto elseif statements

From: Larry Finger
Date: Sun Feb 05 2012 - 11:05:51 EST


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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/