Renee,
Thanks for the review and sorry for the late reply. Below is our response to
your comments. Please let us know if you have any additional questions.
-------------------------------------------------------------------------------
FEEDBACK ON TECHNICAL DOCUMENTATION/CODE
-------------------------------------------------------------------------------
Reviewer Name: Renee Danson
Document/Module Title: wifi-1018-diff
Document/Module Version/Date: 11/10/06
No comments:
usr/src/uts/common/io/ath/ath_impl.h
-------------------------------------------------------------------------------
usr/src/uts/common/io/ath/ath_aux.c
-------------------------------------------------------------------------------
RD-17 289-290 Cos You should be able to merge these two lines.
| DISCUSS
| Lines 289-290 are:
|
| uint16_t flags;
| ix = ath_hal_mhz2ieee(ah, c->channel, c->channelFlags);
|
| In what way should the be merged?
RD-18 298-299 Com I'm puzzled by the if conditional and the
comment; they don't seem to match. And I can't
find the implementation of ath_hal_mhz2ieee to
figure out what ix really is. This is probably
okay, I'm just confused; but I'd like to
understand it!
| EXPLAIN
| Function ath_hal_mhz2ieee() is defined in ath HAL, which is provided
| by opensource MadWifi project in binary only form, thus no source
| code for it. It's used to convert a frequence value into IEEE
| channel number. For example, frequency 2412MHz is IEEE802.11b/g
| channel 1. So here 'ix' means channel number, which is used to
| index into array ic_sup_channels[].
| For the comments
| /* can't handle stuff <2400 right now */
| As explain previously, 2412MHz is IEEE802.11b/g channel 1. But
| some chipsets supports frequency less than 2412 MHz, like 2372MHz.
| In this case, channel number is returned as negative value. Here
| we ignored such frequencies.
|
| To make it clear, modified comments as below
| /*
| * can't handle frequency <2400MHz (negative
| * channels) right now
| */
RD-19 704 Cos Typo: "reseting" should be "resetting"
| ACCEPT
-------------------------------------------------------------------------------
usr/src/uts/common/io/ath/ath_hal.h
-------------------------------------------------------------------------------
RD-20 108 Com typo: "interference used for as AR..." Probably
need to delete the 'as'.
| ACCEPT
RD-21 265 Com The comment for this line is a dup of that on
the line above; I don't think that's right.
| ACCEPT
| Will change to:
| HAL_XR_DATA = 5 /* extended range data */
RD-22 279-280 Func Just to be sure: it's intentional that the
first two items in the enum are set to 0x0001?
| EXPLAIN
| Yes. These two definitions are the same as the ones in opensource
| MadWifi project and FreeBSD.
| Added comments to explain this as below:
| * When 0x0001 is set, both TXQ_TXOKINT and TXQ_TXERRINT
* will be enabled.
RD-23 320-322 Com I don't understand the second sentence of
this
comment; the word 'no' seems misplaced, but I
can't quite figure out how it should read.
| ACCEPT
| Should be:
|
| * Fragment burst backoff policy. Normally no backoff is done
| * after a successful transmission; the next fragment is sent
| * at SIFS.
RD-24 335-336 Com There's an extra 'to' in there.
| ACCEPT.
RD-25 528, 529 Com The comments for these two lines seem to be
swapped.
| ACCEPT.
RD-26 760, 761 Com You didn't change this, but the comments for
these two lines seem to be swapped, as well.
| ACCEPT.
-------------------------------------------------------------------------------
usr/src/uts/common/io/ath/ath_main.c
-------------------------------------------------------------------------------
RD-27 1133 Com typo: "descariptor" should be "descriptor"
| ACCEPT.
-------------------------------------------------------------------------------
Comment type key:
Func comment on functionality
Perf comment on performance
CodeStd comment on coding standards
Design comment on design
Edit editorial comment
Cos cosmetic comment
Com comment on missing comments
escription of feedback:
ACCEPT Request accepted
REJECT Request rejected
EXPLAIN Explanation given
DISCUSS Request requires further discussion to resolve
DEFER Request deferred (e.g. because work is out-of-scope)
_______________________________________________
networking-discuss mailing list
[email protected]
