[rt2x00-users] [PATCH 09/12] rt2x00: Fix ht40+/ht40- channels for rt2800pci/rt2800usb

Ivo van Doorn ivdoorn at gmail.com
Sun Aug 30 14:15:44 UTC 2009 

> diff --git a/drivers/net/wireless/rt2x00/rt2800pci.h b/drivers/net/wireless/rt2x00/rt2800pci.h
> index 3877dd4..3a86e44 100644
> --- a/drivers/net/wireless/rt2x00/rt2800pci.h
> +++ b/drivers/net/wireless/rt2x00/rt2800pci.h
> @@ -800,7 +800,7 @@
>   * TX_BAND_CFG: 0x1 use upper 20MHz, 0x0 use lower 20MHz
>   */
>  #define TX_BAND_CFG			0x132c
> -#define TX_BAND_CFG_HT40_PLUS		FIELD32(0x00000001)
> +#define TX_BAND_CFG_HT40MINUS		FIELD32(0x00000001)
>  #define TX_BAND_CFG_A			FIELD32(0x00000002)
>  #define TX_BAND_CFG_BG			FIELD32(0x00000004)


This change is not correct.

Setting FIELD32(0x00000001) to 1 means the upper 40Mhz band should be used.
When set to 0 the lower band must be used. With that the current name is correct...

> @@ -1479,7 +1479,7 @@ struct mac_iveiv_entry {
>   * BBP 3: RX Antenna
>   */
>  #define BBP3_RX_ANTENNA			FIELD8(0x18)
> -#define BBP3_HT40_PLUS			FIELD8(0x20)
> +#define BBP3_RX_HT40MINUS		FIELD8(0x20)

Also not correct. In the legacy driver bit 0x20 is cleared when setting to the
lower band.

Both of the comments apply to both rt2800pci and rt2800usb.

> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.c b/drivers/net/wireless/rt2x00/rt2800usb.c
> index f8c94fe..2339960 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.c
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.c
> @@ -825,7 +825,7 @@ static void rt2800usb_config_channel(struct rt2x00_dev *rt2x00dev,
>  	}
>  
>  	rt2x00usb_register_read(rt2x00dev, TX_BAND_CFG, &reg);
> -	rt2x00_set_field32(&reg, TX_BAND_CFG_HT40_PLUS, conf_is_ht40_plus(conf));
> +	rt2x00_set_field32(&reg, TX_BAND_CFG_HT40MINUS, conf_is_ht40_minus(conf));
>  	rt2x00_set_field32(&reg, TX_BAND_CFG_A, rf->channel > 14);
>  	rt2x00_set_field32(&reg, TX_BAND_CFG_BG, rf->channel <= 14);
>  	rt2x00usb_register_write(rt2x00dev, TX_BAND_CFG, reg);
> @@ -858,7 +858,7 @@ static void rt2800usb_config_channel(struct rt2x00_dev *rt2x00dev,
>  	rt2800usb_bbp_write(rt2x00dev, 4, bbp);
>  
>  	rt2800usb_bbp_read(rt2x00dev, 3, &bbp);
> -	rt2x00_set_field8(&bbp, BBP3_HT40_PLUS, conf_is_ht40_plus(conf));
> +	rt2x00_set_field8(&bbp, BBP3_RX_HT40MINUS, conf_is_ht40_minus(conf));
>  	rt2800usb_bbp_write(rt2x00dev, 3, bbp);
>  
>  	if (rt2x00_rev(&rt2x00dev->chip) == RT2860C_VERSION) {
> diff --git a/drivers/net/wireless/rt2x00/rt2800usb.h b/drivers/net/wireless/rt2x00/rt2800usb.h
> index f5b7288..421d3f2 100644
> --- a/drivers/net/wireless/rt2x00/rt2800usb.h
> +++ b/drivers/net/wireless/rt2x00/rt2800usb.h
> @@ -809,7 +809,7 @@
>   * TX_BAND_CFG: 0x1 use upper 20MHz, 0x0 use lower 20MHz
>   */
>  #define TX_BAND_CFG			0x132c
> -#define TX_BAND_CFG_HT40_PLUS		FIELD32(0x00000001)
> +#define TX_BAND_CFG_HT40MINUS		FIELD32(0x00000001)
>  #define TX_BAND_CFG_A			FIELD32(0x00000002)
>  #define TX_BAND_CFG_BG			FIELD32(0x00000004)
>  
> @@ -1487,7 +1487,7 @@ struct mac_iveiv_entry {
>   * BBP 3: RX Antenna
>   */
>  #define BBP3_RX_ANTENNA			FIELD8(0x18)
> -#define BBP3_HT40_PLUS			FIELD8(0x20)
> +#define BBP3_RX_HT40MINUS		FIELD8(0x20)
>  
>  /*
>   * BBP 4: Bandwidth
> diff --git a/drivers/net/wireless/rt2x00/rt2x00config.c b/drivers/net/wireless/rt2x00/rt2x00config.c
> index 3501788..baba4da 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00config.c
> +++ b/drivers/net/wireless/rt2x00/rt2x00config.c
> @@ -176,6 +176,56 @@ void rt2x00lib_config_antenna(struct rt2x00_dev *rt2x00dev,
>  		rt2x00lib_toggle_rx(rt2x00dev, STATE_RADIO_RX_ON_LINK);
>  }
>  
> +/*
> + * This function converts the value of conf->channel->hw_value if we are using
> + * an ht40+ or ht40- channel. The new value is returned in *hw_value. Do not
> + * call this function in other cases. It returns 1 on success, 0 on failure
> + */
> +
> +static int rt2x00lib_adjust_channel(struct rt2x00_dev *rt2x00dev,
> +				    struct ieee80211_conf * conf,
> +				    u16 * hw_value)
> +{
> +	struct hw_mode_spec *spec = &rt2x00dev->spec;
> +	int i;
> +	int center_channel;
> +
> +	center_channel = spec->channels[conf->channel->hw_value].channel;
> +
> +	/*
> +	 * When using ht40+/ht40- channels, we need to adjust the center
> +	 * frequency. For instance, channel 104 ht40- needs to be converted
> +	 * into channel 102 as far as RF programming is concerned.
> +	 *
> +	 * Since an ht40 channel is 40 MHz wide (20 MHz appart from the center
> +	 * frequency on each side) and since a channel number is associated to
> +	 * the upper or lower 20 MHz part, we need to adjust the center
> +	 * frequency by +/- 10 MHz, ie 2 channels.
> +	 */
> +	

No extra empty line after a comment please.

> +	if (conf_is_ht40_minus(conf))
> +		center_channel -= 2;
> +	if (conf_is_ht40_plus(conf))
> +		center_channel += 2;
> +
> +	/* 
> +	 * Look up the new hw_value. We can speed up the search if we assume
> +	 * that the entry for ht40- is at index (hw_value - 1) and the entry
> +	 * for ht40+ is at index (hw_value + 1). Right now, we do a full
> +	 * search.
> +	 */

Since we build up the table ourselves, can't we make this optimization?

> +	for (i = 0; i < spec->num_channels; i ++) {
> +		if (spec->channels[i].channel == center_channel) {
> +			*hw_value = (u16)i;
> +			return 1;
> +		}
> +	}
> +		
> +	/* if not found, we should issue an error */
> +	return 0;
> +}
> +
>  void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
>  		      struct ieee80211_conf *conf,
>  		      unsigned int ieee80211_flags)
> @@ -187,17 +237,33 @@ void rt2x00lib_config(struct rt2x00_dev *rt2x00dev,
>  	libconf.conf = conf;
>  
>  	if (ieee80211_flags & IEEE80211_CONF_CHANGE_CHANNEL) {
> -		if (conf_is_ht40(conf))
> +
> +		u16 hw_value = conf->channel->hw_value;

Not really needed, I think.

> +		/*
> +		 * When using ht40+/ht40- channels, we need to adjust the
> +		 * center frequency. For instance, channel 104 ht40- needs to
> +		 * be converted into channel 102 as far as RF programming is
> +		 * concerned
> +		 */
> +
> +		if (conf_is_ht40(conf)) {
> +			if (!rt2x00lib_adjust_channel(rt2x00dev, conf,
> +						      &hw_value)) {
> +				WARNING(rt2x00dev, "Unable to adjust "
> +					"channel\n");
> +			}

Can't we move the error message into rt2x00lib_adjust_channel()?
Also 0 should mean success and otherwise an error code should be returned.
See the errno defines for the correct value.

>  			__set_bit(CONFIG_CHANNEL_HT40, &rt2x00dev->flags);
> -		else
> +		} else {
>  			__clear_bit(CONFIG_CHANNEL_HT40, &rt2x00dev->flags);
> +		}
>  
>  		memcpy(&libconf.rf,
> -		       &rt2x00dev->spec.channels[conf->channel->hw_value],
> +		       &rt2x00dev->spec.channels[hw_value],
>  		       sizeof(libconf.rf));
>  
>  		memcpy(&libconf.channel,
> -		       &rt2x00dev->spec.channels_info[conf->channel->hw_value],
> +		       &rt2x00dev->spec.channels_info[hw_value],
>  		       sizeof(libconf.channel));
>  	}
>  

Ivo

More information about the users mailing list