Return-path: Received: from cora.hrz.tu-chemnitz.de ([134.109.228.40]:47716 "EHLO cora.hrz.tu-chemnitz.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755272Ab3I3QEp (ORCPT ); Mon, 30 Sep 2013 12:04:45 -0400 Date: Mon, 30 Sep 2013 18:04:37 +0200 From: Simon Wunderlich To: Lorenzo Bianconi Cc: linville@tuxdriver.com, linux-wireless@vger.kernel.org, mcgrof@qca.qualcomm.com, simon.wunderlich@s2003.tu-chemnitz.de Subject: Re: [RFC] ath9k: add HT40 spectral scan capability Message-ID: <20130930160437.GA15780@pandem0nium> (sfid-20130930_180448_917922_6DD7D93F) References: <1380206258-16452-1-git-send-email-lorenzo.bianconi83@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="opJtzjQTFsWo+cga" In-Reply-To: <1380206258-16452-1-git-send-email-lorenzo.bianconi83@gmail.com> Sender: linux-wireless-owner@vger.kernel.org List-ID: --opJtzjQTFsWo+cga Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hey Lorenzo, thanks for your work. Please find a few comments inline: On Thu, Sep 26, 2013 at 04:37:38PM +0200, Lorenzo Bianconi wrote: > Add spectral scan feature on HT40 channels for ath9k. This patch extends > previous capability added by Simon Wunderlich >=20 > Signed-off-by: Lorenzo Bianconi > --- > drivers/net/wireless/ath/ath9k/ath9k.h | 47 +++++++---- > drivers/net/wireless/ath/ath9k/calib.c | 10 +-- > drivers/net/wireless/ath/ath9k/calib.h | 3 +- > drivers/net/wireless/ath/ath9k/hw.c | 2 +- > drivers/net/wireless/ath/ath9k/link.c | 3 +- > drivers/net/wireless/ath/ath9k/recv.c | 142 ++++++++++++++++++++++-----= ------ > 6 files changed, 139 insertions(+), 68 deletions(-) >=20 > [...] > diff --git a/drivers/net/wireless/ath/ath9k/calib.c b/drivers/net/wireles= s/ath/ath9k/calib.c > index 5e8219a..d09eaeb 100644 > --- a/drivers/net/wireless/ath/ath9k/calib.c > +++ b/drivers/net/wireless/ath/ath9k/calib.c > @@ -63,13 +63,13 @@ static s16 ath9k_hw_get_default_nf(struct ath_hw *ah, > return ath9k_hw_get_nf_limits(ah, chan)->nominal; > } > =20 > -s16 ath9k_hw_getchan_noise(struct ath_hw *ah, struct ath9k_channel *chan) > +s16 ath9k_hw_getchan_noise(struct ath_hw *ah, struct ath9k_channel *chan, > + s16 nf) > { > s8 noise =3D ATH_DEFAULT_NOISE_FLOOR; > =20 > - if (chan && chan->noisefloor) { > - s8 delta =3D chan->noisefloor - > - ATH9K_NF_CAL_NOISE_THRESH - > + if (nf) { > + s8 delta =3D nf - ATH9K_NF_CAL_NOISE_THRESH - > ath9k_hw_get_default_nf(ah, chan); > if (delta > 0) > noise +=3D delta; > @@ -394,7 +394,7 @@ bool ath9k_hw_getnf(struct ath_hw *ah, struct ath9k_c= hannel *chan) > caldata->nfcal_pending =3D false; > ath9k_hw_update_nfcal_hist_buffer(ah, caldata, nfarray); > chan->noisefloor =3D h[0].privNF; > - ah->noise =3D ath9k_hw_getchan_noise(ah, chan); > + ah->noise =3D ath9k_hw_getchan_noise(ah, chan, chan->noisefloor); > return true; > } > EXPORT_SYMBOL(ath9k_hw_getnf); > diff --git a/drivers/net/wireless/ath/ath9k/calib.h b/drivers/net/wireles= s/ath/ath9k/calib.h > index 3d70b8c..b8ed95e 100644 > --- a/drivers/net/wireless/ath/ath9k/calib.h > +++ b/drivers/net/wireless/ath/ath9k/calib.h > @@ -116,7 +116,8 @@ void ath9k_init_nfcal_hist_buffer(struct ath_hw *ah, > void ath9k_hw_bstuck_nfcal(struct ath_hw *ah); > void ath9k_hw_reset_calibration(struct ath_hw *ah, > struct ath9k_cal_list *currCal); > -s16 ath9k_hw_getchan_noise(struct ath_hw *ah, struct ath9k_channel *chan= ); > +s16 ath9k_hw_getchan_noise(struct ath_hw *ah, struct ath9k_channel *chan, > + s16 nf); I'm not entirely sure how these noise changes are connected to the patch. I= t appears you use it later, but maybe it would be good to put this in a separate patch? > [...] > =20 > diff --git a/drivers/net/wireless/ath/ath9k/recv.c b/drivers/net/wireless= /ath/ath9k/recv.c > index 4ee472a..525f07c 100644 > --- a/drivers/net/wireless/ath/ath9k/recv.c > +++ b/drivers/net/wireless/ath/ath9k/recv.c > @@ -972,14 +972,13 @@ static int ath_process_fft(struct ath_softc *sc, st= ruct ieee80211_hdr *hdr, > { > #ifdef CONFIG_ATH9K_DEBUGFS > struct ath_hw *ah =3D sc->sc_ah; > - u8 bins[SPECTRAL_HT20_NUM_BINS]; > - u8 *vdata =3D (u8 *)hdr; > - struct fft_sample_ht20 fft_sample; > + u8 type, num_bins, *data, *vdata =3D (u8 *) hdr; > + struct fft_sample fft_data; If you keep the variable name fft_sample, you can save quite a lot renames = below. And that would make it easier for this patch to review. > struct ath_radar_info *radar_info; > - struct ath_ht20_mag_info *mag_info; > int len =3D rs->rs_datalen; > int dc_pos; > - u16 length, max_magnitude; > + u16 length, fft_len; > + enum nl80211_channel_type chtype; > =20 > /* AR9280 and before report via ATH9K_PHYERR_RADAR, AR93xx and newer > * via ATH9K_PHYERR_SPECTRAL. Haven't seen ATH9K_PHYERR_FALSE_RADAR_EXT > @@ -997,45 +996,53 @@ static int ath_process_fft(struct ath_softc *sc, st= ruct ieee80211_hdr *hdr, > if (!(radar_info->pulse_bw_info & SPECTRAL_SCAN_BITMASK)) > return 0; > =20 > - /* Variation in the data length is possible and will be fixed later. > - * Note that we only support HT20 for now. > - * > - * TODO: add HT20_40 support as well. > - */ > - if ((len > SPECTRAL_HT20_TOTAL_DATA_LEN + 2) || > - (len < SPECTRAL_HT20_TOTAL_DATA_LEN - 1)) > + chtype =3D cfg80211_get_chandef_type(&sc->hw->conf.chandef); > + if ((chtype =3D=3D NL80211_CHAN_HT40MINUS) || > + (chtype =3D=3D NL80211_CHAN_HT40PLUS)) { > + type =3D FFT_SAMPLE_HT20_40; > + num_bins =3D SPECTRAL_HT20_40_NUM_BINS; > + fft_len =3D SPECTRAL_HT20_40_TOTAL_DATA_LEN; > + data =3D (u8 *) fft_data.ht20_40.data; > + } else { > + type =3D FFT_SAMPLE_HT20; > + num_bins =3D SPECTRAL_HT20_NUM_BINS; > + fft_len =3D SPECTRAL_HT20_TOTAL_DATA_LEN; > + data =3D (u8 *) fft_data.ht20.data; > + } > + > + /* variation in the data length is possible and will be fixed later */ > + if ((len > fft_len + 2) || (len < fft_len - 1)) > return 1; > =20 > - fft_sample.tlv.type =3D ATH_FFT_SAMPLE_HT20; > - length =3D sizeof(fft_sample) - sizeof(fft_sample.tlv); > - fft_sample.tlv.length =3D __cpu_to_be16(length); > + fft_data.tlv.type =3D __cpu_to_be32(type); > + length =3D sizeof(fft_data) - sizeof(fft_data.tlv); > + fft_data.tlv.length =3D __cpu_to_be16(length); > =20 > - fft_sample.freq =3D __cpu_to_be16(ah->curchan->chan->center_freq); > - fft_sample.rssi =3D fix_rssi_inv_only(rs->rs_rssi_ctl0); > - fft_sample.noise =3D ah->noise; > + fft_data.freq =3D __cpu_to_be16(ah->curchan->chan->center_freq); > + fft_data.tsf =3D __cpu_to_be64(tsf); > =20 > - switch (len - SPECTRAL_HT20_TOTAL_DATA_LEN) { > + switch (len - fft_len) { > case 0: > /* length correct, nothing to do. */ > - memcpy(bins, vdata, SPECTRAL_HT20_NUM_BINS); > + memcpy(data, vdata, num_bins); > break; > case -1: > /* first byte missing, duplicate it. */ > - memcpy(&bins[1], vdata, SPECTRAL_HT20_NUM_BINS - 1); > - bins[0] =3D vdata[0]; > + memcpy(&data[1], vdata, num_bins - 1); > + data[0] =3D vdata[0]; > break; > case 2: > /* MAC added 2 extra bytes at bin 30 and 32, remove them. */ > - memcpy(bins, vdata, 30); > - bins[30] =3D vdata[31]; > - memcpy(&bins[31], &vdata[33], SPECTRAL_HT20_NUM_BINS - 31); > + memcpy(data, vdata, 30); > + data[30] =3D vdata[31]; > + memcpy(&data[31], &vdata[33], num_bins - 31); > break; > case 1: > /* MAC added 2 extra bytes AND first byte is missing. */ > - bins[0] =3D vdata[0]; > - memcpy(&bins[0], vdata, 30); > - bins[31] =3D vdata[31]; > - memcpy(&bins[32], &vdata[33], SPECTRAL_HT20_NUM_BINS - 32); > + data[0] =3D vdata[0]; > + memcpy(&data[1], vdata, 30); > + data[31] =3D vdata[31]; > + memcpy(&data[32], &vdata[33], num_bins - 32); I hope you tested whether this byte shuffling works as expected for HT40? > break; > default: > return 1; > @@ -1044,23 +1051,68 @@ static int ath_process_fft(struct ath_softc *sc, = struct ieee80211_hdr *hdr, > /* DC value (value in the middle) is the blind spot of the spectral > * sample and invalid, interpolate it. > */ > - dc_pos =3D SPECTRAL_HT20_NUM_BINS / 2; > - bins[dc_pos] =3D (bins[dc_pos + 1] + bins[dc_pos - 1]) / 2; Same for the DC value here ... > - > - /* mag data is at the end of the frame, in front of radar_info */ > - mag_info =3D ((struct ath_ht20_mag_info *)radar_info) - 1; > - > - /* copy raw bins without scaling them */ > - memcpy(fft_sample.data, bins, SPECTRAL_HT20_NUM_BINS); > - fft_sample.max_exp =3D mag_info->max_exp & 0xf; > - > - max_magnitude =3D spectral_max_magnitude(mag_info->all_bins); > - fft_sample.max_magnitude =3D __cpu_to_be16(max_magnitude); > - fft_sample.max_index =3D spectral_max_index(mag_info->all_bins); > - fft_sample.bitmap_weight =3D spectral_bitmap_weight(mag_info->all_bins); > - fft_sample.tsf =3D __cpu_to_be64(tsf); > + dc_pos =3D num_bins / 2; > + data[dc_pos] =3D (data[dc_pos + 1] + data[dc_pos - 1]) / 2; > + > + if ((chtype =3D=3D NL80211_CHAN_HT40MINUS) || > + (chtype =3D=3D NL80211_CHAN_HT40PLUS)) { > + u16 lower_max_mag, upper_max_mag; > + s16 nf_ext; > + struct ath_ht20_40_mag_info *mag_info; > + struct ath9k_hw_cal_data *caldata =3D ah->caldata; > + > + if (caldata) > + nf_ext =3D ath9k_hw_getchan_noise(ah, ah->curchan, > + caldata->nfCalHist[3].privNF); > + else > + nf_ext =3D ATH_DEFAULT_NOISE_FLOOR; > + > + mag_info =3D ((struct ath_ht20_40_mag_info *)radar_info) - 1; > + lower_max_mag =3D spectral_max_magnitude(mag_info->lower_bins); > + upper_max_mag =3D spectral_max_magnitude(mag_info->upper_bins); > + > + if (chtype =3D=3D NL80211_CHAN_HT40PLUS) { > + fft_data.ht20_40.lower_rssi =3D > + fix_rssi_inv_only(rs->rs_rssi_ctl0); > + fft_data.ht20_40.upper_rssi =3D > + fix_rssi_inv_only(rs->rs_rssi_ext0); > + fft_data.ht20_40.lower_nf =3D ah->noise; > + fft_data.ht20_40.upper_nf =3D nf_ext; > + } else { > + fft_data.ht20_40.lower_rssi =3D > + fix_rssi_inv_only(rs->rs_rssi_ext0); > + fft_data.ht20_40.upper_rssi =3D > + fix_rssi_inv_only(rs->rs_rssi_ctl0); > + fft_data.ht20_40.lower_nf =3D nf_ext; > + fft_data.ht20_40.upper_nf =3D ah->noise; > + } > + fft_data.ht20_40.lower_max_mag =3D __cpu_to_be16(lower_max_mag); > + fft_data.ht20_40.lower_max_idx =3D > + spectral_max_index(mag_info->lower_bins); > + fft_data.ht20_40.lower_bitmap_w =3D > + spectral_bitmap_weight(mag_info->lower_bins); > + fft_data.ht20_40.upper_max_mag =3D __cpu_to_be16(upper_max_mag); > + fft_data.ht20_40.upper_max_idx =3D > + spectral_max_index(mag_info->upper_bins); > + fft_data.ht20_40.upper_bitmap_w =3D > + spectral_bitmap_weight(mag_info->upper_bins); > + fft_data.ht20_40.max_exp =3D mag_info->max_exp & 0xf; Could you please try to use some local variables here? I think this could avoid a few of these ugly multi-line assignments ... > + } else { > + u16 max_mag; > + struct ath_ht20_mag_info *mag_info; > + > + mag_info =3D ((struct ath_ht20_mag_info *)radar_info) - 1; > + max_mag =3D spectral_max_magnitude(mag_info->all_bins); > + fft_data.ht20.rssi =3D fix_rssi_inv_only(rs->rs_rssi_ctl0); > + fft_data.ht20.nf =3D ah->noise; > + fft_data.ht20.max_mag =3D __cpu_to_be16(max_mag); > + fft_data.ht20.max_idx =3D spectral_max_index(mag_info->all_bins); > + fft_data.ht20.bitmap_w =3D > + spectral_bitmap_weight(mag_info->all_bins); > + fft_data.ht20.max_exp =3D mag_info->max_exp & 0xf; > + } > =20 > - ath_debug_send_fft_sample(sc, &fft_sample.tlv); > + ath_debug_send_fft_sample(sc, &fft_data.tlv); > return 1; > #else > return 0; > --=20 > 1.8.1.2 >=20 Cheers, Simon --opJtzjQTFsWo+cga Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iEYEARECAAYFAlJJoRUACgkQrzg/fFk7axahYACeIpO8EeMzuw23qvhp9cadk/8I y2AAmwQ+tDGkTG2Kin42aTRHhinb8dEo =gXMM -----END PGP SIGNATURE----- --opJtzjQTFsWo+cga--