Re: [PATCH V2] Add iwlwifi wireless drivers

["John W. Linville" <linville@tuxdriver.com>]

On Mon, Aug 27, 2007 at 01:20:12PM +0800, Zhu Yi wrote:

> This is the second version of the patch (still against 2.6.23-rc3) adds
> the mac80211 based wireless drivers for the Intel PRO/Wireless
> 3945ABG/BG Network Connection and Intel Wireless WiFi Link AGN (4965)
> adapters. You can find it from:
> http://intellinuxwireless.org/iwlwifi/v5-Add-iwlwifi-wireless-drivers.patch

A few comments below -- many of them are nits or somewhat minor.
I think the ieee80211_rate.h one needs to be resolved for sure.  Also,
splitting iwl-base.c is definitely worth considering -- building two
object from one source is a bit ugly.

Please respond to the questions/comments, and indicate if/when you
will split iwl-base.c.  Also, let's figure-out what really needs to
be exportd to drivers from ieee80211_rate.h and get that done.

Thanks for the post!

John

--------------------------------------------------------------------------------

Big plus: both drivers work fairly well, and have received a lot of
testing in Fedora (F7 and rawhide).

--------------------------------------------------------------------------------

As others have pointed-out, building two drivers from a single source
is a bit ugly.  I hacked-up a couple of awk scripts to separate this
into two files (btw, let me know if you want them).  It seems the two
files differ by 5-10% (depending on how you score it).  I'm not sure
what the cut-off should be for requiring a split...?

$ wc iwl-base.c
  9565  29226 265696 iwl-base.c

$ wc iwl3945-base.c
  8701  26834 242294 iwl3945-base.c

$ wc iwl4965-base.c
  9197  28016 256601 iwl4965-base.c

FWIW, it looks like some of the header files also use the IWL defintion
for "two objects from one source" compilation.

--------------------------------------------------------------------------------

Nit: does iwl_hw_valid_rtc_data_addr need to be inline?

--------------------------------------------------------------------------------

IWL_DEBUG_RATE("enter\n")
IWL_DEBUG_RATE("leave\n")
IWL_DEBUG_RATE("NOP\n");

Multiples of each of these, particularly in the rate scaling algorithms.
Are these necessary?  Some seem to like them, others don't...

--------------------------------------------------------------------------------

iwl-4965-rs.c:

#include "../net/mac80211/ieee80211_rate.h"

I think this is unacceptable.  Let's figure-out what needs to be
exported to drivers and get it moved to an appopriate header, rather
than just pulling a header from a random location out of the tree.

--------------------------------------------------------------------------------

Same file line 586:

#define IWL_LEGACY_SWITCH_ANTENNA      0
#define IWL_LECACY_SWITCH_SISO         1
#define IWL_LEGACY_SWITCH_MIMO         2

#define IWL_RS_GOOD_RATIO              12800

#define IWL_ACTION_LIMIT               3
#define IWL_LEGACY_FAILURE_LIMIT       160
#define IWL_LEGACY_SUCCESS_LIMIT       480
#define IWL_LEGACY_TABLE_COUNT         160

#define IWL_NONE_LEGACY_FAILURE_LIMIT  400
#define IWL_NONE_LEGACY_SUCCESS_LIMIT  4500
#define IWL_NONE_LEGACY_TABLE_COUNT    1500

#define IWL_RATE_SCALE_SWITCH          (10880)

Shouldn't these be in a header file, or at least at the top of this
file?

--------------------------------------------------------------------------------

Same file line 1115:

#define IWL_SISO_SWITCH_ANTENNA  0
#define IWL_SISO_SWITCH_MIMO     1
#define IWL_SISO_SWITCH_GI       2

Ditto?

--------------------------------------------------------------------------------

Same file line 1210:

#define IWL_MIMO_SWITCH_ANTENNA_A      0
#define IWL_MIMO_SWITCH_ANTENNA_B      1
#define IWL_MIMO_SWITCH_GI             2

Ditto?

--------------------------------------------------------------------------------

Does iwl_rate_index_from_plcp need to be inline?  It seems a bit long,
and called from several places.  (And defined in multiple places...)

--------------------------------------------------------------------------------

iwl-4965.c line 1815:

#define TX_POWER_IWL_ILLEGAL_VDET    -100000
#define TX_POWER_IWL_ILLEGAL_VOLTAGE -10000
#define TX_POWER_IWL_CLOSED_LOOP_MIN_POWER 18
#define TX_POWER_IWL_CLOSED_LOOP_MAX_POWER 34
#define TX_POWER_IWL_VDET_SLOPE_BELOW_NOMINAL 17
#define TX_POWER_IWL_VDET_SLOPE_ABOVE_NOMINAL 20
#define TX_POWER_IWL_NOMINAL_POWER            26
#define TX_POWER_IWL_CLOSED_LOOP_ITERATION_LIMIT 1
#define TX_POWER_IWL_VOLTAGE_CODES_PER_03V       7
#define TX_POWER_IWL_DEGREES_PER_VDET_CODE       11
#define IWL_TX_POWER_MAX_NUM_PA_MEASUREMENTS 1
#define IWL_TX_POWER_CCK_COMPENSATION_B_STEP (9)
#define IWL_TX_POWER_CCK_COMPENSATION_C_STEP (5)

Same as previous "#define in middle of file" comments...

--------------------------------------------------------------------------------

iwl-4965.c line 2800:

#define IWL4965_LEGACY_SWITCH_ANTENNA  0
#define IWL4965_LECACY_SWITCH_SISO             1
#define IWL4965_LEGACY_SWITCH_MIMO             2

#define IWL4965_GOOD_RATIO                     12800

#define IWL_ACTION_LIMIT               3
#define IWL4965_LEGACY_FAILURE_LIMIT   160
#define IWL4965_LEGACY_SUCCESS_LIMIT   480
#define IWL4965_LEGACY_TABLE_COUNT             160

#define IWL4965_NONE_LEGACY_FAILURE_LIMIT      400
#define IWL4965_NONE_LEGACY_SUCCESS_LIMIT      4500
#define IWL4965_NONE_LEGACY_TABLE_COUNT        1500

#define IWL4965_RATE_SCALE_SWITCH  (10880)

Ditto?

--------------------------------------------------------------------------------

There are several more examples like the above -- I'm not going to keep
documenting them...

--------------------------------------------------------------------------------

Otherwise, it mostly seems acceptable.  I think the mac80211 guys
have a few issues -- hopefully they will chime-in here now?

-- 
John W. Linville
linville@tuxdriver.com